-
Notifications
You must be signed in to change notification settings - Fork 75
feat: Implement _initiateRelayerRefund and begin Optimism-specific SpokePool #20
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
Changes from all commits
011278a
edecc6f
be559ea
3897440
1066d32
3543a9f
e6c247c
7481c58
a666d86
35d9d87
22ad18a
eb505e9
8fb750a
2079344
67ef9d8
ce78455
37ef6bb
a61f777
2d417e9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| //SPDX-License-Identifier: Unlicense | ||
| pragma solidity ^0.8.0; | ||
|
|
||
| import "@eth-optimism/contracts/libraries/bridge/CrossDomainEnabled.sol"; | ||
| import "@eth-optimism/contracts/libraries/constants/Lib_PredeployAddresses.sol"; | ||
| import "./SpokePool.sol"; | ||
| import "./SpokePoolInterface.sol"; | ||
|
|
||
| /** | ||
| * @notice OVM specific SpokePool. | ||
| * @dev Uses OVM cross-domain-enabled logic for access control. | ||
| */ | ||
|
|
||
| contract Optimism_SpokePool is CrossDomainEnabled, SpokePoolInterface, SpokePool { | ||
| // Address of the L1 contract that acts as the owner of this SpokePool. | ||
| address public override crossDomainAdmin; | ||
|
|
||
| event SetXDomainAdmin(address indexed newAdmin); | ||
|
|
||
| constructor( | ||
| address _crossDomainAdmin, | ||
| address _wethAddress, | ||
| uint64 _depositQuoteTimeBuffer, | ||
| address timerAddress | ||
| ) | ||
| CrossDomainEnabled(Lib_PredeployAddresses.L2_CROSS_DOMAIN_MESSENGER) | ||
| SpokePool(_wethAddress, _depositQuoteTimeBuffer, timerAddress) | ||
| { | ||
| _setCrossDomainAdmin(_crossDomainAdmin); | ||
| } | ||
|
|
||
| /************************************** | ||
| * ADMIN FUNCTIONS * | ||
| **************************************/ | ||
|
|
||
| /** | ||
| * @notice Changes the L1 contract that can trigger admin functions on this contract. | ||
| * @dev This should be set to the address of the L1 contract that ultimately relays a cross-domain message, which | ||
| * is expected to be the Optimism_Adapter. | ||
| * @dev Only callable by the existing admin via the Optimism cross domain messenger. | ||
| * @param newCrossDomainAdmin address of the new L1 admin contract. | ||
| */ | ||
| function setCrossDomainAdmin(address newCrossDomainAdmin) | ||
| public | ||
| override | ||
| onlyFromCrossDomainAccount(crossDomainAdmin) | ||
| { | ||
| _setCrossDomainAdmin(newCrossDomainAdmin); | ||
| } | ||
|
|
||
| function setEnableRoute( | ||
| address originToken, | ||
| uint256 destinationChainId, | ||
| bool enable | ||
| ) public override onlyFromCrossDomainAccount(crossDomainAdmin) { | ||
| _setEnableRoute(originToken, destinationChainId, enable); | ||
| } | ||
|
|
||
| function setDepositQuoteTimeBuffer(uint64 buffer) public override onlyFromCrossDomainAccount(crossDomainAdmin) { | ||
| _setDepositQuoteTimeBuffer(buffer); | ||
| } | ||
|
|
||
| function initializeRelayerRefund(bytes32 relayerRepaymentDistributionProof) | ||
| public | ||
| override | ||
| onlyFromCrossDomainAccount(crossDomainAdmin) | ||
| { | ||
| _initializeRelayerRefund(relayerRepaymentDistributionProof); | ||
| } | ||
|
|
||
| /************************************** | ||
| * INTERNAL FUNCTIONS * | ||
| **************************************/ | ||
|
|
||
| function _setCrossDomainAdmin(address newCrossDomainAdmin) internal { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any way to call this function after construction? Is this something we'd like to be able to change? (genuine question, not suggesting a change)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there is a
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah i added in this function without telling you. We should allow this to be changed from L1
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I'm a little confused. Are there two functions that allow you to
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No there's only one, implemented in this contract. |
||
| require(newCrossDomainAdmin != address(0), "Bad bridge router address"); | ||
| crossDomainAdmin = newCrossDomainAdmin; | ||
| emit SetXDomainAdmin(crossDomainAdmin); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ import "@openzeppelin/contracts/utils/Address.sol"; | |
| import "@uma/core/contracts/common/implementation/Testable.sol"; | ||
| import "@uma/core/contracts/common/implementation/Lockable.sol"; | ||
| import "@uma/core/contracts/common/implementation/MultiCaller.sol"; | ||
| import "./MerkleLib.sol"; | ||
|
|
||
| interface WETH9Like { | ||
| function withdraw(uint256 wad) external; | ||
|
|
@@ -43,6 +44,15 @@ abstract contract SpokePool is Testable, Lockable, MultiCaller { | |
| // Origin token to destination token routings can be turned on or off. | ||
| mapping(address => mapping(uint256 => bool)) public enabledDepositRoutes; | ||
|
|
||
| struct RelayerRefund { | ||
| // Merkle root of relayer refunds. | ||
| bytes32 distributionRoot; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One interesting tradeoff about doing claims via merkle proofs one-by-one is that this will result in increased calldata costs on L2 over passing in all the leaves and storing (or maybe even executing) in a single call. This is because you will have to submit each leaf individually with another bytes32 element for each layer in the tree. This means that your overall calldata might be significantly larger than just passing in the leaves alone. I'm not sure how significant this would end up being, but I wonder how hard it would be for us to take in all the leaves and generate the proofs internally on-chain to show that they roll up into the root. Not suggesting a change, just wanted to mention this.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm so you think this could work by the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I dont think @mrice32 is proposing the HubPool passing the leafs; it cant do that in a efficient way. I think @mrice32 is proposing a technique where you can execute multiple leafs at once in a compressed structure and prove they are within the provided root. @mrice32 what you describe would fit into the execution stage of the process, right?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How do you imagine we improve on just making the contract multicall and assuming
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanna be super clear that I don't think we should do this now, and I think before we add this complexity, we should try to better understand the amount of calldata/gas savings we'd see from something like this with different tree sizes. Agree @chrismaree. I'm not suggesting the HubPool do anything different since any added data anywhere on mainnet is going to be expensive. If you did multicall, you would effectively be passing: [{distributionStruct1, proofArray1}, {distributionStruct2, proofArray2}...] What I'm suggesting is an additional method that allows you to pass all leaves this way: [{distriubtionStruct1}, {distributionStruct2}, ...] If we get the logic right in the contract, the leaf data could be used to generate the proofs so you wouldn't need to pass them in, thereby reducing calldata. |
||
| // This is a 2D bitmap tracking which leafs in the relayer refund root have been claimed, with max size of | ||
| // 256x256 leaves per root. | ||
| mapping(uint256 => uint256) claimsBitmap; | ||
| } | ||
| RelayerRefund[] public relayerRefunds; | ||
|
|
||
| struct RelayData { | ||
| address depositor; | ||
| address recipient; | ||
|
|
@@ -89,6 +99,7 @@ abstract contract SpokePool is Testable, Lockable, MultiCaller { | |
| address depositor, | ||
| address recipient | ||
| ); | ||
| event InitializedRelayerRefund(uint256 indexed relayerRefundId, bytes32 relayerRepaymentDistributionProof); | ||
|
|
||
| constructor( | ||
| address _wethAddress, | ||
|
|
@@ -257,17 +268,23 @@ abstract contract SpokePool is Testable, Lockable, MultiCaller { | |
| ); | ||
| } | ||
|
|
||
| function initializeRelayerRefund(bytes32 relayerRepaymentDistributionProof) public virtual { | ||
| return; | ||
| // This internal method should be called by an external "initializeRelayerRefund" function that validates the | ||
| // cross domain sender is the HubPool. This validation step differs for each L2, which is why the implementation | ||
| // specifics are left to the implementor of this abstract contract. | ||
| // Once this method is executed and a distribution root is stored in this contract, then `distributeRelayerRefund` | ||
| // can be called to execute each leaf in the root. | ||
| function _initializeRelayerRefund(bytes32 relayerRepaymentDistributionProof) internal { | ||
| uint256 relayerRefundId = relayerRefunds.length; | ||
| relayerRefunds.push().distributionRoot = relayerRepaymentDistributionProof; | ||
| emit InitializedRelayerRefund(relayerRefundId, relayerRepaymentDistributionProof); | ||
| } | ||
|
|
||
| // Call this method to execute a leaf within the `distributionRoot` stored on this contract. Caller must include a | ||
| // valid `inclusionProof` to verify that the leaf is contained within the root. The `relayerRefundId` is the index | ||
| // of the specific distribution root containing the passed in leaf. | ||
| function distributeRelayerRefund( | ||
| uint256 relayerRefundId, | ||
| uint256 leafId, | ||
| address l2TokenAddress, | ||
| uint256 netSendAmount, | ||
| address[] memory relayerRefundAddresses, | ||
| uint256[] memory relayerRefundAmounts, | ||
| MerkleLib.DestinationDistribution memory distributionLeaf, | ||
| bytes32[] memory inclusionProof | ||
| ) public {} | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| //SPDX-License-Identifier: Unlicense | ||
| pragma solidity ^0.8.0; | ||
|
|
||
| interface SpokePoolInterface { | ||
| function crossDomainAdmin() external returns (address); | ||
|
|
||
| function setCrossDomainAdmin(address newCrossDomainAdmin) external; | ||
|
|
||
| function setEnableRoute( | ||
| address originToken, | ||
| uint256 destinationChainId, | ||
| bool enable | ||
| ) external; | ||
|
|
||
| function setDepositQuoteTimeBuffer(uint64 buffer) external; | ||
|
|
||
| function initializeRelayerRefund(bytes32 relayerRepaymentDistributionProof) external; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| // TODO: Test OVM specific functionality and implementation of SpokePool internal methods. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| import { expect } from "chai"; | ||
| import { Contract } from "ethers"; | ||
| import { ethers } from "hardhat"; | ||
| import { SignerWithAddress } from "./utils"; | ||
| import { spokePoolFixture } from "./SpokePool.Fixture"; | ||
| import { mockDestinationDistributionRoot } from "./constants"; | ||
|
|
||
| let spokePool: Contract; | ||
| let caller: SignerWithAddress; | ||
|
|
||
| describe("SpokePool Initialize Relayer Refund Logic", async function () { | ||
| beforeEach(async function () { | ||
| [caller] = await ethers.getSigners(); | ||
| ({ spokePool } = await spokePoolFixture()); | ||
| }); | ||
| it("Initializing root stores root and emits event", async function () { | ||
| await expect(spokePool.connect(caller).initializeRelayerRefund(mockDestinationDistributionRoot)) | ||
| .to.emit(spokePool, "InitializedRelayerRefund") | ||
| .withArgs(0, mockDestinationDistributionRoot); | ||
| expect(await spokePool.relayerRefunds(0)).to.equal(mockDestinationDistributionRoot); | ||
| }); | ||
| }); |
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.
@chrismaree @mrice32 should
crossDomainAdminbe in theSpokePoolor do you think this notion of a cross domain admin will be different for each. L2?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.
What is the cross-domain admin? The optimism bridge address on L2? Or is it the BridgeAdmin address on L1?
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.
Its the contract that calls
relayMessageon the canonical bridge, so its either theBridgeAdminor theHubPoolThere 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.
for now
HubPoolme thinks. we might want to change this later but I think lets leave it there optimistically at the moment.