Multichain Inputs #49
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
I believe if we make the |
| ) internal view returns (bytes32) { | ||
| return keccak256( | ||
| abi.encodePacked( | ||
| address(this), |
There was a problem hiding this comment.
Do we wanna consider multichain orders with non-same address InputSettlers?
This is particularly relevant for cross-vm inputs.
My, current, best bad idea for this would be to encode it in the inputs instead of the core identifier.
There was a problem hiding this comment.
I'm okay with keeping the assumption of same address for simplicity. We can reevaluate it for a future version.
This may be a dumb question but can you remind me why it's critical to include the address in the identifier? I think I can see some risk in the escrow variant, if you have two settlers whose order ids overlap, a single fill on destination could allow claiming funds from two separate orders, although this seems unlikely? In The Compact the user's signature will include a single arbiter per chain so I don't see a risk there. Am I missing something?
reednaa
left a comment
reednaa
left a comment
There was a problem hiding this comment.
Self review:
Documentation sucks and MultichainCompactOrderType needs to be cleaned.
More documentation is needed but this sets a good initial full implementation
|
We need to figure out a way to handle the input oracle. It has to be the same address on every chain which is not gonna be true. |
WalkthroughThis PR adds multichain input settlement (compact and escrow), new multichain order/witness hashing types, Permit2 multichain witness support, refactors caller validation (adds UnexpectedCaller/_validateIsCaller and removes _orderOwnerIsCaller), renames test references from "Coin" → "Simple", updates snapshots, and reduces foundry fuzz runs. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant InputSettler as InputSettlerMultichain<br/>(Compact/Escrow)
participant TheCompact
participant OutputSettler as OutputSettlerSimple
participant Wormhole
rect rgb(230,240,255)
Note over User,InputSettler: Phase 1 — Deposit / Open
User->>InputSettler: open(order) or openFor(order, sig)
InputSettler->>InputSettler: validate order, compute orderId
InputSettler->>InputSettler: collect inputs (Permit2 / ERC20)
InputSettler-->>User: Order Deposited (state saved)
end
rect rgb(240,255,230)
Note over OutputSettler,Wormhole: Phase 2 — Output Execution (dest chain)
OutputSettler->>OutputSettler: fill(outputs)
OutputSettler->>Wormhole: emit MandateOutput payload (VAA)
Wormhole-->>User: VAA (cross-chain proof)
end
rect rgb(255,250,230)
Note over User,TheCompact: Phase 3 — Finalise / Claim
User->>InputSettler: finalise(order, solveParams, destination, signatures/vaa)
InputSettler->>InputSettler: _validateIsCaller / validate destination & fills
InputSettler->>TheCompact: batchMultichainClaim or exogenousBatchClaim(claim, vaa)
TheCompact->>TheCompact: verify claim, execute settlement
InputSettler-->>User: Order Finalised (transfers completed)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus on:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
test/output/OutputSettlerSimple.fillOrderOutputs.t.sol (2)
4-9: Minor: Import formatting style changed.Import statements have been reformatted to remove spaces inside curly braces (e.g.,
{Test}vs{ Test }). While this is valid, ensure this aligns with the project's style guidelines.
60-66: Optional: Consider consistent formatting for multi-condition statements.Several
vm.assumeand conditional statements have been reformatted to span multiple lines. While this can improve readability for long conditions, the formatting style varies throughout the file. Consider adopting a consistent approach.Also applies to: 226-227, 257-259, 332-334, 423-425
test/output/OutputSettlerSimple.fill.t.sol (1)
4-10: Minor: Import formatting style changed.Same as in the fillOrderOutputs test file, imports have been reformatted to a more compact style. Ensure consistency across the codebase.
test/input/escrow/InputSettlerMultichainEscrow.base.t.sol (2)
136-149: Document the hardcoded VAA structure values.The hex literals (
hex"000003e8",hex"00000001",hex"0000000000000539",hex"0f") represent specific fields in the Wormhole VAA structure but lack explanation, making the code harder to understand and maintain.Consider adding inline comments or extracting to named constants:
function _buildPreMessage( uint16 emitterChainId, bytes32 emitterAddress ) internal pure returns (bytes memory preMessage) { return abi.encodePacked( - hex"000003e8" - hex"00000001", + hex"000003e8" // timestamp + hex"00000001", // nonce emitterChainId, emitterAddress, - hex"0000000000000539" - hex"0f" + hex"0000000000000539" // sequence + hex"0f" // consistency level ); }
247-287: Extract hardcoded type strings to use existing production constants.The MandateOutput type string is duplicated identically in witnessHash (line 209), outputsHash (line 230), and getPermit2Signature (line 273). The Permit2 PermitBatchWitnessTransferFrom type string format is correct, but both concerns can be resolved by importing constants already defined in the production code (
src/input/escrow/Permit2MultichainWitnessType.sol,src/input/types/MandateOutputType.sol) instead of hardcoding them inline.Import and reuse the existing constants from the production type modules rather than duplicating type strings across test functions or creating redundant test-specific constants.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
foundry.toml(1 hunks)snapshots/inputSettler.json(1 hunks)snapshots/outputSettler.json(1 hunks)src/input/InputSettlerBase.sol(2 hunks)src/input/InputSettlerPurchase.sol(0 hunks)src/input/compact/InputSettlerCompact.sol(1 hunks)src/input/compact/InputSettlerMultichainCompact.sol(1 hunks)src/input/escrow/InputSettlerEscrow.sol(18 hunks)src/input/escrow/InputSettlerMultichainEscrow.sol(1 hunks)src/input/escrow/Permit2MultichainWitnessType.sol(1 hunks)src/input/types/MandateOutputType.sol(3 hunks)src/input/types/MultichainCompactOrderType.sol(1 hunks)src/input/types/MultichainOrderComponentType.sol(1 hunks)test/exploit/FillOutputsTwice.t.sol(6 hunks)test/input/compact/InputSettlerCompact.base.t.sol(2 hunks)test/input/compact/InputSettlerCompact.t.sol(14 hunks)test/input/compact/InputSettlerMultichainCompact.base.t.sol(1 hunks)test/input/compact/InputSettlerMultichainCompact.t.sol(1 hunks)test/input/escrow/InputSettlerEscrow.base.t.sol(5 hunks)test/input/escrow/InputSettlerEscrow.t.sol(4 hunks)test/input/escrow/InputSettlerMultichainEscrow.base.t.sol(1 hunks)test/input/escrow/InputSettlerMultichainEscrow.t.sol(1 hunks)test/integration/SettlerCompact.crosschain.t.sol(19 hunks)test/libs/IsContractLib.t.sol(1 hunks)test/libs/MultichainOrderComponentsType.sol(1 hunks)test/output/OutputSettlerSimple.fill.t.sol(42 hunks)test/output/OutputSettlerSimple.fillOrderOutputs.t.sol(20 hunks)
💤 Files with no reviewable changes (1)
- src/input/InputSettlerPurchase.sol
🔇 Additional comments (21)
test/input/compact/InputSettlerCompact.base.t.sol (1)
58-58: LGTM: Clean variable rename.The rename from
outputSettlerCointooutputSettlerSimpleis consistent with the broader refactoring across the test suite.Also applies to: 99-99
foundry.toml (1)
10-10: Verify adequacy of reduced fuzz iterations.The fuzzing iterations have been reduced from 10,000 to 1,000 (90% reduction). While this speeds up test execution, it significantly reduces the probability of discovering edge cases, particularly in the new authorization logic (
_validateIsCaller) and multichain settlement flows introduced in this PR.Consider whether this reduction is appropriate given the scope of changes. If test times are a concern, you might instead reduce iterations selectively for specific test files rather than globally.
snapshots/outputSettler.json (1)
2-8: LGTM: Snapshot keys updated consistently.All snapshot keys have been updated to reflect the
OutputSettlerSimplenaming. The preserved values confirm this is a mechanical rename with no gas cost impact.test/libs/IsContractLib.t.sol (1)
22-22: LGTM: Consistent rename throughout.All references to
outputSettlerCoinhave been correctly updated tooutputSettlerSimple, including the variable declaration, initialization, and test assertions.Also applies to: 30-30, 36-39
test/exploit/FillOutputsTwice.t.sol (1)
19-19: LGTM: Comprehensive rename applied correctly.All references to
outputSettlerCoinhave been systematically updated tooutputSettlerSimpleacross variable declarations, initialization, approvals, fill calls, MandateOutput construction, and attestation checks.Also applies to: 25-25, 37-37, 42-42, 54-54, 75-75, 83-83, 96-96, 117-117
test/output/OutputSettlerSimple.fillOrderOutputs.t.sol (1)
22-36: LGTM: Variable declarations and setup updated correctly.The test setup has been properly updated to use
OutputSettlerSimplewith consistent naming throughout.test/output/OutputSettlerSimple.fill.t.sol (2)
28-46: LGTM: Test setup properly updated.All test infrastructure variables have been correctly renamed to use
OutputSettlerSimple, with addresses and references updated consistently.
77-86: LGTM: MandateOutput initialization includes all required fields.The MandateOutput structs now include
recipient,callbackData, andcontextfields, which aligns with the expanded MandateOutput structure mentioned in the AI summary.Also applies to: 152-161, 194-203
snapshots/inputSettler.json (1)
4-6: LGTM: Gas snapshots reflect authorization refactor.The small gas cost adjustments (±5-7 gas) are consistent with the replacement of
_orderOwnerIsCallerwith_validateIsCallerin the authorization paths mentioned in the AI summary.Also applies to: 8-8
test/input/escrow/InputSettlerEscrow.t.sol (1)
345-353: Identifier-based settler wiring looks correct.Routing
outputSettlerSimplethroughtoIdentifier()keeps the witness hashing and oracle expectations consistent with the new encoding path. Looks solid.src/input/compact/InputSettlerCompact.sol (1)
150-155: Unified caller validation matches the base helper.Switching to
_validateIsCaller(orderOwner)aligns the compact flow with the sharedUnexpectedCallersurface and removes bespoke checks. Nice consolidation.src/input/InputSettlerBase.sol (1)
145-156: Caller guard helper is tight.Leveraging
LibAddress.fromIdentifierfor the comparison and funnelling mismatches throughUnexpectedCallergives downstream settlers a single, predictable error path.src/input/types/MandateOutputType.sol (1)
29-120: Type hash & memory helpers stay consistent.Updating the type string to
bytes calland mirroring calldata/memory hashing ensures Permit2 witnesses and Compact claims hash the same payloads everywhere.src/input/escrow/InputSettlerEscrow.sol (1)
438-456: Escrow finalise now reuses the base caller gate.Using
_validateIsCaller(orderOwner)gives the escrow path the same protection and revert semantics that the tests are asserting. Everything lines up.test/input/escrow/InputSettlerEscrow.base.t.sol (1)
151-186: Permit2 witness hashing update is accurate.Pre-hashing each
TokenPermissionsentry and collapsing with a single keccak matches the EIP-712 array encoding and syncs the test helper with on-chain expectations.test/integration/SettlerCompact.crosschain.t.sol (1)
239-327: Cross-chain witness helpers reflect the new schema.Renaming to
outputSettlerSimpleand updating the witness/type strings tobytes callkeeps the integration flow aligned with the contract changes.test/input/escrow/InputSettlerMultichainEscrow.base.t.sol (5)
1-31: LGTM: Clean imports and interface declarations.The imports are well-organized and the EIP712 interface provides a clean abstraction for accessing domain separators in tests.
33-49: LGTM: Appropriate test double for Wormhole messages.The mock implementation correctly exposes protected methods for testing and provides a minimal publishMessage implementation suitable for test scenarios.
72-107: LGTM: Comprehensive test fixture setup.The setUp function properly initializes all required contracts, oracles, test accounts, and the guardian set with appropriate validation.
151-196: LGTM: Correct cryptographic signature construction.Both
makeValidVAAandgetOrderOpenSignatureproperly implement their respective signature schemes with correct hashing and ECDSA signature encoding.
198-245: Type strings verified—no issues found.The EIP-712 type strings in
witnessHash(line 209) andoutputsHash(line 230) exactly match their counterparts in the implementation files:
- Line 209 matches
MULTICHAIN_PERMIT2_WITNESS_TYPEinsrc/input/escrow/Permit2MultichainWitnessType.sol:31- Line 230 matches
MANDATE_OUTPUT_TYPE_STUBinsrc/input/types/MandateOutputType.sol:30The
MandateOutputtype string duplication across multiple files is expected and correct for EIP-712, as the full type hierarchy must be included in witness type strings.
| chainIndex: order.chainIndex - 1, // We use chainIndex as the offset to elements array where compact | ||
| // uses it as offset to the notarized. | ||
| notarizedChainId: order.chainIdField |
There was a problem hiding this comment.
Guard chainIndex - 1 to avoid underflow on exogenous chains.
In the exogenous path we do chainIndex: order.chainIndex - 1. If a component for a non-origin chain is positioned at index 0 in the global elements ordering (totally valid for two components ordered [remote, origin]), this subtraction underflows and hard reverts, making those orders impossible to finalise. Please require order.chainIndex > 0 (and revert with a clear error) before subtracting, or otherwise adjust the logic so we never subtract blindly.
🤖 Prompt for AI Agents
In src/input/compact/InputSettlerMultichainCompact.sol around lines 223 to 225,
the exogenous path subtracts 1 from order.chainIndex without checking and can
underflow when chainIndex == 0; add a guard that requires order.chainIndex > 0
(revert with a clear error string) before performing chainIndex - 1, or alter
the branch so it never performs the subtraction for index 0 (e.g., handle the
origin-at-zero case separately), and update any dependent variables to use the
validated value.
| function openFor(MultichainOrderComponent calldata order, address sponsor, bytes calldata signature) external { | ||
| // Validate the order structure. | ||
| _validateTimestampHasNotPassed(order.fillDeadline); | ||
| _validateTimestampHasNotPassed(order.expires); | ||
|
|
||
| bytes32 orderId = _orderIdentifier(order); | ||
|
|
||
| if (orderStatus[orderId] != OrderStatus.None) revert InvalidOrderStatus(); | ||
| // Mark order as deposited. If we can't make the deposit, we will | ||
| // revert and it will unmark it. This acts as a reentry check. | ||
| orderStatus[orderId] = OrderStatus.Deposited; | ||
|
|
||
| // Check the first byte of the signature for signature type then collect inputs. | ||
| bytes1 signatureType = signature.length > 0 ? signature[0] : SIGNATURE_TYPE_SELF; | ||
| if (signatureType == SIGNATURE_TYPE_PERMIT2) { | ||
| _openForWithPermit2(order, orderId, sponsor, signature[1:], address(this)); | ||
| } else if (msg.sender == sponsor && signatureType == SIGNATURE_TYPE_SELF) { | ||
| _open(order); | ||
| } else { | ||
| revert SignatureNotSupported(signatureType); | ||
| } | ||
|
|
||
| // Validate that there has been no reentrancy. | ||
| if (orderStatus[orderId] != OrderStatus.Deposited) revert ReentrancyDetected(); | ||
|
|
||
| emit Open(orderId, abi.encode(order)); | ||
| } |
There was a problem hiding this comment.
Add _validateInputChain to openFor.
openFor currently skips the chain check, so a sponsor can deposit on the wrong chain. Later finalise/refund re-run _validateInputChain and revert, leaving the escrowed tokens stuck. Mirror the open path by validating the chain field before marking the order deposited.
_validateTimestampHasNotPassed(order.fillDeadline);
_validateTimestampHasNotPassed(order.expires);
+ _validateInputChain(order.chainIdField);📝 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.
| function openFor(MultichainOrderComponent calldata order, address sponsor, bytes calldata signature) external { | |
| // Validate the order structure. | |
| _validateTimestampHasNotPassed(order.fillDeadline); | |
| _validateTimestampHasNotPassed(order.expires); | |
| bytes32 orderId = _orderIdentifier(order); | |
| if (orderStatus[orderId] != OrderStatus.None) revert InvalidOrderStatus(); | |
| // Mark order as deposited. If we can't make the deposit, we will | |
| // revert and it will unmark it. This acts as a reentry check. | |
| orderStatus[orderId] = OrderStatus.Deposited; | |
| // Check the first byte of the signature for signature type then collect inputs. | |
| bytes1 signatureType = signature.length > 0 ? signature[0] : SIGNATURE_TYPE_SELF; | |
| if (signatureType == SIGNATURE_TYPE_PERMIT2) { | |
| _openForWithPermit2(order, orderId, sponsor, signature[1:], address(this)); | |
| } else if (msg.sender == sponsor && signatureType == SIGNATURE_TYPE_SELF) { | |
| _open(order); | |
| } else { | |
| revert SignatureNotSupported(signatureType); | |
| } | |
| // Validate that there has been no reentrancy. | |
| if (orderStatus[orderId] != OrderStatus.Deposited) revert ReentrancyDetected(); | |
| emit Open(orderId, abi.encode(order)); | |
| } | |
| function openFor(MultichainOrderComponent calldata order, address sponsor, bytes calldata signature) external { | |
| // Validate the order structure. | |
| _validateTimestampHasNotPassed(order.fillDeadline); | |
| _validateTimestampHasNotPassed(order.expires); | |
| _validateInputChain(order.chainIdField); | |
| bytes32 orderId = _orderIdentifier(order); | |
| if (orderStatus[orderId] != OrderStatus.None) revert InvalidOrderStatus(); | |
| // Mark order as deposited. If we can't make the deposit, we will | |
| // revert and it will unmark it. This acts as a reentry check. | |
| orderStatus[orderId] = OrderStatus.Deposited; | |
| // Check the first byte of the signature for signature type then collect inputs. | |
| bytes1 signatureType = signature.length > 0 ? signature[0] : SIGNATURE_TYPE_SELF; | |
| if (signatureType == SIGNATURE_TYPE_PERMIT2) { | |
| _openForWithPermit2(order, orderId, sponsor, signature[1:], address(this)); | |
| } else if (msg.sender == sponsor && signatureType == SIGNATURE_TYPE_SELF) { | |
| _open(order); | |
| } else { | |
| revert SignatureNotSupported(signatureType); | |
| } | |
| // Validate that there has been no reentrancy. | |
| if (orderStatus[orderId] != OrderStatus.Deposited) revert ReentrancyDetected(); | |
| emit Open(orderId, abi.encode(order)); | |
| } |
🤖 Prompt for AI Agents
In src/input/escrow/InputSettlerMultichainEscrow.sol around lines 132 to 158,
openFor lacks the chain validation present in open, which allows sponsors to
deposit on the wrong chain and lock funds; call
_validateInputChain(order.inputChain) early (before marking orderStatus =
OrderStatus.Deposited and before performing any state changes) so the input
chain is checked and the function reverts on mismatch, mirroring the open path’s
behavior.
| Lock memory lock | ||
| ) internal pure returns (uint256[2] memory arr) { | ||
| bytes12 lockTag = lock.lockTag; | ||
| address lockToken = lock.token; | ||
| uint256 elem0; | ||
| assembly { | ||
| elem0 := add(lockTag, lockToken) | ||
| } | ||
| arr[0] = elem0; | ||
| arr[1] = lock.amount; |
There was a problem hiding this comment.
Fix lock encoding so round-trips preserve the lock tag.
Adding lockTag and lockToken drops the lock tag into the low 12 bytes instead of the high 12 bytes, so uintToLock later recovers a zero lockTag. Pack the fields by shifting the 12‑byte tag left by 160 bits and OR-ing the 20‑byte token address, matching the identifier format expected on-chain.
- assembly {
- elem0 := add(lockTag, lockToken)
- }
+ uint256 elem0 = (uint256(uint96(lockTag)) << 160) | uint256(uint160(lockToken));Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test/input/compact/InputSettlerMultichainCompact.base.t.sol around lines 144
to 153, the current assembly uses add(lockTag, lockToken) which places the
12-byte lockTag into the low bytes instead of the high bytes and causes
uintToLock to recover a zero tag; replace the addition with a pack that shifts
the 12-byte tag left by 160 bits and ORs (or uses bitwise or) the 20-byte token
into the low 160 bits so the identifier matches on-chain format (i.e., compute
elem0 = (uint256(lockTag) << 160) | uint256(uint160(lockToken))).
…to compact-multichain-1
…to compact-multichain-1
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/input/compact/InputSettlerMultichainCompact.sol (1)
208-223: Guard againstchainIndex - 1underflow in the exogenous claim.Line 219 computes
order.chainIndex - 1with no prior check. If the exogenous component happens to occupy position 0 in the global ordering, this subtraction underflows and every exogenous finalise hard-reverts. Add an explicit guard (or adjust the arithmetic) before subtracting so index 0 is either handled separately or rejected with a clear error.+ if (order.chainIdField != block.chainid) { + if (order.chainIndex == 0) revert InvalidChainIndex(); + } ... - chainIndex: order.chainIndex - 1, + chainIndex: order.chainIndex - 1,(Add
error InvalidChainIndex();near the other custom errors.)src/input/escrow/InputSettlerMultichainEscrow.sol (1)
132-162: Validate the chain field inopenForto avoid trapping deposits.Unlike
open,openForskips_validateInputChain. A sponsor can therefore deposit on the wrong chain,orderStatusflips to Deposited, and every laterfinalise/refundhits_validateInputChainand reverts—locking user funds. Call_validateInputChain(order.chainIdField);before computing the orderId to mirror theopenpath._validateTimestampHasNotPassed(order.fillDeadline); _validateTimestampHasNotPassed(order.expires); + _validateInputChain(order.chainIdField); bytes32 orderId = _orderIdentifier(order);src/input/types/MultichainCompactOrderType.sol (1)
16-116: Remove forge console from the production hashing path.Line 108 still calls
console.logBytes(...), and the import at Line 16 brings in Foundry’s cheatcode. On a live chain this external cheatcode address doesn’t exist, so anyorderIdentifiercall will revert, breaking every consumer (including InputSettlerMultichainCompact.orderIdentifier). Please drop the console import/call before shipping.-import { console } from "forge-std/console.sol"; ... - console.logBytes( - abi.encode( - ELEMENTS_COMPACT_TYPEHASH_WITH_WITNESS, - address(this), - block.chainid, - inputsToLocksHash(order.inputs), - witnessHash(order) - ) - );test/input/compact/InputSettlerMultichainCompact.base.t.sol (1)
125-134: Lock packing still stomps the tag bytes.
add(lockTag, lockToken)writes the tag into the low 160 bits, souintToLocklater recovers a zerolockTag. We need to left-shift the tag by 160 bits before OR-ing the token address to preserve the on-chain layout.- uint256 elem0; - assembly { - elem0 := add(lockTag, lockToken) - } + uint256 elem0 = (uint256(uint96(lockTag)) << 160) | uint256(uint160(lockToken));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
snapshots/inputSettler.json(1 hunks)src/input/InputSettlerBase.sol(2 hunks)src/input/compact/InputSettlerMultichainCompact.sol(1 hunks)src/input/escrow/InputSettlerEscrow.sol(2 hunks)src/input/escrow/InputSettlerMultichainEscrow.sol(1 hunks)src/input/escrow/Permit2MultichainWitnessType.sol(1 hunks)src/input/types/MandateOutputType.sol(2 hunks)src/input/types/MultichainCompactOrderType.sol(1 hunks)test/input/compact/InputSettlerCompact.base.t.sol(2 hunks)test/input/compact/InputSettlerCompact.t.sol(6 hunks)test/input/compact/InputSettlerMultichainCompact.base.t.sol(1 hunks)test/input/compact/InputSettlerMultichainCompact.t.sol(1 hunks)test/input/escrow/InputSettlerEscrow.base.t.sol(5 hunks)test/input/escrow/InputSettlerMultichainEscrow.base.t.sol(1 hunks)test/input/escrow/InputSettlerMultichainEscrow.t.sol(1 hunks)test/integration/SettlerCompact.crosschain.t.sol(10 hunks)test/libs/MultichainOrderComponentsType.sol(1 hunks)test/output/OutputSettlerSimple.fill.t.sol(41 hunks)test/output/OutputSettlerSimple.fillOrderOutputs.t.sol(18 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/input/compact/InputSettlerCompact.base.t.sol
🚧 Files skipped from review as they are similar to previous changes (7)
- src/input/escrow/InputSettlerEscrow.sol
- test/input/compact/InputSettlerCompact.t.sol
- test/output/OutputSettlerSimple.fillOrderOutputs.t.sol
- test/integration/SettlerCompact.crosschain.t.sol
- src/input/types/MandateOutputType.sol
- src/input/InputSettlerBase.sol
- snapshots/inputSettler.json
🔇 Additional comments (3)
test/output/OutputSettlerSimple.fill.t.sol (1)
24-865: LGTM! Clean and consistent refactoring.The systematic renaming from
OutputSettlerCointoOutputSettlerSimpleis complete and consistent throughout the test file. All variable declarations, address references, approvals, gas snapshot labels, and MandateOutput constructions have been properly updated. The test logic remains unchanged, ensuring continued coverage of the OutputSettler functionality.test/input/escrow/InputSettlerEscrow.base.t.sol (2)
36-36: LGTM: Consistent renaming to OutputSettlerSimple.The variable rename from
outputSettlerCointooutputSettlerSimpleis applied consistently across declaration, initialization, and usage. This aligns with the broader test refactoring mentioned in the PR summary.Also applies to: 72-72, 89-89
156-163: LGTM: Correct EIP-712 array encoding for Permit2.The refactored token permissions hashing correctly implements EIP-712 encoding for arrays of structs: each
TokenPermissionselement is hashed individually, then the array of hashes is packed and hashed together. This aligns with thePermitBatchWitnessTransferFromwitness type structure on line 172.Also applies to: 174-174
|
The current version has a severe bug related to the encoding of the oracles. Since oracle addresses vary by chain, it is likely that the same orderId cannot be replicated on different chains. The oracles should be considered moved into the input components. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/input/types/MultichainCompactOrderType.sol (1)
16-16: Remove unused console import.The
consoleimport is no longer used after theconsole.logBytescall was removed per the previous review. Development dependencies like forge-std should not remain in production code.Apply this diff to remove the unused import:
-import { console } from "forge-std/console.sol";
🧹 Nitpick comments (2)
src/input/types/MultichainCompactOrderType.sol (2)
18-23: Update library documentation to match actual functionality.The documentation references
TYPE_PARTIAL,TYPE_STUB, andTYPEwhich are not used in the code. Consider replacing this with a description of the library's actual purpose: computing EIP-712 structured hashes and order identifiers for multichain compact orders.
83-83: Consider the suggested assembly optimization.The TODO suggests replacing the ternary with
sub(i, gt(i, index))which would be equivalent and potentially save gas. This is a valid micro-optimization if gas efficiency is a priority.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/input/types/MultichainCompactOrderType.sol(1 hunks)test/input/compact/InputSettlerMultichainCompact.base.t.sol(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Foundry Smart Contract Test
test/input/compact/InputSettlerMultichainCompact.base.t.sol
[error] 1-1: forge fmt --check found formatting diffs that need to be fixed in InputSettlerMultichainCompact.base.t.sol.
🔇 Additional comments (6)
src/input/types/MultichainCompactOrderType.sol (4)
43-62: LGTM!The function correctly computes an EIP-712 compliant hash of the input locks. The memory-safe assembly usage is appropriate for efficient array construction.
64-90: LGTM!The insertion and hashing logic is correct. The function properly inserts the new element at the specified index and adjusts array indices accordingly.
95-114: Verify cross-chain orderId consistency with contract address dependency.The function uses
address(this)(line 102) in the elementHash computation, which will differ if contracts are deployed at different addresses on different chains. This aligns with the known bug mentioned in the PR objectives: "oracle addresses vary by chain, making identical orderId replication across chains unlikely."Please confirm this limitation is acceptable for the current implementation or if it requires immediate resolution before merge.
116-128: LGTM!The witness hash computation correctly reuses the StandardOrder witness type and properly encodes the mandate fields. The pure function designation is appropriate.
test/input/compact/InputSettlerMultichainCompact.base.t.sol (2)
131-135: Fix lock identifier packing.
add(lockTag, lockToken)pushes the 12‑byte tag down into the low bits, souintToLocklater reads a zero tag; the lock identifier no longer round-trips. Please shift the tag into the upper 96 bits and OR the token into the low 160 bits so the encoding matches the on-chain format. Based on learningsduplicate_commentraise_critical_issue
Apply this diff:- assembly { - elem0 := add(lockTag, lockToken) - } + uint256 elem0 = + (uint256(uint96(lockTag)) << 160) | uint256(uint160(lockToken));
248-256:vm.signbreakspuremutability.This helper calls the cheatcode
vm.sign, which is neitherviewnorpure. Marking the functionpuremakes the contract fail to compile. Drop thepuremutability (or move the signing out) so the code builds. raise_critical_issue
Apply this diff:- ) internal pure returns (bytes memory sig) { + ) internal returns (bytes memory sig) {
Description
A multichain Input intent is an intent that has assets inputs, provided by the user, on multiple chains in exchange for a singular set of outputs. Such an intent has strong conditional guarantees on the outputs: Either all outputs are delivered in exchange for all inputs or none inputs are provided. An example of such an intent could be:
User provides:
In exchange for:
The system now supports 2 types of orders:
StandardOrder: Multiple inputs on 1 chain with multiple outputs on multiple chains.MultichainOrderComponent: Multiple inputs on multiple chains with multiple outputs on multiple chains.Additionally, 2 collection mechanisms have been implemented in accordance with discussions:
The Compact: Maps aMultichainOrderComponenttoMultichainCompactwithMultichainOrderComponent.additionalChainsbeing element hashes.Escrow: EachMultichainOrderComponentis a separate orders that has to be collected withMultichainOrderComponent.additionalChainsbeing a custom hashing of component inputs.Bug:
The current version has a severe bug related to the encoding of the oracles. Since oracle addresses vary by chain, it is likely that the same orderId cannot be replicated on different chains. The oracles should be considered moved into the input components.
Notable Quirks:
OrderComponents
A multichain order is never fully represented on chain. Multichain order can potentially be fairly big and it would significant gas and calldata overhead. As a result, each order is only represented on-chain in components. Thus they need a way to be passed off-chain. This PR does not relate to that structure. Additionally, like the
StandardOrdereach component is never signed in its entirety. It is signed by proxy through either Compact or Permit2. As a result, there are 3 types of orders in the system:MultichainOrderComponentPermitBatchWitnessTransferFromorMultichainCompactCompact claim hash
Instead of using a custom orderId for
InputSettlerMultichainCompactthe Compact claim hash is used instead. This ensures that the orderId stays fixed cross-chain. However, this requires re-ordering the action items inside the settler so the claim is settled before the outputs are verified.Purchase logic.
Purchase logic has been removed. It does not fit cleanly inside these implementations.
Open Tasks
Compact:
Escrow:
Check how TheCompact does it)Summary by CodeRabbit
New Features
Refactor
Public API
Chores