-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sdk: include rfq protocol fees #1871
Conversation
WalkthroughThe Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1871 +/- ##
===================================================
+ Coverage 51.18525% 51.19794% +0.01268%
===================================================
Files 397 397
Lines 27125 27130 +5
Branches 307 307
===================================================
+ Hits 13884 13890 +6
Misses 11888 11888
+ Partials 1353 1352 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- packages/sdk-router/src/rfq/fastBridgeRouter.test.ts (2 hunks)
- packages/sdk-router/src/rfq/fastBridgeRouter.ts (1 hunks)
- packages/sdk-router/src/rfq/fastBridgeRouterSet.test.ts (1 hunks)
- packages/sdk-router/src/rfq/fastBridgeRouterSet.ts (2 hunks)
Additional comments: 7
packages/sdk-router/src/rfq/fastBridgeRouterSet.test.ts (1)
- 105-119: The tests for the
applyProtocolFeeRate
method are correctly checking the behavior of applying 0 bps and 10 bps fee rates. The expectations are set correctly to match the amount when no fee is applied and to match the reduced amount when a 10 bps fee rate is applied.packages/sdk-router/src/rfq/fastBridgeRouter.ts (1)
- 142-148: The
getProtocolFeeRate
method has been correctly added to theFastBridgeRouter
class. It retrieves the protocol fee rate from thefastBridgeContract
asynchronously and returns it as aBigNumber
, which aligns with the PR objectives.packages/sdk-router/src/rfq/fastBridgeRouterSet.ts (2)
- 115-127: The
applyProtocolFeeRate
method is correctly integrated into thegetBridgeRoutes
method. The protocol fee rate is fetched and applied to theoriginQuery.minAmountOut
before applying the quote, which is consistent with the PR objectives to handle protocol fees.- 237-248: The
applyProtocolFeeRate
method is correctly implemented to calculate the amount after the protocol fee is applied. The method uses the correct formula to calculate the fee and subtracts it from the amount, which is consistent with the expected behavior.packages/sdk-router/src/rfq/fastBridgeRouter.test.ts (3)
- 229-293: The tests for the
getOriginAmountOut
method are correctly implemented. They mock the contract's behavior and verify that the method returns the expected values when the protocol fee rate is 0 bps.- 295-318: The tests for the
getOriginAmountOut
method with a protocol fee rate of 10 bps are correctly implemented. The mock implementation of theprotocolFeeRate
and the expectations are set up correctly to ensure that the method's behavior is as expected.- 321-329: The test for the
getProtocolFeeRate
method is correctly implemented. It mocks theprotocolFeeRate
method of thefastBridgeContractCache
and asserts that thegetProtocolFeeRate
method of theFastBridgeRouter
returns the correct value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question but otherwise lgtm
destAmountOut: applyQuote(quote, originQuery.minAmountOut), | ||
destAmountOut: applyQuote( | ||
quote, | ||
this.applyProtocolFeeRate(originQuery.minAmountOut, protocolFeeRate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied on origin amount tho so is it relevant for FE as only used by caller of claim()
which will be relayer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other consideration would be on refund()
, bridge sends back to origin sender transaction.originAmount + transaction.originFeeAmount
so protocol doesn't take a fee on unsuccessful bridges, if that affects SDK in any way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am replicating the internal logic of FastBrdige.bridge()
wrt the origin amount used for the cross-chain swap:
originAmount: originAmount, |
This should be the amount that the off-chain Relayer will recognize, and therefore to get an accurate final quote for the user, the Relayer's quote is applied to the amount after the protocol fees.
Does this make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup! That makes sense then
Description
This PR adds support for protocol fees in the
FastBridge
contract.Summary by CodeRabbit