solhint warning fixes [SLT-287] #3207
Conversation
WalkthroughThe 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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3207 +/- ##
====================================================
+ Coverage 40.97804% 90.66774% +49.68969%
====================================================
Files 459 60 -399
Lines 25643 1243 -24400
Branches 343 148 -195
====================================================
- Hits 10508 1127 -9381
+ Misses 14383 112 -14271
+ Partials 752 4 -748
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: |
95daba4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://614fe45d.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-fbv2-solhint-warnings-6nc6.sanguine-fe.pages.dev |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| function dispute(bytes32 transactionId) external onlyRole(GUARD_ROLE) { | ||
| if (bridgeTxDetails[transactionId].status != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect(); | ||
| if (_timeSince(bridgeTxDetails[transactionId].proofBlockTimestamp) > DISPUTE_PERIOD) { | ||
| revert DisputePeriodPassed(); | ||
| } | ||
|
|
||
| // @dev relayer gets slashed effectively if dest relay has gone thru | ||
| bridgeTxDetails[transactionId].status = BridgeStatus.REQUESTED; | ||
| bridgeTxDetails[transactionId].proofRelayer = address(0); | ||
| bridgeTxDetails[transactionId].proofBlockTimestamp = 0; | ||
| bridgeTxDetails[transactionId].proofBlockNumber = 0; | ||
|
|
||
| emit BridgeProofDisputed(transactionId, msg.sender); | ||
| } |
Check notice
Code scanning / Slither
Block timestamp Low
| function refund(bytes memory request) external { | ||
| bytes32 transactionId = keccak256(request); | ||
|
|
||
| BridgeTransaction memory transaction = getBridgeTransaction(request); | ||
|
|
||
| if (bridgeTxDetails[transactionId].status != BridgeStatus.REQUESTED) revert StatusIncorrect(); | ||
|
|
||
| if (hasRole(REFUNDER_ROLE, msg.sender)) { | ||
| // Refunder can refund if deadline has passed | ||
| if (block.timestamp <= transaction.deadline) revert DeadlineNotExceeded(); | ||
| } else { | ||
| // Permissionless refund is allowed after REFUND_DELAY | ||
| if (block.timestamp <= transaction.deadline + REFUND_DELAY) revert DeadlineNotExceeded(); | ||
| } | ||
|
|
||
| // if all checks passed, set to REFUNDED status | ||
| bridgeTxDetails[transactionId].status = BridgeStatus.REFUNDED; | ||
|
|
||
| // transfer origin collateral back to original sender | ||
| address to = transaction.originSender; | ||
| address token = transaction.originToken; | ||
| uint256 amount = transaction.originAmount + transaction.originFeeAmount; | ||
| token.universalTransfer(to, amount); | ||
|
|
||
| emit BridgeDepositRefunded(transactionId, to, token, amount); | ||
| } |
Check notice
Code scanning / Slither
Reentrancy vulnerabilities Low
| function refund(bytes memory request) external { | ||
| bytes32 transactionId = keccak256(request); | ||
|
|
||
| BridgeTransaction memory transaction = getBridgeTransaction(request); | ||
|
|
||
| if (bridgeTxDetails[transactionId].status != BridgeStatus.REQUESTED) revert StatusIncorrect(); | ||
|
|
||
| if (hasRole(REFUNDER_ROLE, msg.sender)) { | ||
| // Refunder can refund if deadline has passed | ||
| if (block.timestamp <= transaction.deadline) revert DeadlineNotExceeded(); | ||
| } else { | ||
| // Permissionless refund is allowed after REFUND_DELAY | ||
| if (block.timestamp <= transaction.deadline + REFUND_DELAY) revert DeadlineNotExceeded(); | ||
| } | ||
|
|
||
| // if all checks passed, set to REFUNDED status | ||
| bridgeTxDetails[transactionId].status = BridgeStatus.REFUNDED; | ||
|
|
||
| // transfer origin collateral back to original sender | ||
| address to = transaction.originSender; | ||
| address token = transaction.originToken; | ||
| uint256 amount = transaction.originAmount + transaction.originFeeAmount; | ||
| token.universalTransfer(to, amount); | ||
|
|
||
| emit BridgeDepositRefunded(transactionId, to, token, amount); | ||
| } |
Check notice
Code scanning / Slither
Block timestamp Low
| function canClaim(bytes32 transactionId, address relayer) external view returns (bool) { | ||
| if (bridgeTxDetails[transactionId].status != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect(); | ||
| if (bridgeTxDetails[transactionId].proofRelayer != relayer) revert SenderIncorrect(); | ||
| return _timeSince(bridgeTxDetails[transactionId].proofBlockTimestamp) > DISPUTE_PERIOD; | ||
| } |
Check notice
Code scanning / Slither
Block timestamp Low
![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 (1)
packages/contracts-rfq/test/FastBridgeV2.t.sol (1)
6-6: Approved: solhint disable comment is correctly added.The solhint disable comment for 'no-unused-import' is properly formatted and aligns with the PR objectives. It addresses the "not used" warning for FastBridgeV2 in the tests.
Consider adding an explanatory comment for future maintainers, explaining why this import is necessary despite appearing unused. For example:
// solhint-disable-next-line no-unused-import // This import is required for forge tests, removing it causes errors import {IFastBridgeV2} from "../contracts/interfaces/IFastBridgeV2.sol";
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (2 hunks)
- packages/contracts-rfq/test/FastBridgeV2.t.sol (1 hunks)
🔇 Additional comments (4)
packages/contracts-rfq/test/FastBridgeV2.t.sol (2)
5-5: LGTM: New import statement is correctly added.The import statement for the
IFastBridgeV2interface is properly formatted and imported from the correct relative path. This import is necessary for the contract's functionality.
Line range hint
1-146: Summary: Changes align well with PR objectives.The modifications in this file effectively address the solhint warnings related to the Fastbridge v2 canonical ordering. The new import statement for
IFastBridgeV2and the associated solhint disable comment are correctly implemented and justified.These changes align perfectly with the PR objectives, which aimed to fix solhint warnings and handle the "not used" warning for FastBridgeV2 in the tests. The author's decision to keep the import despite the warning is well-explained and necessary for the proper functioning of the forge tests.
packages/contracts-rfq/contracts/FastBridgeV2.sol (2)
98-101: Functionprovecorrectly forwards the callThe
provefunction correctly computes thetransactionIdfrom therequestand forwards the call to the overloadedprovefunction withmsg.senderas the relayer.
104-106: Functionclaimcorrectly delegates with default parameterThe
claimfunction properly delegates to the internalclaimfunction, usingaddress(0)as the defaulttoaddress, allowing for permissionless claims by defaulting to the relayer.
| bytes32 transactionId = keccak256(request); | ||
| prove(transactionId, destTxHash, msg.sender); |
There was a problem hiding this comment.
Let's use named variables syntax here (and in relay/claim) for easier maintainability. This wasn't really required when the overload function was literally a few lines away, but with the function ordering enforced that would be helpful:
| bytes32 transactionId = keccak256(request); | |
| prove(transactionId, destTxHash, msg.sender); | |
| prove({transactionId: keccak256(request), destTxHash: destTxHash, relayer: msg.sender}); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/contracts-rfq/contracts/FastBridgeV2.sol (3)
95-101: Approved: Improved relay and prove functionsThe modifications to
relayandprovefunctions enhance flexibility and improve relayer handling. The use of named parameters in function calls increases readability.For consistency, consider using named parameters in the
relayfunction call as well:- relay({request: request, relayer: msg.sender}); + relay({request: request, relayer: msg.sender});
108-122: Approved: New dispute function with proper checksThe
disputefunction is a valuable addition that allows authorized guards to challenge potentially fraudulent transactions. The implementation includes appropriate status checks and a time-based dispute window.However, the comment on line 115 needs clarification:
- // @dev relayer gets slashed effectively if dest relay has gone thru + // @dev relayer loses their claim to the bridged funds if the destination relay has already been executedThis change more accurately describes the consequence for the relayer, as there's no explicit slashing mechanism in the current implementation.
124-150: Approved: Comprehensive refund function with role-based checksThe
refundfunction is well-implemented with appropriate checks for transaction status, roles, and deadlines. It provides a mechanism for both authorized and permissionless refunds after different time periods.Consider a minor optimization:
- uint256 amount = transaction.originAmount + transaction.originFeeAmount; - token.universalTransfer(to, amount); + token.universalTransfer(to, transaction.originAmount + transaction.originFeeAmount);This change reduces the number of local variables and slightly improves gas efficiency.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (3 hunks)
🔇 Additional comments (5)
packages/contracts-rfq/contracts/FastBridgeV2.sol (5)
34-34: Approved: Addition of nonce variable for replay protectionThe introduction of a public
noncevariable is a good practice for preventing replay attacks in bridge transactions. This ensures that each transaction has a unique identifier.
103-106: Approved: New claim function for permissionless claimsThe addition of this
claimfunction enhances the contract's functionality by allowing permissionless claims. This aligns with the improvements mentioned in the PR summary and increases the flexibility of the bridging process.
152-156: Approved: Efficient canClaim function for claim eligibility checksThe
canClaimfunction is a well-implemented view function that efficiently checks if a transaction can be claimed by a specific relayer. It performs the necessary status and relayer checks, and correctly uses the_timeSincefunction to verify if the dispute period has passed.
251-258: Approved: Useful getter functions for bridge transaction detailsThe addition of
bridgeStatusesandbridgeProofsfunctions provides a convenient way to query important details about bridge transactions. These functions enhance the contract's transparency and allow external contracts or off-chain systems to easily access transaction statuses and proof information.
260-263: Approved: Efficient bridgeRelays function for relay status checksThe
bridgeRelaysfunction is a well-implemented view function that efficiently checks if a transaction has been relayed. It provides a simple boolean return, making it easy for external contracts or off-chain systems to verify the relay status of a transaction.
Changes to gas cost
🧾 Summary (50% most significant diffs)
Full diff report 👇
|
Bundle ReportChanges will increase total bundle size by 407.74kB (1.14%) ⬆️. This is within the configured threshold ✅ Detailed changes
ℹ️ *Bundle size includes cached data from a previous commit |
solhint warning fixes for Fastbridge v2 canonical ordering. Rearranged everything to the enforced order.
Had to ignore FastBridgeV2 "not used" warning on tests - removing this reference caused forge test errors because it is actually used
Summary by CodeRabbit
New Features
claim,refund, anddispute.Bug Fixes
Refactor
_pullTokenfunction for better clarity and maintainability.relayandprovefunctions to include additional validation checks.Tests
FastBridgeV2contract with structured setup for bridge transactions.