Fix: FastBridgeV2 (Zellic - 01)
#3493
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Sequence DiagramsequenceDiagram
participant User
participant FastBridgeV2
participant Token
participant ProtocolFee
User->>FastBridgeV2: bridgeV2(params)
FastBridgeV2->>FastBridgeV2: Validate bridge parameters
FastBridgeV2->>Token: Transfer origin amount
FastBridgeV2->>ProtocolFee: Calculate and deduct protocol fee
FastBridgeV2->>FastBridgeV2: Emit bridge event
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used📓 Learnings (1)packages/contracts-rfq/contracts/FastBridgeV2.sol (1)⏰ Context from checks skipped due to timeout of 90000ms (17)
🔇 Additional comments (7)
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
|
Deploying sanguine-fe with
|
| Latest commit: |
678f978
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://57c06e0f.sanguine-fe.pages.dev |
| Branch Preview URL: | https://fix-fbv2-zellic-01.sanguine-fe.pages.dev |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3493 +/- ##
===================================================
- Coverage 98.89241% 93.76125% -5.13116%
===================================================
Files 11 65 +54
Lines 632 1667 +1035
Branches 0 82 +82
===================================================
+ Hits 625 1563 +938
- Misses 7 102 +95
- Partials 0 2 +2
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
🧹 Nitpick comments (3)
packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
491-492: Redundant local references
These lines duplicate existing references toparams.originTokenandparams.originAmountwithin the same function. Consider directly usingparamsto avoid increasing local variable count, which can trigger stack-depth issues.- address originToken = params.originToken; - uint256 originAmount = params.originAmount; + // Access params.originToken and params.originAmount directly belowpackages/contracts-rfq/test/mocks/WeirdERC20Mock.sol (1)
16-18: Set FastBridge address
Providing a setter forfastBridgeis convenient for test scenarios. If this setter is used only for testing, consider restricting it to specialized roles or test environments.packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol (1)
56-59: Consider minting weirdToken for both userA and userB.
Currently, only userA receives weirdToken, whereas both userA and userB receive srcToken. If future tests require userB bridging with weirdToken, consider minting it for userB as well for symmetry.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/contracts-rfq/contracts/FastBridgeV2.sol(7 hunks)packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol(1 hunks)packages/contracts-rfq/test/FastBridgeV2.Src.FBRV2.t.sol(1 hunks)packages/contracts-rfq/test/FastBridgeV2.Src.t.sol(1 hunks)packages/contracts-rfq/test/FastBridgeV2.t.sol(4 hunks)packages/contracts-rfq/test/mocks/WeirdERC20Mock.sol(1 hunks)
🧰 Additional context used
📓 Learnings (4)
packages/contracts-rfq/test/FastBridgeV2.Src.FBRV2.t.sol (1)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.t.sol:53-56
Timestamp: 2024-11-12T03:37:02.230Z
Learning: Modifying test utility contracts in the `test` directory is unnecessary and would only pollute the code.
packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (3)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.Src.t.sol:919-993
Timestamp: 2024-11-12T03:37:02.230Z
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.
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol:97-97
Timestamp: 2024-11-12T03:37:08.148Z
Learning: In `packages/contracts-rfq/test/FastBridgeV2.t.sol`, when testing expected revert behaviors, `abi.encodeWithSelector` is used with custom errors and should not be replaced with `BridgeTransactionV2Lib.encodeV2`.
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.t.sol:53-56
Timestamp: 2024-11-12T03:37:02.230Z
Learning: Modifying test utility contracts in the `test` directory is unnecessary and would only pollute the code.
packages/contracts-rfq/test/FastBridgeV2.t.sol (1)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.t.sol:53-56
Timestamp: 2024-11-12T03:37:02.230Z
Learning: Modifying test utility contracts in the `test` directory is unnecessary and would only pollute the code.
packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118
Timestamp: 2024-11-12T03:37:08.148Z
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.
🪛 GitHub Actions: Solidity
packages/contracts-rfq/contracts/FastBridgeV2.sol
[error] 239-239: Stack too deep error. Compiler suggests using --via-ir flag or enabling viaIR: true in JSON settings, or reducing local variables.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Slither (contracts-rfq)
- GitHub Check: Foundry Size Check (contracts-rfq)
- GitHub Check: Foundry Gas Diff (contracts-rfq)
- GitHub Check: Deploy Docs (contracts-rfq)
- GitHub Check: Foundry Coverage (contracts-rfq)
- GitHub Check: Go Generate (Solidity Only) (agents)
- GitHub Check: test
- GitHub Check: SonarCloud
- GitHub Check: lint
- GitHub Check: changesets-integrity-checker
🔇 Additional comments (19)
packages/contracts-rfq/contracts/FastBridgeV2.sol (11)
192-194: Initialize local variables neatly
DefiningoriginToken,originAmount, and initializingprotocolFeeAmountto zero provides a clear setup for fee calculations and token references.
196-196: Check for potential overflow in fee calculation
Although Solidity 0.8+ automatically checks for overflow/underflow, consider validating extreme edge cases (e.g., largeoriginAmountorprotocolFeeRate) to ensure the calculation remains safe.
199-199: Subtracting fee from the user’s amount
SubtractingprotocolFeeAmountfromoriginAmountis correct. Ensure subsequent references tooriginAmountreflect the updated value.
209-209: Reference to local variable
PassingoriginTokendirectly to the struct is consistent with the new approach of calculating fees beforehand.
213-213: Renamed fee variable
Storing computed fee inoriginFeeAmountclarifies that this fee was calculated at origin. This aligns well with the new naming (protocolFeeAmount).
229-229: Emit events first
Emitting events before external calls follows best practices to mitigate risk in the event of reentrancy.
242-251: Prevent use of fee-on-transfer or rebasing tokens
This logic checks if the balance increase matches the intended transfer, reverting if not. It effectively excludes fee-on-transfer or rebasing tokens from usage.
410-411: Accumulate protocol fees
AddingprotocolFeeAmountintoprotocolFees[token]ensures correct fee accounting. Confirm that the rest of the system correctly handles and tracks these accumulated fees.
495-495: Zero amounts
Reverting if either amount is zero eliminates no-op or accidental bridging. This is a valid guard check.
497-497: Zero addresses
Reverting for zero addresses ensures invalid bridging addresses and tokens do not slip through.
510-520: Distinct handling of native gas vs. ERC20
This block fully covers ensuring the correctmsg.valuefor native tokens and rejecting non-zero Ether with ERC20 usage. It also rejects non-contract tokens.packages/contracts-rfq/test/mocks/WeirdERC20Mock.sol (4)
7-12: Contract initialization
Appending a custom constructor to theMockERC20base is straightforward. Ensure thedecimals_argument matches test expectations.
20-22: Set user-defined transfer amount
The ability to define a customtransferToFastBridgeValuehelps test weird transfer behaviors. Just confirm you reset or control it in each test to avoid test interference.
24-30: Overridetransfer
IftoisfastBridge, a custom transfer amount is used instead of the originalvalue. This approach effectively simulates tokens with unusual fee or rebasing mechanics.
32-38: OverridetransferFrom
Same logic astransferoverride, ensuring consistent “weird” behavior from both direct and allowance-based transfers.packages/contracts-rfq/test/FastBridgeV2.Src.FBRV2.t.sol (1)
22-24: Extend setup for WeirdERC20
Approvingrouterand minting torouterforweirdTokenensures that tests exercise logic involving custom token behavior. Verify test coverage for edge cases concerning weird transfers.packages/contracts-rfq/test/FastBridgeV2.t.sol (1)
16-16: The new weirdToken references look consistent.
All newly introduced import statements, variable declarations, and initializations forWeirdERC20Mockseamlessly integrate into the test scaffolding. ThesetFastBridgecall is a good addition and ensures the mock token can simulate bridging behavior accurately. No immediate issues are observed.Also applies to: 34-34, 81-81, 90-90
packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (2)
297-302: Test ensures correct revert when actual transfer exceeds originAmount.
This thorough check confirms that bridging fails if the token transfer surpasses the definedoriginAmount. Great test coverage.
304-309: Test ensures correct revert when actual transfer is below originAmount.
This scenario confirms that bridging fails if the token transfer is less than theoriginAmount. Solid negative test coverage.
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
Description
This PR fixes removes support for rebasing tokens or tokens with fees on transfers from
FastBridgeV2.FastBridgeV2balance change, the user-provided amount is used.originFeeAmountvariables were renamed intoprotocolFeeAmountto better reflect their meaning.Summary by CodeRabbit
Release Notes
New Features
WeirdERC20Mock.Testing
weirdToken.Improvements