-
Notifications
You must be signed in to change notification settings - Fork 79
refactor: migrate tasks #1226
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
base: master
Are you sure you want to change the base?
refactor: migrate tasks #1226
Conversation
Signed-off-by: Taylor Webb <[email protected]>
…onstrustor args) Signed-off-by: Taylor Webb <[email protected]>
Signed-off-by: Taylor Webb <[email protected]>
Signed-off-by: Taylor Webb <[email protected]>
Signed-off-by: Taylor Webb <[email protected]>
Signed-off-by: Taylor Webb <[email protected]>
grasphoper
left a comment
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 minor comments!
script/tasks/TestChainAdapter.s.sol
Outdated
| require(adapterAddress != address(0), string.concat("Adapter not found: ", adapterName)); | ||
|
|
||
| this.run(spokeChainId, adapterAddress, l1Token, amount, l2Token); | ||
| } |
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 suggest that we either add a usage example for this method, or remove it completely. I think at this point it's easier(less of a mental load heh) for a dev to manually insert "wanted adapter address" than to figure out how to call this other method (considering the example call is unavailable in the comments)
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.
Agreed - removed it here ef84754
script/tasks/TestChainAdapter.s.sol
Outdated
| * forge script script/tasks/TestChainAdapter.s.sol:TestChainAdapter \ | ||
| * --sig "run(uint256,address,address,uint256,address)" \ | ||
| * 10 0x... 0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48 1000000 0x... \ | ||
| * --rpc-url mainnet --broadcast |
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.
Mb include an example with real addrs? Or we can perhaps remove the second example as it's not materially different from Usage. Wdyt?
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.
Agree its redundant - 60142c8
script/tasks/UpgradeSpokePool.s.sol
Outdated
| * Example: | ||
| * forge script script/tasks/UpgradeSpokePool.s.sol:UpgradeSpokePool \ | ||
| * --sig "run(address)" 0x1234567890123456789012345678901234567890 \ | ||
| * -vvvv |
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.
Are you sure we need both of these examples?
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.
Nope haha definitely redundant 7d11e78
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.
This impl looks good to me. Have you checked against the old script to confirm that the outputs match? As we use this to generate multisig data (so it's sensitive)
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.
Yep! Outputs do match
Signed-off-by: Taylor Webb <[email protected]>
Signed-off-by: Taylor Webb <[email protected]>
Signed-off-by: Taylor Webb <[email protected]>
Migrates
TestChainAdapterandUpdateSpokePooltasks to Foundry - these seem to be the only tasks that are currently used. Deletes old hardhat tasks and updates relevant documentation