Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use token bridge creator to deploy token bridges #17

Merged
merged 28 commits into from
Jan 9, 2024

Conversation

gvladika
Copy link
Contributor

@gvladika gvladika commented Oct 31, 2023

  • deploy L1-L2 token bridge using token bridge creator instead of using SDK
  • add flag to deploy L2-L3 token bridge, also using token bridge creator contract

Base automatically changed from fee-token-support to master October 31, 2023 19:11
@gvladika gvladika requested a review from gzeoneth November 6, 2023 10:09
Copy link
Collaborator

@tsahee tsahee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally seems good. two comments. Didn't try it yet.

test-node.bash Outdated
docker-compose run -e ARB_KEY=$devprivkey -e ETH_KEY=$devprivkey tokenbridge gen:network
docker-compose run --entrypoint sh tokenbridge -c "cat localNetwork.json"
echo == Deploying L1-L2 token bridge
sleep 120
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a LOT of sleep.
If really needed - add a comment to the print above so users will know to let it run, and add a comment explaining why so long.
Also - if possible, would be better to reduce it (e.g. waiting for something earlier to finish? can we wait/poll directly for the condition we're waiting for?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, actually delay is not needed at all anymore. Removed here - 683f887

},
token: {
string: true,
describe: "chain's custom fee token",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like we should be able to have a valid default for this .. weird that it's not in l3deployment.json?
I'd consider adding it there.. and if not, would even create a new testnode-only config file that will be created by the bash shell to have a default here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, token address is already in l3deployment.json. I removed token as input param, and now reading it from json - 8bb39c2

@gvladika gvladika requested a review from tsahee December 22, 2023 10:47
from: {
string: true,
describe: "account (see general help)",
default: "funnel",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in my tests funnel doesn't have l3tokens initially, so it shouldn't be the default
set the default to bridge-native-token-to-l3.

docker-compose run scripts send-l2 --ethamount 100 --to user_token_bridge_deployer --wait
docker-compose run scripts send-l2 --ethamount 100 --to user_fee_token_deployer --wait
docker compose run scripts send-l2 --ethamount 100 --to user_token_bridge_deployer --wait
docker compose run scripts send-l2 --ethamount 100 --to user_fee_token_deployer --wait
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's add to scripts/accounts.ts, under "Valid account names" - a description of:

  • user_token_bridge_deployer
  • user_fee_token_deployer
  • l3owner
  • l3sequencer

@tsahee tsahee merged commit 05ab75a into master Jan 9, 2024
Sneh1999 pushed a commit to Sneh1999/nitro-testnode that referenced this pull request Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants