feat: interop access list#14873
Conversation
* feat: access list poc * chore: remove is deposit tests * fix: natspec and enhance comments * chore: remove l1 block interop on solidity files * fix: pr fixes * chore: remove go logic over l1 block interop * chore: remove some l1 block interop stuff on solidity side * chore: remove from frozen files * chore: remove missing l1 block interop stuff on go tests * refactor: improve natspec and comments * feat: add is warm tests * feat: wip trying to integrate with common test * fix: integrate with common test reading artifact json * fix: pre-pr and tests * fix: tests * fix: tests Co-authored-by: 0xng <ng@defi.sucks> Co-authored-by: skeletor-spaceman <skeletor@defi.sucks> * fix: pr review comments * fix: op e2e inbox interface --------- Co-authored-by: agusduha <agusnduha@gmail.com> Co-authored-by: 0xng <ng@defi.sucks> Co-authored-by: skeletor-spaceman <skeletor@defi.sucks>
|
/ci authorize 3ac6e5d |
clabby
left a comment
clabby
left a comment
There was a problem hiding this comment.
The removal of the L1Block logic in solidity + Go looks good. Couple comments on the contract, but nothing super important.
Glad to approve in a bit 👍
There was a problem hiding this comment.
Small pref for readability:
| bytes32 internal constant _MSB_MASK = 0x00ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff; | |
| bytes32 internal constant _MSB_MASK = bytes32(~uint256(0xff << 248));; |
There was a problem hiding this comment.
^ same
| bytes32 internal constant _TYPE_3_MASK = 0x0300000000000000000000000000000000000000000000000000000000000000; | |
| bytes32 internal constant _TYPE_3_MASK = bytes32(uint256(0x03 << 248)); |
There was a problem hiding this comment.
You can get away with shifting these values on the stack rather than having to touch memory w/ abi.encodePacked
bytes32 idPacked = bytes32(uint256(uint256(_id.blockNumber) << 128 | uint256(_id.timestamp) << 64 | uint256(_id.logIndex)) & type(uint192).max);
There was a problem hiding this comment.
we wanted to make the "most traditional" solidity way possible. we can add a gas-optimized yul version later on.
| /// @param _id The identifier of the message. | ||
| /// @param _msgHash The hash of the message. | ||
| /// @return checksum_ The checksum of the message. | ||
| function _calculateChecksum(Identifier memory _id, bytes32 _msgHash) internal pure returns (bytes32 checksum_) { |
There was a problem hiding this comment.
For all of the 64-byte hashes in this function, it'd be a bit more efficient to just use [0x00, 0x40) rather than having to allocate net-new memory w/ abi.encodePacked. Not blocking though if you consider the readability hit concerning.
There was a problem hiding this comment.
Original spec PR had yul to do exactly that, but readability of abi.encodePacked won over that.
There was a problem hiding this comment.
Is this just to reduce ABI encoded size? It is an ABI breaking change to the contract.
protolambda
left a comment
protolambda
left a comment
There was a problem hiding this comment.
LGTM, but I think this is still blocked on the Identifier ABI decision
|
Follows on the internal PR #14883 |
6 participants
Description
Introducing a new validation mechanism for cross-chain message execution using transaction access-lists. The
CrossL2Inboxcontract will implement gas introspection to validateExecutingMessagetransactions. Each cross-chain message must be pre-declared in the transaction's access list.Tests
We have added full unit test coverage for the functionality. However, we still need to define a proper way to test the access list behavior and gather more samples to thoroughly test the client's on-chain checksum calculation equivalence. This topic remains open for discussion.
Additional context
Design doc: ethereum-optimism/design-docs#214
Specs: ethereum-optimism/specs#612
Co-authored-by: @skeletor-spaceman @0xng @0xDiscotech