Test MessageRelayer #142
Test MessageRelayer #142
Conversation
The ETHBridge uses a mock signal service so any deposit is considered valid
WalkthroughA comprehensive set of new Solidity test suites and scenario contracts were introduced to cover message relayer logic, including deposit recipient, tip recipient, fund amount, gas limit, and relay recipient behaviors. Supporting contracts and interfaces, such as a generic recipient and initial state setup, were added. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MessageRelayer
participant Recipient
participant TipRecipient
User->>MessageRelayer: relayMessage{deposit, proof, ...} (with msg.value)
alt msg.value >= tip
MessageRelayer->>Recipient: receiveMessage{...} (value: amount - tip)
MessageRelayer->>TipRecipient: transfer tip
Recipient-->>MessageRelayer: (optional: reentrancy or revert)
else msg.value < tip
MessageRelayer-->>User: revert InsufficientValue()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: in erc20bridge and similar bridge contracts, when deploying bridged tokens, it's important to check ...Applied to files:
📚 Learning: in provermanager invariant tests, it's acceptable to rely on solidity mapping behavior that returns ...Applied to files:
🔇 Additional comments (6)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Changes to gas cost
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/MessageRelayer/GenericRecipient.t.sol (1)
24-24: Use standard error handling pattern.The current syntax
require(callWillSucceed, CallFailed())is non-standard. Use either the traditional require with string message or the modern revert with custom error pattern.- require(callWillSucceed, CallFailed()); + if (!callWillSucceed) { + revert CallFailed(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
test/MessageRelayer/DepositRecipientScenarios.t.sol(1 hunks)test/MessageRelayer/GenericRecipient.t.sol(1 hunks)test/MessageRelayer/InitialState.t.sol(1 hunks)test/MessageRelayer/TipRecipientScenarios.t.sol(1 hunks)test/mocks/MockSignalService.sol(1 hunks)
🧠 Learnings (1)
test/MessageRelayer/GenericRecipient.t.sol (1)
Learnt from: ggonzalez94
PR: #139
File: src/protocol/taiko_alethia/MessageRelayer.sol:87-90
Timestamp: 2025-07-14T20:46:02.022Z
Learning: In Solidity, when a function already has error handling that will catch invalid states (like sending to address(0)), adding explicit validation beforehand may be redundant if the same failure outcome occurs. The existing require statement after the call will handle the error appropriately.
🧰 Additional context used
🧠 Learnings (1)
test/MessageRelayer/GenericRecipient.t.sol (1)
Learnt from: ggonzalez94
PR: #139
File: src/protocol/taiko_alethia/MessageRelayer.sol:87-90
Timestamp: 2025-07-14T20:46:02.022Z
Learning: In Solidity, when a function already has error handling that will catch invalid states (like sending to address(0)), adding explicit validation beforehand may be redundant if the same failure outcome occurs. The existing require statement after the call will handle the error appropriately.
🔇 Additional comments (17)
test/mocks/MockSignalService.sol (2)
6-25: LGTM! Mock implementation is appropriate for testing.The mock contract correctly implements the
ISignalServiceinterface with appropriate behavior for testing:
- Functions not needed for tests revert with
NotImplementedverifySignalis a no-op allowing tests to proceed without actual verification- Clean and focused implementation for its intended purpose
6-25: Appropriate mock design for testing.The MockSignalService correctly implements the minimal interface needed for testing, with clear separation between implemented (verifySignal) and unimplemented functions. The no-op verification allows tests to focus on MessageRelayer behavior rather than signal service complexity.
test/MessageRelayer/TipRecipientScenarios.t.sol (6)
10-22: LGTM! Well-structured test for valid tip recipient scenario.The test contract correctly verifies both positive and negative cases:
- Confirms the user-selected tip recipient receives the tip
- Confirms the relayer-selected tip recipient doesn't receive payment
- Uses proper balance checking methodology
24-36: LGTM! Proper handling of zero address tip recipient.The test correctly:
- Overrides setUp to set user tip recipient to address(0)
- Re-encodes the call data after the change
- Verifies fallback to relayer-selected tip recipient
38-49: LGTM! Appropriate test for failing tip recipient scenario.The test design is sound:
- Correctly bypasses the parent class that expects successful calls
- Sets up a failing recipient using the GenericRecipient mock
- Properly expects the FailedClaim revert
The comment on line 38 provides helpful context for the inheritance choice.
10-22: Excellent test coverage for valid tip recipient scenario.The tests properly verify both that the correct recipient receives the tip and that the wrong recipient does not, using balance comparisons which is the most reliable way to test ETH transfers.
24-36: Proper handling of zero address fallback scenario.The test correctly demonstrates the fallback behavior when user sets tip recipient to zero address, with proper setup override and balance verification.
38-49: Well-designed failure scenario test.The comment at line 38 provides valuable context about bypassing the parent class due to vm.expectCall requirements. The test properly simulates recipient failure and expects the correct error type.
test/MessageRelayer/InitialState.t.sol (4)
29-51: LGTM! Comprehensive test setup with proper initialization.The setUp function correctly:
- Creates all necessary mock and real contracts
- Funds the bridge contract with the required amount
- Initializes the MessageRelayer with the bridge
- Sets up a complete ETHDeposit structure with reasonable defaults
- Calls helper function to encode the call data
53-69: LGTM! Well-designed helper functions for testing.The helper functions provide good abstractions:
_encodeReceiveCallproperly encodes the message relay call data_relayMessageprovides a clean interface for triggering relay operations_randomAddressand_domainSeparatorcreate deterministic but pseudo-random addresses for testing
29-51: Comprehensive and well-structured test setup.The InitialState contract provides an excellent foundation for testing with proper initialization of all components, consistent parameter values, and clean separation of concerns. The use of vm.deal and proper contract initialization follows testing best practices.
63-69: Good practice using domain separation for address generation.The _randomAddress function with domain separation provides deterministic yet pseudo-random addresses, which is excellent for reproducible testing.
test/MessageRelayer/DepositRecipientScenarios.t.sol (4)
31-41: LGTM! Well-structured tests for MessageRelayer as recipient.The abstract contract appropriately tests:
- MessageRelayer's receiveMessage is called during relay
- MessageRelayer's receiveMessage is called during direct claim
- Uses proper vm.expectCall assertions
25-28: Fix incorrect expectCall usage.The
vm.expectCallwith a count of 0 is incorrect - this expects the call to NOT happen, but the test name suggests it should verify the call doesn't occur. However,expectCallwith count 0 will actually expect the call to happen 0 times, which contradicts the assertion logic.Apply this diff to fix the test:
function test_DepositRecipientIsNotMessageRelayer_relayMessage_shouldNotInvokeReceiveMessage() public { - vm.expectCall(address(messageRelayer), ethDeposit.data, 0); _relayMessage(); + // This test should verify that receiveMessage is NOT called + // Since we're bypassing the MessageRelayer, we can simply verify the relay succeeds + // without the MessageRelayer being involved }Alternatively, if you want to explicitly verify the call doesn't happen, consider restructuring the test or using a different assertion approach.
⛔ Skipped due to learnings
Learnt from: nikeshnazareth PR: OpenZeppelin/minimal-rollup#134 File: test/ProverManager/CurrentPeriodIsOpenTest.t.sol:0-0 Timestamp: 2025-06-26T01:42:08.393Z Learning: In Foundry testing, the syntax `vm.expectRevert(ErrorSelector, 0)` is valid and means "expect zero reverts with that specific error message." This is used to assert that a particular error does NOT occur during function execution, while potentially allowing other errors. This is useful for testing that specific validation checks are bypassed in certain conditions.
31-41: Well-designed test scenarios for deposit recipient behavior.The abstract/concrete contract pattern effectively separates concerns and the test cases properly verify both positive (calls should happen) and negative (calls should not happen) scenarios using appropriate Foundry testing patterns.
25-28: Confirm vm.expectCall(count=0) Correctly Asserts No CallsI wasn’t able to locate the
expectCall(address,bytes,uint)overload in the repo’s code, so please verify against your Foundry version and test setup that:
vm.expectCall(address(messageRelayer), ethDeposit.data, 0)is a supported overload.- Supplying
0indeed asserts zero invocations ofreceiveMessage.- The test suite fails if
receiveMessageis ever called.If your Foundry version doesn’t support a
countparameter, consider switching to an alternative negative‐test pattern (e.g.vm.record/vm.accessesor a custom revert hook).test/MessageRelayer/GenericRecipient.t.sol (1)
4-27: Well-designed test mock contract.The GenericRecipient contract provides excellent controllable behavior for testing scenarios, properly handling both fallback and receive functions, and maintaining clean state management.
| function _simulateFunctionCall() internal { | ||
| require(callWillSucceed, CallFailed()); | ||
| emit FunctionCalled(); | ||
| } |
There was a problem hiding this comment.
Fix syntax error in require statement.
The require statement with a custom error is using incorrect syntax. The require function expects a string message, not a custom error.
Apply this diff to fix the syntax error:
function _simulateFunctionCall() internal {
- require(callWillSucceed, CallFailed());
+ if (!callWillSucceed) {
+ revert CallFailed();
+ }
emit FunctionCalled();
}📝 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 _simulateFunctionCall() internal { | |
| require(callWillSucceed, CallFailed()); | |
| emit FunctionCalled(); | |
| } | |
| function _simulateFunctionCall() internal { | |
| - require(callWillSucceed, CallFailed()); | |
| + if (!callWillSucceed) { | |
| + revert CallFailed(); | |
| + } | |
| emit FunctionCalled(); | |
| } |
🤖 Prompt for AI Agents
In test/MessageRelayer/GenericRecipient.t.sol around lines 23 to 26, the require
statement uses a custom error CallFailed() which is invalid syntax because
require expects a string message. Replace the custom error with a string message
describing the failure, for example require(callWillSucceed, "Call failed"), to
fix the syntax error.
made scripts easier to work with fmt
LeoPatOZ
force-pushed
the
tests/message-relayer
branch
from
7335e0c to
fa42d46
Compare
LeoPatOZ
force-pushed
the
tests/message-relayer
branch
from
34b8e5f to
ffd4e33
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/MessageRelayer/GenericRecipient.t.sol (1)
40-47: Fix syntax error in require statement.The
requirestatement with a custom error is using incorrect syntax. Therequirefunction expects a string message, not a custom error.Apply this diff to fix the syntax error:
function _simulateFunctionCall() internal { - require(callWillSucceed, CallFailed()); + if (!callWillSucceed) { + revert CallFailed(); + } emit FunctionCalled();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
Cargo.toml(1 hunks)offchain/sample_deposit_proof.rs(5 hunks)offchain/sample_signal_proof.rs(2 hunks)src/protocol/IMessageRelayer.sol(1 hunks)src/protocol/taiko_alethia/MessageRelayer.sol(1 hunks)test/ETHBridge/SampleDepositProof.t.sol(9 hunks)test/MessageRelayer/DepositRecipientScenarios.t.sol(1 hunks)test/MessageRelayer/FundAmountScenarios.t.sol(1 hunks)test/MessageRelayer/GasLimitScenarios.t.sol(1 hunks)test/MessageRelayer/GenericRecipient.t.sol(1 hunks)test/MessageRelayer/InitialState.t.sol(1 hunks)test/MessageRelayer/RelayRecipientScenarios.t.sol(1 hunks)test/MessageRelayer/TipRecipientScenarios.t.sol(1 hunks)test/SignalService/SampleProof.t.sol(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- offchain/sample_signal_proof.rs
- test/SignalService/SampleProof.t.sol
🚧 Files skipped from review as they are similar to previous changes (2)
- test/MessageRelayer/DepositRecipientScenarios.t.sol
- test/MessageRelayer/InitialState.t.sol
🧰 Additional context used
🧠 Learnings (3)
src/protocol/taiko_alethia/MessageRelayer.sol (1)
Learnt from: ggonzalez94
PR: #139
File: src/protocol/taiko_alethia/MessageRelayer.sol:87-90
Timestamp: 2025-07-14T20:46:02.022Z
Learning: In Solidity, when a function already has error handling that will catch invalid states (like sending to address(0)), adding explicit validation beforehand may be redundant if the same failure outcome occurs. The existing require statement after the call will handle the error appropriately.
test/MessageRelayer/RelayRecipientScenarios.t.sol (1)
Learnt from: nikeshnazareth
PR: #134
File: test/ProverManager/CurrentPeriodIsOpenTest.t.sol:0-0
Timestamp: 2025-06-26T01:42:08.393Z
Learning: In Foundry testing, the syntax vm.expectRevert(ErrorSelector, 0) is valid and means "expect zero reverts with that specific error message." This is used to assert that a particular error does NOT occur during function execution, while potentially allowing other errors. This is useful for testing that specific validation checks are bypassed in certain conditions.
test/MessageRelayer/GenericRecipient.t.sol (2)
Learnt from: ggonzalez94
PR: #139
File: src/protocol/taiko_alethia/MessageRelayer.sol:87-90
Timestamp: 2025-07-14T20:46:02.022Z
Learning: In Solidity, when a function already has error handling that will catch invalid states (like sending to address(0)), adding explicit validation beforehand may be redundant if the same failure outcome occurs. The existing require statement after the call will handle the error appropriately.
Learnt from: nikeshnazareth
PR: #134
File: test/ProverManager/CurrentPeriodIsOpenTest.t.sol:0-0
Timestamp: 2025-06-26T01:42:08.393Z
Learning: In Foundry testing, the syntax vm.expectRevert(ErrorSelector, 0) is valid and means "expect zero reverts with that specific error message." This is used to assert that a particular error does NOT occur during function execution, while potentially allowing other errors. This is useful for testing that specific validation checks are bypassed in certain conditions.
🧬 Code Graph Analysis (1)
offchain/sample_deposit_proof.rs (2)
offchain/signal_slot.rs (1)
get_signal_slot(22-25)offchain/lib.rs (4)
deploy_eth_bridge(59-69)deploy_signal_service(52-57)get_proofs(71-95)get_provider(39-50)
🔇 Additional comments (21)
Cargo.toml (1)
6-8: LGTM!Converting from a binary target to a library target improves code organization and allows shared utilities to be used across the offchain codebase. The naming convention
minimal_rollupis consistent with the package name.src/protocol/taiko_alethia/MessageRelayer.sol (1)
93-93: LGTM!The validation check ensures that
msg.valueis sufficient to cover the tip before calculatingvalueToSend, preventing potential underflow in the subtraction operation. This is a necessary safety check that aligns with the newInsufficientValue()error definition.test/ETHBridge/SampleDepositProof.t.sol (1)
4-6: LGTM!This file is auto-generated as indicated by the header comment. The updated proof data aligns with coordinated changes between onchain test data and offchain proof generation mentioned in the AI summary.
test/MessageRelayer/RelayRecipientScenarios.t.sol (1)
1-22: LGTM!The test contract provides comprehensive coverage of relay recipient failure scenarios:
- Rejection scenario: Tests when the recipient contract refuses to accept the deposit
- Reentrancy scenario: Tests protection against reentrancy attacks
Both tests properly use
vm.expectRevertwith the specificIETHBridge.FailedClaim.selectorerror, ensuring the expected failure behavior is validated. The inheritance fromDepositRecipientIsMessageRelayerprovides good test structure reuse.offchain/sample_deposit_proof.rs (7)
1-2: LGTM! Import refactoring aligns with crate restructuring.The import changes properly support the transition from local utils module to the shared
minimal_rollupcrate, and the new alloy imports enable ABI encoding functionality.Also applies to: 8-10
16-21: LGTM! Proper use of sol! macro for ABI encoding.The Solidity function definitions are correctly structured and will enable type-safe ABI encoding for the test calldata.
33-33: LGTM! Making utility function public improves reusability.Exposing
create_deposit_callas a public function enables its use in other modules or tests.
81-81: LGTM! Improved type safety and clarity with parse_units.Using
parse_unitswith ether denomination is much clearer and safer than raw numeric literals.
84-105: Excellent refactoring! ABI encoding replaces error-prone hex strings.This change significantly improves code quality by:
- Using type-safe ABI encoding instead of hardcoded hex strings
- Making the calldata generation more maintainable and readable
- Reducing the risk of encoding errors
134-134: LGTM! Minor formatting improvement.The blank line improves code readability.
183-183: Verified: Template file exists at updated pathThe template
offchain/tmpl/sample_deposit_proof.tmplwas found as expected, so the path update inoffchain/sample_deposit_proof.rs(line 183) is valid. No further changes are needed.test/MessageRelayer/GasLimitScenarios.t.sol (3)
4-9: LGTM! Proper imports for gas limit testing.The imports correctly reference the base test infrastructure and required interfaces.
16-25: LGTM! Comprehensive gas limit edge case testing.The tests properly validate both failure and success scenarios:
- High gas limit rejection with appropriate error
- Normal gas limit with expected event emission
27-37: LGTM! Well-designed test setup modifiers.The modifiers effectively create the necessary test conditions:
gasLimitHigherThanValuecreates an invalid edge casewhenGasLimitIsReasonablesets up normal operation conditionsBoth properly encode the receive call after setup.
test/MessageRelayer/TipRecipientScenarios.t.sol (3)
10-29: LGTM! Comprehensive tip recipient validation.The test contract thoroughly validates tip payment logic:
- Confirms user-selected recipient receives tip
- Verifies relayer-selected recipient is not paid incorrectly
- Tests both relay and direct claim scenarios
- Uses proper balance verification approach
31-53: LGTM! Proper zero address tip recipient handling.The contract correctly tests the edge case where user sets tip recipient to zero address:
- Relayer-selected recipient receives tip during message relay
- No tip payment during direct deposit claim (correct behavior)
The setup properly configures the zero address scenario.
55-66: LGTM! Appropriate failure scenario testing.The contract properly tests the tip recipient failure case:
- Uses
InitialStatebase to avoid interference with success expectations- Configures recipient to fail calls
- Correctly expects
FailedClaimerror on relay attemptThe design comment explains the inheritance choice well.
test/MessageRelayer/GenericRecipient.t.sol (2)
11-31: LGTM! Well-designed mock contract for comprehensive testing.The
GenericRecipientcontract provides excellent testing capabilities:
- Configurable success/failure behavior
- Reentrancy attack simulation
- Clean interface with setter functions
- Proper constructor parameter handling
32-38: LGTM! Proper fallback and receive function implementation.Both functions correctly delegate to the same internal simulation logic, ensuring consistent behavior for all incoming calls.
test/MessageRelayer/FundAmountScenarios.t.sol (2)
16-30: LGTM! Comprehensive fund amount edge case testing.The tests properly validate critical scenarios:
- Zero amount with tip correctly reverts (insufficient funds)
- Zero amount without tip succeeds (valid case)
- Tip exceeding amount correctly reverts (insufficient funds)
Event emission verification is appropriate for the success case.
32-49: LGTM! Well-designed test setup modifiers.The modifiers effectively create the necessary edge case scenarios:
zeroTipZeroAmount: Valid zero-value casezeroAmount: Invalid case with tip but no fundsamountTooLow: Insufficient funds for tipAll properly encode the receive call after configuration.
|
|
||
| contract InitialStateTest is InitialState { | ||
| function test_tipRecipientTransientStorage_isZero() public { | ||
| bytes32 value = vm.load(address(messageRelayer), TIP_RECIPIENT_SLOT); |
There was a problem hiding this comment.
Isn't this checking the permanent storage? I couldn't find a simple way to get Foundry to read transient storage
It was checking permanent storage instead of temporary storage
…ipientIsMessageRelayer
This means we don't need to write an identical shouldRevert test everytime we clear the relayShouldSucceed or claimShouldSucceed flag
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/MessageRelayer/GenericRecipient.t.sol (1)
44-44: Fix syntax error in require statement.The
requirestatement cannot use custom errors. Useifwithrevertinstead to properly utilize the custom error.Apply this diff to fix the syntax error:
- require(callWillSucceed, CallFailed()); + if (!callWillSucceed) { + revert CallFailed(); + }
🧹 Nitpick comments (1)
test/MessageRelayer/TipRecipientScenarios.t.sol (1)
10-10: Remove unused console import.The
consoleimport from forge-std appears to be unused in this file. Consider removing it to keep the test file clean.-import {console} from "forge-std/console.sol"; -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
test/MessageRelayer/DepositRecipientScenarios.t.sol(1 hunks)test/MessageRelayer/FundAmountScenarios.t.sol(1 hunks)test/MessageRelayer/GasLimitScenarios.t.sol(1 hunks)test/MessageRelayer/GenericRecipient.t.sol(1 hunks)test/MessageRelayer/InitialState.t.sol(1 hunks)test/MessageRelayer/RelayRecipientScenarios.t.sol(1 hunks)test/MessageRelayer/TipRecipientScenarios.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- test/MessageRelayer/DepositRecipientScenarios.t.sol
- test/MessageRelayer/RelayRecipientScenarios.t.sol
- test/MessageRelayer/InitialState.t.sol
- test/MessageRelayer/FundAmountScenarios.t.sol
- test/MessageRelayer/GasLimitScenarios.t.sol
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: in solidity, when a function already has error handling that will catch invalid states (like sending...
Learnt from: ggonzalez94
PR: OpenZeppelin/minimal-rollup#139
File: src/protocol/taiko_alethia/MessageRelayer.sol:87-90
Timestamp: 2025-07-14T20:46:02.022Z
Learning: In Solidity, when a function already has error handling that will catch invalid states (like sending to address(0)), adding explicit validation beforehand may be redundant if the same failure outcome occurs. The existing require statement after the call will handle the error appropriately.
Applied to files:
test/MessageRelayer/GenericRecipient.t.sol
📚 Learning: in foundry testing, the syntax `vm.expectrevert(errorselector, 0)` is valid and means "expect zero r...
Learnt from: nikeshnazareth
PR: OpenZeppelin/minimal-rollup#134
File: test/ProverManager/CurrentPeriodIsOpenTest.t.sol:0-0
Timestamp: 2025-06-26T01:42:08.393Z
Learning: In Foundry testing, the syntax `vm.expectRevert(ErrorSelector, 0)` is valid and means "expect zero reverts with that specific error message." This is used to assert that a particular error does NOT occur during function execution, while potentially allowing other errors. This is useful for testing that specific validation checks are bypassed in certain conditions.
Applied to files:
test/MessageRelayer/GenericRecipient.t.sol
Technically, the InitialState is a valid default scenario but I think it still makes sense structurally for each group of tests to be built on a previous one
The tests succeed locally but are failing too early in the CI. Use a slightly larger gas limit to try to account for both environments
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/MessageRelayer/TipRecipientScenarios.t.sol (1)
116-117: Add newline at end of fileFollowing standard practice, add a newline character at the end of the file.
// A valid scenario that can be used as a default scenario by unrelated tests. abstract contract DefaultTipRecipientScenario is ValidUserTipRecipientOverrulesRelayer {} +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/protocol/IMessageRelayer.sol(1 hunks)test/MessageRelayer/DepositRecipientScenarios.t.sol(1 hunks)test/MessageRelayer/FundAmountScenarios.t.sol(1 hunks)test/MessageRelayer/GasLimitScenarios.t.sol(1 hunks)test/MessageRelayer/RelayRecipientScenarios.t.sol(1 hunks)test/MessageRelayer/TipRecipientScenarios.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/protocol/IMessageRelayer.sol
- test/MessageRelayer/FundAmountScenarios.t.sol
- test/MessageRelayer/DepositRecipientScenarios.t.sol
- test/MessageRelayer/GasLimitScenarios.t.sol
- test/MessageRelayer/RelayRecipientScenarios.t.sol
🔇 Additional comments (5)
test/MessageRelayer/TipRecipientScenarios.t.sol (5)
4-9: Well-organized importsThe imports are properly grouped and separated, following good organizational practices.
10-37: Well-designed base test contract with clear recipient selection logicThe abstract contract properly implements tip recipient precedence logic where user-selected recipients override relayer-selected ones unless unset. The test functions correctly verify that only the appropriate recipient receives tips.
41-65: Comprehensive user-selected tip recipient test scenariosThe three scenarios properly cover valid, zero, and invalid user-selected tip recipients with appropriate success/failure expectations. The distinction between zero recipient (which falls back to relayer) and invalid recipient (which causes failures) is well-handled.
69-83: Appropriate relayer-selected tip recipient scenariosThe relayer scenarios correctly focus on fallback behavior since relayer recipients are only used when user-selected recipients are unset. The asymmetry with user scenarios is intentional and correct.
87-113: Excellent coverage of combined tip recipient scenariosThe combined test contracts systematically cover all meaningful combinations of user and relayer tip recipients, including precedence rules and failure cases. The setUp methods correctly handle multiple inheritance.
The MockSignalService merged from main (after the bridge PR) no longer verifies by default
Work in progress. Should replace #124 in line with this comment
Summary by CodeRabbit
New Features
Bug Fixes
Chores