-
Notifications
You must be signed in to change notification settings - Fork 75
chrismaree/arbitrum l1 adapter #30
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
Conversation
chrismaree
commented
Feb 11, 2022
- nit
- nit
- nit
- Update contracts/HubPool.sol
- review nit
- review nit
- feat(hubpool): Refactor hub pool to use 1D bitmap integer
- nit
- WIP
- review nit
- review nit
- nit
- nit
- nit
- nit
- nit
- nit
- WIP
- nit
- nit
- nit
- nit
- nit
- nit
- nit
- nit
- nit
- nit
- nit
- WIP
- nit
- nit
- WIP
- nit
- nit
- nit
- nit
- nit
- nit
- nit
- nit
- feat(slack-config): Add sync method and liquidity utilization
- nit
- Delete settings.json
- nit
- nit
- nit
- nit
- nit
- nit
- nit
- nit
- nit
- nit
- nit
- nit
- nit
- nit
- nit
- nit
- feat: Add arbitrum adapter
Co-authored-by: Matt Rice <matthewcrice32@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
| emit l2RefundL2AddressSet(l2RefundL2Address); | ||
| } | ||
|
|
||
| function relayMessage(address target, bytes memory message) external payable override onlyHubPool { |
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.
nit: should make this function nonReentrant since it makes an external call?
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.
ye, for consistency sake. will revert.
|
|
||
| function relayMessage(address target, bytes memory message) external payable override onlyHubPool { | ||
| uint256 requiredL1CallValue = l2MaxSubmissionCost + l2GasPrice * l2GasLimit; | ||
| require(address(this).balance >= requiredL1CallValue, "Insufficient ETH balance"); |
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.
Do you think we'' have an EOA send ETH to this contract prior to cross chain calls, or will the HubPool send ETH over?
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.
EOA just drops funds on it I think.
|
|
||
| function relayMessage(address target, bytes memory message) external payable override onlyHubPool { | ||
| uint256 requiredL1CallValue = l2MaxSubmissionCost + l2GasPrice * l2GasLimit; | ||
| require(address(this).balance >= requiredL1CallValue, "Insufficient ETH balance"); |
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.
WDYT about making this required L1 call value publicly queryable, for a potential EOA's convenience? We do this with the cross-chain oracle here but it may not be useful for this case if the HubPool deterministically decides when to send over ETH
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.
ye, that's a good idea. it will also let us automate when to top up the contract. I'll refactor now.
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 dont, however, think this needs to be nonReentrantView like your method. that's way overkill IMHO
|
|
||
| event L2GasPriceSet(uint256 newL2GasPrice); | ||
|
|
||
| event l2RefundL2AddressSet(address newL2RefundL2Address); |
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.
nit: forgot to capitalize this event
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.
<3
nicholaspai
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.
Awesome work, my only questions are surrounding the design and plan for funding the Arbitrum_Adapter with ETH to ensure createRetryableTicket works!