Chore/updated relayer encoding fix #430
Conversation
✅ Deploy Preview for veascan ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughConsolidates inbox sendMessage API to (address, bytes) and updates Merkle leaf encoding to include msg.sender. Outboxes gain an allowlist and a new from parameter in sendMessage, with node hash updated accordingly. Test mocks, integration tests, relayer CLI, and subgraph parsing are updated to the new payload shape and flow. Changes
Sequence Diagram(s)sequenceDiagram
participant SenderGateway
participant VeaInbox
participant MerkleTree
participant Relayer
participant VeaOutbox
participant Receiver
SenderGateway->>VeaInbox: sendMessage(to, abi.encodeWithSelector(selector, args))
VeaInbox->>MerkleTree: leaf = (nonce, to, msg.sender, data)
Relayer->>VeaOutbox: sendMessage(proof, nonce, to, from, data)
VeaOutbox->>VeaOutbox: verify proof & allowlist[to][from] || [to][0x0]
VeaOutbox->>Receiver: low-level call(data)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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/Issue comments)Type 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: 2
🔭 Outside diff range comments (2)
contracts/src/arbitrumToGnosis/VeaOutboxArbToGnosis.sol (1)
441-443: Critical bug: assignment replaced with equality in claim hash updateThis line uses == instead of =, so the updated claim hash is never written, breaking the escape hatch flow.
Apply this fix:
- claimHashes[_epoch] == hashClaim(_claim); + claimHashes[_epoch] = hashClaim(_claim);contracts/src/arbitrumToEth/VeaOutboxArbToEth.sol (1)
498-498: Critical bug: equality used instead of assignment in claim hash updateSame issue as in Arb->Gnosis outbox escape hatch path: uses == instead of =, so state isn’t updated.
Apply this fix:
- claimHashes[_epoch] == hashClaim(_claim); + claimHashes[_epoch] = hashClaim(_claim);
🧹 Nitpick comments (17)
contracts/src/test/gateways/IReceiverGatewayMock.sol (1)
17-18: API naming nit: prefer plural for array variantConsider renaming receiveMessageArray to receiveMessages for semantic clarity and to match common naming patterns for batch methods.
Apply:
- /// Receive the message array from the sender gateway. - function receiveMessageArray(uint256[] calldata _data) external; + /// Receive a batch of messages from the sender gateway. + function receiveMessages(uint256[] calldata _data) external;contracts/src/interfaces/outboxes/IVeaOutboxOnL2.sol (2)
18-18: Doc specificity: clarify what “sender” means across chainsTo avoid ambiguity, explicitly state that _from is the originating contract address on the other domain (the authenticated sender expected by the receiver).
- /// @param _from The address of the message sender. + /// @param _from The originating contract address on the source chain (authenticated sender expected by the receiver).
34-38: Clarify self-service allowlist semantics in docsThe implementation is intended to be permissionless/self-service per receiver. Make this explicit so integrators understand setAllowlist config applies to msg.sender’s allowlist. Also mention how address(0) interacts with specific entries.
- /// @dev Sets the allowlist for the sender gateway. - /// Note: Address(0) is used to allow all addresses. + /// @dev Sets the allowlist for the caller (msg.sender). + /// Self-service: each receiver manages its own allowlist by calling this function. + /// Note: Address(0) enables or disables a global allow for all senders for the caller. - /// @param _from The address to allow or disallow + /// @param _from The sender address to allow or disallow (use address(0) for global toggle).I can replicate similar doc clarifications across the L1 interface for consistency—say the word.
contracts/src/interfaces/inboxes/IVeaInbox.sol (1)
15-17: API shift to (address, bytes): docs should reflect selector and sender argument placementGood change to calldata. Tighten the comment to guide callers to embed the selector and, where required by receivers, include the authenticated “from” as the first ABI-encoded argument.
- /// @param _data The message calldata, abi.encodeWithSelector(...) + /// @param _data The full calldata payload, abi.encodeWithSelector(...) + /// If the receiver expects an authenticated sender, encode it as the first argument. - function sendMessage(address _to, bytes calldata _data) external returns (uint64 msgId); + function sendMessage(address _to, bytes calldata _data) external returns (uint64 msgId);The signature change to calldata is an improvement in gas and aligns with the broader refactor.
contracts/src/utils/veaInboxTouch.sol (1)
23-23: Use address(0) constant for clarityPrefer the typed address(0) over a hex literal.
- veaInbox.sendMessage(0x0000000000000000000000000000000000000000, abi.encode(random)); + veaInbox.sendMessage(address(0), abi.encode(random));contracts/src/interfaces/outboxes/IVeaOutboxOnL1.sol (1)
37-41: Clarify self-service allowlist semantics (mirror L2 docs)Document that the allowlist is managed by the receiver (msg.sender) and how the global toggle behaves.
- /// @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 + /// @dev Sets the allowlist for the caller (msg.sender). + /// Self-service: each receiver manages its own allowlist by calling this function. + /// Note: Address(0) enables or disables a global allow for all senders for the caller. + /// @param _from The sender address to allow or disallow (use address(0) for global toggle).contracts/src/gnosisToArbitrum/VeaInboxGnosisToArb.sol (1)
74-76: Public API consolidation looks good; update event docs to reflect new leaf layoutThe signature change to (address _to, bytes calldata _data) and the clarified param doc are aligned with the PR objective.
However, the MessageSent event doc above still claims abi.encodePacked(msgId, to, message), which is now outdated because the leaf includes msg.sender as a separate field. Please update the event doc to avoid tooling/subgraph confusion.
Proposed doc tweak (outside the selected range):
/// @dev Relayers watch for these events to construct merkle proofs to execute transactions on Ethereum. /// @param _nodeData The data to create leaves in the merkle tree. abi.encodePacked(msgId, to, from, message), outbox relays to.call(message). event MessageSent(bytes _nodeData);contracts/src/arbitrumToGnosis/VeaOutboxArbToGnosis.sol (1)
299-305: Permissionless allowlist setter is correct; consider clarifying wildcard behavior in NatSpecImplementation aligns with the intended self-service pattern. As a minor doc improvement, mention that _from = address(0) allows all senders for msg.sender.
Suggested doc tweak:
-/// @dev Sets the allowlist for the sender gateway. +/// @dev Sets the allowlist for the receiver (msg.sender). +/// Note: Setting `_from` to address(0) allows all senders for this receiver.contracts/src/arbitrumToGnosis/VeaInboxArbToGnosis.sol (1)
73-76: API consolidation to (address, bytes) is correct; update event docs for leaf shapeThe signature change and calldata usage improve gas and align with embedding the selector in _data.
The MessageSent event doc still says abi.encodePacked(msgId, to, message) but the node now includes msg.sender. Please update to abi.encodePacked(msgId, to, from, message) to keep relayers/subgraphs consistent.
Proposed doc tweak (outside the selected range):
/// @dev Relayers watch for these events to construct merkle proofs to execute transactions on Gnosis. /// @param _nodeData The data to create leaves in the merkle tree. abi.encodePacked(msgId, to, from, message), outbox relays to.call(message). event MessageSent(bytes _nodeData);contracts/test/integration/ArbToGnosis.ts (4)
250-311: Test flow updated to new payload shape — good coverage; consider using decoded nonce consistentlyThe test correctly:
- Uses decodeMessage to extract (nonce, to, from, msgData).
- Constructs the new leaf as MerkleTree.makeLeafNode(nonce, to, from, msgData).
- Calls the outbox with 5-arg sendMessage.
Minor consistency/nitpick: In later tests you sometimes pass a literal 0 for _msgId instead of the decoded nonce. Prefer passing nonce everywhere to reduce brittleness.
Example tweak:
-const relayTx = await veaOutbox.connect(receiver).sendMessage(proof, 0, receiverGateway.target, from, msgData); +const relayTx = await veaOutbox.connect(receiver).sendMessage(proof, nonce, receiverGateway.target, from, msgData);
761-771: Use decoded nonce instead of literal 0 for relay consistencyYou decode nonce earlier; use it directly rather than hardcoding 0. Avoids coupling to current message ordering.
Apply similarly to all occurrences:
-const relayTx = await veaOutbox.connect(receiver).sendMessage(proof, 0, receiverGateway.target, from, msgData); +const relayTx = await veaOutbox.connect(receiver).sendMessage(proof, nonce, receiverGateway.target, from, msgData);
939-948: Same nit: prefer decoded nonce over 0For consistency and resilience, pass nonce instead of 0 as _msgId.
-const relayTx = await veaOutbox.connect(receiver).sendMessage(proof, 0, receiverGateway.target, from, msgData); +const relayTx = await veaOutbox.connect(receiver).sendMessage(proof, nonce, receiverGateway.target, from, msgData);
1087-1093: Add basic validation to decodeMessage to catch malformed inputsCurrent slicing assumes msg is long enough. Consider asserting minimum length and returning typed fields.
Example:
-async function decodeMessage(msg: any) { +async function decodeMessage(msg: string): Promise<{ nonce: string; to: string; from: string; msgData: string }> { + if (typeof msg !== "string" || !msg.startsWith("0x") || msg.length < 2 + 16 + 40 + 40) { + throw new Error("Invalid nodeData encoding"); + } 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/gnosisToArbitrum/VeaOutboxGnosisToArb.sol (1)
307-308: Consider adding an event for better observability when messages are rejected.While the allowlist check logic is correct, adding an event when a message is rejected due to allowlist restrictions would improve monitoring and debugging capabilities.
Consider emitting an event when the allowlist check fails:
+ /// @dev This event indicates that a message was rejected due to allowlist restrictions. + /// @param _msgId The msgId of the rejected message. + /// @param _from The sender address that was not allowed. + /// @param _to The receiver address that rejected the message. + event MessageRejected(uint64 indexed _msgId, address indexed _from, address indexed _to); function sendMessage( bytes32[] memory _proof, uint64 _msgId, address _to, address _from, bytes memory _message ) external { require(_proof.length < 64, "Proof too long."); bool isAllowed = allowlist[_to][_from] || allowlist[_to][address(0)]; - require(isAllowed, "Message sender not allowed to call receiver."); + if (!isAllowed) { + emit MessageRejected(_msgId, _from, _to); + revert("Message sender not allowed to call receiver."); + }contracts/src/test/gateways/ReceiverGatewayMock.sol (1)
47-50: Consider adding validation for the data array.The
receiveMessageArrayfunction doesn't validate the input array before storing it. Consider adding checks for array size limits to prevent potential gas issues with unbounded arrays.Consider adding array size validation:
function receiveMessageArray(uint256[] calldata _data) external onlyFromVeaBridge { + require(_data.length > 0 && _data.length <= 100, "Invalid array size"); _receiveMessage(); dataArray = _data; }relayer-cli/src/utils/relay.ts (1)
145-145: Remove or convert console.log to proper logging.Debug console.log statement should be removed or converted to use proper logging mechanism for production code.
- console.log(targets, datas); + // Consider using a proper logger instead of console.log + // logger.debug("Batch relay targets and data", { targets, datas });contracts/test/integration/ArbToEth.ts (1)
1241-1302: Consider adding return type annotation for better type safety.The
claimAndVerifyhelper function would benefit from explicit return type annotation.async function claimAndVerify({ veaInbox, veaOutbox, bridger, epoch, batchMerkleRoot, ethers, network, mine, }: { veaInbox: any; veaOutbox: any; bridger: any; epoch: number; batchMerkleRoot: string; ethers: any; network: any; mine: (blocks: number) => Promise<void>; -}) { +}): Promise<void> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
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(29 hunks)contracts/test/integration/ArbToGnosis.ts(7 hunks)relayer-cli/src/utils/proof.ts(2 hunks)relayer-cli/src/utils/relay.test.ts(1 hunks)relayer-cli/src/utils/relay.ts(4 hunks)relayer-subgraph-inbox/src/vea-inbox.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (13)
📚 Learning: 2025-08-11T09:26:45.968Z
Learnt from: mani99brar
PR: kleros/vea#429
File: contracts/src/interfaces/outboxes/IVeaOutboxOnL1.sol:0-0
Timestamp: 2025-08-11T09:26:45.968Z
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/src/interfaces/outboxes/IVeaOutboxOnL2.solcontracts/src/gnosisToArbitrum/VeaOutboxGnosisToArb.solcontracts/src/interfaces/outboxes/IVeaOutboxOnL1.solcontracts/src/arbitrumToGnosis/VeaOutboxArbToGnosis.solcontracts/src/test/gateways/ReceiverGatewayMock.solcontracts/src/arbitrumToEth/VeaOutboxArbToEth.sol
📚 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/ArbToGnosis.tsrelayer-cli/src/utils/relay.tscontracts/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/ArbToGnosis.tscontracts/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: 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-16T09:24:33.478Z
Learnt from: mani99brar
PR: kleros/vea#383
File: contracts/test/integration/ArbToGnosis.ts:520-520
Timestamp: 2024-12-16T09:24:33.478Z
Learning: In the TypeScript integration tests (`contracts/test/integration/ArbToGnosis.ts`), adding null checks after retrieving blocks using `await ethers.provider.getBlock` is unnecessary because the test will fail if the block is not found.
Applied to files:
contracts/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-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-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
📚 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-09T10:54:57.068Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/claim.ts:56-69
Timestamp: 2024-12-09T10:54:57.068Z
Learning: In the TypeScript file 'bridger-cli/src/utils/claim.ts', the upper layers handle input validation for the 'hashClaim' function. Therefore, explicit input validation within 'hashClaim' is not necessary.
Applied to files:
contracts/test/integration/ArbToEth.ts
🧬 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). (6)
- GitHub Check: Redirect rules - veascan
- GitHub Check: Analyze (javascript)
- GitHub Check: dependency-review
- GitHub Check: test
- GitHub Check: Header rules - veascan
- GitHub Check: Pages changed - veascan
🔇 Additional comments (41)
contracts/src/test/gateways/IReceiverGatewayMock.sol (1)
15-15: No signature mismatch—ignore this comment
IReceiverGateway doesn’t declare anyreceiveMessagemethods, so the mock’s single-argumentreceiveMessage(uint256)is self-contained and correctly implemented by ReceiverGatewayMock. There’s no dropped parameter on a parent interface.Likely an incorrect or invalid review comment.
contracts/src/interfaces/outboxes/IVeaOutboxOnL2.sol (1)
20-26: All sendMessage call sites and implementations now use the five-argument signature
No references to the old four-argumentsendMessage(...)remain outside of the interface. All OutboxOnL2 implementations and their callers have been updated to include the new_fromparameter.contracts/src/interfaces/outboxes/IVeaOutboxOnL1.sol (1)
20-28: Ignore outdated five-argument sendMessage concernAll L1 outbox contracts and the
IVeaOutboxOnL1interface now use the 4-argument signaturesendMessage(bytes32[] calldata _proof, uint64 _msgId, address _to, bytes calldata _message)and proofs are encoded/hashed as
(msgId, to, message). There is no_fromparameter to propagate through leaf/node encoding or auth checks—those are handled natively viamsg.senderand the bridge. You can safely disregard the previous 5-arg verification request.Likely an incorrect or invalid review comment.
contracts/src/arbitrumToEth/VeaInboxArbToEth.sol (4)
3-3: LGTM! Author added to attribution.The addition of @mani99brar to the authors list properly acknowledges their contribution to this contract update.
73-73: Documentation accurately reflects the new encoding approach.The parameter documentation correctly specifies that
_datashould be encoded withabi.encodeWithSelector, which aligns with the removal of the separate_fnSelectorparameter.
75-75: Function signature simplified appropriately.The change from
bytes memorytobytes calldatais a good optimization that avoids unnecessary memory allocation for the input data.
82-82: Msg.sender inclusion is consistent and outboxes require no on-chain decode changesAll three inbox contracts now pack
msg.senderinto their Merkle leaves:
- src/arbitrumToEth/VeaInboxArbToEth.sol:82
- src/arbitrumToGnosis/VeaInboxArbToGnosis.sol:82
- src/gnosisToArbitrum/VeaInboxGnosisToArb.sol:83
Outboxes verify proofs over
abi.encodePacked(msgId, to, message), wheremessageitself already contains the selector and paddedmsg.sender. There is no on-chainabi.decodeof a 4-tuple; relayers off-chain extract and pass the fullmessagepayload (including sender) intosendMessage. No further code changes are needed.relayer-cli/src/utils/proof.ts (3)
8-10: LGTM! Message structure properly extended.The addition of
msgSenderfield to theMessageSentDatainterface correctly reflects the new message structure that includes the sender information.
40-42: GraphQL query correctly fetches sender information.The query properly retrieves the
msgSender.idfield, which is essential for the new message relay flow.
48-48: All consumers properly handle the updated three‐element return formatThe helper
getMessageDataToRelaynow returns[to, msgSender, data], and every usage inrelay.tsdestructures it as[to, from, data]before callingsendMessage, matching the updated contract signature.relayer-cli/src/utils/relay.test.ts (1)
64-64: Test mock correctly updated for new data structure.The mock now returns the three-element array
["to", "from", "data"]which properly reflects the updated message structure used throughout the codebase.relayer-subgraph-inbox/src/vea-inbox.ts (2)
36-38: Data extraction correctly adjusted for new offset.The data now starts at byte 48 (8 bytes nonce + 20 bytes to + 20 bytes msgSender), which correctly aligns with the new message structure.
34-34: msgSender extraction offset is correctThe 20-byte slice starting at offset 28 (8 byte nonce + 20 byte
toaddress) aligns with the contract’sabi.encodePacked(uint64, address, address, bytes)layout—no change needed.contracts/src/test/gateways/SenderGatewayMock.sol (2)
25-29: Message encoding correctly updated to use single bytes parameter.The change to use
abi.encodeWithSelectorproperly embeds the function selector within the data payload, aligning with the newIVeaInbox.sendMessage(address, bytes)signature.
31-35: Array message functionality validated– IReceiverGatewayMock.sol defines
receiveMessageArray(uint256[] calldata)at line 18
– ReceiverGatewayMock.sol implementsreceiveMessageArray(uint256[] calldata)at line 47
–sendMessageArrayin SenderGatewayMock correctly encodes and dispatches the array payloadAll required interface and implementation pieces are present.
contracts/src/gnosisToArbitrum/VeaInboxGnosisToArb.sol (1)
83-83: Leaf encoding now includes msg.sender — consistent with Outbox verificationEncoding nodeData as abi.encodePacked(oldCount, _to, msg.sender, _data) matches the updated outbox verification: keccak256(abi.encodePacked(_msgId, _to, _from, _message)) with double-hash. This keeps proofs consistent. LGTM.
contracts/src/arbitrumToGnosis/VeaOutboxArbToGnosis.sol (3)
45-45: Allowlist mapping shape and semantics are appropriatePer the design, allowlist is per-receiver and supports address(0) as a wildcard. This matches the “self-service” allowlist approach captured in our learnings; no admin or events required.
313-324: New 5-arg sendMessage gating and node hash are correct
- Proof length bound (< 64) matches 64-level tree max for uint64 messages.
- Allowlist check (per-sender or global) prevents unsolicited relays.
- Node hash now includes _from and matches the updated inbox node layout.
367-373: Replay protection placement is safe against reentrancy on relay pathBitmap is updated before the untrusted call. This prevents reentrancy replay of the same message within the same tx. Good defensive ordering.
contracts/src/arbitrumToGnosis/VeaInboxArbToGnosis.sol (2)
3-3: Author metadata update acknowledgedNo code impact; consistent with PR authorship changes.
82-83: Leaf encoding includes msg.sender — matches updated Outbox verificationabi.encodePacked(oldCount, _to, msg.sender, _data) is consistent with outbox expectations (and double-hash process). Good.
contracts/src/arbitrumToEth/VeaOutboxArbToEth.sol (3)
45-45: Per-receiver allowlist with address(0) wildcard is correctMatches the intended self-service allowlist design. Public mapping exposure is helpful for off-chain tooling.
355-362: Allowlist setter matches intended security modelClear doc notes address(0) semantics. Implementation is minimal and correct.
370-381: 5-arg relay method with allowlist check and updated leaf hash is correct
- Proof length guard (< 64) is appropriate.
- Gating: allowlist[_to][_from] || allowlist[_to][address(0)] matches design.
- Node hash includes _from and aligns with inbox leaf encoding.
contracts/test/integration/ArbToGnosis.ts (2)
316-369: Negative test for allowlist rejection matches on-chain revert reasonAsserts "Message sender not allowed to call receiver." which matches the contract. Good.
371-433: Global allowlist path is exercised correctlyallowlistAllSender(true) scenario verifies the address(0) wildcard behavior and the relay succeeds. Good.
contracts/src/gnosisToArbitrum/VeaOutboxGnosisToArb.sol (3)
44-44: LGTM! Allowlist mapping design is appropriate.The allowlist mapping structure
mapping(address => mapping(address => bool))follows the correct pattern where the receiver controls their own allowlist of senders. This aligns with the self-service design pattern mentioned in the retrieved learnings.
285-291: LGTM! Allowlist setter follows self-service design pattern.The
setAllowlistfunction correctly implements the permissionless, self-service pattern where receivers (msg.sender) control which sender addresses can relay messages to them. The use ofaddress(0)as a wildcard for "allow all" is a good design choice.
310-310: LGTM! Merkle leaf encoding correctly includes sender address.The updated node hash calculation correctly includes the
_fromparameter in the expected position, maintaining compatibility with the inbox's message encoding format.contracts/src/test/gateways/ReceiverGatewayMock.sol (2)
34-36: LGTM! Allowlist management functions correctly implemented.The
allowlistSenderfunction properly calls the outbox'ssetAllowlistwith thesenderGatewayaddress, enabling the receiver to control who can send messages to it.
38-40: LGTM! Global allowlist management correctly implemented.The
allowlistAllSenderfunction properly usesaddress(0)as the wildcard to allow or disallow all senders, following the established pattern.relayer-cli/src/utils/relay.ts (3)
62-63: LGTM! Message data destructuring correctly updated.The destructuring now correctly extracts three elements
[to, from, data]from the message data, aligning with the new message format that includes the sender address.
130-133: LGTM! Batch relay correctly handles the new message format.The batch relay function properly destructures the message data and uses the 5-argument sendMessage signature in both the static call and the encoded function data.
197-199: LGTM! relayAllFrom correctly handles the new message format.The function properly destructures the message data and encodes the function data with all five parameters including the
fromaddress.contracts/test/integration/ArbToEth.ts (7)
56-56: LGTM! Test setup correctly enables allowlist.The test correctly sets up the allowlist by calling
allowlistSender(true)in the beforeEach hook, ensuring the sender gateway is allowed to send messages to the receiver gateway.
253-253: LGTM! Message decoding and Merkle tree construction updated correctly.The test correctly:
- Uses the new
decodeMessageutility to extract all four fields (nonce, to, from, msgData)- Constructs Merkle tree leaves with all four parameters using
MerkleTree.makeLeafNode(nonce, to, from, msgData)This aligns with the updated message format that includes the sender address.
Also applies to: 264-265
276-285: LGTM! Good extraction of common test logic.The extraction of claim and verification logic into the
claimAndVerifyhelper function improves code reusability and reduces duplication across test cases.
300-353: LGTM! Comprehensive test for allowlist rejection.The test properly verifies that messages are rejected when the sender is not in the allowlist, with the expected error message "Message sender not allowed to call receiver."
355-413: LGTM! Dynamic array test correctly implemented.The test properly verifies that messages with dynamic arrays can be sent and relayed through the bridge, using the new
sendMessageArrayfunction and correctly handling the message format.
415-470: LGTM! Global allowlist test validates wildcard functionality.The test correctly verifies that setting
allowlistAllSender(true)allows any sender to relay messages, even when the specific sender is not explicitly allowed.
1304-1310: LGTM! Clean utility function for message decoding.The
decodeMessageutility function correctly extracts all four components (nonce, to, from, msgData) from the encoded message data, following the expected format.
| /// @param _from The address of the message sender | ||
| /// @param _message The message in the vea inbox as abi.encodeWithSelector(fnSelector, param1, param2, ...) |
There was a problem hiding this comment.
Parameter documentation is incomplete.
The NatSpec comment for the _from parameter doesn't match the actual parameter name or purpose correctly.
Apply this diff to fix the documentation:
- /// @param _from The address of the message sender
+ /// @param _from The address of the message sender on the source chain.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// @param _from The address of the message sender | |
| /// @param _message The message in the vea inbox as abi.encodeWithSelector(fnSelector, param1, param2, ...) | |
| /// @param _from The address of the message sender on the source chain. | |
| /// @param _message The message in the vea inbox as abi.encodeWithSelector(fnSelector, param1, param2, ...) |
🤖 Prompt for AI Agents
In contracts/src/gnosisToArbitrum/VeaOutboxGnosisToArb.sol around lines 297 to
298, the NatSpec @param for `_from` is incorrect or misleading; update the
comment to use the correct parameter name and describe its actual purpose (e.g.,
the original sender/address that posted the message into the Vea inbox on the
source chain) and ensure the `_message` param line remains accurate (message
ABI-encoded with selector and parameters).
| /// @return msgId The index of the message in the inbox, as a message Id, needed to relay the message. | ||
| function sendMessage(address _to, bytes4 _fnSelection, bytes memory _data) external returns (uint64 msgId); | ||
| function sendMessage(address _to, bytes calldata _data) external returns (uint64 msgId); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Locate and update all callers to the new 2-arg signature
Make sure internal tools, relayers, and tests no longer pass a separate selector/arg.
🏁 Script executed:
#!/bin/bash
# Find old-style 3-arg calls
rg -n -A1 -B2 $'sendMessage\\s*\\(\\s*[^,]+,\\s*[^,]+,\\s*[^)]*\\)' -g '!**/interfaces/**'
# List all sendMessage calls for manual review
rg -n -A1 -B2 $'sendMessage\\s*\\('Length of output: 312375
Update all sendMessage invocations to drop the removed selector/from argument
The inbox sendMessage signature has been simplified to two parameters (address _to, bytes calldata _data), and the outbox sendMessage no longer takes a separate sender argument. Please remove the extra argument from every call and merge the former from (or selector) into the calldata.
Affected call sites:
• relay-cli/src/utils/relay.ts (around lines 61–64):
- Change
veaOutbox.sendMessage(proof, nonce, to, from, data)
to
veaOutbox.sendMessage(proof, nonce, to, dataWithSender)
• contracts/test/integration/ArbToGnosis.ts (lines 304, 308, 366, 430, 769, 946): - Replace each
…sendMessage(proof, nonce, to, from, msgData)
with
…sendMessage(proof, nonce, to, msgDataWithSender)
• contracts/test/integration/ArbToEth.ts (lines 287, 292, 295, 348, 402, 407, 410, 463, 468, 541, 625, 847, 975): - Same 5-arg → 4-arg update
• Search for any other.sendMessage(invocations with three or more comma-separated args (outside/interfaces) and refactor accordingly.
Ensure the passed bytes payload includes both selector and original sender where required.
🤖 Prompt for AI Agents
In contracts/src/interfaces/inboxes/IVeaInbox.sol around line 17, the
sendMessage signature was simplified to (address _to, bytes calldata _data);
update all call sites to remove the extra sender/selector argument and instead
embed the former from/selector into the bytes calldata payload. Specifically, in
relay-cli/src/utils/relay.ts (around lines 61–64) change
veaOutbox.sendMessage(proof, nonce, to, from, data) to
veaOutbox.sendMessage(proof, nonce, to, dataWithSender) where dataWithSender is
data prefixed or merged with the sender/selector; in
contracts/test/integration/ArbToGnosis.ts (lines ~304, 308, 366, 430, 769, 946)
and contracts/test/integration/ArbToEth.ts (lines ~287, 292, 295, 348, 402, 407,
410, 463, 468, 541, 625, 847, 975) replace each 5-arg call with the 4-arg
variant and construct msgDataWithSender by concatenating the selector/sender
into the bytes payload; also grep the repo for any other .sendMessage( calls
with 3+ comma-separated args (excluding /interfaces) and refactor them similarly
so the bytes payload contains selector and original sender where required.
PR-Codex overview
This PR focuses on enhancing the messaging functionality between contracts by refining the handling of message data, including sender information, and implementing an allowlist mechanism for message relay authorization.
Detailed summary
sendMessagefunction parameters in several contracts to includefromaddress.receiveMessageArrayfunction toIReceiverGatewayMock.IVeaOutboxOnL1and related contracts.Summary by CodeRabbit
New Features
Breaking Changes
CLI
Subgraph