Fix watcher #214
Fix watcher #214
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (9)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR introduces deadline-based expiration for promises/triggers across the protocol, refactors transmitter fee settlement in the watcher, updates fee management to support amount-based blocking/unblocking, adds default deadline tracking to switchboards with reverting trigger support, and modifies processTrigger to return overrides data. Multiple formatting corrections applied across Solana utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Socket
participant Switchboard
participant Watcher
participant AsyncPromise
participant FeesManager
App->>Socket: triggerAppGateway(data, plugOverrides)
Socket->>Switchboard: processTrigger(plug, triggerId, payload, overrides)
Note over Switchboard: Extract/apply defaultDeadline<br/>if not in overrides
Switchboard-->>Socket: return overridesData (with deadline)
Socket->>Socket: Emit AppGatewayCallRequested<br/>(using overridesData)
Socket->>Watcher: resolve(payloadId, data)
Note over Watcher: Check deadline expired?
alt Deadline not exceeded
Watcher->>FeesManager: unblockAndAssignCredits<br/>(payloadId, transmitter, amount)
FeesManager->>FeesManager: Burn amount from consumeFrom<br/>Mint amount to assignTo
Watcher->>AsyncPromise: markResolved(payloadId)
Note over AsyncPromise: Check promiseDeadline<br/>Revert if exceeded
AsyncPromise->>AsyncPromise: Update state to RESOLVED
else Deadline exceeded
AsyncPromise-->>Watcher: DeadlinePassed revert
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Comment |
feat: updated payloadId implementation
feat: updated deposit to trigger
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
contracts/evmx/watcher/precompiles/WritePrecompile.sol (2)
186-199: Fix missing gasLimit assignment when user provides non-zero value.The
_validatefunction only handles the case whererawPayload_.overrideParams.gasLimit == 0, setting defaults for each chain. If a user provides a non-zero gasLimit, the function returns with an uninitialized gasLimit (0), silently dropping the user's override. This breaks the override mechanism.Apply this diff to fix the logic:
- if (rawPayload_.overrideParams.gasLimit == 0) { + gasLimit = rawPayload_.overrideParams.gasLimit; + 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 } } + return gasLimit;
273-278: Remove hardcoded Solana addresses before production.Lines 273–278 contain explicit TODO comments about hardcoded socket and transmitter addresses for Solana. These placeholders should be replaced with dynamic configuration before this contract goes live, as hardcoded values will break if addresses change and prevent multi-environment deployment.
contracts/protocol/interfaces/ISwitchboard.sol (1)
34-39: Two ISwitchboard implementations still missing the return type—compilation will fail.The interface now requires
processTriggerto returnbytes memory overridesData, but these implementations haven't been updated:
deprecated/test/protocol/switchboards/FastSwitchboardTest.t.sol(lines 24–31)deprecated/test/mock/MockFastSwitchboard.sol(lines 48–56)Both have the
overridekeyword and will violate the new interface signature. Addreturns (bytes memory overridesData)to both and return an appropriate value (even if justbytes(0)for test mocks).
🧹 Nitpick comments (4)
contracts/evmx/watcher/Watcher.sol (1)
138-141: Settlement flag timing creates atomicity risk.Setting
isTransmitterFeesSettled = truebefore callingfeesManager__().unblockAndAssignCredits()means that if the unblock call reverts, the flag will remain true in storage (since the entire transaction reverts), but on retry, the settlement logic will be skipped. This is actually safe because the whole transaction reverts atomically. However, the pattern is slightly inconsistent with Line 150-151 where settlement happens after successful operations.For consistency and clarity, consider whether the flag should be set after successful unblock, though the current implementation is functionally correct due to transaction atomicity.
contracts/protocol/switchboard/FastSwitchboard.sol (1)
120-123: Consider adding bounds validation for deadline.The
setDefaultDeadlinefunction accepts anyuint256value without validation. Consider adding reasonable bounds:
- Minimum: e.g., 1 hour (3600 seconds) to prevent accidentally setting too short deadlines
- Maximum: e.g., 30 days (2592000 seconds) to prevent excessive deadline periods
function setDefaultDeadline(uint256 defaultDeadline_) external onlyOwner { + require(defaultDeadline_ >= 1 hours && defaultDeadline_ <= 30 days, "Invalid deadline range"); defaultDeadline = defaultDeadline_; emit DefaultDeadlineSet(defaultDeadline_); }This prevents configuration errors that could cause operational issues.
contracts/evmx/helpers/AsyncPromise.sol (2)
84-93: Consider validating deadline parameter.The
initializefunction acceptsdeadline_without validation and setspromiseDeadline = deadline_ + block.timestamp. Consider adding bounds checking:function initialize( bytes32 payloadId_, address invoker_, address addressResolver_, uint256 deadline_ ) public reinitializer(1) { + require(deadline_ > 0 && deadline_ <= 30 days, "Invalid deadline duration"); localInvoker = invoker_; payloadId = payloadId_; _setAddressResolver(addressResolver_); promiseDeadline = deadline_ + block.timestamp; }This prevents:
- Deadline of 0 (immediate expiration)
- Excessively long deadlines that could cause operational issues
102-103: Consider reordering checks for clarity.The current order checks the deadline before the state:
if (block.timestamp > promiseDeadline) revert DeadlinePassed(); if (state == AsyncPromiseState.RESOLVED || state == AsyncPromiseState.ONCHAIN_REVERTING) revert PromiseAlreadyResolved();If a promise is already resolved and the deadline has passed, this reverts with
DeadlinePassedinstead ofPromiseAlreadyResolved.Consider reversing the order for clearer error messages:
+if (state == AsyncPromiseState.RESOLVED || state == AsyncPromiseState.ONCHAIN_REVERTING) revert PromiseAlreadyResolved(); if (block.timestamp > promiseDeadline) revert DeadlinePassed(); -if (state == AsyncPromiseState.RESOLVED || state == AsyncPromiseState.ONCHAIN_REVERTING) revert PromiseAlreadyResolved();This ensures that if a promise is already resolved, the error message reflects that fact rather than indicating a deadline issue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
contracts/evmx/fees/FeesManager.sol(1 hunks)contracts/evmx/fees/MessageResolver.sol(7 hunks)contracts/evmx/helpers/AsyncDeployer.sol(3 hunks)contracts/evmx/helpers/AsyncPromise.sol(6 hunks)contracts/evmx/helpers/solana-utils/Ed25519.sol(1 hunks)contracts/evmx/helpers/solana-utils/Ed25519_pow.sol(1 hunks)contracts/evmx/helpers/solana-utils/Sha512.sol(1 hunks)contracts/evmx/helpers/solana-utils/SolanaPda.sol(1 hunks)contracts/evmx/helpers/solana-utils/SolanaSignature.sol(1 hunks)contracts/evmx/helpers/solana-utils/program-pda/FeesPlugPdas.sol(1 hunks)contracts/evmx/interfaces/IFeesManager.sol(1 hunks)contracts/evmx/watcher/Watcher.sol(4 hunks)contracts/evmx/watcher/borsh-serde/BorshDecoder.sol(1 hunks)contracts/evmx/watcher/borsh-serde/BorshEncoder.sol(1 hunks)contracts/evmx/watcher/borsh-serde/BorshUtils.sol(1 hunks)contracts/evmx/watcher/precompiles/WritePrecompile.sol(2 hunks)contracts/protocol/Socket.sol(5 hunks)contracts/protocol/SocketConfig.sol(4 hunks)contracts/protocol/interfaces/ISwitchboard.sol(3 hunks)contracts/protocol/switchboard/FastSwitchboard.sol(5 hunks)contracts/protocol/switchboard/MessageSwitchboard.sol(17 hunks)contracts/protocol/switchboard/SwitchboardBase.sol(2 hunks)contracts/utils/common/Structs.sol(2 hunks)hardhat-scripts/deploy/6.connect.ts(2 hunks)script/counter/IncrementCountersFromApp.s.sol(0 hunks)test/SetupTest.t.sol(4 hunks)test/Utils.t.sol(1 hunks)test/mocks/MockPlug.sol(1 hunks)test/switchboard/MessageSwitchboard.t.sol(15 hunks)
💤 Files with no reviewable changes (1)
- script/counter/IncrementCountersFromApp.s.sol
🔇 Additional comments (27)
contracts/evmx/helpers/solana-utils/Sha512.sol (1)
278-278: No functional changes to review.This file contains only a trailing newline addition. The SHA512 implementation remains unchanged and correct.
contracts/evmx/watcher/borsh-serde/BorshEncoder.sol (1)
317-317: Trailing newline addition.This is a formatting-only change with no functional impact.
contracts/evmx/helpers/solana-utils/SolanaSignature.sol (1)
1-17: LGTM—formatting aligned with best practices.This is purely a formatting fix (EOF newline). The contract logic is unchanged:
verifyMessagecorrectly delegates toEd25519.verifyand returns the result. No correctness, security, or functionality concerns.contracts/evmx/helpers/solana-utils/SolanaPda.sol (1)
270-270: Formatting correction applied correctly.Adding the trailing newline to the file end is a standard linting convention and causes no functional change. This aligns with the PR's formatting improvements across Solana utilities.
hardhat-scripts/deploy/6.connect.ts (2)
4-8: Formatting improvement for imports.Multi-line grouped import improves readability and aligns with the formatting consistency observed across this PR.
177-179: Good formatting of complex ABI type string.Splitting the ABI type array across multiple lines improves readability of the complex tuple structure. The trailing comma enables cleaner future diffs if additional types are added. The type string itself remains functionally correct.
contracts/evmx/helpers/solana-utils/Ed25519_pow.sol (1)
1-329: Formatting-only change; no functional updates.This file contains only a trailing newline adjustment. The Ed25519 cryptographic implementation itself remains unchanged and is unaffected by the PR's core logic changes (watcher refactoring, deadline-based expiration, fee settlement). The code is correct.
contracts/evmx/watcher/borsh-serde/BorshDecoder.sol (1)
359-359: Trailing newline formatting aligns with best practices.This change is purely stylistic—adding a newline at EOF follows POSIX conventions and improves consistency with the rest of the codebase as mentioned in the PR summary.
contracts/evmx/helpers/solana-utils/Ed25519.sol (1)
906-906: Non-functional formatting change only.This file shows only a trailing newline correction at the end of the file. Since this is a non-functional formatting fix and the Ed25519 library implementation remains unchanged, no code review is warranted here. This change appears orthogonal to the PR's primary objectives around watcher fixes and deadline-based expiration.
contracts/evmx/watcher/precompiles/WritePrecompile.sol (1)
97-97: Clarify intent of commented-out_initializeWatchercall.Line 97 has a commented-out call to
_initializeWatcher. Confirm whether this is intentional (e.g., feature flag, temporary debug) or if it should be removed or restored.contracts/protocol/SocketConfig.sol (1)
30-30: LGTM - Formatting changes improve readability.The multi-line formatting applied to function calls and conditionals is consistent and makes the code more readable without any behavioral impact.
Also applies to: 124-127, 131-134, 146-146, 190-193
contracts/evmx/watcher/Watcher.sol (4)
104-104: LGTM: Proper initialization of settlement flag.The new
isTransmitterFeesSettledfield is correctly initialized tofalsewhen the payload is created.
223-224: LGTM: Deadline validation is correct.The deadline check correctly prevents late triggers by reverting when
deadline < block.timestamp(deadline is in the past).
150-152: The three-way credit split is correct; verify upstream validation prevents underflow.The settlement correctly splits credits three ways:
unblockAndAssignCredits(..., transmitter, feesUsed_)assigns fees to transmitterunblockAndAssignCredits(..., watcher, watcherFees)assigns fees to watcherunblockCredits(payloadId)returns the remainder toconsumeFromEach call reduces
blockedCredits[payloadId]by its amount, so the remainder is properly returned to the original user. The implementation is intentional and safe.However, ensure that upstream code (where
p.watcherFeesandfeesUsed_are calculated) validates thatfeesUsed_ + watcherFees ≤ maxFees. If either amount exceeds what remains inblockedCredits, theuserBlockedCreditssubtraction will underflow.
277-278: Cancel execution fee distribution is correct; naming is misleading but logic is sound.The original concern mischaracterizes the cancellation scenario.
cancelExecution()is not cancelling an unused payload—it settles fees for a failed execution attempt.The code calls
cancelExecution()when:
markOnchainRevert(): execution was attempted and reverted onchain (after deadline)_revertTx(): explicit revert of an attempted executionIn these scenarios, the transmitter deserves compensation for work attempted. The fee split—transmitter gets
maxFees - watcherFees, watcher getswatcherFees—is intentional and mirrors the success path where both parties are paid.The weak point is naming:
cancelExecutionobscures that this settles failed executions, not unused payloads. Better naming would reduce confusion, but the logic itself is correct.Likely an incorrect or invalid review comment.
test/switchboard/MessageSwitchboard.t.sol (1)
223-223: LGTM: Test overrides correctly updated with deadline field.All test cases now include
deadline: 86400(1 day in seconds) in their overrides, which correctly aligns with:
- The new
deadlinefield inMessageOverridesstruct- The
defaultDeadline = 1 daysin switchboard contracts- The deadline validation logic introduced in the PR
The consistent use of 86400 across all tests ensures proper coverage of the deadline feature.
Also applies to: 259-259, 452-452, 521-521, 534-534, 625-625, 636-636, 1018-1018, 1066-1066, 1116-1116
contracts/evmx/helpers/AsyncDeployer.sol (1)
147-148: LGTM: Deadline correctly propagated to AsyncPromise.The
defaultDeadlineis properly included in the initialization data for AsyncPromise contracts, ensuring promises are created with the correct deadline configuration.contracts/protocol/switchboard/FastSwitchboard.sol (3)
14-14: LGTM: Default deadline set to reasonable value.Setting
defaultDeadline = 1 daysprovides a reasonable default expiration period for payloads.
86-95: Verify overrides structure consistency between switchboards.FastSwitchboard's
processTrigger:
- Decodes
overrides_as a singleuint256 deadline- Returns either the original
overrides_or a new encoded deadlineHowever, MessageSwitchboard's
processTrigger(lines 166-174):
- Decodes
overrides_as a complexMessageOverridesstruct (version, dstChainSlug, gasLimit, value, refundAddress, deadline, etc.)- Returns
abi.encode(overrides)with the full structThis inconsistency means:
- FastSwitchboard and MessageSwitchboard expect different overrides formats
- The ISwitchboard interface doesn't specify the overrides structure
- AppGateways using different switchboards must provide different override formats
This is acceptable if intentional (different switchboards have different requirements), but should be documented clearly. Verify that:
- This design is intentional
- AppGateways know which override format to use for their connected switchboard
- The difference in complexity is justified (MessageSwitchboard needs more fields for messaging)
115-118: LGTM: Setter properly restricted to owner.The
setRevertingTriggerfunction correctly usesonlyOwnermodifier and emits an appropriate event.contracts/protocol/Socket.sol (3)
216-228: LGTM: Switchboard can now normalize overrides before emission.Capturing
overridesDatafromprocessTriggerand using it in theAppGatewayCallRequestedevent allows switchboards to:
- Apply default values (e.g., deadline)
- Normalize the overrides format
- Return computed values back to the event for off-chain tracking
This improves flexibility and ensures emitted events reflect the actual overrides that will be used.
238-244: LGTM: Passing sender enables authorization in switchboard.Adding
msg.senderto theincreaseFeesForPayloadcall allows the switchboard to verify that the plug attempting to increase fees is the same plug that created the payload. This prevents unauthorized fee manipulation.
252-253: LGTM: Stricter switchboard validation improves security.Adding the check
isValidSwitchboard[switchboardId] != SwitchboardStatus.REGISTEREDensures that only fully registered switchboards can be used. This prevents operations on switchboards in intermediate or invalid states.contracts/evmx/helpers/AsyncPromise.sol (1)
65-65: LGTM: New error appropriately defines the rejection case.The
PromiseAlreadyResolvederror clearly communicates when a promise cannot be resolved again.contracts/protocol/switchboard/MessageSwitchboard.sol (3)
46-46: LGTM: Consistent default deadline across switchboards.Setting
defaultDeadline = 1 daysmatches FastSwitchboard's default, providing consistency across the protocol.
166-174: LGTM: Overrides properly decoded, defaulted, and returned.The pattern of:
- Decode overrides
- Apply defaults (in
_decodeOverrides)- Re-encode and return
Ensures that the returned
overridesDatacontains the complete, normalized overrides with all defaults applied.
240-242: LGTM: Correct removal of pure modifier.Changing
_decodeOverridesfrominternal puretointernalis necessary because the function now accesses state (defaultDeadline) and usesblock.timestamp, which are not allowed in pure functions.
Summary by CodeRabbit
Release Notes