fix(contracts-rfq): gas estimation tests [SLT-275] #3204
Conversation
WalkthroughThe pull request introduces significant updates to the Solidity testing framework, particularly for the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Changes to gas cost
🧾 Summary (50% most significant diffs)
Full diff report 👇
|
Deploying sanguine-fe with
|
| Latest commit: |
129cde2
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://11498d1f.sanguine-fe.pages.dev |
| Branch Preview URL: | https://fix-gas-estimation-tests.sanguine-fe.pages.dev |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3204 +/- ##
====================================================
+ Coverage 40.97804% 90.94584% +49.96780%
====================================================
Files 459 60 -399
Lines 25643 1237 -24406
Branches 343 148 -195
====================================================
- Hits 10508 1125 -9383
+ Misses 14383 109 -14274
+ Partials 752 3 -749
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (9)
packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.PFees.t.sol (1)
25-35: LGTM: Transaction copies and nonce assignment look good.The creation of separate copies for bridged and proven transactions with unique nonces is a good practice for isolated testing.
Consider adding brief comments explaining the significance of each nonce value (0, 1, 2, 3) in relation to the transaction types. This would enhance code readability and maintainability.
packages/contracts-rfq/test/FastBridgeV2.Dst.Base.t.sol (1)
38-45: Adjust function declaration formatting for consistencyThe formatting of the
relayWithAddressfunction declaration deviates from standard Solidity style guidelines. Specifically, placing thepublicvisibility modifier on a separate line and the opening brace{on a new line reduces readability.Consider reformatting the function declaration for better clarity:
function relayWithAddress( address caller, address relayer, uint256 msgValue, IFastBridge.BridgeTransaction memory bridgeTx -) - public -{ +) public {packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.t.sol (1)
7-8: Enhance clarity of documentation commentsConsider rephrasing the comments for better clarity and grammar:
/// @notice Estimates the gas cost of FastBridgeV2 destination chain functions. /// @dev Minimal state checks are performed; ensure full coverage in other tests.packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (6)
Line range hint
22-25: Consider correcting the mislabeled parameter in theBridgeProofDisputedevent definition.In the function
expectBridgeProofDisputed, there's a comment indicating that theBridgeProofDisputedevent has a mislabeled address parameter (relayer), which is actually theguard. To improve clarity and maintainability, consider updating the event definition to correctly label the parameter or adding documentation to clarify this discrepancy.
Line range hint
41-49: Address the TODO to unskiptest_bridge_userSpecificNoncefunction.The test function
test_bridge_userSpecificNonceis currently skipped usingvm.skip(true); // TODO: unskip when implemented. Implement the necessary functionality and unskip this test to ensure comprehensive test coverage.Would you like assistance in implementing this test or resolving any issues preventing it from running?
Line range hint
166-171: Ensurecalleris not the zero address intest_prove_revert_callerNotRelayer.In the function
test_prove_revert_callerNotRelayer, consider adding an assumption to ensure thatcalleris not the zero address to prevent unintended behavior during testing.Apply this change:
function test_prove_revert_callerNotRelayer(address caller) public { vm.assume(caller != relayerA && caller != relayerB); + vm.assume(caller != address(0)); bridge({caller: userA, msgValue: 0, params: tokenParams}); expectUnauthorized(caller, fastBridge.RELAYER_ROLE()); prove({caller: caller, bridgeTx: tokenTx, destTxHash: hex"01"}); }
Line range hint
223-230: Ensurecalleris not the zero address intest_claim_token_permissionless.In the function
test_claim_token_permissionless, consider adding an assumption to ensure thatcalleris not the zero address.Apply this change:
function test_claim_token_permissionless(address caller) public { vm.assume(caller != relayerA); + vm.assume(caller != address(0)); bytes32 txId = getTxId(tokenTx); bridge({caller: userA, msgValue: 0, params: tokenParams}); prove({caller: relayerA, bridgeTx: tokenTx, destTxHash: hex"01"}); skip(CLAIM_DELAY + 1); expectBridgeDepositClaimed({bridgeTx: tokenTx, txId: txId, relayer: relayerA, to: relayerA}); claim({caller: caller, bridgeTx: tokenTx}); assertEq(fastBridge.bridgeStatuses(txId), FastBridgeV2.BridgeStatus.RELAYER_CLAIMED); checkTokenBalancesAfterClaim(relayerA); }
Line range hint
299-309: Ensurecalleris not the zero address intest_refund_token_permissionless.In the function
test_refund_token_permissionless, consider adding an assumption to ensure thatcalleris not the zero address.Apply this change:
function test_refund_token_permissionless(address caller) public { vm.assume(caller != refunder); + vm.assume(caller != address(0)); bytes32 txId = getTxId(tokenTx); bridge({caller: userA, msgValue: 0, params: tokenParams}); skip(DEADLINE + PERMISSIONLESS_REFUND_DELAY + 1); expectBridgeDepositRefunded({bridgeParams: tokenParams, txId: txId}); refund({caller: caller, bridgeTx: tokenTx}); assertEq(fastBridge.bridgeStatuses(txId), FastBridgeV2.BridgeStatus.REFUNDED); assertEq(fastBridge.protocolFees(address(srcToken)), INITIAL_PROTOCOL_FEES_TOKEN); assertEq(srcToken.balanceOf(userA), LEFTOVER_BALANCE + tokenParams.originAmount); assertEq(srcToken.balanceOf(address(fastBridge)), INITIAL_PROTOCOL_FEES_TOKEN); }
Line range hint
360-370: Ensurecalleris not the zero address intest_refund_eth_permissionless.In the function
test_refund_eth_permissionless, consider adding an assumption to ensure thatcalleris not the zero address.Apply this change:
function test_refund_eth_permissionless(address caller) public { vm.assume(caller != refunder); + vm.assume(caller != address(0)); bytes32 txId = getTxId(ethTx); bridge({caller: userA, msgValue: 0, params: tokenParams}); bridge({caller: userA, msgValue: ethParams.originAmount, params: ethParams}); skip(DEADLINE + PERMISSIONLESS_REFUND_DELAY + 1); expectBridgeDepositRefunded({bridgeParams: ethParams, txId: txId}); refund({caller: caller, bridgeTx: ethTx}); assertEq(fastBridge.bridgeStatuses(txId), FastBridgeV2.BridgeStatus.REFUNDED); assertEq(fastBridge.protocolFees(ETH_ADDRESS), INITIAL_PROTOCOL_FEES_ETH); assertEq(address(userA).balance, LEFTOVER_BALANCE + ethParams.originAmount); assertEq(address(fastBridge).balance, INITIAL_PROTOCOL_FEES_ETH); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- .github/workflows/solidity.yml (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.Base.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.PFees.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (1 hunks)
🔇 Additional comments (16)
packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.PFees.t.sol (3)
1-7: LGTM: Contract declaration and import look good.The contract is properly set up with the correct license, Solidity version, and import statement. The naming convention clearly indicates the purpose of this test contract.
14-23: LGTM:createFixturesfunction sets up test data correctly.The function properly overrides the base implementation and sets up specific test values for both token and ETH transactions. The values are consistent across different transaction types and use appropriate units.
8-12: LGTM:configureFastBridgefunction is well-implemented.The function correctly overrides the base implementation and sets up the test environment with a 1% protocol fee rate.
However, please verify if granting the
GOVERNOR_ROLEto the contract itself is necessary for these tests. If it is, consider adding a comment explaining why this is needed.✅ Verification successful
LGTM:
configureFastBridgefunction is well-implemented.The function correctly overrides the base implementation and sets up the test environment with a 1% protocol fee rate. Granting the
GOVERNOR_ROLEtoaddress(this)appears intentional for these specific tests.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if GOVERNOR_ROLE is used in other test files rg --type solidity "GOVERNOR_ROLE" packages/contracts-rfq/testLength of output: 95
Script:
#!/bin/bash # Description: Check if GOVERNOR_ROLE is used in other Solidity test files rg "GOVERNOR_ROLE" packages/contracts-rfq/test --glob "*.sol"Length of output: 2363
Script:
#!/bin/bash # Description: Find to whom GOVERNOR_ROLE is granted in test files rg "grantRole\s*\(\s*fastBridge\.GOVERNOR_ROLE\(\)\s*,\s*(.+?)\s*\)" packages/contracts-rfq/test --glob "*.sol" -oLength of output: 536
packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (3)
6-6: LGTM: Import statement updated correctly.The import statement has been updated to include
FastBridgeV2DstBaseTestandIFastBridgefrom the new base test file. This change aligns with the modification in contract inheritance and follows good practices by importing only the necessary components.
Line range hint
20-138: LGTM: Remaining test functions are focused and appropriate.The removal of several functions and constants (e.g.,
LEFTOVER_BALANCE,setUp,deployFastBridge,mintTokens,relay, andrelayWithAddress) suggests that these have likely been moved to the base test fileFastBridgeV2DstBaseTest. This refactoring appears to improve the organization of the test suite.The remaining functions, including
expectBridgeRelayedand various test scenarios, are focused and appropriate for testing theFastBridgeV2Dstcontract. They cover different aspects of token and ETH relaying, including edge cases and revert conditions.To ensure that the removed functions are indeed present in the base test file and that no functionality has been lost, please run the following command:
✅ Verification successful
Verified: Removed functions and constants are correctly present in the base test file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that removed functions exist in the base test file # Test: Check for the presence of removed functions in the base test file rg -i "function (setUp|deployFastBridge|mintTokens|relay|relayWithAddress)" packages/contracts-rfq/test/FastBridgeV2.Dst.Base.t.sol # Also check for the LEFTOVER_BALANCE constant rg -i "LEFTOVER_BALANCE" packages/contracts-rfq/test/FastBridgeV2.Dst.Base.t.solLength of output: 874
9-9: Verify test functionality after inheritance change.The contract now inherits from
FastBridgeV2DstBaseTestinstead ofFastBridgeV2Test. This change is consistent with the updated import statement and suggests a refactoring of the test structure.Please ensure that all tests still function correctly after this change. Run the following command to verify:
packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.t.sol (4)
13-15: Verify proper initialization of user balancesEnsure that
userBreceives the intended balance in both Ether and tokens. Confirm thatdealanddstToken.mintcorrectly initializeuserB's balances as expected.
27-28: EnsureLEFTOVER_BALANCEis defined and initializedThe variable
LEFTOVER_BALANCEis used in assertions but its declaration and initialization are not shown in this file. Verify thatLEFTOVER_BALANCEis properly defined in the test context to prevent assertion failures.
38-39: Consistency in relayer balance assertionsIn
test_relay_token_withRelayerAddress, the balance ofrelayerBis asserted againstLEFTOVER_BALANCE. Confirm that this behavior is intentional and thatrelayerBshould receive theLEFTOVER_BALANCEinstead ofrelayerA, consideringrelayerAis specified in therelayWithAddresscall.
51-52: Confirm initialization ofINITIAL_USER_BALANCEandethParams.destAmountThe test relies on
INITIAL_USER_BALANCEandethParams.destAmountfor balance assertions. Verify that these variables are correctly initialized to reflect the expected balances during the test execution.packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol (1)
86-88: Potential conflict with existingassertEqfunctionThe
assertEqfunction defined on lines 86 to 88 may override or conflict with an existingassertEqfunction from your testing framework. This could lead to confusion or unexpected behavior in your tests.To verify if this causes any conflicts, run the following script to search for other definitions of
assertEqin your codebase:✅ Verification successful
No conflicting
assertEqdefinitions foundBased on the verification, there are no other
assertEqfunctions defined in the codebase that would conflict with the one inFastBridgeV2.Src.Base.t.sol.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for existing `assertEq` function definitions. # Test: Find all occurrences of `function assertEq` in Solidity files. # Expect: Identify if multiple `assertEq` functions exist that could conflict. rg --type solidity --word-regex --line-number --pretty '^function assertEq'Length of output: 521
Script:
#!/bin/bash # Description: Search for existing `assertEq` function definitions. # Test: Find all occurrences of `function assertEq` in Solidity files. # Expect: Identify if multiple `assertEq` functions exist that could conflict. rg --type solidity --word-regexp --line-number --pretty '^function assertEq'Length of output: 111
Script:
#!/bin/bash # Description: Search for existing `assertEq` function definitions. # Test: Find all occurrences of `function assertEq` in Solidity (.sol) files. # Expect: Identify if multiple `assertEq` functions exist that could conflict. rg --glob '*.sol' --word-regexp --line-number --pretty '^function assertEq'Length of output: 77
packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol (3)
24-31: Initialization functions are well-structuredThe
setUp,createFixtures,mintTokens, andinitExistingTxsfunctions are correctly implemented and effectively set up the test environment for the gas benchmarking tests.Also applies to: 33-47, 49-55, 57-72
85-153: Test functions for token bridging operations are comprehensiveThe test functions for token operations (
bridge,prove,claim,dispute, andrefund) are well-implemented and cover various scenarios to ensure the correct functioning of theFastBridgeV2contract's token handling.
154-222: Test functions for ETH bridging operations are comprehensiveThe test functions for ETH operations are effectively designed to benchmark gas usage and validate the behavior of the
FastBridgeV2contract when handling ETH bridging, including edge cases like disputes and refunds.packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (2)
Line range hint
299-299: Correct the spelling of "permisionless" to "permissionless" in the function name.The function name
test_refund_token_permisionlesscontains a typo. It should betest_refund_token_permissionlessto reflect the correct spelling.[typo]
Apply this diff to fix the typo:
-function test_refund_token_permisionless(address caller) public { +function test_refund_token_permissionless(address caller) public {
Line range hint
360-360: Correct the spelling of "permisionless" to "permissionless" in the function name.The function name
test_refund_eth_permisionlesscontains a typo. It should betest_refund_eth_permissionlessto reflect the correct spelling.[typo]
Apply this diff to fix the typo:
-function test_refund_eth_permisionless(address caller) public { +function test_refund_eth_permissionless(address caller) public {
* feat: scaffold exclusivity params * test: update to use V2 structs * test: exclusivity on DST chain * test: exclusivity on SRC chain * feat: exclusivity params in bridge * feat: update decoding in relay * feat: check for exclusivity period in relay * test: disable parity tests (no longer backwards compatible) * test: expect quoteID-related event * feat: additional event for tracking quoteID * fix: ignore code-complexity error for now * test: more coverage for `bridgeRelayDetails` * refactor: remove unnecessary casts in tests * refactor: move txId check, comments * test: update for changes from #3204 * test: benchmark for SRC exclusivity * test: benchmark for DST exclusivity * fix: decode into BridgeTransactionV2 Note: technically this isn't necessary AS OF NOW, as V2 struct new fields are ignored by the V1 decoding func. * test: coverage for V1, V2 encoding * test: coverage for using V1 request instead of V2 * refactor: unroll the nested v2 structure * test: update for the unrolled struct * refactor: make backwards-compatible view external * chore: `foundryup` -> `forge fmt` yes, this is slightly annoying :( * refactor: rename event * fix: post-merge getBridgeTransaction -> getBridgeTransactionV2 * refactor: move public `bridge()`, named vars * test: use `quoteRelayer` as exclusivity flag * fix: always use `quoteExclusivitySeconds` as offset in `bridge()` * fix: don't check `exclusivityEndTime` on relays when `exclusivityRelayer` is not set * test: add cases for negative `quoteExclusivitySeconds` * test: enforce `0 < exclusivityEndTime <= deadline` * feat: negative `quoteExclusivitySeconds`
Description
Introduces a separate set of tests meant for accurate average gas estimation.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests