Fix/message encoding #429
Fix/message encoding #429
Conversation
✅ Deploy Preview for veascan ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughInbox APIs remove the separate function selector, accepting full calldata and including msg.sender in leaf encoding. Outboxes add per-receiver allowlists, require an explicit _from in sendMessage, include _from in Merkle leaves/hashes, and expose setAllowlist. Interfaces, mocks, utils, and integration tests updated to match. Changes
Sequence Diagram(s)sequenceDiagram
participant L2Sender as L2 Sender (contract)
participant Inbox as Inbox (L2)
participant Bridge as Bridge / Snapshot
participant Outbox as Outbox (L1/L2)
participant Receiver as Receiver (L1/L2)
L2Sender->>Inbox: sendMessage(to, data) (data includes selector)
Inbox->>Inbox: leaf = keccak256(oldCount, to, msg.sender, data)
Inbox-->>Bridge: publish snapshot / batch root
Bridge-->>Outbox: proof + batch root
Outbox->>Outbox: verify proof for (msgId, to, from, message)
Outbox->>Outbox: require allowlist[to][from] || allowlist[to][0x0]
Outbox->>Receiver: call(message) (relayed as from)
Receiver-->>Outbox: return / revert
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
contracts/src/arbitrumToEth/VeaOutboxArbToEth.sol (1)
496-500: Critical: assignment typo prevents claimHashes updateIn withdrawChallengerEscapeHatch, this line uses equality (==) instead of assignment (=), leaving claimHashes unchanged:
- claimHashes[_epoch] == hashClaim(_claim); + claimHashes[_epoch] = hashClaim(_claim);This blocks correctly evolving the claim state in the escape-hatch path.
🧹 Nitpick comments (16)
contracts/src/test/gateways/IReceiverGatewayMock.sol (1)
17-18: Array entrypoint looks good; consider naming consistencyConsider naming the method receiveMessageBatch for consistency with common batch terminology across gateways.
contracts/src/interfaces/outboxes/IVeaOutboxOnL1.sol (1)
20-20: Doc nit: address(0) spellingUse address(0) (lowercase) consistently in docs.
contracts/src/utils/veaInboxTouch.sol (1)
23-23: Clarify intent and drop payable (not used)
- This utility intentionally sends opaque bytes (no selector) to address(0) purely to advance the inbox and snapshot. Add a note to avoid confusion with IVeaInbox docs that mention full calldata.
- payable is unused; remove to prevent accidental ETH sends.
-contract VeaInboxTouch { +contract VeaInboxTouch { @@ - function touch(uint256 random) external payable { - veaInbox.sendMessage(0x0000000000000000000000000000000000000000, abi.encode(random)); + /// @dev Sends an opaque, non-executable payload to address(0) to cheaply advance the inbox before snapshotting. + function touch(uint256 random) external { + // Not intended for execution; selector omitted on purpose. + veaInbox.sendMessage(address(0), abi.encode(random)); veaInbox.saveSnapshot(); }Add a brief comment above sendMessage explaining that this message will never be relayed/executed.
contracts/src/test/gateways/SenderGatewayMock.sol (2)
27-29: Prefer abi.encodeCall for type-safety and readabilityabi.encodeCall ensures the selector and argument types match the target signature at compile time.
- bytes4 methodSelector = IReceiverGatewayMock.receiveMessage.selector; - bytes memory data = abi.encodeWithSelector(methodSelector, _data); - veaInbox.sendMessage(receiverGateway, data); + bytes memory data = abi.encodeCall(IReceiverGatewayMock.receiveMessage, (_data)); + veaInbox.sendMessage(receiverGateway, data);
31-35: Same here: use abi.encodeCall for the array pathConsistent with the single-item path, improves safety and clarity.
- bytes4 methodSelector = IReceiverGatewayMock.receiveMessageArray.selector; - bytes memory data = abi.encodeWithSelector(methodSelector, _data); - veaInbox.sendMessage(receiverGateway, data); + bytes memory data = abi.encodeCall(IReceiverGatewayMock.receiveMessageArray, (_data)); + veaInbox.sendMessage(receiverGateway, data);contracts/src/interfaces/inboxes/IVeaInbox.sol (1)
15-15: Doc: emphasize “full calldata” and show abi.encodeCall exampleTo reduce ambiguity, state that _data must be the full calldata (including the function selector). Consider documenting abi.encodeCall usage as the preferred pattern.
contracts/src/arbitrumToEth/VeaInboxArbToEth.sol (4)
73-76: Docs and signature aligned with full-calldata patternComment now references abi.encodeWithSelector; signature matches IVeaInbox. Consider adding “full calldata (including selector)” wording for clarity, mirroring the interface doc.
41-43: Update MessageSent event doc to reflect new nodeData shape (includes sender)nodeData now encodes oldCount, _to, msg.sender, _data. The event doc still mentions only msgId, to, message.
- /// @param _nodeData The data to create leaves in the merkle tree. abi.encodePacked(msgId, to, message), outbox relays to.call(message). + /// @param _nodeData The data to create leaves in the merkle tree: abi.encodePacked(msgId, to, from, message). + /// The Outbox relays to.call(message) after authenticating that 'from' is allowlisted.Also applies to: 82-83
75-75: Future optimization after interface switch to calldataIf IVeaInbox is updated to bytes calldata, mirror it here for gas savings:
- function sendMessage(address _to, bytes memory _data) external override returns (uint64) { + function sendMessage(address _to, bytes calldata _data) external override returns (uint64) {Ensure all chain-specific inboxes adopt the same change to keep interfaces consistent.
78-80: Minor grammar nit in comment“Atleast” → “at least”.
- // Given arbitrum's speed limit of 7 million gas / second, it would take atleast 8 million years of full blocks to overflow. + // Given Arbitrum's speed limit of 7 million gas/second, it would take at least 8 million years of full blocks to overflow.contracts/src/gnosisToArbitrum/VeaInboxGnosisToArb.sol (1)
69-76: Fix docs: direction and calldata wording are outdated
- Line 69 says “Sends an arbitrary message to Gnosis” but this inbox sends from Gnosis to Arbitrum. Replace “to Gnosis” with “to Arbitrum”.
- Line 74: Replace “abi.encodeWithSelector(fnSelector, …)” with “abi.encodeWithSelector(selector, …)” and clarify that _data is the full calldata (including selector) the outbox will call with.
Example:
-/// @dev Sends an arbitrary message to Gnosis. +/// @dev Sends an arbitrary message to Arbitrum. ... -/// @param _data The message calldata, abi.encodeWithSelector(fnSelector, param1, param2, ...) +/// @param _data The full calldata to relay (e.g., abi.encodeWithSelector(selector, arg1, arg2, ...)).contracts/src/interfaces/outboxes/IVeaOutboxOnL2.sol (1)
34-39: Consider an AllowlistUpdated event for observabilityRecommend adding an event to track allowlist changes (e.g., AllowlistUpdated(address indexed receiver, address indexed from, bool allowed)) so off-chain components can react to ACL changes.
contracts/src/arbitrumToGnosis/VeaInboxArbToGnosis.sol (1)
73-76: Polish _data param docs to reflect the new APIClarify that _data is the full calldata (including the function selector). Replace “fnSelector” wording.
-/// @param _data The message calldata, abi.encodeWithSelector(fnSelector, param1, param2, ...) +/// @param _data The full calldata to relay (e.g., abi.encodeWithSelector(selector, arg1, arg2, ...)).contracts/test/integration/ArbToEth.ts (2)
354-356: Use standard revert assertion (drop “itself”)Prefer the canonical form for revert assertions.
- await expect(veaOutbox.connect(relayer).sendMessage(proof, nonce, to, from, msgData)).itself.be.revertedWith( + await expect(veaOutbox.connect(relayer).sendMessage(proof, nonce, to, from, msgData)).to.be.revertedWith( "Message sender not allowed to call receiver." );
261-265: Deduplicate node decoding by reusing decodeMessage(msg2)You already added decodeMessage; use it instead of manual slicing for msg2 to reduce duplication and errors.
Example:
const { nonce: nonce2, to: to2, from: from2, msgData: msgData2 } = await decodeMessage(msg2);Also applies to: 323-327, 378-382, 441-445
contracts/src/arbitrumToEth/VeaOutboxArbToEth.sol (1)
44-45: Allowlist + sender-aware relay: solid design
- Per-receiver allowlist with wildcard (address(0)) is simple and effective.
- sendMessage now binds the proof to (_msgId, _to, _from, _message), which aligns with inbox leaf encoding.
- Gatekeeping before proof verification avoids unnecessary hash work on unauthorized calls.
Consider emitting an event on allowlist updates for monitoring:
event AllowlistUpdated(address indexed receiver, address indexed from, bool allowed);and emit in setAllowlist.
Also applies to: 355-362, 367-375, 377-381
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
contracts/src/arbitrumToEth/VeaInboxArbToEth.sol(2 hunks)contracts/src/arbitrumToEth/VeaOutboxArbToEth.sol(3 hunks)contracts/src/arbitrumToGnosis/VeaInboxArbToGnosis.sol(2 hunks)contracts/src/arbitrumToGnosis/VeaOutboxArbToGnosis.sol(3 hunks)contracts/src/gnosisToArbitrum/VeaInboxGnosisToArb.sol(1 hunks)contracts/src/gnosisToArbitrum/VeaOutboxGnosisToArb.sol(2 hunks)contracts/src/interfaces/inboxes/IVeaInbox.sol(1 hunks)contracts/src/interfaces/outboxes/IVeaOutboxOnL1.sol(1 hunks)contracts/src/interfaces/outboxes/IVeaOutboxOnL2.sol(1 hunks)contracts/src/test/gateways/IReceiverGatewayMock.sol(1 hunks)contracts/src/test/gateways/ReceiverGatewayMock.sol(2 hunks)contracts/src/test/gateways/SenderGatewayMock.sol(1 hunks)contracts/src/utils/veaInboxTouch.sol(1 hunks)contracts/test/integration/ArbToEth.ts(12 hunks)contracts/test/integration/ArbToGnosis.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2024-12-09T09:40:28.400Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/transactionHandler.ts:13-13
Timestamp: 2024-12-09T09:40:28.400Z
Learning: In `bridger-cli/src/utils/transactionHandler.ts`, the `veaOutbox` implementations differ for each chain, but a common interface should be defined for type checks to enhance type safety.
Applied to files:
contracts/src/interfaces/outboxes/IVeaOutboxOnL1.solcontracts/test/integration/ArbToEth.ts
📚 Learning: 2024-12-16T09:22:57.434Z
Learnt from: mani99brar
PR: kleros/vea#383
File: contracts/test/integration/ArbToGnosis.ts:114-114
Timestamp: 2024-12-16T09:22:57.434Z
Learning: In the local Hardhat deployment for testing, `VeaInboxArbToGnosisMock` is deployed with the identifier name `VeaInboxArbToGnosis`, so it's appropriate to retrieve it using `getContract("VeaInboxArbToGnosis")` and cast it to `VeaInboxArbToGnosisMock` in `ArbToGnosis.ts`.
Applied to files:
contracts/test/integration/ArbToGnosis.tscontracts/test/integration/ArbToEth.ts
📚 Learning: 2024-12-02T10:16:56.825Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:829-840
Timestamp: 2024-12-02T10:16:56.825Z
Learning: In the `validator-cli/src/ArbToEth/watcherArbToGnosis.ts` file, avoid adding redundant error handling in functions like `reconstructChallengeProgress` when error handling is already adequately managed.
Applied to files:
contracts/test/integration/ArbToEth.ts
📚 Learning: 2024-11-26T08:37:47.591Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:829-1112
Timestamp: 2024-11-26T08:37:47.591Z
Learning: Refactoring the `reconstructChallengeProgress` function in `validator-cli/src/ArbToEth/watcherArbToGnosis.ts` into smaller functions is considered unnecessary abstraction.
Applied to files:
contracts/test/integration/ArbToEth.ts
📚 Learning: 2024-11-20T10:49:25.392Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts:93-98
Timestamp: 2024-11-20T10:49:25.392Z
Learning: In `contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts`, when resurrecting testnets, it's acceptable to use `getContractAddress()` for contract address prediction.
Applied to files:
contracts/test/integration/ArbToEth.ts
📚 Learning: 2025-06-05T12:17:21.782Z
Learnt from: mani99brar
PR: kleros/vea#424
File: validator-cli/src/helpers/claimer.ts:72-76
Timestamp: 2025-06-05T12:17:21.782Z
Learning: In validator-cli/src/helpers/claimer.ts, the outboxStateRoot captured at the beginning of the checkAndClaim function won't change for the epoch being claimed, so there's no race condition concern when reusing it in makeClaim/makeClaimDevnet functions.
Applied to files:
contracts/test/integration/ArbToEth.ts
📚 Learning: 2024-12-09T09:42:34.067Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/transactionHandler.ts:64-77
Timestamp: 2024-12-09T09:42:34.067Z
Learning: In the `TransactionHandler` class (`bridger-cli/src/utils/transactionHandler.ts`), it's acceptable to let methods like `makeClaim` fail without additional error handling.
Applied to files:
contracts/test/integration/ArbToEth.ts
📚 Learning: 2024-12-09T10:54:09.943Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/claim.ts:42-51
Timestamp: 2024-12-09T10:54:09.943Z
Learning: In the `bridger-cli/src/utils/claim.ts` file, within the `fetchClaim` function, explicit error handling for provider calls and event queries is not required; errors should be allowed to propagate naturally.
Applied to files:
contracts/test/integration/ArbToEth.ts
📚 Learning: 2024-12-09T09:42:51.590Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/transactionHandler.ts:79-101
Timestamp: 2024-12-09T09:42:51.590Z
Learning: In the `bridger-cli/src/utils/transactionHandler.ts` file, within the `TransactionHandler` class, it's acceptable to let methods like `startVerification` fail without additional error handling.
Applied to files:
contracts/test/integration/ArbToEth.ts
📚 Learning: 2024-12-11T08:52:17.062Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/epochHandler.ts:13-13
Timestamp: 2024-12-11T08:52:17.062Z
Learning: In `bridger-cli/src/utils/epochHandler.ts`, the `veaOutbox` parameter is intentionally typed as `any` because the script is designed to be agnostic for multiple contracts.
Applied to files:
contracts/test/integration/ArbToEth.ts
🧬 Code Graph Analysis (2)
contracts/test/integration/ArbToGnosis.ts (2)
contracts/test/integration/ArbToEth.ts (1)
decodeMessage(1322-1328)contracts/test/merkle/MerkleTree.ts (1)
MerkleTree(9-248)
contracts/test/integration/ArbToEth.ts (1)
contracts/test/merkle/MerkleTree.ts (1)
MerkleTree(9-248)
🪛 Biome (2.1.2)
contracts/test/integration/ArbToEth.ts
[error] 1321-1328: Do not export from a test file.
(lint/suspicious/noExportsInTest)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: dependency-review
- GitHub Check: test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (22)
contracts/src/test/gateways/IReceiverGatewayMock.sol (1)
15-16: receiveMessage signature alignment validatedIReceiverGateway and ReceiverGatewayMock both declare
function receiveMessage(uint256 _data) external;ensuring they share the exact signature and there are no inheritance conflicts.
contracts/src/arbitrumToEth/VeaInboxArbToEth.sol (2)
3-3: Author metadata updateLooks fine.
82-82: Outbox leaf hashing and allowlist enforcement align with inbox encodingThe outbox in contracts/src/arbitrumToEth/VeaOutboxArbToEth.sol:
- Reconstructs the node hash with
keccak256(abi.encodePacked(_msgId, _to, _from, _message))
(line 380), matching the inbox’s
abi.encodePacked(oldCount, _to, msg.sender, _data).- Enforces that the captured sender (
_from) is allowed via
allowlist[_to][_from] || allowlist[_to][address(0)]
immediately before relay.No changes needed here.
contracts/src/gnosisToArbitrum/VeaInboxGnosisToArb.sol (1)
83-87: Leaf encoding LGTM; includes sender for allowlist/authabi.encodePacked(oldCount, _to, msg.sender, _data) aligns with the new outbox hashing that includes _from. Double-hashing mitigates second preimage. No ambiguity because bytes payload is last.
contracts/src/interfaces/outboxes/IVeaOutboxOnL2.sol (1)
20-26: API update matches new leaf encoding and relay flowAdding _from to sendMessage is consistent with including the sender in the leaf and with allowlist authentication at the receiver.
contracts/src/arbitrumToGnosis/VeaInboxArbToGnosis.sol (2)
3-3: Author update acknowledged
82-86: Leaf encoding LGTM; sender inclusion is correctIncluding msg.sender in nodeData matches new outbox verification and allowlist checks. Double-hash remains for preimage resistance.
contracts/src/arbitrumToGnosis/VeaOutboxArbToGnosis.sol (3)
45-45: LGTM! Well-structured allowlist mapping.The two-level mapping with address(0) as a wildcard is an efficient design for per-receiver access control.
299-304: LGTM! Proper receiver-controlled allowlist management.The function correctly allows receivers to manage their own allowlist entries.
306-323: LGTM! Secure implementation of sender authentication.The allowlist check correctly handles both specific and wildcard allowances, and the leaf hash properly includes the sender address for verification.
contracts/src/test/gateways/ReceiverGatewayMock.sol (5)
12-12: LGTM! Required import for allowlist functionality.
22-22: LGTM! Test data storage for array messages.
29-32: LGTM! Correct bridge-based authentication.
34-40: LGTM! Test helpers for allowlist configuration.The functions properly configure specific and wildcard allowlists for testing.
43-59: LGTM! Proper message handling for testing.The functions correctly handle both single and array messages with appropriate bridge authentication.
contracts/src/gnosisToArbitrum/VeaOutboxGnosisToArb.sol (1)
310-310: LGTM! Correct leaf hash computation.The leaf hash properly includes the sender address.
contracts/test/integration/ArbToGnosis.ts (6)
5-5: LGTM! Good code reuse.Importing the decodeMessage helper reduces code duplication.
250-312: LGTM! Comprehensive test for message relay with allowlist.The test properly validates the new allowlist mechanism and array message handling.
314-370: LGTM! Important negative test case.Validates that the allowlist is properly enforced.
372-434: LGTM! Tests wildcard allowlist functionality.Validates that address(0) wildcard properly allows all senders.
746-772: LGTM! Dispute resolution test properly updated.Test correctly handles allowlist setup for message relay after dispute resolution.
923-949: LGTM! Consistent test updates for dishonest claim scenario.
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (2)
contracts/test/integration/ArbToEth.ts (1)
861-869: Withdraw should be performed by the bridger signerWithout .connect(bridger), the call is sent by the default signer and may revert due to access control/intent.
- await veaOutbox.withdrawClaimDeposit(epoch, { + await veaOutbox.connect(bridger).withdrawClaimDeposit(epoch, { stateRoot: batchMerkleRoot, claimer: bridger.address, timestampClaimed: block.timestamp, timestampVerification: blockStartValidation.timestamp!, blocknumberVerification: startValidationTxn.blockNumber!, honest: 1, challenger: challenger.address, });contracts/test/integration/ArbToGnosis.ts (1)
382-386: Same issue: includefromfor dynamic array test- let nodes: string[] = []; - nodes.push(MerkleTree.makeLeafNode(nonce, to, msgData)); - const mt = new MerkleTree(nodes); - const proof = mt.getHexProof(nodes[0]); + let nodes: string[] = []; + nodes.push(MerkleTree.makeLeafNode(nonce, to, from, msgData)); + const mt = new MerkleTree(nodes); + const proof = mt.getHexProof(nodes[0]);Also applies to: 380-386
♻️ Duplicate comments (1)
contracts/src/interfaces/outboxes/IVeaOutboxOnL1.sol (1)
37-41: Document permissionless self-service semantics; clarify address(0) wildcard; fix capitalizationPer the intended design, receivers (msg.sender) manage their own allowlists. Make this explicit and correct the address(0) notation.
- /// @dev Sets the allowlist for the sender gateway. - /// Note: Address(0) is used to allow all addresses. - /// @param _from The address to allow or disallow - /// @param _allow Whether to allow or disallow the address. + /// @dev Sets the per-receiver allowlist of permitted L2 senders for msg.sender (self-service). + /// Access: permissionless — this only affects msg.sender's own allowlist. + /// Note: address(0) acts as a wildcard to allow all senders; disabling it reverts to per-address checks. + /// @param _from The L2 sender to allow or disallow (use address(0) for wildcard). + /// @param _allow Whether to allow or disallow the address. function setAllowlist(address _from, bool _allow) external;Optional (nice-to-have): consider a view helper like isAllowed(address receiver, address from) to aid off-chain consumers since this is event-less by design.
if an earlier review suggested event emission, intentionally skipping that per the recorded design choice (permissionless, no events).
🧹 Nitpick comments (9)
contracts/src/interfaces/outboxes/IVeaOutboxOnL1.sol (2)
20-28: Clarify_fromsemantics (allowlist + leaf verification) in NatSpecTighten the docs so implementers consistently include
_fromin the leaf/hash and in allowlist checks.- /// @param _from The address of the contract on L2 that sent the message, used for authentication. + /// @param _from The L2 sender address. Implementations MUST include `_from` in the leaf/hash verification and enforce the per-receiver allowlist (for `_to`) against `_from`.
35-35: Prefer calldata for Claim to avoid unnecessary copiesSmall gas/readability improvement for external interface parameters.
- function resolveDisputedClaim(uint256 _epoch, bytes32 _stateRoot, Claim memory _claim) external; + function resolveDisputedClaim(uint256 _epoch, bytes32 _stateRoot, Claim calldata _claim) external;contracts/src/arbitrumToEth/VeaInboxArbToEth.sol (2)
73-73: Doc nit: avoid referring to a removed parameter nameThe example references fnSelector even though the selector is now embedded in _data. Reword to avoid implying a separate param.
- /// @param _data The message calldata, abi.encodeWithSelector(fnSelector, param1, param2, ...) + /// @param _data The full message calldata. It must start with a function selector, e.g. + /// abi.encodeWithSelector(selector, param1, param2, ...)
82-82: Leaf change: include sender in nodeData (aligns with outbox allowlist)Encoding oldCount, _to, msg.sender, _data is sound. One follow-up: event docs still say abi.encodePacked(msgId, to, message) without mentioning from. Consider updating event docs in this file and the interface for clarity.
- /// @param _nodeData The data to create leaves in the merkle tree. abi.encodePacked(msgId, to, message), outbox relays to.call(message). + /// @param _nodeData The data to create leaves in the merkle tree: + /// abi.encodePacked(msgId, to, from, message). + /// The outbox relays with the provided message and may use `from` for allowlist checks.contracts/test/integration/ArbToEth.ts (2)
259-263: Use the same decode helper for msg2 to avoid manual slicingKeeps tests consistent and less error-prone.
- const msg2 = MessageSentEvent[1].args._nodeData; - - let nodes: string[] = []; - - const nonce2 = "0x" + msg2.slice(2, 18); - const to2 = "0x" + msg2.slice(18, 58); //18+40 - const from2 = "0x" + msg2.slice(58, 98); //58+40 - const msgData2 = "0x" + msg2.slice(98); + const msg2 = MessageSentEvent[1].args._nodeData; + let nodes: string[] = []; + const { nonce: nonce2, to: to2, from: from2, msgData: msgData2 } = await decodeMessage(msg2);Also applies to: 253-254
1306-1312: Helper can be non-async and typedMinor polish; removes unnecessary async and clarifies return shape.
-async function decodeMessage(msg: any) { +type DecodedMsg = { nonce: string; to: string; from: string; msgData: string }; +function decodeMessage(msg: string): DecodedMsg { const nonce = "0x" + msg.slice(2, 18); const to = "0x" + msg.slice(18, 58); //18+40 const from = "0x" + msg.slice(58, 98); //58+40 const msgData = "0x" + msg.slice(98); return { nonce, to, from, msgData }; }contracts/src/arbitrumToGnosis/VeaInboxArbToGnosis.sol (2)
73-73: Doc nit: remove reference to nonexistent parameter nameSame as ArbToEth inbox; keep the guidance but avoid implying a param.
- /// @param _data The message calldata, abi.encodeWithSelector(fnSelector, param1, param2, ...) + /// @param _data The full message calldata. It must start with a function selector, e.g. + /// abi.encodeWithSelector(selector, param1, param2, ...)
82-82: Leaf now includes sender; update event doc accordinglyEncoding abi.encodePacked(oldCount, _to, msg.sender, _data) is correct. Consider updating the MessageSent doc to include from like in the ArbToEth review.
- /// @param _nodeData The data to create leaves in the merkle tree. abi.encodePacked(msgId, to, message), outbox relays to.call(message). + /// @param _nodeData The data to create leaves in the merkle tree: + /// abi.encodePacked(msgId, to, from, message). + /// The outbox relays with the provided message and may use `from` for allowlist checks.contracts/test/integration/ArbToGnosis.ts (1)
1087-1093: Helper can be non-async and typedKeeps helpers consistent with ArbToEth and improves clarity.
-async function decodeMessage(msg: any) { +type DecodedMsg = { nonce: string; to: string; from: string; msgData: string }; +function decodeMessage(msg: string): DecodedMsg { const nonce = "0x" + msg.slice(2, 18); const to = "0x" + msg.slice(18, 58); //18+40 const from = "0x" + msg.slice(58, 98); //58+40 const msgData = "0x" + msg.slice(98); return { nonce, to, from, msgData }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
contracts/src/arbitrumToEth/VeaInboxArbToEth.sol(2 hunks)contracts/src/arbitrumToEth/VeaOutboxArbToEth.sol(3 hunks)contracts/src/arbitrumToGnosis/VeaInboxArbToGnosis.sol(2 hunks)contracts/src/arbitrumToGnosis/VeaOutboxArbToGnosis.sol(3 hunks)contracts/src/gnosisToArbitrum/VeaInboxGnosisToArb.sol(1 hunks)contracts/src/gnosisToArbitrum/VeaOutboxGnosisToArb.sol(2 hunks)contracts/src/interfaces/inboxes/IVeaInbox.sol(1 hunks)contracts/src/interfaces/outboxes/IVeaOutboxOnL1.sol(1 hunks)contracts/src/interfaces/outboxes/IVeaOutboxOnL2.sol(1 hunks)contracts/test/integration/ArbToEth.ts(29 hunks)contracts/test/integration/ArbToGnosis.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- contracts/src/gnosisToArbitrum/VeaInboxGnosisToArb.sol
- contracts/src/arbitrumToGnosis/VeaOutboxArbToGnosis.sol
- contracts/src/gnosisToArbitrum/VeaOutboxGnosisToArb.sol
- contracts/src/arbitrumToEth/VeaOutboxArbToEth.sol
- contracts/src/interfaces/outboxes/IVeaOutboxOnL2.sol
- contracts/src/interfaces/inboxes/IVeaInbox.sol
🧰 Additional context used
🧠 Learnings (14)
📚 Learning: 2024-12-16T09:22:57.434Z
Learnt from: mani99brar
PR: kleros/vea#383
File: contracts/test/integration/ArbToGnosis.ts:114-114
Timestamp: 2024-12-16T09:22:57.434Z
Learning: In the local Hardhat deployment for testing, `VeaInboxArbToGnosisMock` is deployed with the identifier name `VeaInboxArbToGnosis`, so it's appropriate to retrieve it using `getContract("VeaInboxArbToGnosis")` and cast it to `VeaInboxArbToGnosisMock` in `ArbToGnosis.ts`.
Applied to files:
contracts/test/integration/ArbToGnosis.tscontracts/test/integration/ArbToEth.ts
📚 Learning: 2025-08-11T09:26:45.958Z
Learnt from: mani99brar
PR: kleros/vea#429
File: contracts/src/interfaces/outboxes/IVeaOutboxOnL1.sol:0-0
Timestamp: 2025-08-11T09:26:45.958Z
Learning: In the Vea bridge contracts, the `setAllowlist` function is designed as a permissionless, self-service function where receivers (msg.sender) can control which sender addresses are allowed to relay messages to them. No access control or events are needed for this function as it's intended to be called by anyone to manage their own allowlist.
Applied to files:
contracts/test/integration/ArbToGnosis.tscontracts/src/interfaces/outboxes/IVeaOutboxOnL1.sol
📚 Learning: 2025-06-05T12:17:21.782Z
Learnt from: mani99brar
PR: kleros/vea#424
File: validator-cli/src/helpers/claimer.ts:72-76
Timestamp: 2025-06-05T12:17:21.782Z
Learning: In validator-cli/src/helpers/claimer.ts, the outboxStateRoot captured at the beginning of the checkAndClaim function won't change for the epoch being claimed, so there's no race condition concern when reusing it in makeClaim/makeClaimDevnet functions.
Applied to files:
contracts/test/integration/ArbToGnosis.tscontracts/test/integration/ArbToEth.ts
📚 Learning: 2024-12-02T10:16:56.825Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:829-840
Timestamp: 2024-12-02T10:16:56.825Z
Learning: In the `validator-cli/src/ArbToEth/watcherArbToGnosis.ts` file, avoid adding redundant error handling in functions like `reconstructChallengeProgress` when error handling is already adequately managed.
Applied to files:
contracts/test/integration/ArbToEth.ts
📚 Learning: 2024-11-27T04:18:05.872Z
Learnt from: mani99brar
PR: kleros/vea#344
File: validator-cli/src/ArbToEth/watcherArbToEth.ts:732-745
Timestamp: 2024-11-27T04:18:05.872Z
Learning: In `validator-cli/src/ArbToEth/watcherArbToEth.ts`, input validation inside the `hashClaim` function is unnecessary because the upper layer ensures valid input before calling it.
Applied to files:
contracts/test/integration/ArbToEth.ts
📚 Learning: 2024-11-26T08:37:47.591Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:829-1112
Timestamp: 2024-11-26T08:37:47.591Z
Learning: Refactoring the `reconstructChallengeProgress` function in `validator-cli/src/ArbToEth/watcherArbToGnosis.ts` into smaller functions is considered unnecessary abstraction.
Applied to files:
contracts/test/integration/ArbToEth.ts
📚 Learning: 2024-11-20T10:49:25.392Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts:93-98
Timestamp: 2024-11-20T10:49:25.392Z
Learning: In `contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts`, when resurrecting testnets, it's acceptable to use `getContractAddress()` for contract address prediction.
Applied to files:
contracts/test/integration/ArbToEth.ts
📚 Learning: 2024-12-11T08:52:23.697Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/epochHandler.ts:38-50
Timestamp: 2024-12-11T08:52:23.697Z
Learning: The `getBlockNumberFromEpoch` function in `bridger-cli/src/utils/epochHandler.ts` is currently unused and will be addressed in a future issue.
Applied to files:
contracts/test/integration/ArbToEth.ts
📚 Learning: 2024-12-09T09:40:28.400Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/transactionHandler.ts:13-13
Timestamp: 2024-12-09T09:40:28.400Z
Learning: In `bridger-cli/src/utils/transactionHandler.ts`, the `veaOutbox` implementations differ for each chain, but a common interface should be defined for type checks to enhance type safety.
Applied to files:
contracts/test/integration/ArbToEth.tscontracts/src/interfaces/outboxes/IVeaOutboxOnL1.sol
📚 Learning: 2024-12-09T09:42:34.067Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/transactionHandler.ts:64-77
Timestamp: 2024-12-09T09:42:34.067Z
Learning: In the `TransactionHandler` class (`bridger-cli/src/utils/transactionHandler.ts`), it's acceptable to let methods like `makeClaim` fail without additional error handling.
Applied to files:
contracts/test/integration/ArbToEth.ts
📚 Learning: 2024-12-09T10:54:09.943Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/claim.ts:42-51
Timestamp: 2024-12-09T10:54:09.943Z
Learning: In the `bridger-cli/src/utils/claim.ts` file, within the `fetchClaim` function, explicit error handling for provider calls and event queries is not required; errors should be allowed to propagate naturally.
Applied to files:
contracts/test/integration/ArbToEth.ts
📚 Learning: 2024-12-11T08:52:17.062Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/epochHandler.ts:13-13
Timestamp: 2024-12-11T08:52:17.062Z
Learning: In `bridger-cli/src/utils/epochHandler.ts`, the `veaOutbox` parameter is intentionally typed as `any` because the script is designed to be agnostic for multiple contracts.
Applied to files:
contracts/test/integration/ArbToEth.tscontracts/src/interfaces/outboxes/IVeaOutboxOnL1.sol
📚 Learning: 2024-12-09T09:42:51.590Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/transactionHandler.ts:79-101
Timestamp: 2024-12-09T09:42:51.590Z
Learning: In the `bridger-cli/src/utils/transactionHandler.ts` file, within the `TransactionHandler` class, it's acceptable to let methods like `startVerification` fail without additional error handling.
Applied to files:
contracts/test/integration/ArbToEth.ts
📚 Learning: 2024-12-09T10:53:55.715Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/bridger.ts:25-109
Timestamp: 2024-12-09T10:53:55.715Z
Learning: In the `bridger-cli/src/bridger.ts` file, refactoring the main loop and adding additional error handling are not needed unless deemed necessary by the developer.
Applied to files:
contracts/src/interfaces/outboxes/IVeaOutboxOnL1.sol
🧬 Code Graph Analysis (2)
contracts/test/integration/ArbToGnosis.ts (1)
contracts/test/merkle/MerkleTree.ts (1)
MerkleTree(9-248)
contracts/test/integration/ArbToEth.ts (1)
contracts/test/merkle/MerkleTree.ts (1)
MerkleTree(9-248)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: dependency-review
- GitHub Check: test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
contracts/src/arbitrumToEth/VeaInboxArbToEth.sol (2)
3-3: Author/header update looks goodNo issues. Matches project conventions.
75-75: Signature/data location change is correct and gas-friendlybytes calldata is appropriate; interface alignment is good.
contracts/test/integration/ArbToEth.ts (1)
56-56: Enable per-sender allowlist in setupGood default for tests that need allowlist enforcement.
contracts/src/arbitrumToGnosis/VeaInboxArbToGnosis.sol (2)
3-3: Author/header update looks goodNo issues.
75-75: Signature/data location change is correctbytes calldata is appropriate; interface alignment is good.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
contracts/test/integration/ArbToGnosis.ts (2)
255-257: Make event query deterministic (use block range).Querying without a block range can pick up stale logs across redeploys. Prefer querying from the block of the send tx.
- await senderGateway.connect(sender).sendMessageArray(data); + const sendTx = await senderGateway.connect(sender).sendMessageArray(data); + const sendRcpt = await sendTx.wait(); await veaInbox.connect(bridger).saveSnapshot(); - const MessageSent = veaInbox.filters.MessageSent(); - const MessageSentEvent = await veaInbox.queryFilter(MessageSent); + const MessageSent = veaInbox.filters.MessageSent(); + const MessageSentEvent = await veaInbox.queryFilter( + MessageSent, + sendRcpt!.blockNumber, + sendRcpt!.blockNumber + ); const msg = MessageSentEvent[0].args._nodeData;
1087-1093: Type and robustness improvements for decodeMessage.
- Not asynchronous; remove async.
- Add typing and basic guards (prefix and length).
- Optional: avoid magic numbers by documenting field sizes.
-async function decodeMessage(msg: any) { - const nonce = "0x" + msg.slice(2, 18); - const to = "0x" + msg.slice(18, 58); //18+40 - const from = "0x" + msg.slice(58, 98); //58+40 - const msgData = "0x" + msg.slice(98); - return { nonce, to, from, msgData }; -} +function decodeMessage( + msg: string +): { nonce: string; to: string; from: string; msgData: string } { + if (typeof msg !== "string" || !msg.startsWith("0x")) { + throw new Error("Invalid nodeData format"); + } + // bytes8 (16 hex chars) + address (40) + address (40) + const MIN_LEN = 2 + 16 + 40 + 40; // 2 for '0x' + if (msg.length < MIN_LEN) { + throw new Error("nodeData too short"); + } + const nonce = "0x" + msg.slice(2, 18); + const to = "0x" + msg.slice(18, 58); + const from = "0x" + msg.slice(58, 98); + const msgData = "0x" + msg.slice(98); + return { nonce, to, from, msgData }; +}Note: If you adopt these types, drop the unnecessary
awaitat call sites.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/test/integration/ArbToEth.ts(29 hunks)contracts/test/integration/ArbToGnosis.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/test/integration/ArbToEth.ts
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2024-11-21T07:44:06.942Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: relayer-cli/src/devnetRelayExample.ts:5-6
Timestamp: 2024-11-21T07:44:06.942Z
Learning: The files `contracts/deploy/02-inbox/02-arb-sepolia-to-chiado-inbox.ts` and `contracts/deploy/02-inbox/02-arb-sepolia-to-sepolia-inbox.ts` are not part of the current scope in this PR.
Applied to files:
contracts/test/integration/ArbToGnosis.ts
📚 Learning: 2024-11-20T10:49:25.392Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts:93-98
Timestamp: 2024-11-20T10:49:25.392Z
Learning: In `contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts`, when resurrecting testnets, it's acceptable to use `getContractAddress()` for contract address prediction.
Applied to files:
contracts/test/integration/ArbToGnosis.ts
📚 Learning: 2024-12-02T10:08:28.998Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:625-648
Timestamp: 2024-12-02T10:08:28.998Z
Learning: In `validator-cli/src/ArbToEth/watcherArbToGnosis.ts`, the function `findLatestL2BatchAndBlock` already handles the case when `fromArbBlock > latestBlockNumber`, so additional error handling for this case is unnecessary.
Applied to files:
contracts/test/integration/ArbToGnosis.ts
📚 Learning: 2024-12-02T10:16:56.825Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:829-840
Timestamp: 2024-12-02T10:16:56.825Z
Learning: In the `validator-cli/src/ArbToEth/watcherArbToGnosis.ts` file, avoid adding redundant error handling in functions like `reconstructChallengeProgress` when error handling is already adequately managed.
Applied to files:
contracts/test/integration/ArbToGnosis.ts
📚 Learning: 2024-11-20T11:50:19.381Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/utils/arbMsgExecutor.ts:44-44
Timestamp: 2024-11-20T11:50:19.381Z
Learning: In `validator-cli/src/utils/arbMsgExecutor.ts`, when calling `childToParentMessage.execute()`, pass `childProvider` as the argument since `childToParentMessage` already contains the parent (Ethereum) RPC context internally.
Applied to files:
contracts/test/integration/ArbToGnosis.ts
📚 Learning: 2025-06-05T12:17:21.782Z
Learnt from: mani99brar
PR: kleros/vea#424
File: validator-cli/src/helpers/claimer.ts:72-76
Timestamp: 2025-06-05T12:17:21.782Z
Learning: In validator-cli/src/helpers/claimer.ts, the outboxStateRoot captured at the beginning of the checkAndClaim function won't change for the epoch being claimed, so there's no race condition concern when reusing it in makeClaim/makeClaimDevnet functions.
Applied to files:
contracts/test/integration/ArbToGnosis.ts
📚 Learning: 2024-12-10T04:59:10.224Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/claim.ts:23-32
Timestamp: 2024-12-10T04:59:10.224Z
Learning: In `bridger-cli/src/utils/claim.ts`, within the `fetchClaim` function, when `claimData` is undefined, avoid initializing it with default values, as this can result in incorrect claim values and make issues harder to identify.
Applied to files:
contracts/test/integration/ArbToGnosis.ts
📚 Learning: 2024-12-16T09:22:57.434Z
Learnt from: mani99brar
PR: kleros/vea#383
File: contracts/test/integration/ArbToGnosis.ts:114-114
Timestamp: 2024-12-16T09:22:57.434Z
Learning: In the local Hardhat deployment for testing, `VeaInboxArbToGnosisMock` is deployed with the identifier name `VeaInboxArbToGnosis`, so it's appropriate to retrieve it using `getContract("VeaInboxArbToGnosis")` and cast it to `VeaInboxArbToGnosisMock` in `ArbToGnosis.ts`.
Applied to files:
contracts/test/integration/ArbToGnosis.ts
📚 Learning: 2024-12-09T09:40:28.400Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/transactionHandler.ts:13-13
Timestamp: 2024-12-09T09:40:28.400Z
Learning: In `bridger-cli/src/utils/transactionHandler.ts`, the `veaOutbox` implementations differ for each chain, but a common interface should be defined for type checks to enhance type safety.
Applied to files:
contracts/test/integration/ArbToGnosis.ts
🧬 Code Graph Analysis (1)
contracts/test/integration/ArbToGnosis.ts (1)
contracts/test/merkle/MerkleTree.ts (1)
MerkleTree(9-248)
🔇 Additional comments (10)
contracts/test/integration/ArbToGnosis.ts (10)
260-264: Leaf encoding updated correctly to include from.Good fix: node data now hashes nonce, to, from, and msgData; proof generation will match the on-chain encoding.
301-311: Relay path and duplicate-relay assertion look correct.
- Allowlist toggled before relay.
- sendMessage uses new 5-arg signature.
- Duplicate relay reverts as expected.
316-316: Sender mock call matches new encoding flow.Using senderGateway.sendMessage aligns with selector-included encoding.
322-328: Consistent leaf format in non-array flow.Includes from in leaf; proof creation remains correct.
366-369: Correct revert on missing allowlist.Expecting "Message sender not allowed to call receiver." matches the added sender validation.
382-386: Array message leaf/proof generation is correct.Leaf includes from; proof derived from single-node tree is valid (empty proof).
423-433: Global allowance path validated.allowlistAllSender(true) enables relay; duplicate relay reverts as expected.
518-522: Challenge setup helper usage is clean.Good reuse of setupClaimAndChallenge; event assertion remains intact.
803-807: Challenge submission flow looks good.Helper used correctly; expected event asserted.
938-948: Bug: Same nonce=0 issue in dishonest-claim resolution path.Passing 0 instead of the decoded nonce will not match the leaf/proof you computed.
- await receiverGateway.allowlistSender(true); - const relayTx = await veaOutbox.connect(receiver).sendMessage(proof, 0, receiverGateway.target, from, msgData); + await receiverGateway.allowlistSender(true); + const relayTx = await veaOutbox + .connect(receiver) + .sendMessage(proof, nonce, to, from, msgData);Likely an incorrect or invalid review comment.
PR-Codex overview
This PR focuses on enhancing the messaging system between gateways by modifying function signatures, adding allowlist functionalities, and improving message handling to support dynamic arrays.
Detailed summary
sendMessagefunctions to remove function selectors and useabi.encodeWithSelector.receiveMessageArrayinIReceiverGatewayMock.setAllowlistfunction for sender gateways.ReceiverGatewayMockto allow sender allowlisting.Summary by CodeRabbit
New Features
Refactor
Tests
Documentation