feat: decoder return l1<->l2 msgs#597
Conversation
| * @return sha256(header[0x4: 0x23c], diffRoot, l1Tol2MessagesHash) | ||
| */ | ||
| function _computePublicInputsHash(bytes calldata _l2Block) internal pure returns (bytes32) { | ||
| function _computePublicInputsHash(bytes calldata _l2Block) |
There was a problem hiding this comment.
The name of this doesnt make sense anymore if its also returning the message arrays. Comment natspec also needs updated
| function _computePublicInputsHash(bytes calldata _l2Block) | ||
| internal | ||
| pure | ||
| returns (bytes32, bytes32[] memory l2ToL1Msgs, bytes32[] memory l1ToL2Msgs) |
There was a problem hiding this comment.
some named return values while others arent
| } | ||
|
|
||
| return bytes32(uint256(sha256(temp)) % P); | ||
| return (bytes32(uint256(sha256(temp)) % P), l2ToL1Msgs, l1ToL2Msgs); |
There was a problem hiding this comment.
may as well make the mod p variable named on its own then you can spare the return statement. Having a calc in the return statement while others are pre calculated is a bit messy can we be consistent?
| offsets.l1ToL2MessagesOffset = offsets.contractDataOffset + 0x4 + lengths.contractCount * 0x34; | ||
|
|
||
| // load the l2 to l1 msgs | ||
| assembly { |
There was a problem hiding this comment.
Can this array copy be moved down to where the other one is performed?
There was a problem hiding this comment.
I'm using that we just computer the offset values. We will be updating them every loop so was a nice way to have it right away.
| internal | ||
| pure | ||
| returns (bytes32, bytes32) | ||
| returns (bytes32, bytes32[] memory, bytes32[] memory, bytes32) |
There was a problem hiding this comment.
| returns (bytes32, bytes32[] memory, bytes32[] memory, bytes32) | |
| returns (bytes32, bytes32, bytes32[] memory, bytes32[] memory) |
id probably prefer the hashes to be together and the arrays together
|
|
||
| for (uint256 i = 0; i < l2ToL1Msgs.length; i++) { | ||
| assertEq( | ||
| l2ToL1Msgs[i], bytes32(uint256(0x300 + 32 * (1 + i / 2) + i % 2)), "Invalid l2ToL1Msgs" |
There was a problem hiding this comment.
can you lift the value calc up a line so its a bit more readable
(ps. bruh we gotta automate test generation)
There was a problem hiding this comment.
Will be adding the test in the integration test for L1 publisher when adding in rollup, then tested in there for the automatic.
For our use, even if I like foundry more, TS seems like a better fit to have the tests because we will have so much encoding and stuff to deal with 😬 I only really see our foundry tests right now as useful for better debugging when building and for checking stuff without all the encoding/decoding.
| } | ||
|
|
||
| return bytes32(uint256(sha256(temp)) % P); | ||
| publicInputHash = bytes32(uint256(sha256(temp)) % P); |
There was a problem hiding this comment.
sorry to be annoying again but this asm can be in its own function with the diffRoot and msgHash passed in
| offsets.l1ToL2MessagesOffset = offsets.contractDataOffset + 0x4 + lengths.contractCount * 0x34; | ||
|
|
||
| // load the l2 to l1 msgs | ||
| assembly { |
rahul-kothari
left a comment
rahul-kothari
left a comment
There was a problem hiding this comment.
overall LGTM - 1 nit on naming
| bytes32[] memory l1ToL2Messages; | ||
| bytes32 messagesHash; | ||
| { | ||
| uint256 messagesHashPreimageSize = 0x20 * L1_TO_L2_MESSAGES_PER_ROLLUP; |
There was a problem hiding this comment.
nit: Should messagesHashPreimageSize be renamed to L1ToL2MesssagesHashPreimageSize for consistency?
Same for messagesHash. It is unclear to me if it is for L1toL2 or L2toL1 or does it also include other kind of messages. In the natspec, you call this L1ToL2MessagesHash
3 participants
Description
Useful for #523.
The decoder will now return a list of l1 -> l2 and l2 -> l1 messages to the rollup. These can be used for batch insertsion into inbox and outboxes.
Notice that this will be returning zero values for empty messages, so the batch insertion should be altered to skip when "empty" messages are received. This is safe under the assumption that no-one have a preimage that hashes to 0.
Checklist: