Phase 1 #204
Phase 1 #204
Conversation
WalkthroughConverts request/count-based fees to a payload-centric gas model: replaces credits/fees with gas accounting and escrow, makes Watcher manage per-payload lifecycle with payloadId (bytes32), adds Solana support (Ed25519/Borsh/PDA), and bumps Solidity to 0.8.28. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
rect rgba(0,128,255,0.06)
participant App as AppGateway
participant Watcher as Watcher
participant Precomp as Precompile
end
rect rgba(0,200,100,0.06)
participant Escrow as GasEscrow
participant Switch as Switchboard
participant Socket as Socket
end
App->>Watcher: addPayloadData(RawPayload, appGateway)
Watcher->>Watcher: compute payloadId, store RawPayload
App->>Watcher: executePayload()
Watcher->>Precomp: handlePayload(RawPayload, appGateway, payloadId)
Precomp-->>Watcher: (fees, deadline, precompileData)
Watcher->>Escrow: escrowGas(payloadId, amount)
Watcher->>Watcher: emit PayloadSubmitted
Note over Socket,Switch: cross-chain processing
App->>Socket: sendPayload(callData)
Socket->>Switch: processPayload(plug, payload, overrides)
Switch-->>Socket: returns payloadId
Socket->>Socket: set executionStatus[payloadId] = Active
Note over Watcher,Precomp: resolution phase
Watcher->>Precomp: resolvePayload(Payload)
Precomp-->>Watcher: resolution result
Watcher->>Escrow: settleGasPayment(payloadId, consumeFrom, transmitter, amount)
Watcher->>Watcher: emit PromiseResolved / PayloadResolved
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
deprecated/PromiseResolver.sol (2)
57-60: Missing import for DeadlinePassed() causes compile errorDeadlinePassed() is used but not imported. This should import the symbol from Errors.sol.
-import {DeadlineNotPassedForOnChainRevert} from "../../utils/common/Errors.sol"; +import {DeadlineNotPassedForOnChainRevert, DeadlinePassed} from "../../utils/common/Errors.sol";
45-51: updateRequest signature mismatch in PromiseResolver
The call toupdateRequest(requestCount)indeprecated/PromiseResolver.solmust include thefeesUsedargument required by its definition (uint40,uint256); for example:- requestHandler__().updateRequest(requestCount); + requestHandler__().updateRequest(requestCount, 0);contracts/evmx/fees/Credit.sol (1)
140-149: Critical: incorrect minting/accounting when native withdraw failsOn withdraw failure you mint
creditAmount_again, then setnativeAmount_ = 0before adding it back. This double-mints credits and loses the native amount.This should mint
nativeAmount_as credits and update event values correctly.Apply:
- if (!success) { - _mint(depositTo_, creditAmount_); - nativeAmount_ = 0; - creditAmount_ += nativeAmount_; - } + if (!success) { + // convert native to credits + _mint(depositTo_, nativeAmount_); + creditAmount_ += nativeAmount_; + nativeAmount_ = 0; + }contracts/evmx/watcher/precompiles/SchedulePrecompile.sol (1)
140-145: Update resolvePayload to match the new interface.
IPrecompile.resolvePayloadnow expectsPayload calldata, but this override still takesPayloadParams calldata. This should be updated or the contract will no longer compile against the interface and the watcher cannot resolve scheduled payloads. Apply the signature change and forward the new struct fields:- function resolvePayload(PayloadParams calldata payloadParams_) external onlyRequestHandler { - (, uint256 executeAfter) = abi.decode(payloadParams_.precompileData, (uint256, uint256)); - - if (executeAfter > block.timestamp) revert ResolvingScheduleTooEarly(); - emit ScheduleResolved(payloadParams_.payloadId); - } + function resolvePayload(Payload calldata payload) external onlyRequestHandler { + (, uint256 executeAfter) = abi.decode(payload.precompileData, (uint256, uint256)); + + if (executeAfter > block.timestamp) revert ResolvingScheduleTooEarly(); + emit ScheduleResolved(payload.payloadId); + }contracts/evmx/watcher/Configurations.sol (1)
45-122: Restore initialization of owner and watcher resolver.The earlier version called
_initializeWatcherand_initializeOwnerduring deployment. After droppingWatcherBase/Initializablethere is no path that sets the owner or the address resolver, so everyonlyOwner/onlyWatcherentry point will revert. This should reinstate an initializer/constructor that seeds the resolver + calls_initializeOwner(owner_)before these modifiers are used.contracts/evmx/base/AppGatewayBase.sol (1)
12-76: Fix enums and promise wiring to match the new OverrideParams shape.Line 12 still imports
IsPlug,Read, andParallel, but those enums were removed fromStructs.sol. This should drop the imports, otherwise compilation fails. Lines 67-88 manipulateoverrideParams.isAsyncModifierSet, yetOverrideParamsno longer contains that field. Replace this with whatever flag now lives alongside the override state (or remove the flag entirely) so the code compiles and actually reflects the new override lifecycle. Finally, Line 75 setsisValidPromise[promise_] = trueeven thoughpromise_is undefined after the previous retrieval was commented out. This should reinstate the watcher call (or otherwise provide the promise address) before assigning, or_postAsyncwill not compile.
🧹 Nitpick comments (13)
deprecated/RequestHandler.sol (1)
92-98: getPrecompileFees should validate callTypeWithout a guard, this will call .getPrecompileFees on address(0) and revert ambiguously. This should revert with InvalidCallType when unset.
function getPrecompileFees( bytes4 callType_, bytes memory precompileData_ ) external view returns (uint256) { - return precompiles[callType_].getPrecompileFees(precompileData_); + if (address(precompiles[callType_]) == address(0)) revert InvalidCallType(); + return precompiles[callType_].getPrecompileFees(precompileData_); }contracts/evmx/interfaces/IFeesManager.sol (1)
3-3: Trim unused importsThe interface does not use WriteFinality, AppGatewayApprovals, OverrideParams, Transaction, RawPayload, RequestParams. This should import only what’s needed to keep the surface lean.
-import {WriteFinality, AppGatewayApprovals, OverrideParams, Transaction, RawPayload, RequestParams} from "../../utils/common/Structs.sol"; +// No struct types referenced; drop unused importscontracts/evmx/interfaces/IAuctionManager.sol (1)
4-4: Remove unused Structs importsThis interface does not reference RawPayload, OverrideParams, Transaction, Bid, RequestParams. This should remove the import.
-import {RawPayload, OverrideParams, Transaction, Bid, RequestParams} from "../../utils/common/Structs.sol"; +// No struct types referenced; drop unused importsFunctionSignatures.md (1)
482-482: Rename noted; add deprecation note and cross-referenceupdateRequest replaced updateRequestAndProcessBatch but retains the same selector 0x46464471. This should be called out explicitly to avoid confusion in external tooling and off-chain decoders. Add a deprecation note for the old name and a brief mapping.
contracts/evmx/fees/Credit.sol (1)
274-283: Variable name shadows type; use lowerCamelCase for clarityDeclaring
RawPayload memory RawPayload;shadows the type and hurts readability. This should berawPayload.Apply:
- RawPayload memory RawPayload; - RawPayload.overrideParams = overrideParams; - RawPayload.transaction = Transaction({ + RawPayload memory rawPayload; + rawPayload.overrideParams = overrideParams; + rawPayload.transaction = Transaction({ chainSlug: chainSlug_, target: _getFeesPlugAddress(chainSlug_), payload: payload_ }); - RawPayload.switchboardType = sbType; - watcher__().queue(RawPayload, address(this)); + rawPayload.switchboardType = sbType; + watcher__().queue(rawPayload, address(this));test/evmx/Watcher.t.sol (2)
264-265: Fix variable shadowing of type name
RawPayload[] memory RawPayloadshadows the type and reduces readability. This should use a lowerCamelCase identifier and propagate to call sites.- RawPayload[] memory RawPayload = new RawPayload[](0); + RawPayload[] memory rawPayloads = new RawPayload[](0); @@ - RawPayload, + rawPayloads, @@ - RawPayload, + rawPayloads,Also applies to: 272-273, 283-284
402-418: Test name casing and intentConsider renaming to
testRequestHandlerUpdateRequestfor consistency with other test names. No functional change needed.test/mock/MockWatcherPrecompile.sol (1)
26-29: Normalize parameter names (no type-name shadowing)Use lowerCamelCase for parameters to avoid shadowing type names and improve readability.
- function queue( - RawPayload calldata RawPayload_, - address appGateway_ - ) external override returns (address, uint40) {} + function queue( + RawPayload calldata rawPayload_, + address appGateway_ + ) external override returns (address, uint40) {} @@ - function queueAndSubmit( - RawPayload memory queue_, + function queueAndSubmit( + RawPayload memory rawPayload_, uint256 maxFees, address auctionManager, address consumeFrom, bytes calldata onCompleteData ) external override returns (uint40 requestCount, address[] memory promises) {}Also applies to: 40-46
contracts/evmx/interfaces/IAppGateway.sol (1)
13-16: Update docstring to match APIComment still says “Gets the override parameters”. This should reflect ForwarderParams.
- /// @notice Gets the override parameters - /// @return overrideParams_ The override parameters + /// @notice Gets forwarder parameters (including override params for payload execution) + /// @return params The forwarder parameters function getForwarderParams() external view returns (ForwarderParams memory);contracts/evmx/interfaces/IPrecompile.sol (1)
14-23: Doc/param mismatch in handlePayloadhandlePayload now takes (rawPayload, appGateway, payloadId) but docs only describe rawPayload. This should document all params and returns.
- /// @notice Handles payload processing and returns processed payload - /// @param rawPayload The payload parameters to handle - /// @return fees The fees required for processing - /// @return deadline The deadline for processing - /// @return precompileData The encoded precompile data + /// @notice Handles payload processing and returns computed fees, deadline and encoded precompile data + /// @param rawPayload The raw payload to handle (includes override params and transaction) + /// @param appGateway The app gateway invoking/preparing the payload + /// @param payloadId The unique payload identifier + /// @return fees The fees required for processing + /// @return deadline The deadline for processing + /// @return precompileData The encoded precompile datacontracts/evmx/helpers/Forwarder.sol (1)
93-106: RenameRawPayloadvariable in Forwarder.sol torawPayload- RawPayload memory RawPayload; - RawPayload.overrideParams = forwarderParams.overrideParams; - RawPayload.transaction = Transaction({ + RawPayload memory rawPayload; + rawPayload.overrideParams = forwarderParams.overrideParams; + rawPayload.transaction = Transaction({ chainSlug: chainSlug, target: getOnChainAddress(), payload: msg.data }); - watcher__().executePayload( + watcher__().executePayload( forwarderParams.overrideParams.maxFees, forwarderParams.overrideParams.consumeFrom, msgSender, - RawPayload + rawPayload );deprecated/Watcher.sol (1)
28-38: Validate core contract addresses before settingZero addresses here brick the instance at runtime. This should enforce non-zero checks.
function setCoreContracts( address requestHandler_, address configManager_, address promiseResolver_ ) external onlyOwner { + require(requestHandler_ != address(0) && configManager_ != address(0) && promiseResolver_ != address(0), "zero addr"); requestHandler__ = IRequestHandler(requestHandler_); configurations__ = IConfigurations(configManager_); promiseResolver__ = IPromiseResolver(promiseResolver_);contracts/evmx/watcher/Watcher.sol (1)
176-182: Modifier name suggests wrong polarityincreaseFees and cancelRequest use isPayloadCancelled(payloadId_) while they proceed to cancel or modify fees. This should guard on “not cancelled” or a differently named modifier; confirm semantics to avoid logic bugs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
FunctionSignatures.md(1 hunks)contracts/evmx/base/AppGatewayBase.sol(8 hunks)contracts/evmx/fees/Credit.sol(1 hunks)contracts/evmx/helpers/Forwarder.sol(2 hunks)contracts/evmx/interfaces/IAppGateway.sol(2 hunks)contracts/evmx/interfaces/IAuctionManager.sol(1 hunks)contracts/evmx/interfaces/IFeesManager.sol(1 hunks)contracts/evmx/interfaces/IPrecompile.sol(2 hunks)contracts/evmx/interfaces/IRequestHandler.sol(1 hunks)contracts/evmx/interfaces/IWatcher.sol(1 hunks)contracts/evmx/watcher/Configurations.sol(2 hunks)contracts/evmx/watcher/RequestHandler.sol(0 hunks)contracts/evmx/watcher/Watcher.sol(1 hunks)contracts/evmx/watcher/precompiles/ReadPrecompile.sol(1 hunks)contracts/evmx/watcher/precompiles/SchedulePrecompile.sol(1 hunks)contracts/evmx/watcher/precompiles/WritePrecompile.sol(2 hunks)contracts/utils/OverrideParamsLib.sol(1 hunks)contracts/utils/common/Structs.sol(3 hunks)deprecated/AuctionManager.sol(1 hunks)deprecated/Configurations.sol(1 hunks)deprecated/ForwarderV2.sol(1 hunks)deprecated/PromiseResolver.sol(1 hunks)deprecated/RequestHandler.sol(1 hunks)deprecated/Watcher.sol(1 hunks)test/evmx/Watcher.t.sol(4 hunks)test/mock/MockWatcherPrecompile.sol(2 hunks)
💤 Files with no reviewable changes (1)
- contracts/evmx/watcher/RequestHandler.sol
🔇 Additional comments (1)
contracts/utils/OverrideParamsLib.sol (1)
12-26: Fluent builder looks solid; defaults are reasonableclear() initializes sensible defaults and threads switchboardType. Setters correctly mutate memory and return self.
| /// @param payload The payload parameters to resolve | ||
| function resolvePayload(Payload calldata payload) external; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Align resolvePayload docs with new type
Good switch to Payload; ensure implementers are updated.
Verify all IPrecompile implementers use new signatures:
🏁 Script executed:
#!/bin/bash
# List contracts implementing IPrecompile and check handlePayload/resolvePayload signatures
rg -n -C2 -P 'contract\s+\w+\s+is\s+[^}]*\bIPrecompile\b' contracts
rg -n -C2 -P '\bfunction\s+handlePayload\s*\(' contracts | rg -n -C0 -P 'RawPayload'
rg -n -C2 -P '\bfunction\s+resolvePayload\s*\(' contracts | rg -n -C0 -P 'Payload'Length of output: 2693
Fix resolvePayload signature in IPrecompile implementers
- WritePrecompile.sol:246 — replace
PayloadParams calldata payloadParams_withPayload calldata payload - SchedulePrecompile.sol:140 — replace
PayloadParams calldata payloadParams_withPayload calldata payload
🤖 Prompt for AI Agents
In contracts/evmx/interfaces/IPrecompile.sol around lines 26 to 28, implementers
have mismatched function signatures: replace the parameter type and name in
WritePrecompile.sol at line ~246 and SchedulePrecompile.sol at line ~140 from
"PayloadParams calldata payloadParams_" to "Payload calldata payload"; update
the function declaration to match the interface, rename occurrences of
payloadParams_ to payload within each file (including any field accesses), and
ensure any imports or type aliases for Payload are present so the signatures and
internal references compile.
| function submitRequest( | ||
| uint256 maxFees_, | ||
| address auctionManager_, | ||
| address consumeFrom_, | ||
| address appGateway_, | ||
| QueueParams[] calldata queueParams_, | ||
| RawPayload calldata RawPayload_, | ||
| bytes memory onCompleteData_ | ||
| ) external returns (uint40 requestCount, address[] memory promiseList); |
There was a problem hiding this comment.
Align submitRequest with ForwarderParams.
IWatcher.submitRequest now forwards a ForwarderParams struct, so this interface still demanding the old (maxFees_, consumeFrom_, appGateway_) tuple will no longer compile or link. This should switch to the new struct signature (and stick to calldata to avoid copies):
- function submitRequest(
- uint256 maxFees_,
- address consumeFrom_,
- address appGateway_,
- RawPayload calldata RawPayload_,
- bytes memory onCompleteData_
- ) external returns (uint40 requestCount, address[] memory promiseList);
+ function submitRequest(
+ ForwarderParams calldata forwarderParams_,
+ RawPayload calldata rawPayload_,
+ bytes calldata onCompleteData_
+ ) external returns (uint40 requestCount, address[] memory promiseList);📝 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 submitRequest( | |
| uint256 maxFees_, | |
| address auctionManager_, | |
| address consumeFrom_, | |
| address appGateway_, | |
| QueueParams[] calldata queueParams_, | |
| RawPayload calldata RawPayload_, | |
| bytes memory onCompleteData_ | |
| ) external returns (uint40 requestCount, address[] memory promiseList); | |
| function submitRequest( | |
| ForwarderParams calldata forwarderParams_, | |
| RawPayload calldata rawPayload_, | |
| bytes calldata onCompleteData_ | |
| ) external returns (uint40 requestCount, address[] memory promiseList); |
🤖 Prompt for AI Agents
In contracts/evmx/interfaces/IRequestHandler.sol around lines 21 to 27, the
submitRequest signature still expects the old (maxFees_, consumeFrom_,
appGateway_) tuple; update it to accept the ForwarderParams struct as calldata
and use calldata for the onCompleteData to avoid copying: replace the three
separate parameters with ForwarderParams calldata forwarderParams_ and change
bytes memory onCompleteData_ to bytes calldata onCompleteData_, keeping the
return types the same so the interface matches the new IWatcher.forwarder-based
call site.
| if (rawPayload_.overrideParams.gasLimit == 0) { | ||
| if (rawPayload_.transaction.chainSlug == 5000) { | ||
| // Mantle default gas limit | ||
| gasLimit = 8_000_000_000; | ||
| r else if (rawPayload_.transaction.chainSlug == 1329) { | ||
| // Sei default gas limit | ||
| gasLimit = 8_000_000; | ||
| r else if (rawPayload_.transaction.chainSlug == 999) { | ||
| // HyperEVM default gas limit | ||
| gasLimit = 1_500_000; | ||
| } else { | ||
| gasLimit = 10_000_000; // other chains default gas limit | ||
| } | ||
| } |
There was a problem hiding this comment.
Return the correct gas limit.
Line 175 only sets gasLimit if the override value is zero. When a caller provides a positive overrideParams.gasLimit, this function returns the default-initialized 0, which later flows into precompileData, DigestParams, and ultimately the digest hash. This should assign gasLimit = rawPayload_.overrideParams.gasLimit up front and then overwrite it only when the override was zero; otherwise every request will be digested with an incorrect gas limit. While fixing that, also clean up the r else if tokens on Lines 179 and 183—those typos currently break compilation.
🤖 Prompt for AI Agents
In contracts/evmx/watcher/precompiles/WritePrecompile.sol around lines 175 to
188, the function never assigns gasLimit from
rawPayload_.overrideParams.gasLimit so a positive override is ignored and zero
is returned into the digest; also two occurrences of "r else if" are typos that
break compilation. Fix by initializing gasLimit =
rawPayload_.overrideParams.gasLimit before the conditional, then only enter the
chainSlug-based defaults when overrideParams.gasLimit == 0; remove the stray
leading "r" characters so the conditional branches read "else if" correctly.
| function submitRequest( | ||
| uint256 maxFees_, | ||
| address consumeFrom_, | ||
| address appGateway_, | ||
| QueueParams calldata queueParams_, | ||
| bytes memory onCompleteData_ | ||
| ) external onlyWatcher returns (uint40 requestCount, address[] memory promiseList) { | ||
| if (!feesManager__().isCreditSpendable(consumeFrom_, appGateway_, maxFees_)) | ||
| revert InsufficientFees(); | ||
|
|
||
| RequestParams storage r = _requests[requestCount]; | ||
| r.requestFeesDetails.maxFees = maxFees_; | ||
| r.requestFeesDetails.consumeFrom = consumeFrom_; | ||
| requestCount = nextRequestCount++; | ||
|
|
||
| bytes4 callType = queueParams_.overrideParams.callType; | ||
|
|
||
| (PayloadParams memory p, uint256 estimatedFees) = _createRequest( | ||
| queueParams_, | ||
| appGateway_, | ||
| requestCount | ||
| ); | ||
|
|
||
| if (estimatedFees > maxFees_) revert InsufficientMaxFees(); | ||
| feesManager__().blockCredits(requestCount, r.requestFeesDetails.consumeFrom, estimatedFees); | ||
| r.payloadId = p.payloadId; | ||
| _processBatch(r); | ||
|
|
||
| emit RequestSubmitted(callType == WRITE, requestCount, estimatedFees, r, p); | ||
| } |
There was a problem hiding this comment.
submitRequest uses requestCount before assignment; appGateway not stored; promiseList uninitialized
- r is taken from _requests[requestCount] before requestCount is set → writes to index 0.
- r.appGateway is never set, breaking later checks (OnlyAppGateway).
- promiseList is a named return but never assigned → uninitialized return.
This should set requestCount first, then r, store appGateway, and initialize promiseList.
- RequestParams storage r = _requests[requestCount];
- r.requestFeesDetails.maxFees = maxFees_;
- r.requestFeesDetails.consumeFrom = consumeFrom_;
- requestCount = nextRequestCount++;
+ requestCount = nextRequestCount++;
+ RequestParams storage r = _requests[requestCount];
+ r.requestFeesDetails.maxFees = maxFees_;
+ r.requestFeesDetails.consumeFrom = consumeFrom_;
+ r.appGateway = appGateway_;
@@
- emit RequestSubmitted(callType == WRITE, requestCount, estimatedFees, r, p);
+ // No promises in deprecated flow; return empty list for ABI correctness
+ promiseList = new address[](0);
+ emit RequestSubmitted(callType == WRITE, requestCount, estimatedFees, r, p);📝 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 submitRequest( | |
| uint256 maxFees_, | |
| address consumeFrom_, | |
| address appGateway_, | |
| QueueParams calldata queueParams_, | |
| bytes memory onCompleteData_ | |
| ) external onlyWatcher returns (uint40 requestCount, address[] memory promiseList) { | |
| if (!feesManager__().isCreditSpendable(consumeFrom_, appGateway_, maxFees_)) | |
| revert InsufficientFees(); | |
| RequestParams storage r = _requests[requestCount]; | |
| r.requestFeesDetails.maxFees = maxFees_; | |
| r.requestFeesDetails.consumeFrom = consumeFrom_; | |
| requestCount = nextRequestCount++; | |
| bytes4 callType = queueParams_.overrideParams.callType; | |
| (PayloadParams memory p, uint256 estimatedFees) = _createRequest( | |
| queueParams_, | |
| appGateway_, | |
| requestCount | |
| ); | |
| if (estimatedFees > maxFees_) revert InsufficientMaxFees(); | |
| feesManager__().blockCredits(requestCount, r.requestFeesDetails.consumeFrom, estimatedFees); | |
| r.payloadId = p.payloadId; | |
| _processBatch(r); | |
| emit RequestSubmitted(callType == WRITE, requestCount, estimatedFees, r, p); | |
| } | |
| function submitRequest( | |
| uint256 maxFees_, | |
| address consumeFrom_, | |
| address appGateway_, | |
| QueueParams calldata queueParams_, | |
| bytes memory onCompleteData_ | |
| ) external onlyWatcher returns (uint40 requestCount, address[] memory promiseList) { | |
| if (!feesManager__().isCreditSpendable(consumeFrom_, appGateway_, maxFees_)) | |
| revert InsufficientFees(); | |
| // Assign the new requestCount before accessing the storage slot | |
| requestCount = nextRequestCount++; | |
| RequestParams storage r = _requests[requestCount]; | |
| r.requestFeesDetails.maxFees = maxFees_; | |
| r.requestFeesDetails.consumeFrom = consumeFrom_; | |
| r.appGateway = appGateway_; | |
| bytes4 callType = queueParams_.overrideParams.callType; | |
| (PayloadParams memory p, uint256 estimatedFees) = _createRequest( | |
| queueParams_, | |
| appGateway_, | |
| requestCount | |
| ); | |
| if (estimatedFees > maxFees_) revert InsufficientMaxFees(); | |
| feesManager__().blockCredits(requestCount, r.requestFeesDetails.consumeFrom, estimatedFees); | |
| r.payloadId = p.payloadId; | |
| _processBatch(r); | |
| // No promises in deprecated flow; return empty list for ABI correctness | |
| promiseList = new address[](0); | |
| emit RequestSubmitted(callType == WRITE, requestCount, estimatedFees, r, p); | |
| } |
🤖 Prompt for AI Agents
In deprecated/RequestHandler.sol around lines 107 to 136, the function uses
requestCount before assigning it, never sets r.appGateway, and returns an
uninitialized promiseList; fix by assigning requestCount = nextRequestCount++
immediately at the top (before accessing _requests), then load RequestParams
storage r = _requests[requestCount]; set r.appGateway = appGateway_ (and keep
the existing r.requestFeesDetails assignments), ensure fees are estimated and
blocked as before, call _processBatch(r), and initialize promiseList (e.g., an
empty address[] with length 0 or populated as needed) before returning so the
named return is valid and OnlyAppGateway checks later will see the stored
appGateway.
| function updateRequest( | ||
| uint40 requestCount_, | ||
| uint256 feesUsed_ | ||
| ) external onlyPromiseResolver isRequestCancelled(requestCount_) { | ||
| RequestParams storage r = _requests[requestCount_]; | ||
| PayloadParams storage payloadParams = _payloads[r.payloadId]; | ||
| payloadParams.resolvedAt = block.timestamp; | ||
|
|
||
| IPrecompile(precompiles[payloadParams.callType]).resolvePayload(payloadParams); | ||
| r.isRequestExecuted = true; | ||
| _settleRequest(requestCount_, r); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
updateRequest extra parameter is unused and diverges from caller
feesUsed_ is unused here and PromiseResolver calls the single‑arg variant. This should be reconciled to avoid dead params and interface drift.
Option A (align to new API):
- function updateRequest(
- uint40 requestCount_,
- uint256 feesUsed_
- ) external onlyPromiseResolver isRequestCancelled(requestCount_) {
+ function updateRequest(
+ uint40 requestCount_
+ ) external onlyPromiseResolver isRequestCancelled(requestCount_) {
RequestParams storage r = _requests[requestCount_];
PayloadParams storage payloadParams = _payloads[r.payloadId];
payloadParams.resolvedAt = block.timestamp;
IPrecompile(precompiles[payloadParams.callType]).resolvePayload(payloadParams);
r.isRequestExecuted = true;
_settleRequest(requestCount_, r);
}Option B (temporary compatibility): keep signature and pass 0 from PromiseResolver, then remove feesUsed_ in a follow‑up.
📝 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 updateRequest( | |
| uint40 requestCount_, | |
| uint256 feesUsed_ | |
| ) external onlyPromiseResolver isRequestCancelled(requestCount_) { | |
| RequestParams storage r = _requests[requestCount_]; | |
| PayloadParams storage payloadParams = _payloads[r.payloadId]; | |
| payloadParams.resolvedAt = block.timestamp; | |
| IPrecompile(precompiles[payloadParams.callType]).resolvePayload(payloadParams); | |
| r.isRequestExecuted = true; | |
| _settleRequest(requestCount_, r); | |
| } | |
| function updateRequest( | |
| uint40 requestCount_ | |
| ) external onlyPromiseResolver isRequestCancelled(requestCount_) { | |
| RequestParams storage r = _requests[requestCount_]; | |
| PayloadParams storage payloadParams = _payloads[r.payloadId]; | |
| payloadParams.resolvedAt = block.timestamp; | |
| IPrecompile(precompiles[payloadParams.callType]).resolvePayload(payloadParams); | |
| r.isRequestExecuted = true; | |
| _settleRequest(requestCount_, r); | |
| } |
🤖 Prompt for AI Agents
In deprecated/RequestHandler.sol around lines 234–246 the updateRequest function
declares an unused feesUsed_ parameter that diverges from the PromiseResolver
caller; remove the unused feesUsed_ parameter from updateRequest's signature and
any related interface declarations, then update all callers (including
PromiseResolver) to call the new single-argument variant
(updateRequest(requestCount_)); ensure compiler/interfaces and any tests are
updated accordingly and run a full compile/test to confirm no remaining
references to feesUsed_ exist.
| function _submitRequest( | ||
| uint256 maxFees, | ||
| address, | ||
| address consumeFrom, | ||
| bytes memory onCompleteData | ||
| ) internal returns (uint40 requestCount, address[] memory promiseList) { | ||
| if (payloadQueue.length == 0) return (0, new address[](0)); | ||
| address appGateway = msg.sender; | ||
|
|
||
| // this check is to verify that msg.sender (app gateway base) belongs to correct app gateway | ||
| if (appGateway != appGatewayTemp) revert InvalidAppGateway(); | ||
| latestAsyncPromise = address(0); | ||
| appGatewayTemp = address(0); | ||
|
|
There was a problem hiding this comment.
Early return leaves appGatewayTemp/last promise set → deadlock on subsequent calls
If payloadQueue is empty, _submitRequest returns without clearing appGatewayTemp/latestAsyncPromise. Any later queue/submit attempts will be blocked. This should reset temporary state before returning.
- if (payloadQueue.length == 0) return (0, new address[](0));
+ if (payloadQueue.length == 0) {
+ // reset temp state to avoid lock-up if queue was cleared externally
+ latestAsyncPromise = address(0);
+ appGatewayTemp = address(0);
+ return (0, new address[](0));
+ }📝 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 _submitRequest( | |
| uint256 maxFees, | |
| address, | |
| address consumeFrom, | |
| bytes memory onCompleteData | |
| ) internal returns (uint40 requestCount, address[] memory promiseList) { | |
| if (payloadQueue.length == 0) return (0, new address[](0)); | |
| address appGateway = msg.sender; | |
| // this check is to verify that msg.sender (app gateway base) belongs to correct app gateway | |
| if (appGateway != appGatewayTemp) revert InvalidAppGateway(); | |
| latestAsyncPromise = address(0); | |
| appGatewayTemp = address(0); | |
| function _submitRequest( | |
| uint256 maxFees, | |
| address, | |
| address consumeFrom, | |
| bytes memory onCompleteData | |
| ) internal returns (uint40 requestCount, address[] memory promiseList) { | |
| if (payloadQueue.length == 0) { | |
| // reset temp state to avoid lock-up if queue was cleared externally | |
| latestAsyncPromise = address(0); | |
| appGatewayTemp = address(0); | |
| return (0, new address[](0)); | |
| } | |
| address appGateway = msg.sender; | |
| // this check is to verify that msg.sender (app gateway base) belongs to correct app gateway | |
| if (appGateway != appGatewayTemp) revert InvalidAppGateway(); | |
| latestAsyncPromise = address(0); | |
| appGatewayTemp = address(0); | |
| // ... rest of function ... | |
| } |
🤖 Prompt for AI Agents
In deprecated/Watcher.sol around lines 101 to 114, the early return when
payloadQueue.length == 0 leaves appGatewayTemp and latestAsyncPromise uncleared
causing subsequent calls to deadlock; modify the function so that before
returning (0, new address[](0)) you reset latestAsyncPromise = address(0) and
appGatewayTemp = address(0) (same cleanup performed later in the function) to
ensure temporary state is always cleared on the empty-queue path.
| /// @notice Clears the call parameters array | ||
| function clearQueue() public { | ||
| delete payloadQueue; | ||
| } | ||
|
|
There was a problem hiding this comment.
clearQueue is public and unprotected → anyone can nuke pending work (and cause the deadlock above)
This should not be externally callable. Restrict or make internal to eliminate DoS.
- function clearQueue() public {
+ function clearQueue() internal {
delete payloadQueue;
}📝 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.
| /// @notice Clears the call parameters array | |
| function clearQueue() public { | |
| delete payloadQueue; | |
| } | |
| /// @notice Clears the call parameters array | |
| function clearQueue() internal { | |
| delete payloadQueue; | |
| } |
🤖 Prompt for AI Agents
In deprecated/Watcher.sol around lines 126 to 130, the clearQueue function is
declared public which allows anyone to delete the payloadQueue and cause a DoS;
change its visibility to internal or private (or add an access-control modifier
such as onlyOwner) so only the contract/operator can clear the queue, and update
any callers accordingly (or add an admin-only wrapper if external invocation is
needed); ensure the function signature and any modifiers are updated and that
tests reflect the new restricted access.
| if (contractAddress_ == address(0)) revert InvalidContract(); | ||
| if (data_.length == 0) revert InvalidData(); | ||
| if (signature_.length == 0) revert InvalidSignature(); | ||
| if (isNonceUsed[nonce_]) revert NonceUsed(); | ||
| isNonceUsed[nonce_] = true; | ||
|
|
||
| bytes32 digest = keccak256( | ||
| abi.encode(address(this), evmxSlug, nonce_, contractAddress_, data_) | ||
| ); | ||
|
|
||
| // check if signature is valid | ||
| if (_recoverSigner(digest, signature_) != owner()) revert InvalidSignature(); | ||
| } |
There was a problem hiding this comment.
Nonce is marked used before signature validation → trivially DoS valid signatures
Move isNonceUsed assignment after signature verification.
- if (isNonceUsed[nonce_]) revert NonceUsed();
- isNonceUsed[nonce_] = true;
+ if (isNonceUsed[nonce_]) revert NonceUsed();
bytes32 digest = keccak256(
abi.encode(address(this), evmxSlug, nonce_, contractAddress_, data_)
);
- // check if signature is valid
- if (_recoverSigner(digest, signature_) != owner()) revert InvalidSignature();
+ // check if signature is valid
+ if (_recoverSigner(digest, signature_) != owner()) revert InvalidSignature();
+ // mark only after successful validation
+ isNonceUsed[nonce_] = true;📝 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.
| if (contractAddress_ == address(0)) revert InvalidContract(); | |
| if (data_.length == 0) revert InvalidData(); | |
| if (signature_.length == 0) revert InvalidSignature(); | |
| if (isNonceUsed[nonce_]) revert NonceUsed(); | |
| isNonceUsed[nonce_] = true; | |
| bytes32 digest = keccak256( | |
| abi.encode(address(this), evmxSlug, nonce_, contractAddress_, data_) | |
| ); | |
| // check if signature is valid | |
| if (_recoverSigner(digest, signature_) != owner()) revert InvalidSignature(); | |
| } | |
| if (contractAddress_ == address(0)) revert InvalidContract(); | |
| if (data_.length == 0) revert InvalidData(); | |
| if (signature_.length == 0) revert InvalidSignature(); | |
| if (isNonceUsed[nonce_]) revert NonceUsed(); | |
| bytes32 digest = keccak256( | |
| abi.encode(address(this), evmxSlug, nonce_, contractAddress_, data_) | |
| ); | |
| // check if signature is valid | |
| if (_recoverSigner(digest, signature_) != owner()) revert InvalidSignature(); | |
| // mark only after successful validation | |
| isNonceUsed[nonce_] = true; | |
| } |
🤖 Prompt for AI Agents
In deprecated/Watcher.sol around lines 212 to 224, the code marks
isNonceUsed[nonce_] = true before verifying the signature, allowing an attacker
to DoS valid signed requests by pre-marking the nonce; move the isNonceUsed
assignment to after the signature verification (i.e., only set
isNonceUsed[nonce_] = true once _recoverSigner(digest, signature_) == owner()
has succeeded), keep the same digest computation and revert paths, and ensure no
other early state changes occur before signature validation.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
deprecated/test/evmx/Watcher.t.sol (2)
785-794: Fix self-referential local; compute new expiry from contract state
uint256 expiryTime = expiryTime + 100;reads an uninitialized local and will fail. Derive the new value from the contract’s current expiry, and use a distinct name.- uint256 expiryTime = expiryTime + 100; + uint256 newExpiryTime = schedulePrecompile.expiryTime() + 100; hoax(watcherAddress); vm.expectEmit(true, true, true, true); - emit SchedulePrecompile.ExpiryTimeSet(expiryTime); - schedulePrecompile.setExpiryTime(expiryTime); + emit SchedulePrecompile.ExpiryTimeSet(newExpiryTime); + schedulePrecompile.setExpiryTime(newExpiryTime); ... - schedulePrecompile.setExpiryTime(expiryTime); + schedulePrecompile.setExpiryTime(newExpiryTime);
400-416: Align updateRequest signature in test
ReplacepayloadIdwith auint256 feesUsedvalue inupdateRequest(requestCount, …)at Watcher.t.sol:409 and 415.contracts/evmx/fees/Credit.sol (1)
139-151: Critical: double-mint bug when native transfer failsIf
feesPool.withdrawfails, the code mintscreditAmount_again and then adds 0 tocreditAmount_. This inflates credits incorrectly and misreports the event.This should mint
nativeAmount_as credits and update the event values accordingly.- if (!success) { - _mint(depositTo_, creditAmount_); - nativeAmount_ = 0; - creditAmount_ += nativeAmount_; - } + if (!success) { + // Add failed native transfer as credits instead + _mint(depositTo_, nativeAmount_); + creditAmount_ += nativeAmount_; + nativeAmount_ = 0; + }
♻️ Duplicate comments (4)
deprecated/IRequestHandler.sol (1)
21-27: Align submitRequest to ForwarderParams and use calldata for bytesThis should switch to
ForwarderParams calldataandbytes calldatato match the forwarder-based flow and avoid unnecessary copies. Already flagged earlier.- function submitRequest( - uint256 maxFees_, - address consumeFrom_, - address appGateway_, - RawPayload calldata RawPayload_, - bytes memory onCompleteData_ - ) external returns (uint40 requestCount, address[] memory promiseList); + function submitRequest( + ForwarderParams calldata forwarderParams_, + RawPayload calldata rawPayload_, + bytes calldata onCompleteData_ + ) external returns (uint40 requestCount, address[] memory promiseList);contracts/evmx/fees/FeesManager.sol (1)
139-140: Missing state variable:overrideParamsis undefined.Line 140 accesses
overrideParams.consumeFrom, butFeesManagerdoes not declare anoverrideParamsstate variable. This will cause a compilation error.Apply the same fix as in
unblockAndAssignCredits:- // address consumeFrom = _getRequestParams(requestCount_).requestFeesDetails.consumeFrom; - address consumeFrom = overrideParams.consumeFrom; + address consumeFrom = watcher__().getPayload(payloadId_).consumeFrom;contracts/evmx/watcher/precompiles/WritePrecompile.sol (1)
174-187: Gas limit not assigned when override is non-zero.When
rawPayload_.overrideParams.gasLimitis non-zero, the function never assigns it to the return valuegasLimit, so the function returns0. This incorrect value flows intoprecompileData(line 113),DigestParams(line 129), and ultimately the digest hash (line 139), breaking execution.Apply this diff to assign the override value first, then apply chain-specific defaults only when needed:
+ gasLimit = rawPayload_.overrideParams.gasLimit; // todo: can be changed to set the default gas limit for each chain - if (rawPayload_.overrideParams.gasLimit == 0) { + if (gasLimit == 0) { if (rawPayload_.transaction.chainSlug == 5000) { // Mantle default gas limit gasLimit = 8_000_000_000; } else if (rawPayload_.transaction.chainSlug == 1329) { // Sei default gas limit gasLimit = 8_000_000; } else if (rawPayload_.transaction.chainSlug == 999) { // HyperEVM default gas limit gasLimit = 1_500_000; } else { gasLimit = 10_000_000; // other chains default gas limit } }contracts/evmx/watcher/Watcher.sol (1)
4-11: Add missing imports for referenced symbols.The code references symbols not imported here:
fromBytes32Format(used at line 175) needs import fromConverters.solAppGatewayAlreadyCalledandInvalidCallerTriggerederrors (used at lines 173, 177) need import fromErrors.solimport "solady/utils/Initializable.sol"; import "./Configurations.sol"; import {IPrecompile} from "../interfaces/IPrecompile.sol"; import {IFeesManager} from "../interfaces/IFeesManager.sol"; import {IPromise} from "../interfaces/IPromise.sol"; import {IERC20} from "../interfaces/IERC20.sol"; import "../../utils/common/IdUtils.sol"; +import {fromBytes32Format} from "../../utils/common/Converters.sol"; +import {AppGatewayAlreadyCalled, InvalidCallerTriggered} from "../../utils/common/Errors.sol"; import "solady/utils/LibCall.sol";
🧹 Nitpick comments (2)
deprecated/IAuctionManager.sol (1)
4-4: Unused imported symbols and stale NatSpec.This interface doesn’t use RawPayload/OverrideParams/Transaction/Bid/RequestParams. These should be removed to avoid noise. Also, NatSpec still says “request count” while the repo moves to payloadId; add a deprecation note or clarify intent.
-import {RawPayload, OverrideParams, Transaction, Bid, RequestParams} from "../../utils/common/Structs.sol"; +// (deprecated) no struct imports needed herecontracts/protocol/SocketBatcher.sol (1)
66-103: Remove dead/commented CCTP code or guard it behind a feature flagLarge commented blocks rot quickly and confuse maintenance. This should be deleted, or wrapped under a compile-time flag with tests documenting re-enablement criteria.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
contracts/evmx/base/AppGatewayBase.sol(7 hunks)contracts/evmx/fees/Credit.sol(3 hunks)contracts/evmx/fees/FeesManager.sol(5 hunks)contracts/evmx/helpers/AddressResolver.sol(0 hunks)contracts/evmx/helpers/AddressResolverUtil.sol(1 hunks)contracts/evmx/helpers/AsyncPromise.sol(4 hunks)contracts/evmx/helpers/Forwarder.sol(2 hunks)contracts/evmx/interfaces/IAddressResolver.sol(0 hunks)contracts/evmx/interfaces/IAppGateway.sol(1 hunks)contracts/evmx/interfaces/IFeesManager.sol(2 hunks)contracts/evmx/interfaces/IPromise.sol(1 hunks)contracts/evmx/interfaces/IReceiver.sol(0 hunks)contracts/evmx/interfaces/IWatcher.sol(1 hunks)contracts/evmx/watcher/Watcher.sol(1 hunks)contracts/evmx/watcher/precompiles/ReadPrecompile.sol(2 hunks)contracts/evmx/watcher/precompiles/SchedulePrecompile.sol(5 hunks)contracts/evmx/watcher/precompiles/WritePrecompile.sol(5 hunks)contracts/protocol/SocketBatcher.sol(2 hunks)contracts/utils/common/Errors.sol(1 hunks)contracts/utils/common/IdUtils.sol(1 hunks)contracts/utils/common/Structs.sol(3 hunks)deprecated/IAuctionManager.sol(1 hunks)deprecated/IRequestHandler.sol(1 hunks)deprecated/IWatcher.sol(1 hunks)deprecated/test/evmx/ProxyMigration.t.sol(1 hunks)deprecated/test/evmx/Watcher.t.sol(4 hunks)test/mock/MockWatcherPrecompile.sol(0 hunks)
💤 Files with no reviewable changes (4)
- contracts/evmx/helpers/AddressResolver.sol
- contracts/evmx/interfaces/IReceiver.sol
- contracts/evmx/interfaces/IAddressResolver.sol
- test/mock/MockWatcherPrecompile.sol
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/evmx/helpers/Forwarder.sol
🔇 Additional comments (19)
contracts/evmx/helpers/AddressResolverUtil.sol (2)
29-31: Remove incorrect delegation concern
The_isWatcherfunction has always comparedaccount_directly towatcher__(); no delegation logic existed or was removed.Likely an incorrect or invalid review comment.
29-31: Confirm no remaining callers of deployForwarder__(). Ripgrep search across all.solfiles returned no matches fordeployForwarder__. Removal is safe.contracts/evmx/helpers/AsyncPromise.sol (2)
81-87: Initializer versioning sanity-check.initialize remains reinitializer(1). If prior deployments already used version 1, they won’t be able to call this initializer again post-upgrade. Confirm that AsyncPromise instances are clones initialized once per instance (not proxy-upgraded), or bump/init via a new versioned initializer if needed.
184-185: Validation change LGTM.Comparing payloadId to watcher.currentPayloadId() matches the new model.
contracts/evmx/interfaces/IAppGateway.sol (1)
15-15: No breaking change detected – AppGatewayBase now matches the single-value signature and no callers destructure two returns.deprecated/IRequestHandler.sol (1)
29-29: All updateRequest signatures are consistent: interface, implementation, and tests use (uint40, uint256); no bytes32 parameter found.Likely an incorrect or invalid review comment.
contracts/utils/common/Structs.sol (1)
143-147: RawPayload structure is correct; callers must use nested access.The past review comment flagged a missing
switchboardTypefield, but the struct definition is correct—switchboardTypeis accessible viarawPayload.overrideParams.switchboardType(line 129). The issue is in consuming code (e.g.,WritePrecompile.solline 117,ForwarderV2.solline 101) that must access the nested field rather than a direct property.This file's definitions align with the payload-centric model.
contracts/evmx/base/AppGatewayBase.sol (1)
27-27: Override state properly centralized.The
overrideParamspublic state variable (line 27) provides a single source of truth for override configuration, avoiding scattered state. ThegetOverrideParams()(lines 201-203) andapplyOverride()(lines 213-215) methods complete the fluent builder pattern enabled byOverrideParamsLib.contracts/evmx/watcher/precompiles/WritePrecompile.sol (1)
115-118: Correct nested access toswitchboardType.Line 117 correctly accesses
rawPayload.overrideParams.switchboardTypeinstead of the non-existentrawPayload.switchboardType. This aligns with theRawPayloadstructure defined inStructs.sol(lines 143-147) whereswitchboardTypeis a field of the nestedOverrideParams.contracts/evmx/watcher/precompiles/ReadPrecompile.sol (1)
44-69: Clean RawPayload handling with proper access control.The
handlePayloadfunction correctly usesRawPayload(line 45) and accesses nested fields likerawPayload.overrideParams.readAtBlockNumber(line 59) andrawPayload.transaction(lines 58, 65). TheonlyWatchermodifier (line 50) properly gates access. This implementation aligns with the payload-centric model and mirrors the pattern inWritePrecompile.sol.contracts/evmx/watcher/precompiles/SchedulePrecompile.sol (2)
28-33: LGTM!The explicit
watcher__state variable andonlyWatchermodifier provide clear access control. This is more transparent than inheriting fromWatcherBase.
117-145: Remove manual overflow guard: Solidity 0.8.21 uses checked arithmetic soblock.timestamp + rawPayload.overrideParams.delayInSecondsreverts on overflow. Pattern is consistent across precompiles.contracts/evmx/interfaces/IWatcher.sol (1)
40-49: LGTM!The
executePayloadmethod signature correctly acceptsRawPayloadandappGateway_, aligning with the payload-centric model described in the PR objectives.contracts/evmx/watcher/Watcher.sol (5)
153-170: Fix inverted deadline validation in markRevert.Line 160 checks
if (payloadParams.deadline > block.timestamp)which reverts when the deadline is in the future. This is backwards—you should only allow marking revert when the deadline has passed.- if (payloadParams.deadline > block.timestamp) revert DeadlineNotPassedForOnChainRevert(); + if (payloadParams.deadline >= block.timestamp) revert DeadlineNotPassedForOnChainRevert();Or if the intent is to require the deadline to have already passed before allowing onchain revert:
- if (payloadParams.deadline > block.timestamp) revert DeadlineNotPassedForOnChainRevert(); + if (payloadParams.deadline < block.timestamp) revert DeadlineAlreadyPassed();Clarify the intended behavior and fix the logic accordingly.
Likely an incorrect or invalid review comment.
240-246: switchboards mapping key type correct – declared as mapping(uint32 => mapping(bytes32 => uint64)) in Configurations.sol, matchingbytes32 switchboardType_.
203-223: unblockCredits signature matches interface – no change needed
unblockCredits(bytes32 payloadId) in IFeesManager and FeesManager.sol accepts only the payloadId, so the call is correct.Likely an incorrect or invalid review comment.
93-108: Approve payloadPointer synchronization and initialization
Theif (nextPayloadCount == 0) nextPayloadCount = 1guard ensures pointers start at 1, andpayloadPointer: nextPayloadCount++aligns with the emittedcurrentPayloadId.
70-109: Approve executePayload ordering
currentPayloadIdis assigned at lines 79–82 before callingblockCreditson line 84.deprecated/IWatcher.sol (1)
10-89: Deprecated interface isn't used in active code Confirmed no non-deprecated Solidity files import deprecated/IWatcher.sol.
contracts/evmx/fees/Credit.sol
Outdated
| // applyOverride( | ||
| // OverrideParamsLib.setMaxFees(overrideParams, getMaxFees(chainSlug_)).setConsumeFrom(consumeFrom_) | ||
| // ); | ||
| RawPayload memory rawPayload; | ||
| rawPayload.overrideParams = overrideParams; | ||
| rawPayload.transaction = Transaction({ | ||
| chainSlug: chainSlug_, | ||
| target: _getFeesPlugAddress(chainSlug_), | ||
| payload: payload_ | ||
| }); | ||
| queueParams.switchboardType = sbType; | ||
| watcher__().queue(queueParams, address(this)); | ||
| watcher__().executePayload(rawPayload, address(this)); | ||
| } |
There was a problem hiding this comment.
Construct and apply override params before executing payload
overrideParams is used but never constructed; applyOverride(...) is commented out. This leaves consumeFrom/maxFees unset, likely breaking fee accounting/execution.
This should set overrideParams locally with consumeFrom_ and maxFees_ (and any defaults), assign it to rawPayload.overrideParams, then call executePayload.
- // applyOverride(
- // OverrideParamsLib.setMaxFees(overrideParams, getMaxFees(chainSlug_)).setConsumeFrom(consumeFrom_)
- // );
- RawPayload memory rawPayload;
- rawPayload.overrideParams = overrideParams;
+ RawPayload memory rawPayload;
+ OverrideParams memory params;
+ // Populate override params locally to avoid stale/global state
+ params = OverrideParamsLib
+ .setMaxFees(params, getMaxFees(chainSlug_))
+ .setConsumeFrom(consumeFrom_);
+ rawPayload.overrideParams = params;
rawPayload.transaction = Transaction({
chainSlug: chainSlug_,
target: _getFeesPlugAddress(chainSlug_),
payload: payload_
});
watcher__().executePayload(rawPayload, address(this));Add missing import at the top of the file:
+import {OverrideParams} from "../../utils/common/Structs.sol";
+import "../../utils/OverrideParamsLib.sol";📝 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.
| // applyOverride( | |
| // OverrideParamsLib.setMaxFees(overrideParams, getMaxFees(chainSlug_)).setConsumeFrom(consumeFrom_) | |
| // ); | |
| RawPayload memory rawPayload; | |
| rawPayload.overrideParams = overrideParams; | |
| rawPayload.transaction = Transaction({ | |
| chainSlug: chainSlug_, | |
| target: _getFeesPlugAddress(chainSlug_), | |
| payload: payload_ | |
| }); | |
| queueParams.switchboardType = sbType; | |
| watcher__().queue(queueParams, address(this)); | |
| watcher__().executePayload(rawPayload, address(this)); | |
| } | |
| // Add these at the top of the file: | |
| import {OverrideParams} from "../../utils/common/Structs.sol"; | |
| import "../../utils/OverrideParamsLib.sol"; | |
| RawPayload memory rawPayload; | |
| OverrideParams memory params; | |
| // Populate override params locally to avoid stale/global state | |
| params = OverrideParamsLib | |
| .setMaxFees(params, getMaxFees(chainSlug_)) | |
| .setConsumeFrom(consumeFrom_); | |
| rawPayload.overrideParams = params; | |
| rawPayload.transaction = Transaction({ | |
| chainSlug: chainSlug_, | |
| target: _getFeesPlugAddress(chainSlug_), | |
| payload: payload_ | |
| }); | |
| watcher__().executePayload(rawPayload, address(this)); | |
| } |
| /// @notice The payload count of the promise | ||
| bytes32 public override payloadId; | ||
|
|
There was a problem hiding this comment.
CRITICAL: Storage layout break (packed-slot widened).
Replacing a packed uint40 with bytes32 in-place shifts localInvoker and all subsequent variables into new slots. This will corrupt state on upgrades/clones relying on the old layout. This must be fixed before merge.
Preserve the original packed layout and append payloadId at the end (before the gap). Deprecate the old field to keep the slot intact.
Apply within this segment to restore the packed slot:
- /// @notice The payload count of the promise
- bytes32 public override payloadId;
+ /// @dev Deprecated. Kept solely to preserve storage layout (slot 50). Do not use.
+ uint40 private __requestCountDeprecated;Then, append payloadId after existing vars (right before the gap) and update the doc:
@@
/// @notice The revert handler data of the promise
bytes public revertHandlerData;
- // slots [55-102] reserved for gap
- uint256[48] _gap_after;
+ /// @notice The payload id of the promise
+ bytes32 public override payloadId;
+
+ // slots [55-102] reserved for gap (reduced by 1 if you prefer strict accounting)
+ uint256[48] _gap_after;No logic changes needed elsewhere; initialize() can continue setting payloadId. Also update the stale doc “payload count” → “payload id”.
📝 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.
| /// @notice The payload count of the promise | |
| bytes32 public override payloadId; | |
| /// @dev Deprecated. Kept solely to preserve storage layout (slot 50). Do not use. | |
| uint40 private __requestCountDeprecated; |
| /// @notice The payload count of the promise | |
| bytes32 public override payloadId; | |
| /// @notice The revert handler data of the promise | |
| bytes public revertHandlerData; | |
| /// @notice The payload id of the promise | |
| bytes32 public override payloadId; | |
| // slots [55-102] reserved for gap (reduced by 1 if you prefer strict accounting) | |
| uint256[48] _gap_after; |
🤖 Prompt for AI Agents
In contracts/evmx/helpers/AsyncPromise.sol around lines 26 to 28, the change
replacing the packed uint40 with a bytes32 widens the storage slot and will
corrupt subsequent state; restore the original packed uint40 declaration (mark
it deprecated/unused to preserve the slot and name), keep localInvoker and all
following variables in their original packed layout, then add the new bytes32
payloadId field after the existing variables (immediately before the reserved
gap) with updated NatSpec ("payload id" not "payload count"); no other logic
changes are needed — initialize() may still set payloadId.
| function blockCredits(bytes32 payloadId_, address consumeFrom_, uint256 credits_) external; | ||
|
|
||
| function unblockAndAssignCredits(uint40 requestCount_, address assignTo_) external; | ||
| function unblockAndAssignCredits(bytes32 payloadId_, address assignTo_) external; | ||
|
|
||
| function unblockCredits(uint40 requestCount_) external; | ||
| function unblockCredits(bytes32 payloadId_) external; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Breaking change: switch to payloadId (bytes32) for credit blocking
blockCredits/unblockAndAssignCredits/unblockCredits now use bytes32 payloadId_. All callers and events must align with the payload-centric flow.
This should include:
- Updating all call sites.
- Adjusting any storage keyed by requestCount to payloadId (mappings, events, logs).
- Adding natspec for
payloadId_to avoid ambiguity.
Find usages still passing uint40:
🏁 Script executed:
#!/bin/bash
rg -nP --type=sol -C2 '\b(blockCredits|unblockAndAssignCredits|unblockCredits)\s*\('
rg -nP --type=sol -C2 '\bblockCredits\s*\(uint40\b|\bunblockAndAssignCredits\s*\(uint40\b|\bunblockCredits\s*\(uint40\b'Length of output: 55
🏁 Script executed:
#!/bin/bash
rg -n -C2 '\bblockCredits\s*\(' -g '*.sol'
rg -n -C2 '\bunblockAndAssignCredits\s*\(' -g '*.sol'
rg -n -C2 '\bunblockCredits\s*\(' -g '*.sol'
rg -nP '\b(blockCredits|unblockAndAssignCredits|unblockCredits)\s*\(\s*uint40' -g '*.sol'Length of output: 5140
Fix remaining call sites using uint40 requestCount
- In deprecated/RequestHandler.sol (lines 131, 285), replace
requestCountinblockCredits/unblockAndAssignCreditscalls with the correctbytes32 payloadId(e.g.r.payloadId). - Add or update NatSpec on
IFeesManager.solforpayloadId_to clarify its purpose. - Audit any storage mappings or events previously keyed by
uint40 requestCountto usebytes32 payloadIdinstead.
🤖 Prompt for AI Agents
In contracts/evmx/interfaces/IFeesManager.sol around lines 32 to 37, the
functions still reference payloadId_ but lack NatSpec and some call sites still
pass a uint40 requestCount; update deprecated/RequestHandler.sol at lines 131
and 285 to replace requestCount with the correct bytes32 payloadId (e.g.
r.payloadId) in blockCredits and unblockAndAssignCredits calls, add or update
NatSpec on payloadId_ in IFeesManager.sol to explain it is the unique payload
identifier (bytes32) used for credit operations, and audit/update any storage
mappings, event signatures, and indexing previously keyed by uint40 requestCount
to use bytes32 payloadId instead so all callsites and storage use the new
bytes32 key consistently.
| /// @notice The payload id of the promise | ||
| function payloadId() external view returns (bytes32); |
There was a problem hiding this comment.
🧩 Analysis chain
Breaking API change: verify all usages updated.
The migration from requestCount() returning uint40 to payloadId() returning bytes32 is a breaking change that aligns with the payload-centric model. This changes both the identifier type (sequential counter → content-based hash) and the semantic meaning.
Ensure all callers of the removed requestCount() have been updated throughout the codebase, particularly in:
- Promise creation/tracking logic
- Event emissions that reference request identifiers
- Off-chain services or indexers that rely on this interface
Run this script to verify the migration is complete:
🏁 Script executed:
#!/bin/bash
# Verify that requestCount references have been removed and payloadId is used consistently
# Check for any remaining references to requestCount (excluding this interface change)
echo "=== Checking for remaining requestCount references ==="
rg -n 'requestCount' --type=solidity -g '!*.md' -g '!IPromise.sol'
# Verify payloadId usage in Promise implementations and related contracts
echo -e "\n=== Verifying payloadId implementation and usage ==="
ast-grep --pattern $'contract $_ {
$$$
function payloadId() $_ {
$$$
}
$$$
}'
# Check for any hardcoded uint40 types that might be related to the old request counter
echo -e "\n=== Checking for uint40 usage that may need updating ==="
rg -n 'uint40' --type=solidity -C 2Length of output: 232
🏁 Script executed:
#!/bin/bash
echo "=== Checking for remaining requestCount references ==="
rg -n 'requestCount' -g '*.sol' -g '!contracts/evmx/interfaces/IPromise.sol'
echo -e "\n=== Checking for uint40 usage ==="
rg -n 'uint40' -g '*.sol'
echo -e "\n=== Checking for payloadId references ==="
rg -n 'payloadId' -g '*.sol' -g '!contracts/evmx/interfaces/IPromise.sol'Length of output: 62531
Breaking API change: migrate all requestCount usages to payloadId.
This should:
- Replace
uint40 requestCountparameters and returns withbytes32 payloadIdin active contracts (e.g.contracts/evmx/helpers/AsyncDeployer.sol,contracts/evmx/interfaces/IAsyncDeployer.sol,contracts/evmx/base/AppGatewayBase.sol) - Remove all residual
requestCountreferences outsidedeprecated/tests - Update events, variables, and calls to use
payloadId()instead ofrequestCount()
🤖 Prompt for AI Agents
In contracts/evmx/interfaces/IPromise.sol around lines 21-22, the interface
currently exposes payloadId() but the codebase still uses a deprecated uint40
requestCount API; update all active contracts to replace uint40 requestCount
parameters, returns, variables, events and calls with bytes32 payloadId (e.g.,
contracts/evmx/helpers/AsyncDeployer.sol,
contracts/evmx/interfaces/IAsyncDeployer.sol,
contracts/evmx/base/AppGatewayBase.sol), remove all non-test residual
requestCount references (leave deprecated/ tests untouched), change functions
signatures and return types to use bytes32 payloadId, update event definitions
and emitted values to pass payloadId(), and update any call sites to use
promise.payloadId() (or equivalent) instead of requestCount().
| function isWatcher(address account_) external view returns (bool); | ||
| function increaseFees(bytes32 payloadId_, uint256 newMaxFees_) external; | ||
|
|
||
| function cancelRequest(bytes32 payloadId_) external; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify all callers updated for bytes32 payloadId parameter.
The cancelRequest signature changed from uint40 requestCount to bytes32 payloadId. Ensure all calling code has been updated.
🏁 Script executed:
#!/bin/bash
# Find all cancelRequest invocations and verify they pass bytes32
rg -n -A2 'cancelRequest\s*\(' --type=sol --glob='!deprecated/**' | rg -v 'function cancelRequest'Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for all cancelRequest calls in .sol files (excluding the interface definition) to verify updated parameter type
rg -n --glob '*.sol' 'cancelRequest\s*\(' --glob '!contracts/evmx/interfaces/IWatcher.sol' | rg -v 'function cancelRequest'Length of output: 1254
Update cancelRequest callers to use bytes32 payloadId
Deprecated code and tests still call the old signature (cancelRequest(requestCount,…)):
- deprecated/test/evmx/Watcher.t.sol: lines 364, 368, 374, 378, 394, 412
- deprecated/Watcher.sol: 166
- deprecated/RequestHandler.sol: 256, 266, 270
Change these to pass the newbytes32 payloadId(or remove/replace deprecated code).
🤖 Prompt for AI Agents
In contracts/evmx/interfaces/IWatcher.sol around line 60, the cancelRequest
signature was changed to accept bytes32 payloadId_; update all callers to use
the new bytes32 payloadId parameter instead of the old (requestCount, …)
signature: edit deprecated/test/evmx/Watcher.t.sol (lines ~364, 368, 374, 378,
394, 412), deprecated/Watcher.sol (line 166), and deprecated/RequestHandler.sol
(lines ~256, 266, 270) to pass the bytes32 payloadId (or remove/replace
deprecated calls); ensure the caller obtains/constructs the correct payloadId
(bytes32) value and compiles against the updated interface.
| /// @notice Error thrown when payload count mismatch | ||
| error PayloadCountMismatch(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Error rename requires repo-wide updates.
All usages of RequestCountMismatch must be replaced with PayloadCountMismatch to keep builds green.
🏁 Script executed:
#!/bin/bash
# Find old/new error references
rg -nP -C2 '\bRequestCountMismatch\b|\bPayloadCountMismatch\b'Length of output: 1616
Update documentation for renamed error: Replace RequestCountMismatch() with PayloadCountMismatch() in Errors.md (line 148).
🤖 Prompt for AI Agents
In contracts/utils/common/Errors.sol around lines 39 to 41 the error was renamed
to PayloadCountMismatch(); update the documentation to reflect this rename by
replacing any instances of RequestCountMismatch() with PayloadCountMismatch() in
Errors.md (specifically line 148) and scan the docs for other occurrences to
update for consistency.
|
|
||
| import "./ProxyStorage.t.sol"; | ||
| import "../mock/MockWatcherPrecompile.sol"; | ||
| // import "../mock/MockWatcherPrecompile.sol"; |
There was a problem hiding this comment.
Compilation error: MockWatcherPrecompile still referenced.
Commenting out this import breaks compilation. The testUpgradeWithInitializationData function (lines 113-130) still instantiates MockWatcherPrecompile and uses its initialize selector.
Either restore the import or remove/update the test function that depends on it:
-// import "../mock/MockWatcherPrecompile.sol";
+import "../mock/MockWatcherPrecompile.sol";Or if migrating away from MockWatcherPrecompile, remove the dependent test:
- function testUpgradeWithInitializationData() public {
- // Deploy new implementation
- MockWatcherPrecompile newImpl = new MockWatcherPrecompile();
-
- // Prepare initialization data with new defaultLimit
- uint256 newValue = 2000;
- bytes memory initData = abi.encodeWithSelector(
- MockWatcherPrecompile.initialize.selector,
- newValue
- );
-
- upgradeAndCall(address(watcher), address(newImpl), initData);
- assertWatcherSlot();
-
- // Verify new value is set
- bytes32 slotValue = vm.load(address(watcher), bytes32(uint256(160)));
- assertEq(uint256(slotValue), newValue, "newValue mismatch");
- }🤖 Prompt for AI Agents
In deprecated/test/evmx/ProxyMigration.t.sol around line 5 (and tests at
113-130), the commented out import for MockWatcherPrecompile breaks compilation
because testUpgradeWithInitializationData still instantiates and calls the
initialize selector on MockWatcherPrecompile; either restore the import line so
the contract is available, or modify the test: remove the MockWatcherPrecompile
instantiation and any calls to its initialize selector, or replace them with the
new mock/contract that provides the same initialize selector and behavior used
by the test; update any type names and selector usages accordingly and run the
test compile to ensure no remaining references to MockWatcherPrecompile exist.
| RawPayload[] memory RawPayload = new RawPayload[](0); | ||
|
|
||
| hoax(watcherAddress); | ||
| requestHandler.submitRequest( | ||
| 1000, | ||
| address(0x1), | ||
| address(0x2), | ||
| address(0x3), | ||
| queueParams, | ||
| RawPayload, | ||
| "" | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
Avoid naming variables the same as types; fix zero-length RawPayload usage
Using RawPayload as a variable name collides with the type and hurts readability. Also, passing a zero-length payload array may revert depending on validation.
This should:
- Rename the variable to lower camelCase (e.g.,
rawPayloads). - Confirm the API expects an array vs a single
RawPayload. If array, ensure it’s non-empty with a minimally valid payload; if single, switch the declaration accordingly.
- RawPayload[] memory RawPayload = new RawPayload[](0);
+ RawPayload[] memory rawPayloads = new RawPayload[](0);
...
- RawPayload,
+ rawPayloads,
...
- RawPayload,
+ rawPayloads,Ensure the called submitRequest signature matches (array vs single RawPayload) in contracts/evmx/interfaces/IRequestHandler.sol. If it’s single, declare RawPayload memory rawPayload instead. Run:
Also applies to: 270-272, 281-283
🏁 Script executed:
#!/bin/bash
rg -nP -C2 --type=sol '\binterface\s+IRequestHandler\b' && rg -nP -C2 --type=sol '\bsubmitRequest\s*\('Length of output: 27
Searching for IRequestHandler interface and submitRequest signature in .sol files:
🏁 Script executed:
#!/bin/bash
rg -n 'interface IRequestHandler' -C2 -g '*.sol'
rg -n 'function submitRequest' -C2 -g '*.sol'Length of output: 1441
🏁 Script executed:
#!/bin/bash
# Locate RawPayload struct definition
rg -n 'struct RawPayload' -C3 -g '*.sol'
# Inspect Watcher.t.sol imports and variable declarations
sed -n '1,100p' deprecated/test/evmx/Watcher.t.sol
sed -n '250,300p' deprecated/test/evmx/Watcher.t.sol
# Verify submitRequest signature in interface and implementation
rg -n -C5 -g 'deprecated/IRequestHandler.sol' 'function submitRequest'
rg -n -C5 -g 'deprecated/RequestHandler.sol' 'function submitRequest'Length of output: 7580
Fix submitRequest parameter type and variable naming
- The
submitRequestAPI expects a singleRawPayload calldata, not an array. ReplacewithRawPayload[] memory RawPayload = new RawPayload[](0);
RawPayload memory rawPayload = /* construct valid RawPayload struct */;
- Rename the variable to
rawPayloadand update both calls:- requestHandler.submitRequest(..., RawPayload, ""); + requestHandler.submitRequest(..., rawPayload, "");
🤖 Prompt for AI Agents
In deprecated/test/evmx/Watcher.t.sol around lines 262 to 272, the test
currently constructs an array of RawPayload and passes it to submitRequest, but
submitRequest expects a single RawPayload calldata; replace the array creation
with a single RawPayload memory variable named rawPayload (constructed with the
appropriate field values for your test), rename usages from RawPayload to
rawPayload, and pass rawPayload (not an array) to requestHandler.submitRequest
so the parameter types match.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
contracts/evmx/helpers/AsyncPromise.sol (1)
26-27: CRITICAL: Storage layout break - bytes32 widening corrupts packed slot.Replacing the packed
uint40withbytes32at line 27 forcespayloadIdinto a new slot, shiftinglocalInvoker(line 31) and all subsequent storage variables. This will corrupt state on any upgrade or clone expecting the original layout.This was flagged in previous reviews. Verify the fix was properly applied:
- Restore the original packed uint40 in slot 50 (mark it deprecated):
+ /// @dev Deprecated. Kept to preserve storage layout in slot 50. Do not use. + uint40 private __requestCountDeprecated; - /// @notice The payload count of the promise - bytes32 public override payloadId;
- Add
payloadIdafter existing variables (before the gap):/// @notice The revert handler data of the promise bytes public revertHandlerData; + /// @notice The payload id of the promise + bytes32 public override payloadId; + // slots [55-102] reserved for gap uint256[48] _gap_after;The
initialize()function at line 86 can continue settingpayloadIdwithout changes.
🧹 Nitpick comments (1)
contracts/evmx/helpers/AsyncPromise.sol (1)
181-185: LGTM: Validation logic correctly uses payload ID.The function properly validates the caller and checks that the current promise matches the watcher's payload ID, preventing misuse.
Note: The error
PayloadCountMismatch(line 184) is now semantically misleading since you're comparingpayloadId(bytes32) not counts. Consider renaming toPayloadIdMismatchin a follow-up if it won't break error handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
contracts/evmx/base/AppGatewayBase.sol(7 hunks)contracts/evmx/helpers/AsyncPromise.sol(4 hunks)contracts/evmx/interfaces/IWatcher.sol(1 hunks)contracts/evmx/watcher/Watcher.sol(1 hunks)
🔇 Additional comments (19)
contracts/evmx/watcher/Watcher.sol (10)
67-115: LGTM: Payload execution flow is correct.The fee handling sequence properly blocks credits upfront with
maxFees, validates the actual fees returned by the precompile don't exceed the limit, and stores the payload with consistent state. The currentPayloadId is set before blocking credits and deploying the promise, ensuring proper tracking throughout the execution flow.
117-133: LGTM: Payload resolution logic is sound.The function properly guards against double-execution, marks the payload state, invokes the promise resolution and precompile hooks, and settles fees. The flow is clear and maintains consistency.
135-152: LGTM: Promise resolution logic handles both success and failure paths.The function correctly validates the deadline, attempts to mark the promise as resolved, and emits appropriate events for both success and failure cases. The early return on failure is appropriate.
154-176: LGTM: Revert handling is correctly implemented.The function properly enforces that the deadline must have passed before allowing an onchain revert, cancels the execution, and conditionally marks the promise as reverting. The flow is logical and safe.
178-204: LGTM: Trigger flow is well-structured.The function correctly checks for duplicate triggers, validates the plug, transfers fees, attempts the app gateway call with
tryCall, and emits appropriate success/failure events. The state cleanup at the end is proper.Note: This depends on the missing imports flagged earlier being added.
206-226: LGTM: Fee increase logic is correctly implemented.The function properly validates the caller, checks payload state, unblocks the old fees, validates new fees are spendable, blocks the new amount, and emits the event. The guard conditions prevent misuse.
243-249: LGTM: PayloadId generation is consistent.The function correctly retrieves the switchboard ID from the mapping and uses it with the payload count and evmxSlug to create a unique payload identifier. The logic is straightforward.
Verify that
Configurations(the parent contract) exposesswitchboardsas a public mapping with signaturemapping(uint32 => mapping(bytes32 => uint64)).
251-254: LGTM: Simple and correct getter.The function properly exposes payload data for external consumers.
256-259: Verify setTriggerFees access control.Line 256 uses
onlyWatchermodifier. Since this sets a configuration parameter, verify this is the intended access control rather thanonlyOwner. Typically, configuration settings are owner-controlled while execution functions are watcher-controlled.
266-271: LGTM: Fee query is properly delegated.The function correctly forwards the query to the appropriate precompile.
contracts/evmx/helpers/AsyncPromise.sol (2)
76-88: LGTM: Initialize signature updated for payload model.The function correctly accepts and stores
payloadId_(bytes32) instead of the oldrequestCount_(uint40), aligning with the payload-centric architecture. The logic is sound.Note: This depends on the storage layout issue being fixed first.
90-121: LGTM: Promise resolution logic is unchanged and correct.The function properly guards against re-resolution, handles callback execution with
tryCall, and transitions to the appropriate state on failure. The flow is sound.contracts/evmx/base/AppGatewayBase.sol (4)
10-20: LGTM: Imports align with payload-centric model.The updated imports properly support the new
RawPayload,OverrideParams, andOverrideParamsLibusage throughout the contract.
99-109: LGTM: Transaction control functions updated for payload model.Both
_revertTxand_increaseFeescorrectly usebytes32 payloadId_and properly delegate to the watcher. The changes align with the payload-centric architecture.
137-160: LGTM: Helper functions correctly adapted to payload model.The functions properly use
overrideParamsfor configuration and delegate appropriately to the watcher. The logic is sound.
199-215: LGTM: Override management is cleaner and more maintainable.The refactored override handling using
OverrideParamsLibis clearer and more composable than the previous approach. Theclear()method preserves the switchboard type while resetting other params, which is correct.contracts/evmx/interfaces/IWatcher.sol (3)
4-8: LGTM: Interface properly declares dependencies.The interface correctly imports the necessary types and extends
IConfigurations, establishing the proper contract hierarchy.
9-20: LGTM: Events properly cover the payload lifecycle.The events are well-designed with appropriate indexed parameters for efficient filtering and cover all key state transitions in the payload execution flow.
22-73: LGTM: Function signatures are well-defined and consistent.The interface properly declares all public functions with appropriate parameter types and return values (except for the
getCurrentPayloadIdtype mismatch flagged separately). The API surface is clear and complete for the payload-centric execution model.
| function _preAsync() internal { | ||
| isAsyncModifierSet = true; | ||
| _clearOverrides(); | ||
| watcher__().clearQueue(); | ||
| // watcher__().clearQueue(); | ||
|
|
||
| isAsyncModifierSet = true; | ||
| currentPayloadId = _getCurrentPayloadId(); | ||
| } | ||
|
|
||
| function _postAsync() internal { | ||
| isAsyncModifierSet = false; | ||
|
|
||
| (, address[] memory promises) = watcher__().submitRequest( | ||
| maxFees, | ||
| auctionManager, | ||
| consumeFrom, | ||
| onCompleteData | ||
| ); | ||
| _markValidPromises(promises); | ||
| _clearOverrides(); | ||
| // todo: get promise and mark it valid | ||
| // address promise_ = watcher__().latestAsyncPromise(); | ||
| // isValidPromise[promise_] = true; | ||
| } |
There was a problem hiding this comment.
Complete promise handling in _postAsync.
Lines 77-79 show TODOs for getting the promise and marking it valid. This logic is essential for the async flow to work correctly - without it, promises won't be validated and callbacks may fail.
The commented code suggests:
address promise_ = watcher__().latestAsyncPromise();
isValidPromise[promise_] = true;Do you want me to generate the complete implementation or open an issue to track this?
🤖 Prompt for AI Agents
In contracts/evmx/base/AppGatewayBase.sol around lines 67 to 80, complete the
TODO in _postAsync by retrieving the latest async promise from the watcher and
marking it valid: call watcher__().latestAsyncPromise(), store the returned
address in a local variable, set isValidPromise[promise_] = true, and guard
against zero-address (e.g., require(promise_ != address(0)) or return) to avoid
marking an invalid promise; update any related state/events if the project
convention requires emitting a validation event.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
contracts/evmx/fees/Credit.sol (2)
140-147: Fix double-mint on native-withdraw failureOn withdraw failure you mint creditAmount_ again and then add 0 to credits. This should mint nativeAmount_ as credits and update the tallies accordingly.
- if (!success) { - _mint(depositTo_, creditAmount_); - nativeAmount_ = 0; - creditAmount_ += nativeAmount_; - } + if (!success) { + // Convert failed native transfer to credits instead of double-minting credits + _mint(depositTo_, nativeAmount_); + creditAmount_ += nativeAmount_; + nativeAmount_ = 0; + }
201-207: Allowance check must cover required amountisCreditSpendable returns true when allowance is non-zero but still less than amount_, causing later revert in transferFrom. This should require allowance >= amount_.
- if (spender_ != address(watcher__()) && consumeFrom_ != spender_) { - if (allowance(consumeFrom_, spender_) == 0) return false; - } + if (spender_ != address(watcher__()) && consumeFrom_ != spender_) { + if (allowance(consumeFrom_, spender_) < amount_) return false; + } return balanceOf(consumeFrom_) >= amount_;test/SetupTest.t.sol (1)
703-706: Fix ABI decode type for precompileDatalast tuple element must be uint64 (switchboard), not address. Wrong type will revert at decode even if unused.
- (, Transaction memory transaction, , , , ) = abi.decode( - params.precompileData, - (address, Transaction, WriteFinality, uint256, uint256, address) - ); + (, Transaction memory transaction, , , , ) = abi.decode( + params.precompileData, + (address, Transaction, WriteFinality, uint256, uint256, uint64) + );contracts/evmx/helpers/AsyncDeployer.sol (1)
132-152: Potential issue: address prediction requires knowing asyncPromiseCounter.The salt computation at line 151 includes
asyncPromiseCounter, which meansgetAsyncPromiseAddressneeds to know the counter value to predict the address. However,getAsyncPromiseAddress(lines 184-190) doesn't have access to what the counter value will be at deployment time.This creates a race condition:
getAsyncPromiseAddressis called to predict an addressdeployAsyncPromiseContractis called, incrementing the counter- The predicted address won't match the deployed address if other deployments happen in between
Consider either:
- Passing the counter value to
getAsyncPromiseAddress- Removing the counter from the salt and using only
payloadId_+invoker_for uniqueness (if guaranteed unique)- Returning the deployed address from
deployAsyncPromiseContractand not relying on predictionRun this script to check how
getAsyncPromiseAddressis used:#!/bin/bash # Description: Check usage patterns of getAsyncPromiseAddress echo "=== Finding getAsyncPromiseAddress usage ===" rg -n -B5 -A5 'getAsyncPromiseAddress\s*\(' --type=sol echo -e "\n=== Finding deployAsyncPromiseContract usage ===" rg -n -B5 -A5 'deployAsyncPromiseContract\s*\(' --type=solcontracts/evmx/watcher/precompiles/WritePrecompile.sol (1)
240-247: NatSpec says “Only callable by owner” but code enforces onlyWatcher.Update the comment to match behavior or switch to
onlyOwner. Given the rest of the file, keepingonlyWatcheris consistent; fix the doc.- /// @dev Only callable by the contract owner + /// @dev Only callable by the watchercontracts/evmx/watcher/Configurations.sol (1)
145-155: Doc vs behavior: verifyConnections is unrestricted view.NatSpec states “Only callable by the watcher” but the function is public view. Either add an access check in Watcher or adjust the doc here to avoid misleading readers.
♻️ Duplicate comments (6)
contracts/evmx/fees/Credit.sol (1)
269-280: Construct and apply override params before executePayloadoverrideParams is never constructed; commented code leaves consumeFrom/maxFees unset, breaking fee accounting/execution. Build a local OverrideParams and assign it to rawPayload.overrideParams.
- // applyOverride( - // OverrideParamsLib.setMaxFees(overrideParams, getMaxFees(chainSlug_)).setConsumeFrom(consumeFrom_) - // ); - RawPayload memory rawPayload; - rawPayload.overrideParams = overrideParams; + RawPayload memory rawPayload; + OverrideParams memory params; + params = OverrideParamsLib + .setMaxFees(params, getMaxFees(chainSlug_)) + .setConsumeFrom(consumeFrom_); + rawPayload.overrideParams = params; rawPayload.transaction = Transaction({ chainSlug: chainSlug_, target: _getFeesPlugAddress(chainSlug_), payload: payload_ }); watcher__().executePayload(rawPayload, address(this));Add missing imports near the top:
+import {OverrideParams} from "../../utils/common/Structs.sol"; +import "../helpers/OverrideParamsLib.sol";contracts/evmx/fees/FeesManager.sol (1)
58-59: UndefinedoverrideParamsusage causes compilation failure and wrong data source for consumeFrom.FeesManager doesn’t declare
overrideParamsand likely doesn’t inherit it. These references fail compilation and incorrectly fetchconsumeFrom. This should read from the watcher payload for the givenpayloadId_. Also remove the strayoverrideParamswrites in initialize/setMaxFees (they don’t belong here and are redundant withmaxFeesPerChainSlug).Apply these diffs:
- Fix consumeFrom source in both unblocks:
- // address consumeFrom = _getRequestParams(requestCount_).requestFeesDetails.consumeFrom; - address consumeFrom = overrideParams.consumeFrom; + // derive payer from payload storage + address consumeFrom = watcher__().getPayload(payloadId_).consumeFrom;- // address consumeFrom = _getRequestParams(requestCount_).requestFeesDetails.consumeFrom; - address consumeFrom = overrideParams.consumeFrom; + // derive payer from payload storage + address consumeFrom = watcher__().getPayload(payloadId_).consumeFrom;
- Drop invalid overrides in initialize and setMaxFees:
- overrideParams = overrideParams.setSwitchboardType(sbType_).setMaxFees(fees_); + // sbType_ is not used by FeesManager; max fees tracked per-chain below.- function setMaxFees(uint256 fees_) external onlyOwner { - overrideParams = overrideParams.setMaxFees(fees_); - } + function setMaxFees(uint256 fees_) external onlyOwner { + // Keep per-chain cap aligned to evmxSlug’s default + maxFeesPerChainSlug[evmxSlug] = fees_; + }If
Credit.solintentionally exposesoverrideParams, state that explicitly and ensure it is initialized there; otherwise, keep the above fix.Also applies to: 87-87, 120-121, 142-142
contracts/evmx/watcher/precompiles/WritePrecompile.sol (1)
150-190: gasLimit logic returns 0 when override is set.
_validateonly assigns defaults when override is zero, but never initializesgasLimitfrom the override. Digest and precompile data will use 0 even when caller provided a positive value.function _validate( RawPayload calldata rawPayload_, address appGateway_ ) internal view returns (uint256 gasLimit) { + // honor explicit override first + gasLimit = rawPayload_.overrideParams.gasLimit; if ( rawPayload_.overrideParams.value > chainMaxMsgValueLimit[rawPayload_.transaction.chainSlug] ) revert MaxMsgValueLimitExceeded(); if ( rawPayload_.transaction.payload.length == 0 || rawPayload_.transaction.payload.length > PAYLOAD_SIZE_LIMIT ) { revert InvalidPayloadSize(); } if (rawPayload_.transaction.target == bytes32(0)) revert InvalidTarget(); watcher__.verifyConnections( rawPayload_.transaction.chainSlug, rawPayload_.transaction.target, appGateway_, rawPayload_.overrideParams.switchboardType ); - // todo: can be changed to set the default gas limit for each chain - if (rawPayload_.overrideParams.gasLimit == 0) { + // default per chain only when no override was provided + if (gasLimit == 0) { if (rawPayload_.transaction.chainSlug == 5000) { // Mantle default gas limit gasLimit = 8_000_000_000; } else if (rawPayload_.transaction.chainSlug == 1329) { // Sei default gas limit gasLimit = 8_000_000; } else if (rawPayload_.transaction.chainSlug == 999) { // HyperEVM default gas limit gasLimit = 1_500_000; } else { gasLimit = 10_000_000; // other chains default gas limit } } }contracts/evmx/interfaces/IWatcher.sol (1)
61-64: Critical: Type mismatch prevents compilation.Line 63 declares
uint32 switchboardType_but the implementation at Watcher.sol:253 usesbytes32 switchboardType_. This mismatch will block compilation.Apply this diff to fix the interface:
function getCurrentPayloadId( uint32 chainSlug_, - uint32 switchboardType_ + bytes32 switchboardType_ ) external view returns (bytes32);contracts/evmx/watcher/Watcher.sol (2)
4-13: Add missing imports for Converters and error definitions.The contract references
fromBytes32Format(line 188),AppGatewayAlreadyCalled(line 186), andInvalidCallerTriggered(line 190) but doesn't import them. This will cause compilation failure.Add the missing imports:
import "solady/utils/Initializable.sol"; import "./Configurations.sol"; import {IPrecompile} from "../interfaces/IPrecompile.sol"; import {IFeesManager} from "../interfaces/IFeesManager.sol"; import {IPromise} from "../interfaces/IPromise.sol"; import {IERC20} from "../interfaces/IERC20.sol"; import "../../utils/common/IdUtils.sol"; import "solady/utils/LibCall.sol"; import "solady/utils/ECDSA.sol"; +import {fromBytes32Format} from "../../utils/common/Converters.sol";Also ensure the error definitions are available (either through inheritance or import):
AppGatewayAlreadyCalledInvalidCallerTriggeredInsufficientFeesInvalidCallTypeDeadlinePassedDeadlineNotPassedForOnChainRevertOnlyAppGatewayNewMaxFeesLowerThanCurrentInvalidContractInvalidDataInvalidSignatureNonceUsed
246-249: TODO: Complete settlement implementation.The
feesUsed_parameter is accepted but never used, and the actual settlement call is commented out (line 247). This means credits remain blocked after cancellation.Complete the implementation by uncommenting and passing the fees parameter:
- // todo function _settlePayload(bytes32 payloadId_, uint256 feesUsed_) internal { - // feesManager__().unblockAndAssignCredits(payloadId_, address(feesManager__())); + feesManager__().unblockAndAssignCredits(payloadId_, address(feesManager__()), feesUsed_); emit PayloadSettled(payloadId_); }Note: Verify that
unblockAndAssignCreditsaccepts a thirdfeesUsed_parameter. If not, update the IFeesManager interface accordingly.
🧹 Nitpick comments (7)
test/SetupTest.t.sol (1)
721-722: Watcher-only call should impersonate watchermarkRevert likely requires onlyWatcher. This should hoax(watcherEOA) as done in resolvePayload/uploadProof.
- watcher.markRevert(promiseReturnData, isRevertingOnchain_); + hoax(watcherEOA); + watcher.markRevert(promiseReturnData, isRevertingOnchain_);contracts/evmx/interfaces/IConfigurations.sol (1)
46-50: Setter now binds appGateway to msg.sender; update docs and verify callsitesWith appGateway_ removed, setIsValidPlug now implicitly uses msg.sender as the app gateway key while isValidPlug still accepts appGateway in reads. This should be documented in NatSpec and all writers should invoke from the correct address (or via proxy that preserves msg.sender).
test/apps/counter/Counter.sol (1)
20-22: Remove redundant getter function.The
getCounter()function is unnecessary sincecounteris already public and Solidity auto-generates a getter. This adds code bloat without value.Apply this diff:
- function getCounter() external view returns (uint256) { - return counter; - } -contracts/evmx/watcher/precompiles/SchedulePrecompile.sol (1)
117-145: Parameter name should match IPrecompile interface definition.The interface defines this parameter as
address appGateway(IPrecompile.sol:21), but SchedulePrecompile uses an unnamedaddressparameter (line 119). While syntactically valid in Solidity, this violates consistency with the interface contract. WritePrecompile correctly names itappGateway, but ReadPrecompile has the same issue. This should be renamed toappGatewayto match the interface definition, even though it remains unused in this implementation.contracts/evmx/base/AppGatewayBase.sol (1)
133-136: Doc mismatch: return type is bytes32, not uint40.Fix the NatSpec to avoid confusion.
- /// @return uint40 The current payload count + /// @return bytes32 The current payload idcontracts/evmx/watcher/precompiles/WritePrecompile.sol (1)
111-145: Event emits full RawPayload (gas-heavy).Emitting the full struct inflates gas and log size. This should emit minimal fields (e.g., chainSlug, target, callType, gasLimit) and compute the rest off-chain.
test/apps/counter/CounterAppGateway.sol (1)
70-76: uploadPlug is wide open.Anyone can set forwarder addresses in tests. Prefer
onlyOwnerto avoid flaky tests due to accidental calls.- function uploadPlug(uint32 chainSlug_, bytes32 contractId_, bytes32 plug_) public { + function uploadPlug(uint32 chainSlug_, bytes32 contractId_, bytes32 plug_) public onlyOwner {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
contracts/evmx/base/AppGatewayBase.sol(8 hunks)contracts/evmx/fees/Credit.sol(5 hunks)contracts/evmx/fees/FeesManager.sol(5 hunks)contracts/evmx/helpers/AddressResolverUtil.sol(1 hunks)contracts/evmx/helpers/AsyncDeployer.sol(3 hunks)contracts/evmx/interfaces/IAsyncDeployer.sol(1 hunks)contracts/evmx/interfaces/IConfigurations.sol(1 hunks)contracts/evmx/interfaces/IWatcher.sol(1 hunks)contracts/evmx/watcher/Configurations.sol(4 hunks)contracts/evmx/watcher/Watcher.sol(1 hunks)contracts/evmx/watcher/precompiles/SchedulePrecompile.sol(5 hunks)contracts/evmx/watcher/precompiles/WritePrecompile.sol(4 hunks)contracts/utils/OverrideParamsLib.sol(1 hunks)test/SetupTest.t.sol(17 hunks)test/apps/Counter.t.sol(4 hunks)test/apps/counter/Counter.sol(1 hunks)test/apps/counter/CounterAppGateway.sol(1 hunks)test/apps/counter/ICounter.sol(1 hunks)
🔇 Additional comments (24)
contracts/evmx/fees/Credit.sol (1)
36-39: Key-type migration: bytes32 payloadId vs uint40requestBlockedCredits now keys by bytes32. This should include a clear migration plan for any existing state and all callsites must pass bytes32 payloadId. Confirm no remaining uint40-based lookups exist.
test/SetupTest.t.sol (4)
147-158: Core-plug wiring helper looks goodsetupGatewayAndPlugs composes AppGatewayConfig correctly and applies via watcher.setAppGatewayConfigs.
160-177: App gateway config composition LGTMConfig array sizing and field population are tight; hoax(watcherEOA) before set is correct.
228-230: Consistent watcher wiringsetSocket and setSwitchboard with FAST constant align with the new config model.
712-715: Resolve flow fee parameterization LGTMresolvePayload with feesAmount under watcher hoax is consistent with the new flow.
contracts/evmx/helpers/AddressResolverUtil.sol (1)
25-32: Confirm intent: watcher owner access in AddressResolverUtil; fix stale NatSpec; cache watcher__() callThe broadened access in
isWatcher()allowingwatcher__().owner()conflicts with the modifier's NatSpec ("watcher precompile") and diverges from most other implementations (AddressResolver,ReadPrecompile,SchedulePrecompileall enforce strict precompile-only checks). OnlyWritePrecompileintentionally allows owner, suggesting that's an exception for write operations.Either revert to strict
msg.sender == address(watcher__())or confirm owner bypass is intentional and update the NatSpec accordingly—currently contradictory docs ("precompile contract" vs "watcher owner"). Cache thewatcher__()call to avoid two external reads.contracts/evmx/helpers/AsyncDeployer.sol (1)
99-110: LGTM: deployAsyncPromiseContract correctly updated.The function signature and implementation are consistent with the interface change. The
asyncPromiseCounterincrement happens after_createAsyncPromiseParams, which is correct since the counter is used in salt computation.test/apps/counter/Counter.sol (1)
24-27: LGTM: Gateway interaction pattern is correct.The function properly delegates to the gateway through the socket. The commented line about overrides is acceptable as documentation for future functionality.
test/apps/Counter.t.sol (4)
23-32: LGTM: deployCounter properly initializes the counter contract.The deployment flow correctly:
- Creates the Counter instance
- Initializes the socket connection
- Sets up gateway and plug infrastructure
- Registers the plug with the gateway
45-59: LGTM: Test correctly validates counter increment via payload execution.The test properly exercises the payload-centric flow: deploys counter, triggers increment through gateway, executes payload, and validates state change.
61-92: LGTM: Multi-chain test validates independent counter execution.The test properly validates that counters on different chains can be incremented independently through the payload execution model.
113-124: LGTM: Schedule test validates delayed execution.The test correctly validates that scheduled payloads are executed after the specified delay and within the expiry window.
contracts/evmx/watcher/precompiles/SchedulePrecompile.sol (2)
28-33: LGTM: Clean watcher governance pattern.The watcher state variable and access modifier provide clear, explicit access control, replacing the previous inheritance-based approach.
147-152: LGTM: resolvePayload enforces schedule timing constraints.The function correctly validates that the schedule delay has elapsed before allowing resolution. The error handling and event emission are appropriate.
contracts/evmx/interfaces/IAsyncDeployer.sol (1)
37-45: All call sites verified and correctly updated to bytes32.Production code has been fully migrated:
Watcher.solpassescurrentPayloadId(bytes32) todeployAsyncPromiseContractAsyncDeployer.solimplementation correctly encodes bytes32 in constructor args and init data- Salt computation via
keccak256(abi.encodePacked(constructorArgs, asyncPromiseCounter))handles bytes32 without issueDeprecated tests still use uint40, but that's immaterial. Implementation is sound.
contracts/utils/OverrideParamsLib.sol (2)
12-26: LGTM: sane defaults and fluent builder.
clearsets WRITE, LOW finality, zero fees, and acceptsswitchboardType. No issues.
116-134: LGTM: setter coverage is complete.ConsumeFrom, maxFees, gas/value, parallel/read-at-block are covered. No functional concerns.
contracts/evmx/base/AppGatewayBase.sol (1)
90-93: Schedule kick-off looks correct.Setting
SCHEDULE, delay, and executing a minimalRawPayloadwith overrides is consistent with the new flow.contracts/evmx/watcher/Configurations.sol (1)
114-121: setIsValidPlug: scope is appropriate.Binding to
msg.senderensures only the app gateway sets flags for itself. Event emission is correct.test/apps/counter/CounterAppGateway.sol (1)
23-27: Constructor wiring looks correct.Owner/init and maxFees override set via library; good for tests.
contracts/evmx/watcher/Watcher.sol (4)
85-97: Verify fee blocking strategy.Lines 85-89 block
maxFeesbefore computing actual fees from the precompile (line 95-96). If the actualfeesreturned by the precompile is less thanmaxFees, the excess remains blocked until settlement. This ties up more credits than necessary.Confirm this is intentional. If immediate adjustment is desired, unblock and re-block with actual fees:
feesManager__().blockCredits( currentPayloadId, rawPayload_.overrideParams.consumeFrom, rawPayload_.overrideParams.maxFees ); latestAsyncPromise = asyncDeployer__().deployAsyncPromiseContract( appGateway_, currentPayloadId ); (uint256 fees, uint256 deadline, bytes memory precompileData) = IPrecompile(precompile) .handlePayload(rawPayload_, appGateway_, currentPayloadId); if (fees > rawPayload_.overrideParams.maxFees) revert InsufficientFees(); + + // Adjust blocked credits to actual fees + if (fees < rawPayload_.overrideParams.maxFees) { + feesManager__().unblockCredits(currentPayloadId); + feesManager__().blockCredits(currentPayloadId, rawPayload_.overrideParams.consumeFrom, fees); + }
216-233: LGTM - Fee increase logic is sound.The function correctly validates state (not cancelled, not executed, new fees higher), unblocks old credits, validates new credits are available, then reblocks with the new amount. The event emission enables off-chain actors to respond.
259-318: LGTM - Utility functions are well-implemented.The getter/setter functions (getPayload, setTriggerFees, setPrecompile, getPrecompileFees) are straightforward and correct. The signature validation logic properly checks nonce reuse, constructs the digest with contract address and chain slug for replay protection, and uses ECDSA.recover with the EIP-191 prefix.
251-257: Disregard this review comment—the code is correct.
createPayloadIdis consistently called across the codebase passing the destination chain as the third parameter (MessageSwitchboard.sol line 165, Socket.sol). The payload ID must be scoped to the destination chain sincegetCurrentPayloadIdreceiveschainSlug_as input and uses it to look up the switchboard configuration. UsingevmxSlugwould create ID collisions for different destination chains sharing the same payload counter.
| mapping(address => bool) public isValidPromise; | ||
|
|
||
| // slot 58 | ||
| mapping(bytes32 => mapping(uint32 => address)) public override forwarderAddresses; |
There was a problem hiding this comment.
Invalid override on state variable.
public override forwarderAddresses is not allowed on state vars. This should compile as a plain public mapping.
- mapping(bytes32 => mapping(uint32 => address)) public override forwarderAddresses;
+ mapping(bytes32 => mapping(uint32 => address)) public forwarderAddresses;📝 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.
| mapping(bytes32 => mapping(uint32 => address)) public override forwarderAddresses; | |
| mapping(bytes32 => mapping(uint32 => address)) public forwarderAddresses; |
🤖 Prompt for AI Agents
In contracts/evmx/base/AppGatewayBase.sol around line 36, the state variable
declaration uses an invalid `override` modifier on a public mapping; remove
`override` so it is declared as a plain public mapping (i.e., change
`mapping(...) public override forwarderAddresses;` to `mapping(...) public
forwarderAddresses;`) and ensure any required overriding is handled on
functions, not state variables.
| isAsyncModifierSet = true; | ||
| } |
There was a problem hiding this comment.
Async guard not reset and missing zero-address check for promise.
isAsyncModifierSet is set true in _preAsync but never cleared, allowing calls like _setSchedule outside the intended async scope. Also guard against zero promise.
function _postAsync() internal {
- _clearOverrides();
- address promise_ = watcher__().latestAsyncPromise();
- isValidPromise[promise_] = true;
+ _clearOverrides();
+ address promise_ = watcher__().latestAsyncPromise();
+ if (promise_ != address(0)) {
+ isValidPromise[promise_] = true;
+ }
+ // close the async window
+ isAsyncModifierSet = false;
}Also applies to: 72-76
🤖 Prompt for AI Agents
In contracts/evmx/base/AppGatewayBase.sol around lines 69-70 (and similarly
lines 72-76), _preAsync sets isAsyncModifierSet true but nothing clears it and
_setSchedule lacks a zero-address check for the promise; fix by adding
isAsyncModifierSet = false in the corresponding _postAsync function so the guard
is cleared after async execution, and add a require(promise != address(0),
"invalid promise") at the start of _setSchedule to prevent zero-address
promises; ensure the modifier/flow guarantees _postAsync always runs (so the
flag is cleared on all execution paths).
contracts/evmx/fees/Credit.sol
Outdated
| if (!isCreditSpendable(from_, msg.sender, amount_)) revert InsufficientCreditsAvailable(); | ||
|
|
||
| if (_isWatcher(msg.sender)) _approve(from_, msg.sender, amount_); | ||
| if (msg.sender == address(watcher__())) _approve(from_, msg.sender, amount_); | ||
| return super.transferFrom(from_, to_, amount_); | ||
| } |
There was a problem hiding this comment.
Don’t clobber user approvals when caller is watcher
Setting allowance to amount_ for watcher overwrites any existing allowance (including infinite). This should bypass allowance entirely for watcher and perform a direct transfer.
- if (msg.sender == address(watcher__())) _approve(from_, msg.sender, amount_);
- return super.transferFrom(from_, to_, amount_);
+ if (msg.sender == address(watcher__())) {
+ _transfer(from_, to_, amount_);
+ return true;
+ }
+ return super.transferFrom(from_, to_, amount_);📝 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.
| if (!isCreditSpendable(from_, msg.sender, amount_)) revert InsufficientCreditsAvailable(); | |
| if (_isWatcher(msg.sender)) _approve(from_, msg.sender, amount_); | |
| if (msg.sender == address(watcher__())) _approve(from_, msg.sender, amount_); | |
| return super.transferFrom(from_, to_, amount_); | |
| } | |
| if (!isCreditSpendable(from_, msg.sender, amount_)) revert InsufficientCreditsAvailable(); | |
| if (msg.sender == address(watcher__())) { | |
| _transfer(from_, to_, amount_); | |
| return true; | |
| } | |
| return super.transferFrom(from_, to_, amount_); | |
| } |
🤖 Prompt for AI Agents
In contracts/evmx/fees/Credit.sol around lines 223 to 227, the current logic
sets the watcher's allowance to amount_ (clobbering any existing approval)
before calling super.transferFrom; instead, detect when msg.sender ==
address(watcher__()) and bypass allowance handling entirely by performing a
direct internal transfer (call the contract's internal _transfer(from_, to_,
amount_) and return true) so existing user allowances (including infinite
approvals) are not overwritten; otherwise, keep the existing path that uses
super.transferFrom.
| import {DigestParams, Payload} from "../../../utils/common/Structs.sol"; | ||
|
|
||
| import {WRITE, PAYLOAD_SIZE_LIMIT, CHAIN_SLUG_SOLANA_MAINNET, CHAIN_SLUG_SOLANA_DEVNET} from "../../../utils/common/Constants.sol"; | ||
| import {InvalidIndex, MaxMsgValueLimitExceeded, InvalidPayloadSize} from "../../../utils/common/Errors.sol"; | ||
| import {InvalidIndex, MaxMsgValueLimitExceeded, InvalidPayloadSize, OnlyWatcherAllowed, InvalidTarget} from "../../../utils/common/Errors.sol"; |
There was a problem hiding this comment.
Missing RawPayload import and event type will not compile.
The file uses RawPayload in the event and function signatures but doesn’t import it. Add it to the Structs import.
-import {DigestParams, Payload} from "../../../utils/common/Structs.sol";
+import {DigestParams, Payload, RawPayload} from "../../../utils/common/Structs.sol";Also applies to: 63-64, 98-107
🤖 Prompt for AI Agents
In contracts/evmx/watcher/precompiles/WritePrecompile.sol around lines 10-13
(and also referenced at lines 63-64 and 98-107), the RawPayload type is used but
not imported from Structs.sol causing compilation errors; update the import from
"../../../utils/common/Structs.sol" to include RawPayload (e.g., import
{DigestParams, Payload, RawPayload} ...) so the event and function signatures
that reference RawPayload compile correctly.
| function withdrawCredits( | ||
| uint32 chainSlug_, | ||
| address token_, | ||
| uint256 amount_, | ||
| address receiver_ | ||
| ) external { | ||
| _withdrawCredits(chainSlug_, token_, amount_, receiver_); | ||
| } | ||
|
|
There was a problem hiding this comment.
Unrestricted withdrawCredits lets anyone drain credits in tests.
Even for tests, this is risky and can mask real issues. Restrict to owner (or watcher) to reflect real-world usage.
- function withdrawCredits(
+ function withdrawCredits(
uint32 chainSlug_,
address token_,
uint256 amount_,
address receiver_
- ) external {
+ ) external onlyOwner {
_withdrawCredits(chainSlug_, token_, amount_, receiver_);
}📝 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 withdrawCredits( | |
| uint32 chainSlug_, | |
| address token_, | |
| uint256 amount_, | |
| address receiver_ | |
| ) external { | |
| _withdrawCredits(chainSlug_, token_, amount_, receiver_); | |
| } | |
| function withdrawCredits( | |
| uint32 chainSlug_, | |
| address token_, | |
| uint256 amount_, | |
| address receiver_ | |
| ) external onlyOwner { | |
| _withdrawCredits(chainSlug_, token_, amount_, receiver_); | |
| } |
🤖 Prompt for AI Agents
In test/apps/counter/CounterAppGateway.sol around lines 98 to 106, the
withdrawCredits function is publicly callable allowing anyone to drain credits;
restrict it to an authorized actor by adding an access control check (e.g.,
require(msg.sender == owner || msg.sender == watcher) or apply an
onlyOwner/onlyWatcher modifier). Update the contract to store and expose the
owner/watcher address (or inherit OpenZeppelin Ownable) and use that
check/modifier on withdrawCredits, and adjust tests to call withdrawCredits from
the authorized account.
| function increase() external; | ||
|
|
||
| function getCounter() external; | ||
|
|
||
| // A function that is not part of the interface, used for testing on-chian revert. | ||
| function wrongFunction() external; | ||
| } |
There was a problem hiding this comment.
Correct interface: add return type and view mutability
getCounter must declare view returns (uint256) to match implementations and avoid interface/override mismatch. Fix the typo in the comment.
-interface ICounter {
- function increase() external;
-
- function getCounter() external;
-
- // A function that is not part of the interface, used for testing on-chian revert.
- function wrongFunction() external;
-}
+interface ICounter {
+ function increase() external;
+ function getCounter() external view returns (uint256);
+ // A function that is not part of the interface, used for testing on-chain revert.
+ function wrongFunction() external;
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test/apps/counter/ICounter.sol around lines 5 to 11, the interface is
incorrect: change the getCounter declaration to include view and a uint256
return type (i.e., declare it as view returns (uint256)) so it matches
implementations and overrides, and fix the comment typo "on-chian" to "on-chain"
to clarify the intent; leave or remove wrongFunction as desired for tests but
ensure interface signatures compile.
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
contracts/evmx/fees/Credit.sol (2)
138-149: Double‑mint bug on deposit when native transfer failson failure you mint creditAmount_ again and then add zero. This should mint nativeAmount_ as credits and adjust emitted values.
- // Mint tokens to the user - _mint(depositTo_, creditAmount_); + // Mint credits + _mint(depositTo_, creditAmount_); if (nativeAmount_ > 0) { // if native transfer fails, add to credit bool success = feesPool.withdraw(depositTo_, nativeAmount_); if (!success) { - _mint(depositTo_, creditAmount_); - nativeAmount_ = 0; - creditAmount_ += nativeAmount_; + uint256 creditFallback = nativeAmount_; + _mint(depositTo_, creditFallback); + creditAmount_ += creditFallback; + nativeAmount_ = 0; } } emit Deposited(chainSlug_, token_, depositTo_, creditAmount_, nativeAmount_);
178-181: Guard against underflow in balanceOfIf userBlockedCredits exceeds total, this reverts. This should saturate to zero.
- function balanceOf(address account) public view override returns (uint256) { - return super.balanceOf(account) - userBlockedCredits[account]; - } + function balanceOf(address account) public view override returns (uint256) { + uint256 total = super.balanceOf(account); + uint256 blocked = userBlockedCredits[account]; + return blocked > total ? 0 : total - blocked; + }hardhat-scripts/deploy/3.configureChains.ts (2)
148-152: Bug: double toBytes32 conversion forsocket.You compute
socketas bytes32, then convert it again before sending on-chain. This should pass bytes32 exactly once.- const socket = toBytes32FormatHexString(chainAddresses[Contracts.Socket]); + const socket = chainAddresses[Contracts.Socket]; ... - [chain], - toBytes32FormatHexString(socket), + [chain], + toBytes32FormatHexString(socket), ... - [chain, toBytes32FormatHexString(socket)], + [chain, toBytes32FormatHexString(socket)],Alternatively, keep
socketas bytes32 and remove subsequent conversions:- const socket = toBytes32FormatHexString(chainAddresses[Contracts.Socket]); + const socket = toBytes32FormatHexString(chainAddresses[Contracts.Socket]); ... - toBytes32FormatHexString(socket), + socket, ... - [chain, toBytes32FormatHexString(socket)], + [chain, socket],Also applies to: 175-184
186-201: Bug: double toBytes32 conversion forfeesPlug.Same issue:
feesPlugis set as bytes32, then converted again in updateContractSettings. This should only be converted once.- const feesPlug = toBytes32FormatHexString( - chainAddresses[Contracts.FeesPlug]! - ); + const feesPlug = chainAddresses[Contracts.FeesPlug]!; ... - [chain], - toBytes32FormatHexString(feesPlug).toString(), + [chain], + toBytes32FormatHexString(feesPlug), ... - [chain, toBytes32FormatHexString(feesPlug)], + [chain, toBytes32FormatHexString(feesPlug)],contracts/evmx/watcher/precompiles/SchedulePrecompile.sol (3)
62-69: Fix guard: comparing against stale state lets invalid configs slip throughConstructor checks maxScheduleDelayInSeconds against old expiryTime (0), not the new input. This should compare inputs before assignment.
Apply:
maxScheduleDelayInSeconds = maxScheduleDelayInSeconds_; scheduleFeesPerSecond = scheduleFeesPerSecond_; scheduleCallbackFees = scheduleCallbackFees_; - if (maxScheduleDelayInSeconds < expiryTime) revert InvalidScheduleDelay(); - expiryTime = expiryTime_; + if (maxScheduleDelayInSeconds_ < expiryTime_) revert InvalidScheduleDelay(); + expiryTime = expiryTime_; watcher__ = IWatcher(watcher_);
80-84: Fix setter guards: validate new value against the existing counterpartBoth setters validate current values instead of the new input, allowing invalid updates.
Use:
function setMaxScheduleDelayInSeconds(uint256 maxScheduleDelayInSeconds_) external onlyWatcher { - if (maxScheduleDelayInSeconds < expiryTime) revert InvalidScheduleDelay(); + if (maxScheduleDelayInSeconds_ < expiryTime) revert InvalidScheduleDelay(); maxScheduleDelayInSeconds = maxScheduleDelayInSeconds_; emit MaxScheduleDelayInSecondsSet(maxScheduleDelayInSeconds_); } function setExpiryTime(uint256 expiryTime_) external onlyWatcher { - if (maxScheduleDelayInSeconds < expiryTime) revert InvalidScheduleDelay(); + if (maxScheduleDelayInSeconds < expiryTime_) revert InvalidScheduleDelay(); expiryTime = expiryTime_; emit ExpiryTimeSet(expiryTime_); }Also applies to: 108-112
71-74: Encode/decode mismatch: getPrecompileFees decodes 1 item but precompileData encodes 2abi.decode expects the exact layout. You encode (delay, executeAfter) but decode only (delay). This should compute fees from the raw delay to avoid decoding mismatch.
Apply:
- precompileData = abi.encode(rawPayload.overrideParams.delayInSeconds, executeAfter); - fees = getPrecompileFees(precompileData); + precompileData = abi.encode(rawPayload.overrideParams.delayInSeconds, executeAfter); + // compute directly from delay to avoid ABI layout dependency + fees = scheduleFeesPerSecond * rawPayload.overrideParams.delayInSeconds + scheduleCallbackFees;Optionally update getPrecompileFees doc to clarify it expects abi.encode(delayInSeconds).
Also applies to: 126-131
test/SetupTest.t.sol (1)
711-716: Wrong decode type for Write precompileDataWritePrecompile encodes the last field as uint64 switchboardId, not address. This decode will revert.
Apply:
- (, Transaction memory transaction, , , , ) = abi.decode( - params.precompileData, - (address, Transaction, WriteFinality, uint256, uint256, address) - ); + (, Transaction memory transaction, , , , ) = abi.decode( + params.precompileData, + (address, Transaction, WriteFinality, uint256, uint256, uint64) + );
♻️ Duplicate comments (7)
contracts/evmx/base/AppGatewayBase.sol (2)
36-36: Invalidoverridemodifier on state variable.State variables cannot use the
overridekeyword in Solidity. This will cause a compilation error. Remove theoverridemodifier.Apply this diff:
- mapping(bytes32 => mapping(uint32 => address)) public override forwarderAddresses; + mapping(bytes32 => mapping(uint32 => address)) public forwarderAddresses;
65-74: Critical: Async guard never reset and missing zero-address validation.Two issues in the async flow:
isAsyncModifierSetis set totruein_preAsync(line 67) but never reset tofalsein_postAsync. This leaves the async guard permanently open after the first async call, allowing functions like_setScheduleto be called outside their intended async scope.No validation that
promise_is non-zero before marking it valid (line 73). IfexecutePayload()returnsaddress(0), an invalid promise will be marked as valid.Apply this diff:
function _postAsync() internal { _clearOverrides(); address promise_ = watcher__().executePayload(); - isValidPromise[promise_] = true; + if (promise_ != address(0)) { + isValidPromise[promise_] = true; + } + isAsyncModifierSet = false; }contracts/evmx/fees/Credit.sol (2)
71-72: Missing imports for OverrideParams types and libraryusing OverrideParamsLib requires importing both the type and the library. RawPayload/Transaction are used later.
import {toBytes32Format} from "../../utils/common/Converters.sol"; +import {OverrideParams, RawPayload, Transaction} from "../../utils/common/Structs.sol"; +import "../../utils/OverrideParamsLib.sol";
226-228: Do not clobber approvals when caller is watcherApproving amount_ overwrites existing approvals (incl. infinite) and is unnecessary. Bypass allowance and transfer directly when watcher calls.
- if (msg.sender == address(watcher__())) _approve(from_, msg.sender, amount_); - return super.transferFrom(from_, to_, amount_); + if (msg.sender == address(watcher__())) { + _transfer(from_, to_, amount_); + return true; + } + return super.transferFrom(from_, to_, amount_);contracts/evmx/fees/FeesManager.sol (1)
139-146: Fix: wrong source for consumeFrom in unblockCredits (also duplicates a prior issue).This should read consumeFrom from the payload, not from overrideParams. Using overrideParams.consumeFrom will compile-break if the var isn’t declared here and is semantically wrong (it’s not per‑payload). Replace with watcher payload read.
- // address consumeFrom = _getRequestParams(requestCount_).requestFeesDetails.consumeFrom; - address consumeFrom = overrideParams.consumeFrom; + // consumeFrom is stored per-payload by the watcher + address consumeFrom = watcher__().getPayload(payloadId_).consumeFrom;contracts/evmx/watcher/Watcher.sol (1)
196-222: Add missing imports to Watcher.solThe function
_callAppGatewaysusesfromBytes32Formatand custom errors that aren't imported. Solidity doesn't inherit imports through contract inheritance—Watcher.sol needs direct imports even though Configurations already has them.Add these imports after line 11:
import "../../utils/common/IdUtils.sol"; import "solady/utils/LibCall.sol"; +import {fromBytes32Format} from "../../utils/common/Converters.sol"; +import "../../utils/common/Errors.sol";Code won't compile without these.
contracts/evmx/interfaces/IWatcher.sol (1)
65-68: Critical: Type mismatch still not fixed.Line 67 declares
uint32 switchboardType_but the implementation usesbytes32. This was flagged in previous review and remains unfixed. This will cause compilation failure.Apply this diff:
function getCurrentPayloadId( uint32 chainSlug_, - uint32 switchboardType_ + bytes32 switchboardType_ ) external view returns (bytes32);
🧹 Nitpick comments (12)
contracts/evmx/helpers/AsyncDeployer.sol (1)
99-102: Consider adding input validation for payloadId_.The function doesn't validate that
payloadId_is non-zero. An empty payload ID might indicate an error in the calling code.Add validation:
function deployAsyncPromiseContract( address invoker_, bytes32 payloadId_ ) external override onlyWatcher returns (address newAsyncPromise) { + require(payloadId_ != bytes32(0), "Invalid payloadId"); // creates init data and salthardhat-scripts/deploy/3.configureChains.ts (1)
203-212: If re‑enabled: fix contractFactory double conversion before using.The commented block also converts bytes32 twice. This should mirror the fix above.
- const contractFactory = toBytes32FormatHexString( - chainAddresses[Contracts.ContractFactoryPlug] - ); + const contractFactory = chainAddresses[Contracts.ContractFactoryPlug]; ... - toBytes32FormatHexString(contractFactory).toString(), + toBytes32FormatHexString(contractFactory), ... - [chain, toBytes32FormatHexString(contractFactory)], + [chain, toBytes32FormatHexString(contractFactory)],src/enums.ts (1)
49-51: Remove commented enum members to avoid drift.Dead, commented constants should be deleted to keep the public surface clean. Git tracks history.
- // AuctionEnded = "AuctionEnded", - // AuctionRestarted = "AuctionRestarted",src/events.ts (1)
38-41: Delete the commented export block.Keep the module lean; commented exports add noise.
-// export const auctionManagerEvents = [ -// Events.AuctionEnded, -// Events.AuctionRestarted, -// ];contracts/evmx/watcher/precompiles/SchedulePrecompile.sol (2)
12-15: Docs are wrong: this is a contract, not a library with pure functionsUpdate the title/notice/dev comments to match the implementation.
80-112: Access-control docs mismatchNatSpec says “Only callable by the contract owner” but these functions are onlyWatcher-gated. Update the comments.
test/SetupTest.t.sol (1)
503-506: Event signature drift from SchedulePrecompileSchedulePrecompile emits ScheduleRequested(payloadId, executeAfter, deadline, localInvoker, selector, data). The test’s local event has only (payloadId, deadline). If you plan to expect/parse ScheduleRequested, this should mirror the contract signature.
contracts/evmx/watcher/precompiles/ReadPrecompile.sol (2)
25-33: Access control consistency across precompilesRead/Schedule restrict to watcher address; Write allows watcher OR watcher.owner(). This should be consistent to avoid surprising governance paths. Pick one model repo‑wide.
78-81: Docs mismatch: “Only owner” vs onlyWatcherFix NatSpec to reflect onlyWatcher authorization.
contracts/evmx/watcher/precompiles/WritePrecompile.sol (2)
68-73: Access control inconsistency with other precompilesWritePrecompile allows watcher OR watcher.owner(). Read/Schedule restrict to watcher. This should be consistent.
Option A: Restrict to address(watcher__) only. Option B: Allow owner in all three. Document the model.
60-61: Event payload sizeEmitting full RawPayload can bloat logs. If not strictly needed on-chain, emit compact fields (payloadId, digest, chainSlug, gasLimit) to cut gas.
contracts/evmx/watcher/Configurations.sol (1)
163-181: Clarify contractAddress_ parameter purpose.The digest includes both
address(this)andcontractAddress_(line 176), but when called at line 79,contractAddress_is alsoaddress(this). This meansaddress(this)appears twice in the digest. Is this intentional? IfcontractAddress_is meant to allow validation for different contracts, consider whether including it twice adds security value or just creates confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
FunctionSignatures.md(1 hunks)contracts/evmx/base/AppGatewayBase.sol(7 hunks)contracts/evmx/fees/Credit.sol(6 hunks)contracts/evmx/fees/FeesManager.sol(4 hunks)contracts/evmx/helpers/AddressResolver.sol(0 hunks)contracts/evmx/helpers/AsyncDeployer.sol(3 hunks)contracts/evmx/helpers/Forwarder.sol(2 hunks)contracts/evmx/interfaces/IConfigurations.sol(2 hunks)contracts/evmx/interfaces/IWatcher.sol(1 hunks)contracts/evmx/watcher/Configurations.sol(6 hunks)contracts/evmx/watcher/Watcher.sol(2 hunks)contracts/evmx/watcher/precompiles/ReadPrecompile.sol(2 hunks)contracts/evmx/watcher/precompiles/SchedulePrecompile.sol(5 hunks)contracts/evmx/watcher/precompiles/WritePrecompile.sol(4 hunks)hardhat-scripts/deploy/1.deploy.ts(3 hunks)hardhat-scripts/deploy/2.roles.ts(1 hunks)hardhat-scripts/deploy/3.configureChains.ts(1 hunks)hardhat-scripts/deploy/4.configureEVMx.ts(3 hunks)hardhat-scripts/deploy/6.connect.ts(1 hunks)hardhat-scripts/deploy/8.setupEnv.ts(0 hunks)hardhat-scripts/deploy/9.setupTransmitter.ts(0 hunks)hardhat-scripts/deploy/UpgradeForwarder.ts(1 hunks)hardhat-scripts/deploy/UpgradePromise.ts(1 hunks)hardhat-scripts/deploy/WhitelistFeesReceiver.ts(0 hunks)package.json(1 hunks)src/enums.ts(2 hunks)src/events.ts(2 hunks)src/signer.ts(1 hunks)src/types.ts(0 hunks)test/SetupTest.t.sol(17 hunks)test/apps/counter/CounterAppGateway.sol(1 hunks)
💤 Files with no reviewable changes (5)
- hardhat-scripts/deploy/WhitelistFeesReceiver.ts
- src/types.ts
- hardhat-scripts/deploy/8.setupEnv.ts
- hardhat-scripts/deploy/9.setupTransmitter.ts
- contracts/evmx/helpers/AddressResolver.sol
✅ Files skipped from review due to trivial changes (3)
- src/signer.ts
- package.json
- hardhat-scripts/deploy/UpgradePromise.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- contracts/evmx/helpers/Forwarder.sol
- test/apps/counter/CounterAppGateway.sol
- FunctionSignatures.md
🧰 Additional context used
🧬 Code graph analysis (1)
hardhat-scripts/deploy/1.deploy.ts (1)
hardhat-scripts/config/config.ts (2)
EVMX_CHAIN_ID(181-181)transmitter(183-183)
🔇 Additional comments (27)
contracts/evmx/base/AppGatewayBase.sol (8)
83-91: LGTM! Schedule logic is correct.The
callTypeis now correctly set toSCHEDULE(line 85), and the payload is properly constructed with override parameters before being added to the watcher.
93-103: LGTM! Payload-based admin helpers are correct.The refactored
_revertTxand_increaseFeesfunctions correctly usepayloadId_with the new watcher APIs (cancelExecutionandincreaseFees).
143-145: LGTM! Switchboard type setter is correct.The function correctly updates
overrideParams.switchboardType.
157-167: LGTM! Permit helper is correct.The function properly decodes the approval data and forwards it to the token's permit function.
174-188: LGTM! Withdraw credits uses correct override params.The function correctly uses
overrideParams.maxFeesfor the fee calculation in the new payload-based model.
194-213: LGTM! Override parameter helpers are correct.The getter, clear, and apply functions properly manage the
OverrideParamsstate usingOverrideParamsLib.
109-136: LGTM! Utility functions are correct.The
getOnChainAddress,_createContractId, and_getCurrentPayloadIdfunctions correctly implement address resolution and payload ID retrieval in the new payload-based model.
222-222: LGTM! Virtual hook is correct.The
handleReverthook provides the expected extension point for derived contracts.contracts/evmx/helpers/AsyncDeployer.sol (3)
180-186: Implementation is consistent with deployment flow.The address prediction correctly mirrors the salt generation logic in
_createAsyncPromiseParams. However, this prediction is only reliable ifasyncPromiseCounterdoesn't change between prediction and deployment (see comment on lines 99-110).
132-148: ****The AsyncPromise.initialize signature already matches the parameter order in AsyncDeployer._createAsyncPromiseParams. Both encode and invoke parameters as
(payloadId_, invoker_, addressResolver_). The AsyncPromise constructor is intentionally empty (disables initializers for the implementation), and the encoding parameters are used for salt calculation, not constructor invocation. No changes needed.Likely an incorrect or invalid review comment.
99-110: Address prediction function has race condition but is currently unused.The race condition in the review comment is technically valid:
getAsyncPromiseAddressreads the currentasyncPromiseCounterto predict an address, but the counter can increment between prediction and deployment, making the predicted address stale.However, this function is exposed in the
IAsyncDeployerinterface but not called in production code—deployAsyncPromiseContractinWatcher.soldeploys directly without pre-prediction. The counter is intentionally designed to support multiple AsyncPromise contracts per(invoker_, payloadId_)pair.The practical risk depends on intended usage:
- If external integrations will use
getAsyncPromiseAddressfor off-chain address prediction, this race condition is a bug that needs fixing (salt should use onlypayloadId_if it's guaranteed unique per deployment, or remove the function entirely).- If
getAsyncPromiseAddressis meant only for post-deployment verification or won't be used, the counter design is fine as-is.Clarify the intended use case and either remove the unused function or document that it's unsafe for cross-process address prediction.
hardhat-scripts/deploy/1.deploy.ts (1)
304-313: ContractFactoryPlug deployment removedGood cleanup aligned with plugs list. No further action.
contracts/evmx/fees/Credit.sol (1)
36-39: Storage key change for blocked credits requires migration planSwitching mapping key from uint40 requestCount to bytes32 payloadId changes the storage hashing. Existing entries (if any) become unreachable. This should include an upgrade/migration (or explicit reset) plan to prevent stranded balances.
Explain whether this contract is freshly deployed or provide a migration that rekeys old entries to payloadId.
hardhat-scripts/deploy/UpgradeForwarder.ts (1)
4-4: Import verified—ChainSlug is correctly re-exportedChainSlug is defined in
src/chain-enums/chainSlug.tsand re-exported throughsrc/index.tsvia theexport * from "./chain-enums"barrel export. The import from "../../src" will resolve correctly with no type or runtime errors.hardhat-scripts/deploy/6.connect.ts (1)
18-20: This change is incomplete—ContractFactoryPlug still actively used elsewhere in codebaseThe plugs array narrowing achieves its immediate goal in this file (no init/connectSocket calls to ContractFactoryPlug), but leaves the system inconsistent:
hardhat-scripts/utils/gatewayId.ts:12— Active case handler for ContractFactoryPlughardhat-scripts/deploy/3.configureChains.ts:150— Reads ContractFactoryPlug from chainAddresseshardhat-scripts/admin/rescue.ts:79— Expects ContractFactoryPlug in address array- Related deployment & config code commented out in
1.deploy.ts,2.roles.ts,3.configureChains.tsThis suggests either:
- Incomplete refactoring — Must clean up all ContractFactoryPlug references system-wide, or
- Partial deprecation — Needs clear decision: keep it available or remove it entirely
Environment-gating is not a solution here. If ContractFactoryPlug removal is intentional, gate or remove the references throughout. If accidental, complete the refactoring.
contracts/evmx/fees/FeesManager.sol (2)
119-133: Good: per‑payload consumeFrom source and clean accounting.Reading consumeFrom from watcher payload and using the cached blockedCredits_ eliminates mismatch risks. Event payload is correct.
58-59: State variableoverrideParamsis properly declared and inherited.The variable exists in AppGatewayBase.sol (line 27) as a public state variable of type
OverrideParams. FeesManager inherits it through the chain: FeesManager → Credit → AppGatewayBase. Both assignments at lines 58-59 and 86-88 are valid, as is the access at line 141. No compilation errors.src/events.ts (1)
12-24: watcherEvents consolidation verified—removed arrays have no external references.The codebase contains no imports of
promiseResolverEvents,requestHandlerEvents, orconfigurationsEvents, confirming the consolidation is safe. The merged array structure aligns with the payload-centric event flow and reduces consumer complexity.src/enums.ts (1)
32-36: Event surface change verification complete. No stale references found.Request* events are absent from active code; all references are confined to deprecated/. Payload* events are properly defined in src/enums.ts, tracked in src/events.ts, and actively emitted across contracts/evmx/watcher/Watcher.sol at appropriate points (PayloadStored, PayloadResolved, PayloadCancelled, PayloadSettled). The transition is clean and complete.
contracts/evmx/interfaces/IConfigurations.sol (1)
46-47: Interface change properly implemented. No active code issues.The signature change from 4 to 3 parameters has been correctly handled:
- Implementation (contracts/evmx/watcher/Configurations.sol:120) now accepts 3 params and infers
appGateway_frommsg.sender- Active call site (contracts/evmx/base/AppGatewayBase.sol:154) uses the correct 3-arg signature
- Deprecated code has incompatible 4-arg calls, but this is not part of the active contracts
No fixes needed.
contracts/evmx/watcher/precompiles/ReadPrecompile.sol (1)
45-65: Validation and encoding are solidChecks target/payload, computes deadline, and emits with payloadId. No issues.
contracts/evmx/watcher/Watcher.sol (1)
256-259: Suggested fix is incompatible with function signatureThe
unblockAndAssignCreditsmethod accepts only 2 parameters (bytes32 payloadId_,address assignTo_), not 3. PassingfeesUsed_as shown in the suggested diff would cause a compilation error.If
feesUsed_tracking is necessary for accurate settlement, the function signature itself needs modification—both inFeesManager.solandIFeesManager.sol—before the call site can be updated. This is a different change than what the review proposes.Likely an incorrect or invalid review comment.
contracts/evmx/interfaces/IWatcher.sol (1)
8-20: Clean interface migration to payload-centric model.The new interface properly defines payload lifecycle events and aligns with the architectural shift from queue/request-centric to payload-centric model.
contracts/evmx/watcher/Configurations.sol (4)
37-38: LGTM: State variables support signature validation.These variables properly enable nonce-based replay protection and chain-specific digest computation.
187-195: LGTM: Standard EIP-191 signature recovery.The implementation correctly applies the Ethereum Signed Message prefix and recovers the signer.
78-91: Both structs are properly defined and correctly used.WatcherMultiCallParams and AppGatewayConfig are defined in contracts/utils/common/Structs.sol, and all field accesses in the function match their definitions. No issues found.
120-123: Verify callers updated for msg.sender context.The function now uses
msg.senderinstead of an explicitappGateway_parameter. This is more secure but is a breaking change. Ensure all calling code has been updated to call this function from the correct context (the app gateway itself must call it).
contracts/evmx/fees/FeesManager.sol
Outdated
| /// @notice Blocks fees for a request count | ||
| /// @param requestCount_ The batch identifier | ||
| /// @param payloadId_ The payload id | ||
| /// @param consumeFrom_ The fees payer address | ||
| /// @param credits_ The total fees to block | ||
| /// @dev Only callable by delivery helper | ||
| function blockCredits( |
There was a problem hiding this comment.
Docstring drift.
“This should” say “Blocks fees for a payload” not “request count”. Update the NatSpec to match the payloadId flow.
- /// @notice Blocks fees for a request count
+ /// @notice Blocks fees for a payload📝 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.
| /// @notice Blocks fees for a request count | |
| /// @param requestCount_ The batch identifier | |
| /// @param payloadId_ The payload id | |
| /// @param consumeFrom_ The fees payer address | |
| /// @param credits_ The total fees to block | |
| /// @dev Only callable by delivery helper | |
| function blockCredits( | |
| /// @notice Blocks fees for a payload | |
| /// @param payloadId_ The payload id | |
| /// @param consumeFrom_ The fees payer address | |
| /// @param credits_ The total fees to block | |
| /// @dev Only callable by delivery helper | |
| function blockCredits( |
🤖 Prompt for AI Agents
In contracts/evmx/fees/FeesManager.sol around lines 92 to 97, the NatSpec
title/description is incorrect: it currently says "Blocks fees for a request
count" but the function operates on a payloadId; update the docstring to say
"Blocks fees for a payload" (and adjust any other mentions of "request count" to
"payload" or "payloadId" so the documentation matches the function parameters
and behavior).
| function setAppGatewayConfigs(WatcherMultiCallParams memory params_) external; | ||
|
|
There was a problem hiding this comment.
Use calldata for external struct param to avoid unnecessary copies.
This should be calldata for gas and ABI consistency; interfaces constrain implementers.
- function setAppGatewayConfigs(WatcherMultiCallParams memory params_) external;
+ function setAppGatewayConfigs(WatcherMultiCallParams calldata params_) external;📝 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 setAppGatewayConfigs(WatcherMultiCallParams memory params_) external; | |
| function setAppGatewayConfigs(WatcherMultiCallParams calldata params_) external; |
🤖 Prompt for AI Agents
In contracts/evmx/interfaces/IConfigurations.sol around lines 48-49, the
external function parameter of type WatcherMultiCallParams should be declared as
calldata to avoid unnecessary memory copies and enforce ABI consistency; update
the interface signature to use `WatcherMultiCallParams calldata params_`, and
ensure all implementing contracts update their overriding function signatures to
accept the struct as calldata as well.
| /// @notice Configuration contract for the Watcher Precompile system | ||
| /// @dev Handles the mapping between networks, plugs, and app gateways for payload execution | ||
| contract Configurations is ConfigurationsStorage, Initializable, Ownable, WatcherBase { | ||
| contract Configurations is ConfigurationsStorage, Ownable, AddressResolverUtil { |
There was a problem hiding this comment.
🧩 Analysis chain
Verify initialization mechanism.
The contract removed Initializable from inheritance but added evmxSlug state variable (line 37). Ensure there's a mechanism to initialize evmxSlug when the contract is deployed, as it's used in signature validation (line 176).
🏁 Script executed:
#!/bin/bash
# Search for initialization of evmxSlug
rg -n --type=sol 'evmxSlug\s*=' -A2 -B2Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for evmxSlug initialization without type restriction
rg -n 'evmxSlug' -A3 -B3 contracts/evmx/watcher/Configurations.solLength of output: 518
🏁 Script executed:
#!/bin/bash
# Look for constructor in Configurations contract
rg -n 'constructor\s*\(' contracts/evmx/watcher/Configurations.solLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for initialize function
rg -n 'function\s+initialize' contracts/evmx/watcher/Configurations.solLength of output: 0
🏁 Script executed:
#!/bin/bash
# Get full context of the contract to understand initialization strategy
head -100 contracts/evmx/watcher/Configurations.solLength of output: 4223
🏁 Script executed:
#!/bin/bash
# Search for any setter or assignment to evmxSlug
rg -n 'evmxSlug\s*=' contracts/evmx/watcher/Configurations.solLength of output: 0
🏁 Script executed:
#!/bin/bash
# Look for all function definitions in the contract
rg -n 'function\s+\w+' contracts/evmx/watcher/Configurations.sol | head -20Length of output: 563
🏁 Script executed:
#!/bin/bash
# Get the rest of the file to see all functions
tail -n +100 contracts/evmx/watcher/Configurations.sol | head -150Length of output: 4073
This should provide a setter function for evmxSlug or initialize it in a constructor.
The contract removed Initializable from inheritance without providing an alternative way to initialize evmxSlug. Since this variable is used in signature validation at line 176 and defaults to 0, signatures will always be computed with the wrong chain slug value. Either add a constructor that accepts evmxSlug as a parameter, or add a setter function (likely onlyOwner).
🤖 Prompt for AI Agents
In contracts/evmx/watcher/Configurations.sol around line 49, the contract
removed Initializable but never sets evmxSlug, so it remains zero and breaks
signature validation at line 176; add an initialization path: either implement a
constructor that accepts a uint256 evmxSlug parameter and sets the storage
variable, or add an onlyOwner setter function setEvmxSlug(uint256) that updates
the variable and emits an event; ensure the chosen method sets evmxSlug before
any signature validation can occur and include access control (onlyOwner) and an
event for state change.
hardhat-scripts/deploy/2.roles.ts
Outdated
| export const REQUIRED_ROLES = { | ||
| EVMx: { | ||
| AuctionManager: [ROLES.TRANSMITTER_ROLE], | ||
| // AuctionManager: [ROLES.TRANSMITTER_ROLE], |
There was a problem hiding this comment.
🧩 Analysis chain
Roles pruned in REQUIRED_ROLES; align EVMx role setup and watcher targeting
This should keep role assignment consistent. You still grant TRANSMITTER_ROLE to AuctionManager in setRolesForEVMx while AuctionManager is commented in REQUIRED_ROLES. Confirm AM is still deployed/used in Phase 1; otherwise remove that grant.
Replace contract allowlist in watcher targeting with mapping‑driven check to avoid stale includes:
- [Contracts.FastSwitchboard, Contracts.CCTPSwitchboard].includes(
- contractName as Contracts
- ) && roleName === ROLES.WATCHER_ROLE
+ roleName === ROLES.WATCHER_ROLE &&
+ (contractName as Contracts) === Contracts.FastSwitchboardAlso applies to: 33-35, 41-41
🏁 Script executed:
#!/bin/bash
# Verify usage and address presence
rg -nP 'Contracts\.AuctionManager|setDefaultAuctionManager' -C2
rg -nP '"AuctionManager"\s*:' addresses 2>/dev/null || trueLength of output: 1776
🏁 Script executed:
#!/bin/bash
# Check REQUIRED_ROLES object and surrounding context
rg -nP 'REQUIRED_ROLES\s*=|AuctionManager' hardhat-scripts/deploy/2.roles.ts -A30 | head -80Length of output: 2137
Remove AuctionManager role grant to match REQUIRED_ROLES cleanup
AuctionManager was pruned from REQUIRED_ROLES.EVMx (line 28, commented out), but the setRoleForContract call granting TRANSMITTER_ROLE to AuctionManager at lines 123–129 remains active. Remove or comment out lines 123–129 to keep role setup consistent with the REQUIRED_ROLES change.
The optional refactor for watcher targeting logic (lines 32–35, 41) still applies—replace the contract allowlist check with direct comparison to avoid stale includes:
- [Contracts.FastSwitchboard, Contracts.CCTPSwitchboard].includes(
- contractName as Contracts
- ) && roleName === ROLES.WATCHER_ROLE
+ roleName === ROLES.WATCHER_ROLE &&
+ (contractName as Contracts) === Contracts.FastSwitchboardCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In hardhat-scripts/deploy/2.roles.ts (around lines 28, 32–35, 41 and 123–129):
the AuctionManager was removed from REQUIRED_ROLES but the code that grants
TRANSMITTER_ROLE to AuctionManager at lines 123–129 is still active; remove or
comment out those lines (123–129) so role setup matches REQUIRED_ROLES, and
optionally update the watcher targeting logic at lines 32–35 and 41 by replacing
the allowlist/includes check with a direct contract equality comparison to avoid
stale includes.
| // await updateContractSettings( | ||
| // EVMX_CHAIN_ID, | ||
| // Contracts.AddressResolver, | ||
| // "defaultAuctionManager", | ||
| // [], | ||
| // evmxAddresses[Contracts.AuctionManager], | ||
| // "setDefaultAuctionManager", | ||
| // [evmxAddresses[Contracts.AuctionManager]], | ||
| // signer | ||
| // ); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Critical config steps disabled; guard with flags and confirm replacement paths
Commenting out defaultAuctionManager, deployForwarder, and setWatcherCoreContracts changes system wiring. This should be gated and documented, or equivalent config must happen elsewhere to avoid runtime gaps.
-// await updateContractSettings( ... setDefaultAuctionManager ... );
+if (process.env.SET_DEFAULT_AUCTION_MANAGER === "1") {
+ await updateContractSettings(/* setDefaultAuctionManager params */);
+}
-// await updateContractSettings( ... setDeployForwarder ... );
+if (process.env.SET_DEPLOY_FORWARDER === "1") {
+ await updateContractSettings(/* setDeployForwarder params */);
+}
-// await setWatcherCoreContracts(evmxAddresses);
+if (process.env.SET_WATCHER_CORE_CONTRACTS === "1") {
+ await setWatcherCoreContracts(evmxAddresses);
+}Explain where these are now configured (file/step), or run targeted checks post‑deploy to ensure state is set.
Also applies to: 80-90, 124-124
🏁 Script executed:
#!/bin/bash
# View the actual file content around the flagged line ranges
echo "=== Lines 50-75 (context around 58-68) ==="
sed -n '50,75p' hardhat-scripts/deploy/4.configureEVMx.ts
echo -e "\n=== Lines 75-100 (context around 80-90) ==="
sed -n '75,100p' hardhat-scripts/deploy/4.configureEVMx.ts
echo -e "\n=== Lines 115-130 (context around 124) ==="
sed -n '115,130p' hardhat-scripts/deploy/4.configureEVMx.tsLength of output: 1766
🏁 Script executed:
#!/bin/bash
# Search for updateContractSettings calls (active ones)
echo "=== Active updateContractSettings calls ==="
rg -n 'await updateContractSettings' hardhat-scripts/deploy/ -A2 -B1
echo -e "\n=== Active setWatcherCoreContracts calls ==="
rg -n 'await setWatcherCoreContracts' hardhat-scripts/deploy/ -A2 -B1Length of output: 4206
🏁 Script executed:
#!/bin/bash
# Search for AuctionManager, DeployForwarder config elsewhere
echo "=== setDefaultAuctionManager references ==="
rg -n 'setDefaultAuctionManager|defaultAuctionManager' --type ts -C2
echo -e "\n=== setDeployForwarder references ==="
rg -n 'setDeployForwarder|DeployForwarder' --type ts -C2
echo -e "\n=== setWatcherCoreContracts references ==="
rg -n 'setWatcherCoreContracts' --type ts -C2Length of output: 2097
Three critical configurations disabled; no equivalent paths found elsewhere—must be restored or explicitly justified
Verification confirms all three disabled steps are missing:
setDefaultAuctionManager: no active calls exist anywheresetDeployForwarder: no active calls exist anywheresetWatcherCoreContracts: function defined (line 168) but never invoked
The interspersed active calls (setFeesManager, setWatcher, setPrecompile) show configuration intentionally happens here. These three commented blocks represent incomplete state setup that will cause runtime gaps.
This should either be restored and executed unconditionally, or if intentionally disabled, that decision must be documented with clear reasoning in a code comment explaining why these steps are skipped.
🤖 Prompt for AI Agents
In hardhat-scripts/deploy/4.configureEVMx.ts around lines 58 to 68 (and note the
setWatcherCoreContracts definition at ~line 168 is never called), the three
commented configuration steps (setDefaultAuctionManager, setDeployForwarder, and
invoking setWatcherCoreContracts) are disabled and missing elsewhere; either
restore and execute these calls unconditionally in the appropriate sequence here
(re-enable the commented updateContractSettings calls for
setDefaultAuctionManager and setDeployForwarder and add a call to
setWatcherCoreContracts after related setup), or if they must remain disabled,
add a clear code comment at the top of this block explaining why each step is
intentionally skipped, referencing the missing alternative config paths and any
risk/mitigation, so reviewers/runtime users understand the rationale.
Feat/assign transmitter
test: msg sb assign transmitter
feat: evmxWatcherSetId rename
appgatewayid in event
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (15)
contracts/protocol/switchboard/MessageSwitchboard.sol (9)
363-419: Fee-increase helpers should guard uninitialized payloads and constrain sponsored maxFees.
increaseFeesForPayloaddispatches based on a leadingfeesTypetag, which is good. However:
_increaseNativeFeesonly checksfees.plug != plug_. For an uninitialized entry,fees.plugisaddress(0), so you at least get a revert ifplug_ != address(0), but there is no explicit “payload not found” signal._increaseSponsoredFeessimilarly allows arbitrarynewMaxFeeswithout constraints relative to the originalmaxFees.This should hard-fail on unknown payloads and optionally enforce monotonic
maxFeesif your economics require it:function _increaseNativeFees( bytes32 payloadId_, address plug_, bytes calldata feesData_ ) internal { PayloadFees storage fees = payloadFees[payloadId_]; - // Validation: Only the plug that created this payload can increase fees - if (fees.plug != plug_) revert UnauthorizedFeeIncrease(); + // Validation: Payload must exist and only the creating plug can increase fees + if (fees.plug == address(0)) revert("Unknown payload"); + if (fees.plug != plug_) revert UnauthorizedFeeIncrease();function _increaseSponsoredFees( bytes32 payloadId_, address plug_, bytes calldata feesData_ ) internal { SponsoredPayloadFees storage fees = sponsoredPayloadFees[payloadId_]; - // Validation: Only the plug that created this payload can increase fees - if (fees.plug != plug_) revert UnauthorizedFeeIncrease(); + // Validation: Payload must exist and only the creating plug can increase fees + if (fees.plug == address(0)) revert("Unknown payload"); + if (fees.plug != plug_) revert UnauthorizedFeeIncrease(); // Decode new maxFees (skip first byte which is feesType) (, uint256 newMaxFees) = abi.decode(feesData_, (uint8, uint256)); - fees.maxFees = newMaxFees; + // Optional: enforce non-decreasing maxFees + if (newMaxFees < fees.maxFees) revert("maxFees cannot decrease"); + fees.maxFees = newMaxFees;
135-154: Attestation threshold must handletotalWatchers == 0defensively.As in
EVMxSwitchboard,attestmarks digests valid whenattestations[payloadId_][digest_] >= totalWatchers. WithtotalWatchers == 0, this condition is immediately true, so the first attestation will always mark payloads valid if a watcher role exists.This should guard against zero-watchers before applying the threshold:
function attest(bytes32 payloadId_, bytes32 digest_, bytes calldata proof_) public { // Recover watcher from signature address watcher = _recoverSigner( keccak256(abi.encodePacked(toBytes32Format(address(this)), chainSlug, payloadId_, digest_)), proof_ ); // Verify watcher has WATCHER_ROLE if (!_hasRole(WATCHER_ROLE, watcher)) revert WatcherNotFound(); // Prevent double attestation if (isAttestedByWatcher[watcher][payloadId_][digest_]) revert AlreadyAttested(); isAttestedByWatcher[watcher][payloadId_][digest_]= true; attestations[payloadId_][digest_]++; - // Mark digest_ as valid if enough attestations are reached - if (attestations[payloadId_][digest_] >= totalWatchers) isValid[payloadId_][digest_] = true; + // Require a non-zero watcher set and then apply threshold + if (totalWatchers == 0) revert WatcherNotFound(); + if (attestations[payloadId_][digest_] >= totalWatchers) { + isValid[payloadId_][digest_] = true; + }
159-169:allowPayloadshould also respectrevertingPayloadIds.
setRevertingPayloadmarksrevertingPayloadIds[payloadId_], butallowPayloadonly checks sibling topology andisValid[payloadId_][digest_]. Payloads known to revert will continue to be allowed.This should treat “reverting” as an override on validity:
function allowPayload( bytes32 digest_, bytes32 payloadId_, address target_, bytes memory sibling_ ) external view override returns (bool) { (uint32 siblingChainSlug, bytes32 siblingPlug) = _decodePackedSource(sibling_); if (siblingPlugs[siblingChainSlug][target_] != siblingPlug) revert InvalidSource(); - // digest has enough attestations - return isValid[payloadId_][digest_]; + if (revertingPayloadIds[payloadId_]) return false; + // digest has enough attestations + return isValid[payloadId_][digest_]; }
180-246: Native refund path should rejectrefundAddress == address(0)to avoid burning ETH.In the native-fees branch of
processPayload,payloadFees[payloadId].refundAddressis set directly fromoverrides.refundAddresswith no check, andrefundlater transfers allfees.nativeFeesto that address. IfrefundAddressisaddress(0), a valid refund will silently burn ETH.This should at least protect the final transfer, and ideally reject bad overrides up front:
if (msg.value < minMsgValueFees[overrides.dstChainSlug] + overrides.value) revert InsufficientMsgValue(); - payloadFees[payloadId] = PayloadFees({ + if (overrides.refundAddress == address(0)) { + revert("Invalid refund address"); + } + + payloadFees[payloadId] = PayloadFees({ nativeFees: msg.value, refundAddress: overrides.refundAddress, isRefundEligible: false, isRefunded: false, plug: plug_ });and additionally in
refund(see comment below) to ensure no transfer to zero ever happens.
473-484:refundshould explicitly reject zero refund address to avoid ETH burn.The refund path now has
nonReentrant, which is good. However, it still transfersfees.nativeFeestofees.refundAddresswithout checking foraddress(0), so a misconfigured payload will burn ETH.This should defensively block transfers to zero, even if earlier validation is added:
function refund(bytes32 payloadId_) external nonReentrant { PayloadFees storage fees = payloadFees[payloadId_]; if (!fees.isRefundEligible) revert RefundNotEligible(); if (fees.isRefunded) revert AlreadyRefunded(); uint256 feesToRefund = fees.nativeFees; fees.isRefunded = true; fees.nativeFees = 0; - emit Refunded(payloadId_, fees.refundAddress, feesToRefund); - SafeTransferLib.safeTransferETH(fees.refundAddress, feesToRefund); + if (fees.refundAddress == address(0)) { + revert("Invalid refund address"); + } + + emit Refunded(payloadId_, fees.refundAddress, feesToRefund); + SafeTransferLib.safeTransferETH(fees.refundAddress, feesToRefund); }
553-564:setSiblingConfigshould validate non-zero socket, switchboard, and switchboardId.
siblingSockets[chainSlug_],siblingSwitchboards[chainSlug_], andsiblingSwitchboardIds[chainSlug_]are set without validation. If any are zero, subsequent calls to_validateSiblingor_createDigestAndPayloadIdwill revert withSiblingSocketNotFound, effectively misconfiguring the chain.This should fail fast when registering sibling config:
function setSiblingConfig( uint32 chainSlug_, bytes32 socket_, bytes32 switchboard_, uint32 switchboardId_ ) external onlyOwner { + if (socket_ == bytes32(0) || switchboard_ == bytes32(0) || switchboardId_ == 0) { + revert SiblingSocketNotFound(); + } siblingSockets[chainSlug_] = socket_; siblingSwitchboards[chainSlug_] = switchboard_; siblingSwitchboardIds[chainSlug_] = switchboardId_; emit SiblingConfigSet(chainSlug_, socket_, switchboard_); }
594-608:updatePlugConfigshould reject zerosiblingPlugto avoid misconfigured siblings.
updatePlugConfigdecodes(uint32 siblingChainSlug, bytes32 siblingPlug)and validates that sibling socket and switchboard are set, but does not ensuresiblingPlug != bytes32(0). A zero sibling plug will later cause_validateSiblingto revert for that plug.This should reject zero plugs upfront:
function updatePlugConfig( address plug_, bytes memory plugConfig_ ) external override onlySocket { (uint32 siblingChainSlug, bytes32 siblingPlug) = abi.decode(plugConfig_, (uint32, bytes32)); if ( siblingSockets[siblingChainSlug] == bytes32(0) || siblingSwitchboards[siblingChainSlug] == bytes32(0) ) { revert SiblingSocketNotFound(); } - siblingPlugs[siblingChainSlug][plug_] = siblingPlug; + if (siblingPlug == bytes32(0)) { + revert SiblingSocketNotFound(); + } + siblingPlugs[siblingChainSlug][plug_] = siblingPlug; emit PlugConfigUpdated(plug_, siblingChainSlug, siblingPlug); }
623-629:getPlugConfigencoding does not matchupdatePlugConfig’s(uint32, bytes32)decode.
updatePlugConfigdecodesplugConfig_as(uint32 siblingChainSlug, bytes32 siblingPlug), butgetPlugConfigreturns onlyabi.encode(siblingPlugs[chainSlug_][plug_]). Any caller using the natural patternupdatePlugConfig(plug, getPlugConfig(plug, extraData_))will revert on decode.This should return the same tuple shape:
function getPlugConfig( address plug_, bytes memory extraData_ ) external view override returns (bytes memory plugConfig_) { uint32 chainSlug_ = abi.decode(extraData_, (uint32)); - plugConfig_ = abi.encode(siblingPlugs[chainSlug_][plug_]); + plugConfig_ = abi.encode(chainSlug_, siblingPlugs[chainSlug_][plug_]); }
680-705:assignTransmittersignature still does not bindnonce_to the signed message.Here, as previously flagged, the signed message is:
keccak256(abi.encodePacked(toBytes32Format(address(this)), chainSlug, oldDigest, newDigest))while
nonce_is only passed to_validateAndUseNonce. A caller who has a single watcher signature for the digest pair can choose arbitrary unused nonces; nonce consumption on-chain does not reflect what the watcher actually signed.payloadIdToDigestprevents repeated state changes with the same(oldDigest, newDigest), but the usual “(message, nonce)” binding is missing.This should include
nonce_in the signed payload:digestParams_.transmitter = toBytes32Format(newTransmitter_); bytes32 newDigest = createDigest(digestParams_); address watcher = _recoverSigner( - keccak256(abi.encodePacked(toBytes32Format(address(this)), chainSlug, oldDigest, newDigest)), + keccak256( + abi.encodePacked( + toBytes32Format(address(this)), + chainSlug, + oldDigest, + newDigest, + nonce_ + ) + ), signature_ );contracts/protocol/switchboard/EVMxSwitchboard.sol (6)
70-81: Constructor should defensively validate EVMX config and emitEvmxConfigSet.Current constructor accepts
transmitter_,evmxChainSlug_, andevmxWatcherSetId_without validation. A misconfigured deploy (zero chain slug / watcher set / transmitter) will silently succeed and later produce unusable payload IDs and digests.This should fail fast and emit the config event when set:
constructor( uint32 chainSlug_, ISocket socket_, address owner_, address transmitter_, uint32 evmxChainSlug_, uint32 evmxWatcherSetId_ ) SwitchboardBase(chainSlug_, socket_, owner_) { - transmitter = transmitter_; - evmxChainSlug = evmxChainSlug_; - evmxWatcherSetId = evmxWatcherSetId_; + if (evmxChainSlug_ == 0 || evmxWatcherSetId_ == 0 || transmitter_ == address(0)) { + revert("EVMX config not set"); + } + transmitter = transmitter_; + evmxChainSlug = evmxChainSlug_; + evmxWatcherSetId = evmxWatcherSetId_; + emit EvmxConfigSet(evmxChainSlug_, evmxWatcherSetId_); }
92-110: Attestation threshold must not treattotalWatchers == 0as “quorum reached”.
attestmarks a digest valid whenattestations[payloadId_][digest_] >= totalWatchers. WithtotalWatchers == 0this condition is true on the first attestation, so any single watcher (or any account with WATCHER_ROLE if roles are pre-seeded elsewhere) will immediately mark payloads valid.This should guard against zero watchers before using the threshold:
function attest(bytes32 payloadId_, bytes32 digest_, bytes calldata proof_) public virtual { address watcher = _recoverSigner( keccak256( abi.encodePacked(toBytes32Format(address(this)), chainSlug, payloadId_, digest_) ), proof_ ); if (!_hasRole(WATCHER_ROLE, watcher)) revert WatcherNotFound(); // Prevent double attestation if (isAttestedByWatcher[watcher][payloadId_][digest_]) revert AlreadyAttested(); isAttestedByWatcher[watcher][payloadId_][digest_] = true; attestations[payloadId_][digest_]++; - // Mark digest as valid if enough attestations are reached - if (attestations[payloadId_][digest_] >= totalWatchers) isValid[payloadId_][digest_] = true; + // Require a non-zero watcher set and then apply threshold + if (totalWatchers == 0) revert WatcherNotFound(); + if (attestations[payloadId_][digest_] >= totalWatchers) { + isValid[payloadId_][digest_] = true; + } emit Attested(payloadId_, digest_, watcher); }
120-129:allowPayloadshould consultrevertingPayloadIdsbefore returning validity.
setRevertingPayloadupdatesrevertingPayloadIds[payloadId_], butallowPayloadignores it and only checksisValid[payloadId_][digest_]. Payloads explicitly marked as reverting will still be allowed.This should block payloads flagged as reverting:
function allowPayload( bytes32 digest_, bytes32 payloadId_, address target_, bytes memory source_ ) external view returns (bool) { bytes32 appGatewayId = abi.decode(source_, (bytes32)); if (plugAppGatewayIds[target_] != appGatewayId) revert InvalidSource(); - return isValid[payloadId_][digest_]; + if (revertingPayloadIds[payloadId_]) return false; + return isValid[payloadId_][digest_]; }
139-177:processPayloadshould reject unconfigured plugs before creating digest/payloadId.
processPayloadreadsbytes32 appGatewayId = plugAppGatewayIds[plug_]and uses it astargetinDigestParamswithout validating it. If a plug never calledupdatePlugConfig,appGatewayIdis zero and you will create digests for an unconfigured/unknown gateway.This should validate the plug is configured:
payloadId = createPayloadId( chainSlug, switchboardId, evmxChainSlug, evmxWatcherSetId, payloadCounter++ ); - bytes32 appGatewayId = plugAppGatewayIds[plug_]; + bytes32 appGatewayId = plugAppGatewayIds[plug_]; + if (appGatewayId == bytes32(0)) { + // Either introduce a dedicated error or reuse an existing one. + revert("Plug not configured"); + } DigestParams memory digestParams = DigestParams({ socket: toBytes32Format(address(this)), transmitter: toBytes32Format(transmitter), payloadId: payloadId, deadline: overridesParams.deadline, callType: WRITE, gasLimit: overridesParams.gasLimit, value: msg.value, payload: payload_, target: appGatewayId, source: abi.encodePacked(chainSlug, toBytes32Format(plug_)), prevBatchDigestHash: bytes32(0), extraData: bytes("") });
187-194:increaseFeesForPayloadremains an event-only no-op; callers must not assume on-chain fee changes.The function validates
payloadIdToPlug[payloadId_] == plug_and emitsFeesIncreasedbut does not mutate any fee-related state. The NatSpec notes this, but the name suggests an economic effect.This should either:
- Be wired into actual fee accounting when EVMx supports it, or
- Be clearly documented at the interface level so off-chain components and integrators do not assume on-chain side effects beyond the event.
No code change strictly required now, but this behavior is easy to misinterpret.
209-238: NatSpec forsetRevertingPayloaddoes not match watcher-signed implementation.Comment says “Only callable by owner”, but the function is watcher-signed and gated by
WATCHER_ROLEvia_recoverSigner, notonlyOwner. This is misleading for integrators and auditors.This should update the documentation to reflect the watcher-signed pattern:
/** * @notice Sets reverting status for a payload * @param payloadId_ The payload ID to mark * @param isReverting_ True if payload should be marked as reverting - * @dev Only callable by owner. Used to mark payloads that are known to revert. + * @dev Callable by WATCHER_ROLE via off-chain signature. Used to mark payloads that are known to revert. */
🧹 Nitpick comments (3)
contracts/protocol/switchboard/EVMxSwitchboard.sol (1)
306-344: Watcher count invariants rely on avoiding directgrantRole/revokeRole, butrenounceRolecan still desynctotalWatchers.
grantWatcherRole/revokeWatcherRolemaintaintotalWatchers, and overridinggrantRole/revokeRoleprevents the owner from bypassing that. However,renounceRolefromAccessControlremains available to watchers and is not overridden here, so a watcher can renounceWATCHER_ROLEwithout adjustingtotalWatchers.If you rely on
totalWatchersto reflect the live watcher set (for attestation thresholds), this should either:
- Override
renounceRoleto keeptotalWatchersin sync when a watcher self-renounces, or- Explicitly document that
totalWatchersis an upper bound and operational procedures must avoidrenounceRolefor watchers.No diff provided since the exact policy (disallow vs. adjust on renounce) is design-dependent.
contracts/protocol/switchboard/MessageSwitchboard.sol (2)
490-506: Batch approve/revoke of plugs is straightforward; consider gas limits on very large arrays.
approvePlugs/revokePlugssimply iterate overplugs_and flipsponsorApprovals. This is fine, but extremely long arrays will hit gas limits.If you expect untrusted callers to pass arbitrarily long lists, this should document or enforce a reasonable maximum length to prevent accidental out-of-gas in governance tooling. No code change strictly required.
635-651: Watcher role helpers maintaintotalWatchersbut do not handlerenounceRole.
grantWatcherRole/revokeWatcherRolecorrectly keeptotalWatchersin sync and the overriddengrantRole/revokeRoleprevent bypass. However, as inEVMxSwitchboard,renounceRoleis not overridden, so self-renouncing watchers can desynctotalWatchers.If
totalWatchersis used as a strict threshold, this should either overriderenounceRoleto adjust the counter when a watcher renounces, or explicitly document that watcher renounce is operationally disallowed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/protocol/switchboard/EVMxSwitchboard.sol(1 hunks)contracts/protocol/switchboard/MessageSwitchboard.sol(2 hunks)
🔇 Additional comments (10)
contracts/protocol/switchboard/MessageSwitchboard.sol (10)
17-31: Watcher attestation state mirrors EVMxSwitchboard; behavior is consistent.Attestation-related mappings (
isAttestedByWatcher,attestations,isValid,totalWatchers) are structured consistently withEVMxSwitchboard, which keeps the attestation model uniform across switchboards. No changes needed here.
255-311: Override decoding logic is consistent and now correctly feedsdeadline.Versioned decoding for native vs sponsored flows looks correct, and
deadlineis now fed back intoDigestParamsvia_createDigestAndPayloadId. This is aligned with prior feedback about actually using decoded deadlines.
323-355: Digest/payloadId construction is coherent with sibling topology.
_createDigestAndPayloadId:
- Validates
dstSwitchboardId != 0.- Uses
(chainSlug, switchboardId)as sibling tuple and(dstChainSlug, dstSwitchboardId)as verification tuple increatePayloadId.- Builds
DigestParamswith sibling socket,transmitter, encoded(chainSlug, plug_)source, and sibling plug target.This wiring is internally consistent. No change needed.
421-442: Packed source decoding logic is correct and matchesabi.encodePacked(chainSlug, bytes32Plug).The assembly in
_decodePackedSourcecorrectly:
- Requires total length 36 bytes.
- Reads the first 32-byte word and extracts the low 4 bytes as
uint32 chainSlug.- Reads the next 32 bytes starting at offset 36 to reconstruct the
bytes32plug.This matches the intended
abi.encodePacked(uint32, bytes32)layout. No change needed.
450-467:markRefundEligiblecorrectly protects against replay and zero-fee cases.
markRefundEligible:
- Checks
fees.isRefundEligibleandfees.nativeFees == 0.- Builds a watcher-signed digest that includes
payloadId_andnonce_.- Validates WATCHER_ROLE and consumes the nonce via
_validateAndUseNonce.This is consistent with other watcher-signed flows and protects refund eligibility from replay. No changes required here.
516-545:setMinMsgValueFeesBatchsigning scheme is sound and non-malleable.The digest includes:
- Contract and
chainSlug.- Length prefixes and full contents of
siblingChainSlugs_andminFees_.nonce_.This avoids array-concatenation collisions and binds the nonce. Role gating via
FEE_UPDATER_ROLEand_validateAndUseNoncelooks correct.
566-588:setRevertingPayloadcorrectly binds nonce in signature and uses common nonce helper.This watcher-signed flow:
- Includes
payloadId_,isReverting_, andnonce_in the message.- Validates WATCHER_ROLE.
- Uses
_validateAndUseNoncewith the function selector namespace.This is aligned with the expected replay protection pattern. No change needed.
615-618: Default deadline setter is straightforward and consistent.
setDefaultDeadlineIntervalsimply updatesdefaultDeadlineIntervaland emits an event underonlyOwner. This matches the override logic inprocessPayloadand is fine as-is.
657-669: OverriddengrantRole/revokeRolecorrectly block direct WATCHER_ROLE changes.These overrides ensure the owner cannot call
grantRole/revokeRoledirectly forWATCHER_ROLE, forcing all watcher membership changes throughgrantWatcherRole/revokeWatcherRolesototalWatchersstays coherent. This is the right pattern.
712-715:setTransmitterowner-only governance hook looks correct.The owner-only setter for
transmitterupdates the global transmitter used in digest construction and emitsTransmitterSet. This is straightforward and matches expectations.
added this function to mark old digests valid in case watchers were decreased after attestations
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
contracts/protocol/switchboard/MessageSwitchboard.sol (6)
555-566: Validate sibling configuration parameters.
setSiblingConfigacceptssocket_,switchboard_, andswitchboardId_without validation. If any are zero, subsequent calls toprocessPayloadwill revert withSiblingSocketNotFoundat line 321 or 331. This was flagged in a previous review and remains unaddressed.🔎 Add validation
function setSiblingConfig( uint32 chainSlug_, bytes32 socket_, bytes32 switchboard_, uint32 switchboardId_ ) external onlyOwner { + if (socket_ == bytes32(0) || switchboard_ == bytes32(0) || switchboardId_ == 0) { + revert SiblingSocketNotFound(); + } siblingSockets[chainSlug_] = socket_; siblingSwitchboards[chainSlug_] = switchboard_; siblingSwitchboardIds[chainSlug_] = switchboardId_; emit SiblingConfigSet(chainSlug_, socket_, switchboard_); }
596-610: Validate siblingPlug is non-zero.
updatePlugConfigdecodes and storessiblingPlugwithout validating it's notbytes32(0). A zero value would cause_validateSibling(line 318) to revert on subsequent payload processing. This was flagged in a previous review and remains unaddressed.🔎 Add validation
function updatePlugConfig( address plug_, bytes memory plugConfig_ ) external override onlySocket { (uint32 siblingChainSlug, bytes32 siblingPlug) = abi.decode(plugConfig_, (uint32, bytes32)); if ( siblingSockets[siblingChainSlug] == bytes32(0) || siblingSwitchboards[siblingChainSlug] == bytes32(0) ) { revert SiblingSocketNotFound(); } + if (siblingPlug == bytes32(0)) { + revert SiblingSocketNotFound(); + } siblingPlugs[siblingChainSlug][plug_] = siblingPlug; emit PlugConfigUpdated(plug_, siblingChainSlug, siblingPlug); }
625-631: Fix encoding mismatch: getPlugConfig must return tuple matching updatePlugConfig.
updatePlugConfig(line 600) decodesplugConfig_as(uint32, bytes32), butgetPlugConfig(line 630) returns onlyabi.encode(siblingPlugs[chainSlug_][plug_])— a singlebytes32. This breaks the round-trip pattern: callingupdatePlugConfig(plug, getPlugConfig(plug, extraData_))will revert because the decoder expects two words but receives one. This was confirmed in a previous review and remains unaddressed.🔎 Fix the encoding
function getPlugConfig( address plug_, bytes memory extraData_ ) external view override returns (bytes memory plugConfig_) { uint32 chainSlug_ = abi.decode(extraData_, (uint32)); - plugConfig_ = abi.encode(siblingPlugs[chainSlug_][plug_]); + plugConfig_ = abi.encode(chainSlug_, siblingPlugs[chainSlug_][plug_]); }
682-709: Include nonce in the signed message to prevent griefing.The signed message (lines 699-701) is
keccak256(abi.encodePacked(address(this), chainSlug, oldDigest, newDigest)), butnonce_is consumed by_validateAndUseNonceat line 705 without being part of the signature. This means:
- The watcher's signature doesn't commit to which nonce is consumed
- An attacker could call
assignTransmitterwith the samesignature_but differentnonce_values to grief the watcher by burning their nonces before they're used for legitimate operationsThis is inconsistent with other watcher-signed functions (e.g.,
markRefundEligibleline 461 includes the nonce). This was flagged in a previous review and remains unaddressed.🔎 Include nonce in signature
function assignTransmitter( DigestParams memory digestParams_, address oldTransmitter_, address newTransmitter_, uint256 nonce_, bytes calldata signature_ ) external { bytes32 oldTransmitterBytes32 = toBytes32Format(oldTransmitter_); digestParams_.transmitter = oldTransmitterBytes32; bytes32 oldDigest = createDigest(digestParams_); if (payloadIdToDigest[digestParams_.payloadId] != oldDigest) revert InvalidDigest(); digestParams_.transmitter = toBytes32Format(newTransmitter_); bytes32 newDigest = createDigest(digestParams_); address watcher = _recoverSigner( keccak256( - abi.encodePacked(toBytes32Format(address(this)), chainSlug, oldDigest, newDigest) + abi.encodePacked(toBytes32Format(address(this)), chainSlug, oldDigest, newDigest, nonce_) ), signature_ ); if (!_hasRole(WATCHER_ROLE, watcher)) revert WatcherNotFound(); _validateAndUseNonce(this.assignTransmitter.selector, watcher, nonce_); payloadIdToDigest[digestParams_.payloadId] = newDigest; emit TransmitterAssigned(digestParams_.payloadId, newTransmitter_); }
475-486: Add defensive check for zero refund address.While the root cause should be fixed in
processPayload,refund()should also defensively validatefees.refundAddress != address(0)before transferring (line 485) to prevent accidental ETH burn if the storage was corrupted or initialized incorrectly.🔎 Defensive check
function refund(bytes32 payloadId_) external nonReentrant { PayloadFees storage fees = payloadFees[payloadId_]; if (!fees.isRefundEligible) revert RefundNotEligible(); if (fees.isRefunded) revert AlreadyRefunded(); + if (fees.refundAddress == address(0)) revert InvalidRefundAddress(); uint256 feesToRefund = fees.nativeFees; fees.isRefunded = true; fees.nativeFees = 0; emit Refunded(payloadId_, fees.refundAddress, feesToRefund); SafeTransferLib.safeTransferETH(fees.refundAddress, feesToRefund); }
222-234: Prevent ETH burn by validating refundAddress is non-zero.The native flow stores
overrides.refundAddressat line 229 without validating it's notaddress(0). If a plug accidentally passesaddress(0)as refundAddress, therefund()function will successfully transfer ETH to the zero address (line 485), permanently burning the funds. This was flagged in a previous review and remains unaddressed.🔎 Proposed fix
} else { // Native token flow - validate fees and track for potential refund if (msg.value < minMsgValueFees[overrides.dstChainSlug] + overrides.value) revert InsufficientMsgValue(); + if (overrides.refundAddress == address(0)) revert InvalidRefundAddress(); payloadFees[payloadId] = PayloadFees({ nativeFees: msg.value, refundAddress: overrides.refundAddress, isRefundEligible: false, isRefunded: false, plug: plug_ });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
contracts/protocol/switchboard/EVMxSwitchboard.solcontracts/protocol/switchboard/MessageSwitchboard.soltest/SetupTest.t.soltest/protocol/switchboard/EVMxSwitchboard.t.sol
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/protocol/switchboard/EVMxSwitchboard.sol
🔇 Additional comments (3)
contracts/protocol/switchboard/MessageSwitchboard.sol (3)
365-421: Fee increase validation is correct but error could be clearer.Both
_increaseNativeFeesand_increaseSponsoredFeescorrectly validate that only the original plug can increase fees (lines 393, 414). For uninitialized payloads wherefees.plug == address(0), the check will revert withUnauthorizedFeeIncrease, which is cryptic but functionally safe. The sponsored path allows arbitrarynewMaxFeesupdates, which appears intentional for flexibility.
637-671: Well-designed watcher role management.The dedicated
grantWatcherRole/revokeWatcherRolefunctions (lines 637-653) correctly maintain thetotalWatcherscounter, and thegrantRole/revokeRoleoverrides (lines 659-671) prevent direct manipulation ofWATCHER_ROLE, ensuring the counter stays synchronized. This is a solid pattern for role management with derived state.
15-55: DeclarepayloadIdToDigestmapping or verify inheritance from parent contract.The mapping
payloadIdToDigestis used at lines 202, 693, and 707 but is not visible in the state variables section (lines 15-55). This must either be declared inMessageSwitchboard.solor inherited fromSwitchboardBase. Missing declaration will cause compilation failure.
| function attest(bytes32 payloadId_, bytes32 digest_, bytes calldata proof_) public { | ||
| // Recover watcher from signature | ||
| address watcher = _recoverSigner( | ||
| keccak256( | ||
| abi.encodePacked(toBytes32Format(address(this)), chainSlug, payloadId_, digest_) | ||
| ), | ||
| proof_ | ||
| ); | ||
|
|
||
| // Verify watcher has WATCHER_ROLE | ||
| if (!_hasRole(WATCHER_ROLE, watcher)) revert WatcherNotFound(); | ||
|
|
||
| // Prevent double attestation | ||
| if (isAttestedByWatcher[watcher][payloadId_][digest_]) revert AlreadyAttested(); | ||
| isAttestedByWatcher[watcher][payloadId_][digest_] = true; | ||
| attestations[payloadId_][digest_]++; | ||
|
|
||
| // Mark digest_ as valid if enough attestations are reached | ||
| if (attestations[payloadId_][digest_] >= totalWatchers) isValid[payloadId_][digest_] = true; | ||
|
|
||
| emit SiblingConfigSet(chainSlug_, fee_, socket_, switchboard_); | ||
| emit Attested(payloadId_, digest_, watcher); | ||
| } |
There was a problem hiding this comment.
Guard against zero watchers at initialization.
If totalWatchers is 0, line 153's condition attestations[payloadId_][digest_] >= totalWatchers will never mark any digest as valid, effectively bricking payload validation. Add a check in the constructor or require at least one watcher before allowing payload processing.
🔎 Suggested defensive check
Add validation in functions that depend on watchers:
function allowPayload(
bytes32 digest_,
bytes32 payloadId_,
address target_,
bytes memory sibling_
) external view override returns (bool) {
+ if (totalWatchers == 0) return false;
(uint32 siblingChainSlug, bytes32 siblingPlug) = _decodePackedSource(sibling_);
if (siblingPlugs[siblingChainSlug][target_] != siblingPlug) revert InvalidSource();
return isValid[payloadId_][digest_];
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In contracts/protocol/switchboard/MessageSwitchboard.sol around lines 135 to
156, the contract allows totalWatchers to be zero which prevents any digest from
ever becoming valid (attestations >= totalWatchers will always be false). Fix by
enforcing totalWatchers > 0 during contract initialization (constructor) or when
setting/updating the watcher count, and add a defensive require in attest (e.g.
require(totalWatchers > 0, "No watchers configured")) so functions that depend
on watchers cannot proceed when the watcher set is empty; ensure any setter that
can change totalWatchers also validates the new value and update/tests
accordingly.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (11)
contracts/protocol/switchboard/EVMxSwitchboard.sol (6)
42-42: NatSpec comment format still incorrect.Line 42 uses
//instead of///. This was flagged in a previous review and marked as addressed, but the issue persists.-// @notice Mapping of payload ID to plug address +/// @notice Mapping of payload ID to plug address
73-86: Constructor should validate EVMX configuration parameters.The constructor stores
evmxChainSlug_andevmxWatcherSetId_without validation. If either is zero,createPayloadIdwill produce malformed IDs and the contract may not function correctly. Add defensive checks:constructor( uint32 chainSlug_, ISocket socket_, address owner_, address transmitter_, address evmxSocket_, uint32 evmxChainSlug_, uint32 evmxWatcherSetId_ ) SwitchboardBase(chainSlug_, socket_, owner_) { + if (evmxChainSlug_ == 0 || evmxWatcherSetId_ == 0) revert EvmxConfigNotSet(); transmitter = transmitter_; evmxChainSlug = evmxChainSlug_; evmxWatcherSetId = evmxWatcherSetId_; evmxSocket = evmxSocket_; }
97-115: Zero watchers allows immediate payload validation.Line 112 marks a digest valid when
attestations >= totalWatchers. SincetotalWatchersstarts at 0, a single attestation will mark any payload valid before watchers are properly configured. This is a critical security issue.Add a guard in the attest function:
function attest(bytes32 payloadId_, bytes32 digest_, bytes calldata proof_) public virtual { + if (totalWatchers == 0) revert WatcherNotFound(); address watcher = _recoverSigner(
125-134: MissingrevertingPayloadIdscheck in allowPayload.The
revertingPayloadIdsmapping is set viasetRevertingPayload()but never consulted here. Payloads marked as reverting should be rejected.function allowPayload( bytes32 digest_, bytes32 payloadId_, address target_, bytes memory source_ ) external view returns (bool) { bytes32 appGatewayId = abi.decode(source_, (bytes32)); if (plugAppGatewayIds[target_] != appGatewayId) revert InvalidSource(); + if (revertingPayloadIds[payloadId_]) return false; return isValid[payloadId_][digest_]; }
144-182: Validate plug configuration before creating payload.Line 162 uses
plugAppGatewayIds[plug_]as the target in digest creation. If the plug hasn't calledupdatePlugConfig()yet, this will bebytes32(0), creating an invalid digest. Add validation:function processPayload( address plug_, bytes calldata payload_, bytes memory overrides_ ) external payable override onlySocket returns (bytes32 payloadId) { + if (plugAppGatewayIds[plug_] == bytes32(0)) revert PlugNotConnected(); EVMxOverrides memory overridesParams = abi.decode(overrides_, (EVMxOverrides));
214-243: NatSpec still claims owner-only but implementation uses watcher signature.Line 218 states "Only callable by owner" but the implementation uses
WATCHER_ROLEsignature verification. Update the documentation:- * @dev Only callable by owner. Used to mark payloads that are known to revert. + * @dev Requires watcher signature. Used to mark payloads that are known to revert.contracts/protocol/switchboard/MessageSwitchboard.sol (5)
135-156: Zero watchers allows immediate payload validation.Same issue as EVMxSwitchboard: line 153 marks a digest valid when
attestations >= totalWatchers. WithtotalWatchers == 0, a single attestation marks any payload valid. Add a guard:function attest(bytes32 payloadId_, bytes32 digest_, bytes calldata proof_) public { + if (totalWatchers == 0) revert WatcherNotFound(); // Recover watcher from signature address watcher = _recoverSigner(
222-234: Refunds toaddress(0)will burn ETH.The native flow stores
overrides.refundAddresswithout validating it's non-zero. If the refund address isaddress(0), therefund()function will successfully transfer ETH to the zero address, permanently burning funds.} else { // Native token flow - validate fees and track for potential refund if (msg.value < minMsgValueFees[overrides.dstChainSlug] + overrides.value) revert InsufficientMsgValue(); + if (overrides.refundAddress == address(0)) revert InvalidRefundAddress(); payloadFees[payloadId] = PayloadFees({
555-566: Add validation for sibling configuration parameters.Zero values for
socket_,switchboard_, orswitchboardId_will causeSiblingSocketNotFounderrors in_validateSiblingand_createDigestAndPayloadId. Validate inputs:function setSiblingConfig( uint32 chainSlug_, bytes32 socket_, bytes32 switchboard_, uint32 switchboardId_ ) external onlyOwner { + if (socket_ == bytes32(0) || switchboard_ == bytes32(0) || switchboardId_ == 0) { + revert SiblingSocketNotFound(); + } siblingSockets[chainSlug_] = socket_;
596-610: ValidatesiblingPlug_is non-zero.A zero
siblingPlug_would cause_validateSiblingto revert on subsequent calls. Add validation:if ( siblingSockets[siblingChainSlug] == bytes32(0) || siblingSwitchboards[siblingChainSlug] == bytes32(0) ) { revert SiblingSocketNotFound(); } + if (siblingPlug == bytes32(0)) { + revert SiblingSocketNotFound(); + } siblingPlugs[siblingChainSlug][plug_] = siblingPlug;
625-631: Encoding mismatch withupdatePlugConfig.
updatePlugConfigdecodes(uint32, bytes32)butgetPlugConfigreturns onlybytes32. A round-trip callupdatePlugConfig(plug, getPlugConfig(plug, data))will revert on decode.function getPlugConfig( address plug_, bytes memory extraData_ ) external view override returns (bytes memory plugConfig_) { uint32 chainSlug_ = abi.decode(extraData_, (uint32)); - plugConfig_ = abi.encode(siblingPlugs[chainSlug_][plug_]); + plugConfig_ = abi.encode(chainSlug_, siblingPlugs[chainSlug_][plug_]); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
contracts/protocol/switchboard/EVMxSwitchboard.solcontracts/protocol/switchboard/MessageSwitchboard.soltest/protocol/switchboard/EVMxSwitchboard.t.soltest/protocol/switchboard/MessageSwitchboard.t.sol
🔇 Additional comments (12)
contracts/protocol/switchboard/EVMxSwitchboard.sol (3)
192-199: Acceptable: Fee increase is event-only as documented.The implementation matches the comment stating that fee increases are currently off-chain verified. The plug-payload binding validation is correct.
251-284: Nonce binding in signature is now correct.The signed message now includes
nonce_(line 274), properly binding the watcher's signature to the specific nonce being consumed. This addresses the previous review concern.
327-361: Watcher role management is correctly implemented.The
grantWatcherRoleandrevokeWatcherRolefunctions properly tracktotalWatchers, and the overriddengrantRole/revokeRolefunctions prevent bypassing this tracking. This is a clean implementation.contracts/protocol/switchboard/MessageSwitchboard.sol (9)
1-55: State variables and imports are well-structured.The ReentrancyGuard addition addresses past reentrancy concerns. State variables are properly documented with NatSpec comments.
257-313: Versioned override decoding is well-structured.Clear separation between native (v1) and sponsored (v2) flows with explicit struct initialization. The revert on unsupported versions is appropriate.
385-421: Fee increase validation is correct.The plug validation
fees.plug != plug_correctly rejects unauthorized fee increases and handles uninitialized payloads (wherefees.plugwould beaddress(0)).
430-444: Assembly decoding is correct.The packed source decoding properly extracts
uint32from the first 4 bytes andbytes32from the remaining 32 bytes. The length validation ensures correct buffer size.
475-486: Reentrancy protection correctly added.The
nonReentrantmodifier addresses the past review concern. State updates (lines 481-482) precede the external call (line 485), following checks-effects-interactions.
637-671: Watcher role management is correctly implemented.The
totalWatcherstracking and override protection for directWATCHER_ROLEmanipulation is consistent with EVMxSwitchboard.
682-715: Nonce binding in signature is now correct.The signed message now includes
nonce_(line 705), properly binding the watcher's signature to the specific nonce being consumed. This addresses the previous review concern.
722-725: Transmitter setter is correct.Simple owner-only setter with proper event emission.
492-508: Batch plug approval/revocation is straightforward.Simple iteration with event emission per plug. Callers are responsible for managing array sizes to avoid gas limits.
| function markIsValid(bytes32 payloadId_, bytes32 digest_) external { | ||
| if (attestations[payloadId_][digest_] >= totalWatchers) isValid[payloadId_][digest_] = true; | ||
| } |
There was a problem hiding this comment.
markIsValid lacks access control and can retroactively validate under-attested payloads.
This public function allows anyone to mark digests as valid after watcher removal. Scenario: 5 watchers, payload gets 4 attestations (insufficient). One watcher is revoked → totalWatchers = 4. Anyone calls markIsValid → payload becomes valid.
If this is intentional, document it. If not, add access control or remove the function since attest() already handles the validity check.
-function markIsValid(bytes32 payloadId_, bytes32 digest_) external {
+function markIsValid(bytes32 payloadId_, bytes32 digest_) external onlyOwner {
if (attestations[payloadId_][digest_] >= totalWatchers) isValid[payloadId_][digest_] = true;
}🤖 Prompt for AI Agents
In contracts/protocol/switchboard/EVMxSwitchboard.sol around lines 286-288,
markIsValid lacks access control and lets anyone retroactively mark
under-attested payloads valid after watcher count changes; fix by either
removing this function (since attest() already sets validity) or restricting it
(e.g., add onlyOwner or onlyWatcher modifier) and enforce the same validity
check as attest(): require(attestations[payloadId_][digest_] >= totalWatchers)
before setting isValid, and if you must support changing watcher counts ensure
validity uses a snapshot or immutable threshold taken at attestation time to
prevent retroactive validation.
| function allowPayload( | ||
| bytes32 digest_, | ||
| bytes32 payloadId_, | ||
| address target_, | ||
| bytes memory sibling_ | ||
| ) external view override returns (bool) { | ||
| (uint32 siblingChainSlug, bytes32 siblingPlug) = _decodePackedSource(sibling_); | ||
| if (siblingPlugs[siblingChainSlug][target_] != siblingPlug) revert InvalidSource(); | ||
| // digest has enough attestations | ||
| return isValid[payloadId_][digest_]; | ||
| } |
There was a problem hiding this comment.
Missing revertingPayloadIds check in allowPayload.
The function should reject payloads marked as reverting via setRevertingPayload. Add the check:
function allowPayload(
bytes32 digest_,
bytes32 payloadId_,
address target_,
bytes memory sibling_
) external view override returns (bool) {
+ if (revertingPayloadIds[payloadId_]) return false;
(uint32 siblingChainSlug, bytes32 siblingPlug) = _decodePackedSource(sibling_);
if (siblingPlugs[siblingChainSlug][target_] != siblingPlug) revert InvalidSource();
return isValid[payloadId_][digest_];
}🤖 Prompt for AI Agents
In contracts/protocol/switchboard/MessageSwitchboard.sol around lines 161 to
171, the allowPayload function is missing a check to reject payloads previously
marked as reverting via setRevertingPayload; add a check before returning that
reverts when revertingPayloadIds[payloadId_] is true (use the contract's
existing revert/error type, e.g., revert RevertingPayload()) so the function
returns false only for non-reverting payloads and otherwise reverts
appropriately.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (15)
contracts/protocol/SocketBatcher.sol (3)
2-2: Consider updating to Solidity 0.8.30.This was previously flagged: Solidity 0.8.28 is valid but outdated. The latest stable 0.8.x release is 0.8.30 (May 2025). Update unless there's a specific reason to pin 0.8.28.
68-105: Past review issue not addressed: Remove commented code.The previous review flagged that this entire block of commented-out CCTP code should be deleted. Commented code reduces readability and serves no purpose - Git history preserves the old implementation if needed. This issue remains unaddressed.
🔎 Remove the entire commented block
- // /** - // * @notice Attests a CCTP payload and proves and executes it - // * @param execParams_ The execution parameters - // * @param cctpParams_ The CCTP parameters - // * @param switchboardId_ The switchboard id - // * @return success True if the payload was executed successfully - // * @return returnData The return data from the execution - // */ - // function attestCCTPAndProveAndExecute( - // CCTPExecutionParams calldata execParams_, - // CCTPBatchParams calldata cctpParams_, - // uint32 switchboardId_ - // ) external payable returns (bool, bytes memory) { - // address switchboard = socket__.switchboardAddresses(switchboardId_); - // bytes32 payloadId = createPayloadId( - // execParams_.executionParams.payloadPointer, - // switchboardId_, - // socket__.chainSlug() - // ); - // ICCTPSwitchboard(switchboard).attestVerifyAndProveExecutions( - // execParams_, - // cctpParams_, - // payloadId - // ); - // (bool success, bytes memory returnData) = socket__.execute{value: msg.value}( - // execParams_.executionParams, - // TransmissionParams({ - // transmitterProof: execParams_.transmitterSignature, - // socketFees: 0, - // extraData: execParams_.executionParams.extraData, - // refundAddress: execParams_.refundAddress - // }) - // ); - - // ICCTPSwitchboard(switchboard).syncOut(payloadId, cctpParams_.nextBatchRemoteChainSlugs); - // return (success, returnData); - // } - // --- Rescue Functions ---
51-66: Past review issue not addressed: Add switchboardId_ validation.The previous review flagged that
socket__.switchboardAddresses(switchboardId_)at line 59 is called without validating the returned address. If the switchboard doesn't exist (returns address(0)), the call will fail with a generic error instead of an explicit validation message. This issue remains unaddressed.🔎 Proposed fix
) external payable returns (bool, bytes memory) { + // Validate switchboard exists + address switchboard = socket__.switchboardAddresses(switchboardId_); + if (switchboard == address(0)) revert InvalidSwitchboard(); + // Attest digest on FastSwitchboard - IFastSwitchboard(socket__.switchboardAddresses(switchboardId_)).attest( + IFastSwitchboard(switchboard).attest( executionParams_.payloadId, digest_, proof_ ); // Execute payload on socket return socket__.execute{value: msg.value}(executionParams_, transmissionParams_); }Add this error declaration near the top of the contract:
error InvalidSwitchboard();contracts/utils/common/Errors.sol (1)
48-49: Documentation mismatch still present.The NatSpec says "Error thrown when a payload is already cancelled" but the error is named
DeadlineNotPassedForOnChainRevert(). These describe different conditions. Either rename the error to match the cancellation semantics or fix the comment to describe deadline checking.contracts/protocol/switchboard/SwitchboardBase.sol (1)
41-42:revertingPayloadIdsmapping is written but never read.This mapping is set via
setRevertingPayload()in derived contracts butallowPayload()implementations don't check it. Payloads marked as reverting can still be executed. Either add the check in derived contracts'allowPayload()or remove this dead storage.contracts/protocol/switchboard/EVMxSwitchboard.sol (5)
161-170:allowPayloadignoresrevertingPayloadIdsmapping.Payloads marked as reverting via
setRevertingPayload()are not rejected here. Add the check:function allowPayload( bytes32 digest_, bytes32 payloadId_, address target_, bytes memory source_ ) external view returns (bool) { bytes32 appGatewayId = abi.decode(source_, (bytes32)); if (plugAppGatewayIds[target_] != appGatewayId) revert InvalidSource(); + if (revertingPayloadIds[payloadId_]) return false; return isValid[payloadId_][digest_]; }
180-218: Validate plug configuration before creating payload.If
plugAppGatewayIds[plug_]is unset (bytes32(0)), the digest will be created with an invalid target. Add validation:function processPayload( address plug_, bytes calldata payload_, bytes memory overrides_ ) external payable override onlySocket returns (bytes32 payloadId) { + if (plugAppGatewayIds[plug_] == bytes32(0)) revert PlugNotConnected(); EVMxOverrides memory overridesParams = abi.decode(overrides_, (EVMxOverrides));
306-308:markIsValidlacks access control.Anyone can call this after watcher removal to validate previously under-attested payloads. Scenario: 5 watchers, payload gets 4 attestations (insufficient). Watcher revoked → totalWatchers=4. Anyone calls markIsValid → payload valid.
Add access control or remove if redundant with
attest():-function markIsValid(bytes32 payloadId_, bytes32 digest_) external { +function markIsValid(bytes32 payloadId_, bytes32 digest_) external onlyOwner { if (attestations[payloadId_][digest_] >= totalWatchers) isValid[payloadId_][digest_] = true; }
144-151: Zero watchers allows any attestation to mark payloads valid.When
totalWatchers == 0, the conditionattestations[payloadId_][digest_] >= totalWatchersis always true after the first attestation. A single attestation from any watcher marks the payload valid. Add a guard:function _processAttestation(bytes32 payloadId_, bytes32 digest_, address watcher_) internal { + if (totalWatchers == 0) revert WatcherNotFound(); if (isAttestedByWatcher[watcher_][payloadId_][digest_]) revert AlreadyAttested();
81-94: Constructor should validate EVMX configuration parameters.If
evmxChainSlug_orevmxWatcherSetId_are zero, the contract deploys successfully but operations may fail later. Add validation:constructor( uint32 chainSlug_, ISocket socket_, address owner_, address transmitter_, address evmxSocket_, uint32 evmxChainSlug_, uint32 evmxWatcherSetId_ ) SwitchboardBase(chainSlug_, socket_, owner_) { + if (evmxChainSlug_ == 0 || evmxWatcherSetId_ == 0) revert EvmxConfigNotSet(); transmitter = transmitter_; evmxChainSlug = evmxChainSlug_; evmxWatcherSetId = evmxWatcherSetId_; evmxSocket = evmxSocket_; }contracts/protocol/switchboard/MessageSwitchboard.sol (5)
178-185: Zero watchers allows any attestation to mark payloads valid.Same issue as EVMxSwitchboard: when
totalWatchers == 0, conditionattestations[payloadId_][digest_] >= totalWatcherspasses on first attestation. Add a guard:function _processAttestation(bytes32 payloadId_, bytes32 digest_, address watcher_) internal { + if (totalWatchers == 0) revert WatcherNotFound(); if (isAttestedByWatcher[watcher_][payloadId_][digest_]) revert AlreadyAttested();
190-200:allowPayloadignoresrevertingPayloadIdsmapping.Payloads marked as reverting via
setRevertingPayload()are not rejected. Add:function allowPayload( bytes32 digest_, bytes32 payloadId_, address target_, bytes memory sibling_ ) external view override returns (bool) { + if (revertingPayloadIds[payloadId_]) return false; (uint32 siblingChainSlug, bytes32 siblingPlug) = _decodePackedSource(sibling_);
251-262: Refund toaddress(0)will burn ETH permanently.Native flow stores
overrides.refundAddresswithout validation. If zero,refund()sends ETH to the zero address. Add validation:} else { // Native token flow - validate fees and track for potential refund if (msg.value < minMsgValueFees[overrides.dstChainSlug] + overrides.value) revert InsufficientMsgValue(); + if (overrides.refundAddress == address(0)) revert ZeroAddress(); payloadFees[payloadId] = PayloadFees({
589-600: Validate sibling configuration parameters.Setting
socket_,switchboard_, orswitchboardId_to zero causesSiblingSocketNotFounderrors in other functions. Fail fast:function setSiblingConfig( uint32 chainSlug_, bytes32 socket_, bytes32 switchboard_, uint32 switchboardId_ ) external onlyOwner { + if (socket_ == bytes32(0) || switchboard_ == bytes32(0) || switchboardId_ == 0) { + revert SiblingSocketNotFound(); + } siblingSockets[chainSlug_] = socket_;
678-684: Encoding mismatch betweengetPlugConfigandupdatePlugConfig.
updatePlugConfig(line 653) decodes(uint32, bytes32)butgetPlugConfigreturns onlyabi.encode(siblingPlugs[chainSlug_][plug_]). Round-trip pattern breaks. Fix:function getPlugConfig( address plug_, bytes memory extraData_ ) external view override returns (bytes memory plugConfig_) { uint32 chainSlug_ = abi.decode(extraData_, (uint32)); - plugConfig_ = abi.encode(siblingPlugs[chainSlug_][plug_]); + plugConfig_ = abi.encode(chainSlug_, siblingPlugs[chainSlug_][plug_]); }
🧹 Nitpick comments (1)
contracts/protocol/switchboard/MessageSwitchboard.sol (1)
765-775:_extractSignaturesis declared but unused.This internal function appears to have no callers. Either remove it or verify it's intended for future use.
#!/bin/bash # Check if _extractSignatures is called anywhere rg -n '_extractSignatures' --glob '*.sol'
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
contracts/protocol/SocketBatcher.solcontracts/protocol/switchboard/EVMxSwitchboard.solcontracts/protocol/switchboard/MessageSwitchboard.solcontracts/protocol/switchboard/SwitchboardBase.solcontracts/utils/common/DigestUtils.solcontracts/utils/common/Errors.soltest/protocol/Socket.t.soltest/protocol/switchboard/EVMxSwitchboard.t.soltest/protocol/switchboard/MessageSwitchboard.t.sol
🔇 Additional comments (14)
contracts/protocol/SocketBatcher.sol (3)
9-9: Clean import consolidation.Consolidating ExecutionParams and TransmissionParams into a single destructured import improves readability and makes dependencies clearer.
21-23: Documentation improvements are clear and accurate.The updated documentation accurately reflects the refactored single-transaction flow. Section headers improve code organization, and the function docs clearly describe the new attestAndExecute behavior and rescue function usage.
Also applies to: 25-30, 37-50, 106-114
16-16: No action needed. All implementations of IFastSwitchboard in active code (MessageSwitchboard, EVMxSwitchboard) already use the new attest signature with payloadId_. All callers in current tests and contracts pass payloadId_ correctly. The deprecated MockFastSwitchboard still uses the old signature, but it's expected to be outdated since it's in the deprecated folder.Likely an incorrect or invalid review comment.
contracts/utils/common/Errors.sol (1)
73-173: Expanded error surface looks solid.New errors are well-documented with clear NatSpec comments. The parameterized
RoleNotAuthorized(bytes32 role)error is a good pattern for debugging access control failures.contracts/utils/common/DigestUtils.sol (1)
10-37: Well-structured digest creation with collision resistance.Length-prefixing variable-length fields before hashing prevents collision attacks where different field boundaries could produce identical hashes. The separation of fixed-size and variable-length encoding is clean.
contracts/protocol/switchboard/SwitchboardBase.sol (3)
71-80: Constructor validation and initialization are solid.Input validation for zero values prevents misdeployment. Calling
socket__.registerSwitchboard()in the constructor ensures atomic ID assignment.
90-94: Namespace isolation for nonces prevents cross-function replay.Hashing the selector with the nonce ensures a nonce used for one function can't be replayed for another. This is a good security pattern.
142-153: Signature validation with role-specific errors improves debugging.Returning different errors for
WATCHER_ROLEvsFEE_UPDATER_ROLEvs generic roles helps identify which permission is missing. Clean implementation.contracts/protocol/switchboard/EVMxSwitchboard.sol (2)
104-111: Attestation flow is well-structured.Message hash includes contract address and chainSlug for domain separation. Batch attestation reuses the same message hash for efficiency. Clean pattern.
Also applies to: 122-135
347-363: Watcher role management correctly maintains count invariant.Blocking direct
grantRole/revokeRoleforWATCHER_ROLEensurestotalWatchersstays synchronized. Good defensive design.Also applies to: 369-381
contracts/protocol/switchboard/MessageSwitchboard.sol (4)
459-473: Assembly-based decoding is correct and well-documented.The inline assembly correctly extracts
uint32from the leftmost 4 bytes andbytes32from the remaining 32 bytes. The length check at line 462 prevents out-of-bounds reads.
511-522: Refund path has reentrancy guard but missing zero-address check.The
nonReentrantmodifier protects against reentrancy. However, iffees.refundAddressisaddress(0), ETH is burned. Add validation inprocessPayload(native flow) or here before transfer.
286-342: Override versioning provides clean extensibility.Version-based decoding with explicit struct mapping allows future override formats without breaking existing flows. The
UnsupportedOverrideVersionerror provides clear feedback.
690-706: Watcher role management correctly maintains count invariant.Same pattern as EVMxSwitchboard - blocking direct role operations for
WATCHER_ROLEkeepstotalWatcherssynchronized.Also applies to: 712-724
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.