test(contracts-rfq): gas benchmark for the arbitrary call [SLT-233] #3273
Conversation
WalkthroughThis pull request introduces several changes across multiple Solidity test files related to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3273 +/- ##
===================================================
+ Coverage 30.68788% 32.29410% +1.60622%
===================================================
Files 447 433 -14
Lines 29787 28739 -1048
Branches 82 155 +73
===================================================
+ Hits 9141 9281 +140
+ Misses 19797 18612 -1185
+ Partials 849 846 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Deploying sanguine-fe with
|
| Latest commit: |
c4393fc
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://03d6c40a.sanguine-fe.pages.dev |
| Branch Preview URL: | https://test-fbv2-arbitrary-call-gas.sanguine-fe.pages.dev |
Changes to gas cost
🧾 Summary (50% most significant diffs)
Full diff report 👇
|
ChiTimesChi
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (11)
packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.Excl.t.sol (1)
16-18: LGTM! Consider adding documentation for new function calls.The changes to
createFixturesV2improve the test setup by ensuring the parent class's implementation is called and adding specific exclusivity configurations for tokens and ETH. This aligns well with the contract's purpose of testing exclusivity.Consider adding brief comments explaining the purpose of
setTokenTestExclusivityParamsandsetEthTestExclusivityParamsfor improved readability. For example:// Set exclusivity parameters for token tests setTokenTestExclusivityParams(relayerA, EXCLUSIVITY_PERIOD); // Set exclusivity parameters for ETH tests setEthTestExclusivityParams(relayerA, EXCLUSIVITY_PERIOD);packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.ArbitraryCall.t.sol (3)
6-7: LGTM: Appropriate contract declaration and linter configuration.The contract declaration and inheritance are correct. The solhint-disable comment is appropriate for a test file.
Consider moving the solhint-disable comment to the top of the file for better visibility and to apply to the entire file.
8-17: LGTM: Well-implemented override with comprehensive setup for arbitrary call testing.The
createFixturesV2()function correctly overrides the parent implementation and extends it to include arbitrary call parameters. This aligns well with the PR objective of implementing gas benchmarks for arbitrary calls.Consider adding a brief comment explaining the purpose of the
mockCallParamsand why it's being set for various transactions. This would improve code readability and maintainability.
1-18: Overall assessment: Well-implemented test contract for gas benchmarking of arbitrary calls.This new test contract effectively extends the existing gas benchmark tests to include arbitrary calls, aligning perfectly with the PR objectives. The code is well-structured, follows good practices, and achieves its intended purpose of enhancing the testing framework for FastBridgeV2.
To further improve this test suite:
- Consider adding specific test cases that utilize these arbitrary call parameters to ensure they're being correctly applied in the actual benchmark tests.
- If not already present in the parent class, consider adding assertions or checks to verify that the gas costs for arbitrary calls are within expected ranges.
packages/contracts-rfq/test/FastBridgeV2.GasBench.Encoding.t.sol (1)
18-21: New test function enhances coverage for arbitrary calls.The addition of
test_getBridgeTransactionV2_withArbitraryCall()improves test coverage by including a scenario with arbitrary call data. This aligns well with the PR objectives of implementing gas benchmarks for various bridge operations, including arbitrary calls.Consider adding a comment explaining the purpose of this test function and the significance of the arbitrary call parameters used.
function test_getBridgeTransactionV2_withArbitraryCall() public { + // Test getBridgeTransactionV2 with arbitrary call data to simulate real-world scenarios setTokenTestCallParams({callParams: abi.encode(userA, keccak256("Random ID"))}); test_getBridgeTransactionV2(); }packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.ArbitraryCall.t.sol (2)
15-20: LGTM: Well-implemented setUp function with clear setup steps.The
setUpfunction is well-structured and correctly overrides the parent function. It properly initializes the test environment by creating a newRecipientMockcontract and labeling it for improved readability.Consider adding a brief comment explaining why
userBis always used as the recipient of tokens in the inherited tests. This would provide more context for future developers working on this code.
22-27: LGTM: Well-implemented createFixturesV2 function with comprehensive setup.The
createFixturesV2function is well-structured and correctly overrides the parent function. It properly sets up mock call parameters for both token and ETH tests, simulating real-world scenarios.Consider extracting the creation of mock call parameters into a separate private function for improved readability and potential reuse. For example:
function createFixturesV2() public virtual override { super.createFixturesV2(); bytes memory mockCallParams = createMockCallParams(); setTokenTestCallParams(mockCallParams); setEthTestCallParams(mockCallParams); } function createMockCallParams() private view returns (bytes memory) { return abi.encode(userA, keccak256("Random ID")); }This refactoring would make the
createFixturesV2function more concise and easier to understand at a glance.packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.PFees.t.sol (1)
30-34: LGTM! Consider adding more context to the comment.The changes to the nonce values are well-structured and the comment provides useful context. Good job on maintaining code clarity.
To further improve clarity, consider expanding the comment to briefly explain why starting from 1 is important. For example:
- // See FastBridgeV2GasBenchmarkSrcTest.initExistingTxs for why these start from 1, not 0 + // Start nonces from 1 to align with FastBridgeV2GasBenchmarkSrcTest.initExistingTxs + // This ensures consistent behavior with real-world scenarios where nonce 0 is typically reservedThis provides more immediate context without requiring the reader to navigate to another file.
packages/contracts-rfq/test/FastBridgeV2.t.sol (1)
235-237: LGTM! Consider adding a brief comment for clarity.The
cheatSenderNoncefunction is a valuable addition to the testing framework, allowing precise control over sender nonces. This aligns well with the PR objective of addressing the "maximum" value issue for bridge gas benchmarks.Consider adding a brief comment explaining the purpose of this function:
+ /// @dev Allows direct manipulation of sender nonces for testing purposes function cheatSenderNonce(address sender, uint256 nonce) public { stdstore.target(address(fastBridge)).sig("senderNonces(address)").with_key(sender).checked_write(nonce); }packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol (2)
103-107: Consider changing visibility ofcheckAfterBridgeToken()tointernalSince
checkAfterBridgeToken()is only called within the contract, changing its visibility tointernalrestricts access appropriately and aligns with Solidity best practices.Apply this diff to change the visibility:
-function checkAfterBridgeToken() public view { +function checkAfterBridgeToken() internal view {
186-190: Modify visibility ofcheckAfterBridgeEth()tointernalSimilar to
checkAfterBridgeToken(), changing the visibility ofcheckAfterBridgeEth()tointernalis advisable since it's only used internally within the contract.Apply this diff to change the visibility:
-function checkAfterBridgeEth() public view { +function checkAfterBridgeEth() internal view {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.ArbitraryCall.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.Excl.t.sol (2 hunks)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Encoding.t.sol (2 hunks)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.ArbitraryCall.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 (4 hunks)
- packages/contracts-rfq/test/FastBridgeV2.t.sol (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.t.sol
🧰 Additional context used
🔇 Additional comments (15)
packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.Excl.t.sol (2)
7-7: LGTM! Contract name updated for consistency.The contract name change to
FastBridgeV2GasBenchmarkDstExclusivityTestis consistent with the import statement update and follows a clear naming convention. This improvement enhances code readability and maintains consistency within the testing framework.
4-4: LGTM! Verify the import change across the codebase.The updated import statement reflects a more descriptive and consistent naming convention for the base test contract. This change aligns well with the PR objectives of enhancing the testing framework.
To ensure consistency, let's verify this change across the codebase:
✅ Verification successful
Import verification successful!
All occurrences of the old contract name have been removed, and the new contract name is consistently used across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the updated import statement and contract name usage. # Test 1: Check for any remaining occurrences of the old contract name echo "Checking for old contract name usage:" rg "FastBridgeV2DstGasBenchmarkTest" --type solidity # Test 2: Verify the usage of the new contract name echo "Verifying new contract name usage:" rg "FastBridgeV2GasBenchmarkDstTest" --type solidityLength of output: 335
Script:
#!/bin/bash # Description: Verify the updated import statement and contract name usage by searching Solidity files. # Test 1: Check for any remaining occurrences of the old contract name echo "Checking for old contract name usage:" rg "FastBridgeV2DstGasBenchmarkTest" -g "*.sol" # Test 2: Verify the usage of the new contract name echo "Verifying new contract name usage:" rg "FastBridgeV2GasBenchmarkDstTest" -g "*.sol"Length of output: 1013
packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.ArbitraryCall.t.sol (2)
1-2: LGTM: Appropriate license and Solidity version.The file correctly starts with the MIT license identifier and uses an up-to-date Solidity version (^0.8.20).
4-4: LGTM: Correct import statement.The import statement correctly imports the parent contract
FastBridgeV2GasBenchmarkSrcTestfrom the relative path.packages/contracts-rfq/test/FastBridgeV2.GasBench.Encoding.t.sol (2)
7-7: Improved contract naming for better clarity.The contract name change from
FastBridgeV2GasBenchmarkSrcProtocolFeesTesttoFastBridgeV2GasBenchmarkEncodingTestbetter reflects the purpose of the contract, focusing on encoding benchmarks. This improvement enhances code readability and maintainability.
Line range hint
8-16: Existing test functions provide a solid baseline.The unchanged functions
test_getBridgeTransaction()andtest_getBridgeTransactionV2()continue to provide essential baseline tests for thegetBridgeTransactionandgetBridgeTransactionV2methods. Their preservation maintains consistency in the testing suite, while the newly added function complements them by covering the specific case of arbitrary calls.packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.ArbitraryCall.t.sol (1)
1-14: LGTM: Well-structured contract declaration with clear purpose.The contract declaration and inheritance are well-defined. The comment provides valuable context about the purpose of using a mock recipient for arbitrary call testing, which is helpful for understanding the test setup.
packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol (8)
41-48: Nonce initialization is clear and well-documentedSetting the nonces explicitly and documenting the reasoning enhances readability and ensures the correct sequencing of transactions for the gas benchmarks.
71-77: Efficient initialization of existing transactionsUsing
cheatSenderNonce(userA, 1)to set the initial nonce avoids inflated gas costs due to storage writes from zero. The subsequent bridge transactions correctly simulate prior activity.
103-107: Good practice by abstracting assertions into helper functioncheckAfterBridgeToken()Extracting repeated assertions into
checkAfterBridgeToken()improves code maintainability and reduces duplication in test cases.
109-112: Refactoredtest_bridge_token()enhances test clarityBy calling the helper function
checkAfterBridgeToken(), the test function becomes more concise and focused on the action rather than the assertions.
117-117: Ensure consistency in using helper functions across testsIn
test_bridge_token_withExclusivity(), correctly usingcheckAfterBridgeToken()maintains consistency and reduces code duplication.
186-190: Effective use ofcheckAfterBridgeEth()for assertionsCreating
checkAfterBridgeEth()to handle repeated assertions enhances code reuse and readability in ETH bridge tests.
192-195: Simplifiedtest_bridge_eth()improves test readabilityRefactoring to use
checkAfterBridgeEth()streamlines the test function and emphasizes the primary action being tested.
200-200: Consistency in test assertions with helper functionsUsing
checkAfterBridgeEth()intest_bridge_eth_withExclusivity()maintains consistent testing patterns across different scenarios.
Description
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests