no empty sender recip [SLT-184] #3171
Conversation
WalkthroughThe changes in this pull request enhance the input validation within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
💤 Files not reviewed due to no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
![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/contracts/FastBridgeV2.sol (1)
82-87: Approve changes with a minor suggestion for readabilityThe added checks for zero addresses in
params.senderandparams.toalign well with the PR objectives and improve the contract's security. This change prevents transactions with invalid addresses, which is a good practice.For improved readability, consider using a helper function for zero address checks. This can make the code more concise and easier to maintain. Here's a suggestion:
function _isZeroAddress(address addr) private pure returns (bool) { return addr == address(0); } // In the bridge function: if ( _isZeroAddress(params.originToken) || _isZeroAddress(params.destToken) || _isZeroAddress(params.sender) || _isZeroAddress(params.to) ) revert ZeroAddress();This change is optional and up to your team's coding standards and preferences.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (0 hunks)
Files not reviewed due to no reviewable changes (1)
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol
Additional comments not posted (2)
packages/contracts-rfq/contracts/FastBridgeV2.sol (2)
Line range hint
1-391: Summary of changes and impactThe changes in this PR successfully address the objective of preventing empty sender and recipient addresses in the
bridgefunction. This improvement enhances the security and robustness of the contract by ensuring that all critical address parameters are valid before proceeding with the bridge operation.Key points:
- The implementation is correct and consistent with the existing code style.
- The changes are localized to the
bridgefunction and do not introduce breaking changes.- The additional checks may slightly increase gas costs, but this is offset by the security benefits.
The changes are approved and ready for merging, pending the verification of gas costs and any documentation updates as suggested in previous comments.
82-87: Verify gas cost impact and update documentationThe additional checks for zero addresses slightly increase the gas cost of the
bridgefunction. While this increase is likely negligible, it's good practice to verify and document it.Run the following script to compare gas costs before and after the change:
Consider updating the contract's documentation or comments to reflect this change in behavior. This could include:
- Adding a comment above the
bridgefunction explaining the zero address checks.- Updating any external documentation or specifications that describe the
bridgefunction's behavior.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3171 +/- ##
===================================================
- Coverage 40.97804% 38.42159% -2.55645%
===================================================
Files 459 424 -35
Lines 25643 24455 -1188
Branches 343 148 -195
===================================================
- Hits 10508 9396 -1112
+ Misses 14383 14318 -65
+ Partials 752 741 -11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
github-actions
bot
added
needs-go-generate-agents
needs-go-generate-services/rfq
labels
| params.destToken == address(0) || | ||
| params.sender == address(0) || | ||
| params.to == address(0) | ||
| ) revert ZeroAddress(); |
There was a problem hiding this comment.
Does two independent checks make more sense to you here? If not, this needs a // forgefmt: disable-next-item before this statement to avoid being formatted in a very ugly way (try doing forge fmt and the result is much less readable than what it is now).
There was a problem hiding this comment.
makes sense to me for readability with forge fmt in mind, but not logical sense :)
i went ahead and made it 2 lines. Also confirmed via forge tests that it only increases gas by a couple units
Deploying sanguine-fe with
|
| Latest commit: |
0b2fcc6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f0145e09.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-fbv2-noemptysenderrecip.sanguine-fe.pages.dev |
Bundle ReportChanges will increase total bundle size by 251.65kB (0.7%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
Changes to gas cost
🧾 Summary (50% most significant diffs)
Full diff report 👇
|
github-actions
bot
added
needs-go-generate-agents
needs-go-generate-services/rfq
labels
Description
Don’t allow empty sender/recipient addresses on bridge()
Summary by CodeRabbit
New Features
Bug Fixes