-
Notifications
You must be signed in to change notification settings - Fork 52
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
feat: Script for interacting with SpokePools #854
Conversation
This is useful for making deposits to the zkSync SpokePool(s) because zkSync's block explorer doesn't yet support making transactions via proxy contracts.
692477b
to
4b1bb8a
Compare
Example output:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few nits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
High level comment: can we make this script's name more clear (or do you intend to add other helper SpokePool related functions in here?) or add a helpful comment at the top explaining why this would be used?
Generally yes - I'd like to support something like the following:
I left a placeholder in the code for adding fill support, but I didn't want to spend any time on actually implementing it now given other priorities: https://github.com/across-protocol/relayer-v2/blob/4b1bb8a633f87a6f34ee2602cf217d3a80a29c45/scripts/spokepool.ts#L165 As general background, it's occurred to me that I miss a good set of cmdline utilities for interacting with our contracts, since it should be possible to execute (or query) standard operations against them without a block explorer (a la zkSync at the moment), and without having to modify code or .env configs. |
This would be a good interface yes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two more comments
Not all tokens are defined on all chains, so don't try to drop case unless the token actually exists on a chain.
Currently limitations: - Does not display updated values (i.e. after a speed-up). - Does not display message data. - Should normalise from the token decimals for better readability. - Does not gracefully handle when the txnHash is not found.
Have made some tweaks to the proposals, but the main point is that the script now accepts native decimals.
Much nicer to use after recent changes by James.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great place to start! Always todos and things to add/change, but this is great!
scripts/spokepool.ts
Outdated
const padLeft = 20; | ||
const padRight = 25; | ||
|
||
function formatAddress(address: string, maxWidth = 18): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just display full addresses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is just my OCD, but it looks really awkward to have a long address alongside a bunch of significantly shorter values. Some of the values look best right-justified (especially fields with a numerical relationship to each other - i.e. relayerFeePct and amount). Making these right-justified alongside the address means that the values end up very far away from the keys/field names, and that can make it difficult to visually connect them.
I was thinking about adding a -v
flag to allow the caller to force the address to be displayed in full.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up scrapping the custom formatting and reverted to "raw" dumping of the event. This removes some of the (imo) nice features like grouping related fields, which the SpokePool event data doesn't always do. However, it's very simple and generic, and ensures that all the fields are dumped. Maybe in future I'll introduce a way of ordering the fields, but that's not worth prioritising now.
scripts/spokepool.ts
Outdated
const depositor = await signer.getAddress(); | ||
const fromChainId = Number(args.from); | ||
const toChainId = Number(args.to); | ||
const recipient = args.recipient ?? depositor; | ||
const baseAmount = Number(args.amount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we validate args here and throw if they aren't provided as expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented: ea24ac1
This addresses a few nits in the code, but also makes it simpler.
This is useful for various SpokePool-related activities:
This is especially handy with the new zkSync SpokePool, because zkSync's block explorer doesn't yet support making transactions via proxy contracts, and doesn't decode Across SpokePool events properly either.
Example: