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

add bridge convenience script #93

Merged
merged 4 commits into from
Jan 19, 2024
Merged

Conversation

simluyt
Copy link
Contributor

@simluyt simluyt commented Jan 15, 2024

Made a basic bridge script for #78

Had two questions:

  • You mean a convenience script like this or as a roll.py subcommand?
  • This solution could be maybe improved for devnet/prod by adding an extra 'preset' flag like the main script, so that devnet users don't have to paste in localhost:8545? prod default would get rpc from config, devnet from wherever that gets stored if possible idk.

account = args.account or config["contract_deployer_account"]

bridge_proxy = json.load(
open(os.path.join('deployments', args.name, 'artifacts', 'addresses.json')))['L1StandardBridgeProxy']
Copy link
Member

Choose a reason for hiding this comment

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

You can use config.addresses_path for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like there is a difference between config_file from config.toml and config from the "Config" object in the generated in deployments/** that I overlooked. I propose (if I may) to leave it like this in this PR.
I'm pretty sure this will resolve itself when I start moving the bridge script into a subcommand since I'll have access to config_file and the Config object directly.

Copy link
Member

Choose a reason for hiding this comment

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

True, I only realized later!

bridge.py Outdated
open(os.path.join('deployments', args.name, 'artifacts', 'addresses.json')))['L1StandardBridgeProxy']

if not private_key or not account:
sys.exit("Private key, account, and amount must be provided either via arguments or config.")
Copy link
Member

Choose a reason for hiding this comment

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

The amount has a default value so it does not need to be provided.

bridge.py Outdated
unit = match.group(4) or 'ether'
return f"{amount}{unit}"
else:
raise ValueError("Invalid amount format.")
Copy link
Member

Choose a reason for hiding this comment

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

Useful function! Could you move it to libroll/libroll.py?

]

lib.run("sending eth to L1StandardBridgeProxy contract.", cast_command)
print("Successfully sent transaction to L1StandardBridgeProxy.")
Copy link
Member

Choose a reason for hiding this comment

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

It would be cool to optionally enable waiting until the bridge has been processed on the other side. Probably a sleep loop that polls, but I'm not exactly sure what to poll on the L2 side. I wonder if there isn't a call that can be made on one of the contracts (the message passer?) with the tx hash or something?

Let's keep that for another PR if you're interested!

@norswap
Copy link
Member

norswap commented Jan 18, 2024

Hey there! Sorry for the delay in the review, and thanks for the contribution 🙏

You mean a convenience script like this or as a roll.py subcommand?

Both can be good, but I'm wondering if it's maybe not better as a subcommand as it allows reusing the config loading logic (which does a few things beyond just loading the TOML, though not sure they are relevant in this use case from the top of my head).

The only downside is polluting the command space a bit, but maybe we can make it a "script" command and make this a "bridge" subcommand (and then if we add other scripts like this we can add them there as well).

This solution could be maybe improved for devnet/prod by adding an extra 'preset' flag like the main script, so that devnet users don't have to paste in localhost:8545? prod default would get rpc from config, devnet from wherever that gets stored if possible idk.

The default in the loading logic I mention above is the dev preset, if not integrating as a subcommand, could use that here too!

The default is http://127.0.0.1:8545 in any case, even without preset (from config/network.py :: l1_rpc_url)


What I propose: let's merge this (maybe if you can move the utility function to the library first?).

Then if you're motivated, an extra PR to integrate this as a subcommand, and/or to optionally wait for the bridge to complete would be more than welcome!

Thanks again!

@norswap norswap merged commit 3215c77 into 0xFableOrg:master Jan 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.

2 participants