-
Notifications
You must be signed in to change notification settings - Fork 142
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 Orbit token bridge deployer and e2e tests #35
Conversation
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.
Really nice tests!
address(this), | ||
valueForGateway + valueForRouter | ||
); | ||
IERC20(nativeToken).approve(router, valueForRouter); |
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.
All this value gets used right? If it doesnt we should probably set approval to 0 at the end of this func
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.
Good point, value is not totally used in case Inbox would for some reason already hold some amount of native tokens. Added re-setting to 0
uint256 valueForGateway, | ||
uint256 valueForRouter, | ||
address creditBackAddress | ||
) public payable override { |
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.
Could remove the payable if the compiler allows
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.
Compiler complains: Error (6959): Overriding function changes state mutability from "payable" to "nonpayable".
callhook | ||
) | ||
|
||
const l1ToL2MessageGasEstimate = new L1ToL2MessageGasEstimator(l2Provider) |
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 we doing all this manually because we dont have support in the sdk? If so, it would be nice to also e2e tests that uses the sdk methods after they're written. I guess this might live in the arb-sdk tests if it works there, but it might be easier just to have them here since the env is already set up here.
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.
Exactly, I'll switch to using SDK once it's done. Current implementation is more like sanity check that all's good and can be used as reference for SDK implementation
Add script which deploys Orbit token bridge to a rollup running in local deployment
Add e2e Hardhat test cases for token deposit and withdrawal