[WIP] YAS 226 L2 to L1 Message Passer#42
Conversation
willmeister
left a comment
There was a problem hiding this comment.
Looks good! 👍
Left a comment re: general yul call consistency with ExecutionManager.sol, including revert pass-through.
As for the questions:
-
I like the idea of passing in the
l2ToL1MessagePasserOvmAddressin the constructor so as to give us more flexibility in the future. If it doesn't need to be immutable, why not make it configurable? Definitely open to reconsider if there is a good reason. -
I'm thinking it should probably not be in the state tree unless there's a good reason. It seems like a contract we'll always need a handle on, not one that we can and should generically invoke. I don't have a very strong opinion on this, though.
| // bitwise right shift 28 * 8 bits so the 4 method ID bytes are in the right-most bytes | ||
| bytes32 methodId = keccak256("ovmCALLER()") >> 224; | ||
| address addr = executionManagerAddress; | ||
|
|
||
| address theCaller; | ||
| assembly { | ||
| let callBytes := mload(0x40) | ||
| calldatacopy(callBytes, 0, calldatasize) | ||
|
|
||
| // replace the first 4 bytes with the right methodID | ||
| mstore8(callBytes, shr(24, methodId)) | ||
| mstore8(add(callBytes, 1), shr(16, methodId)) | ||
| mstore8(add(callBytes, 2), shr(8, methodId)) | ||
| mstore8(add(callBytes, 3), methodId) | ||
|
|
||
| // overwrite call params | ||
| let result := mload(0x40) | ||
| let success := call(gas, addr, 0, callBytes, calldatasize, result, 500000) | ||
|
|
||
| if eq(success, 0) { | ||
| revert(0, 0) | ||
| } |
There was a problem hiding this comment.
This looks good, but a few suggestions:
- No need to shift or mstore8 the methodId since the call doesn't take any params. Just pass the first 4 bytes
- Can get rid of the result and arbitrary return data size in favor of
returndatacopyandreturndatasize - Can add revert reason if it does revert
There was a problem hiding this comment.
| // bitwise right shift 28 * 8 bits so the 4 method ID bytes are in the right-most bytes | |
| bytes32 methodId = keccak256("ovmCALLER()") >> 224; | |
| address addr = executionManagerAddress; | |
| address theCaller; | |
| assembly { | |
| let callBytes := mload(0x40) | |
| calldatacopy(callBytes, 0, calldatasize) | |
| // replace the first 4 bytes with the right methodID | |
| mstore8(callBytes, shr(24, methodId)) | |
| mstore8(add(callBytes, 1), shr(16, methodId)) | |
| mstore8(add(callBytes, 2), shr(8, methodId)) | |
| mstore8(add(callBytes, 3), methodId) | |
| // overwrite call params | |
| let result := mload(0x40) | |
| let success := call(gas, addr, 0, callBytes, calldatasize, result, 500000) | |
| if eq(success, 0) { | |
| revert(0, 0) | |
| } | |
| bytes32 methodId = keccak256("ovmCALLER()"); | |
| address addr = executionManagerAddress; | |
| address theCaller; | |
| assembly { | |
| let callBytes := mload(0x40) | |
| mstore(callBytes, methodId) | |
| let success := call(gas, addr, 0, callBytes, 4, 0, 0) | |
| let result := callBytes | |
| returndatacopy(result, 0, returndatasize) | |
| if eq(success, 0) { | |
| revert(result, returndatasize) | |
| } | |
| theCaller = mload(result) |
Hmm the question of whether to make the precompile address hardcoded vs configurable is a tricky one. The reason is because this very well might not be our only precompile. If we assume that we end up making a number of precompiles then it would be nice to have a somewhat robust system for setting them all up. Feel like this requires some more thought, and it also might be clearer once we continue building out the system. Personally I'm down for either hardcoded or in the constructor at this point & probably favor minimal work because whatever we do now will likely have to change down the line.
Agreed with @willmeister that it probably shouldn't! But definitely can cross that bridge when we come to it. |
Fixes Spearbit issue 42, saves gas by removing extra SLOADs.
Fixes to README and build scripts
--------- Co-authored-by: KonradStaniec <konrad.staniec@gmail.com>
* feat: System config update event parsing * clean up enum
Description
Covers YAS 226, L2 to L1 message passer.
Questions
ovmL2ToL1MessagePasserAddressis hardcoded, right now to a joke address, within the EM, and then stored again inconstants.tsfor off-chain handling.L2ToL1MessagePassershow up in the state tree the. same as another contract?Metadata
Fixes
Blockers
Contributing Agreement