fix(contracts-rfq): gas bench tests for the views [SLT-275] #3217
Conversation
WalkthroughA new Solidity contract named 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
|
ChiTimesChi
changed the title
Changes to gas cost
🧾 Summary (50% most significant diffs)
Full diff report 👇
|
Deploying sanguine-fe with
|
| Latest commit: |
0145376
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://87568a21.sanguine-fe.pages.dev |
| Branch Preview URL: | https://fix-fast-bridge-gas-bench-vi.sanguine-fe.pages.dev |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3217 +/- ##
===================================================
+ Coverage 24.92918% 28.14426% +3.21508%
===================================================
Files 198 184 -14
Lines 13061 12006 -1055
Branches 82 150 +68
===================================================
+ Hits 3256 3379 +123
+ Misses 9523 8346 -1177
+ Partials 282 281 -1
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: 0
🧹 Outside diff range and nitpick comments (4)
packages/contracts-rfq/test/FastBridgeV2.GasBench.Encoding.t.sol (3)
1-6: LGTM: File structure and imports are well-organized.The file structure follows Solidity best practices with proper SPDX license identifier and up-to-date pragma version. The import statement is relevant for the test contract.
Consider adding a brief comment explaining why specific solhint rules are disabled for this file.
15-18: LGTM:test_getBridgeTransactionV2function is well-implemented.The function is correctly set up for gas benchmarking with the
viewmodifier. It encodes the transaction data directly and calls the V2 method on thefastBridgeinstance.Note that this function doesn't use the
extractV1function unliketest_getBridgeTransaction. This difference might be intentional, but it's worth confirming if this is the expected behavior for V2.
8-8: Consider creating a GitHub issue for the TODO item.The TODO comment indicates a plan for future tests. While it's good to note this, it might be more effective to track such tasks in your issue tracking system.
Would you like me to create a GitHub issue to track the task of adding more tests with variable length requests once arbitrary call functionality is implemented?
packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol (1)
Line range hint
1-268: Summary: Excellent improvements to claim test functionsThe changes in this file demonstrate a systematic and consistent approach to improving the test suite for FastBridgeV2 source chain functions. Key improvements include:
- Introduction of
txIdvariables to reduce redundantgetTxIdcalls.- Addition of
canClaimpre-condition checks to enhance test robustness.- Improved code readability through the use of named transaction ID variables.
These changes were consistently applied across all claim test functions for both token and ETH transactions, resulting in a more efficient, readable, and maintainable test suite.
Suggestion for future improvements:
Consider applying similar optimizations to other test functions in this file, such as theprove,dispute, andrefundtests, to maintain consistency and further enhance the overall quality of the test suite.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Encoding.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol (2 hunks)
🔇 Additional comments (6)
packages/contracts-rfq/test/FastBridgeV2.GasBench.Encoding.t.sol (2)
7-7: LGTM: Contract declaration and inheritance are appropriate.The contract name
FastBridgeV2GasBenchmarkSrcProtocolFeesTestis descriptive and follows naming conventions. Inheriting fromFastBridgeV2SrcBaseTestis a good practice for organizing test contracts.
10-13: LGTM:test_getBridgeTransactionfunction looks good.The function is correctly set up for gas benchmarking with the
viewmodifier. It encodes the transaction data and calls the relevant method on thefastBridgeinstance.Could you provide more information about the
extractV1function? It would be helpful to understand its purpose and implementation.packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol (4)
138-142: Excellent improvements to thetest_claim_tokenfunction!The changes enhance the code quality by:
- Introducing a
txIdvariable, which reduces redundant calls togetTxId.- Adding a pre-condition check with
canClaim, improving the test's robustness.- Improving readability by using a named variable for the transaction ID.
These modifications align well with best practices and make the test more efficient and easier to understand.
148-152: Great consistency in improvements fortest_claimWithAddress_token!The changes mirror those made in
test_claim_token, bringing the same benefits:
- Reduced redundant calls to
getTxIdby introducing atxIdvariable.- Added a pre-condition check with
canClaim, enhancing test robustness.- Improved readability by using a named variable for the transaction ID.
This consistent approach across similar functions is commendable and improves the overall quality of the test suite.
224-228: Excellent consistency in improvements for ETH claims intest_claim_eth!The changes are consistent with those made in the token claim functions, demonstrating a systematic approach to improving the test suite:
- Introduction of a
txIdvariable to reduce redundantgetTxIdcalls.- Addition of a
canClaimpre-condition check, enhancing test robustness.- Improved readability through the use of a named transaction ID variable.
This consistency across different asset types (tokens and ETH) is praiseworthy and contributes to a more maintainable and efficient test suite.
234-238: Commendable consistency maintained intest_claimWithAddress_eth!The changes in this function complete a consistent pattern of improvements across all claim functions (both token and ETH):
- Introduction of a
txIdvariable to eliminate redundantgetTxIdcalls.- Addition of a
canClaimpre-condition check, further enhancing test robustness.- Improved code readability through the use of a named transaction ID variable.
This systematic approach to improving all claim functions demonstrates a thorough and well-thought-out refactoring process, contributing to a more efficient, readable, and maintainable test suite.
Bundle ReportBundle size has no change ✅ |
Description
A clear and concise description of the features you're adding in this pull request.
Additional context
Add any other context about the problem you're solving.
Metadata
Summary by CodeRabbit
New Features
Bug Fixes