Adds "real" L1 and L2 message passing contracts.#236
Conversation
karlfloersch
left a comment
There was a problem hiding this comment.
Alright this LG(reat)TM! Left one comment on standardizing the L1 / L2 interfaces & how this can allow us to keep the old mock-ed contracts based on the assumption that some of the inner workings of replay & verification happen in the background and can be unit-tested elsewhere / don't need to be in the mocks.
packages/contracts/contracts/optimistic-ethereum/ovm/precompiles/MockL2ToL1MessagePasser.sol
Show resolved
Hide resolved
| */ | ||
| function sendMessage( | ||
| address _target, | ||
| bytes memory _message |
There was a problem hiding this comment.
As we discussed in person, I think it makes sense to add a gasLimit to this function and this way we can actually keep the old mocks. As long as we assume that the "relay message" on L1 & the "replay" functionality is handled in some standardized way, then developers building for L1 / L2 shouldn't have to unit test that functionality. It should just "happen" in the background.
There was a problem hiding this comment.
Hmmm, interesting. After the Synthetix call today, I feel like having the full interface is probably useful for developers. We can always add a wrapper that ignores these extra parameters later if it seems to be useful.
| /** | ||
| * @title MockL1CrossDomainMessenger | ||
| */ | ||
| contract MockL1CrossDomainMessenger is L1CrossDomainMessenger { |
There was a problem hiding this comment.
So is it possible to just have a MockCrossDomainMessenger like before and not have them inherit from the more complete L1 / L2 messengers?
Basically we don't mock whole async 'relay' functionality in the mock version of the tests because that is all handled in the background for users / dapps
There was a problem hiding this comment.
So these mocks bypass the proof checks within the complete contracts. It keeps the interface the same and does minimal verification so that developers can sanity check for common cases (e.g., attempting to send the same message twice). I feel like it's a good middle ground for now.
There was a problem hiding this comment.
so that developers can sanity check for common cases (e.g., attempting to send the same message twice).
Do you think that developers should write unit tests which call replayMessage(...) ? My guess would be that this is not really required in the same way that developers normally don't add tests for users who send a tx with under-priced gas & then use a replace tx with higher gas price.
Generally down to keep this interface but I just want to make sure that we're on the same page wrt what makes sense to put in unit tests
There was a problem hiding this comment.
To me the biggest thing missing in our mocks is a helper function which you can import called waitForCrossDomainMessage(txHash) which can be used something like this:
// Do something which sends a message to L2
const txHash = await myL1Contract.sendDepositToL2()
// Wait for the L1->L2 message to be processed
await waitForCrossDomainMessage(txHash)
// Do another thing on L2 like spend the deposit
await myL2Contract.spendMyMoney()
This way we can (in both L1->L2 direction & L2->L1 direction) just wait for the transaction to be "mined" / applied. And then when we build the "real" version we add a little util which is used for waiting for the same behavior.
I still am into to keeping the L1XDomainMessenger & L2XDomainMessenger separate--that way we can figure out a better way to mock out the fact that L2->L1 messages take a week. But other than that I feel like this is a solid abstraction.
karlfloersch
left a comment
There was a problem hiding this comment.
I want to get this merged but there are a few updates we need to make:
- Update the MockL1/MockL2CrossDomainMessengers to use the same
sendMessage(...)interface which includesgasLimitfor the L2CrossDomainMessenger. - Add a
waitForCrossDomainMessage(txHash)function which can be used in the mock. This is a standin for what we will need when we implement these unit tests against the real backend. (described here) - Update https://github.com/ethereum-optimism/Buidler-Deposit-Withdraw-Example to reflect these changes.
Once the mocks are set up this should be good for a merge!
karlfloersch
left a comment
There was a problem hiding this comment.
Dope this looks great! I remember you said that you updated the deposit / withdrawal example & it worked - is that pushed somewhere I can take a look at?
Anyway, hype! Excited to get this merged
| ); | ||
|
|
||
| return EthMerkleTrie.proveAccountStorageSlotValue( | ||
| 0x4200000000000000000000000000000000000000, |
There was a problem hiding this comment.
This is a ovm specific precompile address? Maybe a good idea to turn it into a constant so its not a magic number
| /** | ||
| * @title MockL1CrossDomainMessenger | ||
| */ | ||
| contract MockL1CrossDomainMessenger is BaseMockCrossDomainMessenger, L1CrossDomainMessenger { |
There was a problem hiding this comment.
this contract misses the constructor call for the super class
| /** | ||
| * @title MockL2CrossDomainMessenger | ||
| */ | ||
| contract MockL2CrossDomainMessenger is BaseMockCrossDomainMessenger, L2CrossDomainMessenger { |
There was a problem hiding this comment.
ditto. This contract misses the super constructor call as well
* migration: Update to UnreleasedTreasury contract * Update op-chain-ops/cmd/celo-migrate/state.go Co-authored-by: Karl Bartel <karl@karl.berlin> * Update op-chain-ops/cmd/celo-migrate/state.go Co-authored-by: Karl Bartel <karl@karl.berlin> --------- Co-authored-by: Karl Bartel <karl@karl.berlin>
* migration: Update to UnreleasedTreasury contract * Update op-chain-ops/cmd/celo-migrate/state.go Co-authored-by: Karl Bartel <karl@karl.berlin> * Update op-chain-ops/cmd/celo-migrate/state.go Co-authored-by: Karl Bartel <karl@karl.berlin> --------- Co-authored-by: Karl Bartel <karl@karl.berlin>
* feat(ci): Dependabot config ## Overview Introduces jonhoo's dependabot config, which notifies on outdated github actions as well as outdated crate dependencies. * chore: Bump dependencies ## Overview Bumps dependencies in the workspace, before dependabot gets to them.
This PR separates storage and account cursors which makes sense because these two cursors may have very different seek/next implementations. In the case of in-memory, a single cursor impl can still handle both, but for MDBX, it makes sense to implement separate cursors. Closes #236 --------- Co-authored-by: Arun Dhyani <dhyaniarun7@gmail.com>
Description
Helloooo. This PR adds the "real"
L1CrossDomainMessengerandL2CrossDomainMessengercontracts, along with interfaces and mocks for both. We'll have to make sure to updateoptimism-devexto account for these changes.I think I've hit all of the necessary logic here (including checking inclusion proofs on the L1 side). Only thing left is to check that the state batch header timestamp is sufficiently old (requires a larger set of changes to various contracts).
Metadata
Fixes
Contributing Agreement