refactor(contracts-rfq): fix compiler warnings in tests #3357
Conversation
WalkthroughThe pull request includes modifications 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 #3357 +/- ##
===================================================
+ Coverage 90.44834% 92.29607% +1.84772%
===================================================
Files 54 60 +6
Lines 1026 1324 +298
Branches 82 82
===================================================
+ Hits 928 1222 +294
- Misses 95 99 +4
Partials 3 3
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 (2)
packages/contracts-rfq/test/FastBridgeMock.sol (2)
42-42: Consider using a more idiomatic approach for unused parametersWhile adding
sender;silences the compiler warning, consider prefixing the parameter name with an underscore to indicate it's intentionally unused. This is a more conventional approach in Solidity.-function mockBridgeRequest(bytes32 transactionId, address sender, BridgeParams memory params) external { - sender; +function mockBridgeRequest(bytes32 transactionId, address _sender, BridgeParams memory params) external {
77-77: Apply consistent unused parameter handlingSimilar to the previous function, consider using the underscore prefix convention for the unused sender parameter.
-function mockBridgeRequestRaw(bytes32 transactionId, address sender, bytes memory request) external { - sender; +function mockBridgeRequestRaw(bytes32 transactionId, address _sender, bytes memory request) external {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
packages/contracts-rfq/test/FastBridge.t.sol(1 hunks)packages/contracts-rfq/test/FastBridgeMock.sol(3 hunks)packages/contracts-rfq/test/integration/FastBridge.MulticallTarget.t.sol(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/contracts-rfq/test/FastBridge.t.sol (3)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.t.sol:53-56
Timestamp: 2024-10-14T13:24:45.293Z
Learning: Modifying test utility contracts in the `test` directory is unnecessary and would only pollute the code.
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118
Timestamp: 2024-10-14T14:45:57.214Z
Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.Src.t.sol:919-993
Timestamp: 2024-10-14T14:48:01.520Z
Learning: In Solidity test files for FastBridgeV2 (e.g., `packages/contracts-rfq/test/FastBridgeV2.Src.t.sol`), code duplication in test functions is acceptable to maintain readability and maintainability, even if it contradicts DRY principles.
🔇 Additional comments (3)
packages/contracts-rfq/test/integration/FastBridge.MulticallTarget.t.sol (1)
17-17: LGTM! Appropriate visibility modifier change.
The change from view to pure is correct as the function only uses abi.encode without reading any state variables. This fixes the compiler warning while maintaining the function's behavior.
packages/contracts-rfq/test/FastBridgeMock.sol (1)
110-134: LGTM! Clean interface implementation changes
The changes to remove parameter names and update function modifiers from view to pure are appropriate since:
- Parameter names are unnecessary for functions that only revert
- The
puremodifier is more accurate as these functions don't read state
These changes effectively address compiler warnings without affecting functionality.
packages/contracts-rfq/test/FastBridge.t.sol (1)
63-65: Improved variable naming for better clarity
The renaming of variables from timestamp and relayer to proofTimestamp and proofRelayer respectively makes the code more explicit about what these variables represent. This change helps prevent potential variable shadowing and improves code readability.
Deploying sanguine-fe with
|
| Latest commit: |
9a7f592
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://82015463.sanguine-fe.pages.dev |
| Branch Preview URL: | https://refactor-rfq-tests-warnings.sanguine-fe.pages.dev |
Description
See title.
Summary by CodeRabbit
Bug Fixes
FastBridgeTestcontract.pureforgetEncodedBridgeTx, ensuring it does not read from state.Refactor
FastBridgeMockby removing parameter names.These changes enhance code readability and maintainability without altering existing functionalities.