fix(bridge): correct NEAR Intents fee calculation for BTC destinations - #929
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
📝 WalkthroughWalkthroughNear Intents quote fee and slippage calculations now use quote amount fields instead of USD payout ratios. Regression tests validate BTC-denominated fee bounds and corrected fee and slippage outputs. ChangesNear Intents fee correction
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
I have read the CLA Document and I hereby sign the CLA |
|
@gitshreevatsa could you resolve the merge conflicts please? |
5a1f6aa to
1ed60c3
Compare
|
@shoom3301 Merged the conflicts! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/bridging/src/providers/near-intents/NearIntentsBridgeProvider.test.ts (1)
532-539: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using
BigIntmath for exact fee expectations.Using
Number()for token amounts can lead to loss of precision for values larger thanNumber.MAX_SAFE_INTEGER(like the 18-decimalamountOuthere). While it mathematically works for the current fixtures, using exactBigIntmath eliminates floating-point concerns, perfectly models on-chain math, and makes the expectations cleaner.♻️ Proposed refactor for exact token math
- const slippage = (Number(amountOut) - Number(minAmountOut)) / Number(amountOut) const bridgingFee = quote.amountsAndCosts.costs.bridgingFee // The sell-currency fee must be denominated in the sell token (USDC, 6 decimals) and thus // scaled by amountIn; the buy-currency fee in the buy token (ETH, 18 decimals), scaled by // amountOut. Before the fix these two were swapped (each reported in the wrong token). - expect(bridgingFee.amountInSellCurrency).toBe(BigInt(Math.trunc(Number(amountIn) * slippage))) - expect(bridgingFee.amountInBuyCurrency).toBe(BigInt(Math.trunc(Number(amountOut) * slippage))) + const expectedSellFee = (BigInt(amountIn) * (BigInt(amountOut) - BigInt(minAmountOut))) / BigInt(amountOut) + const expectedBuyFee = BigInt(amountOut) - BigInt(minAmountOut) + + expect(bridgingFee.amountInSellCurrency).toBe(expectedSellFee) + expect(bridgingFee.amountInBuyCurrency).toBe(expectedBuyFee)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bridging/src/providers/near-intents/NearIntentsBridgeProvider.test.ts` around lines 532 - 539, Update the fee expectations in the test around slippage, amountIn, and amountOut to use exact BigInt arithmetic instead of Number conversions and Math.trunc. Preserve the existing sell-token scaling by amountIn and buy-token scaling by amountOut while modeling the slippage calculation without floating-point precision loss.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/bridging/src/providers/near-intents/NearIntentsBridgeProvider.test.ts`:
- Around line 532-539: Update the fee expectations in the test around slippage,
amountIn, and amountOut to use exact BigInt arithmetic instead of Number
conversions and Math.trunc. Preserve the existing sell-token scaling by amountIn
and buy-token scaling by amountOut while modeling the slippage calculation
without floating-point precision loss.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a7b72aa4-f1eb-409d-be6b-5e6cca0220cf
📒 Files selected for processing (2)
packages/bridging/src/providers/near-intents/NearIntentsBridgeProvider.test.tspackages/bridging/src/providers/near-intents/NearIntentsBridgeProvider.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bridging/src/providers/near-intents/NearIntentsBridgeProvider.ts
3 participants
What and why
NearIntentsBridgeProvider.getQuote()computed the bridge fee from the ratio ofamountInUsd/amountOutUsd, and swapped which currency each fee variable was denominated in (the "buy currency" fee was computed fromamountIn, and vice versa).This is invisible on same-magnitude routes (e.g. USDC -> USDT), but breaks visibly on BTC destinations: satoshis and USDC base units differ by orders of magnitude, so the mislabeled value produces a nonsensical fee.
Example from a production BTC bridge quote (deposit address
0x712bf81469904cee52f5ba897fd51a338f6b73e4,https://explorer.near-intents.org/transactions/0x712bf81469904cee52f5ba897fd51a338f6b73e4):
the real worst-case gap is
amountOut (7047) - minAmountOut (7011) = 36 sats, but the current code returns a fee of1,263,735sats, larger than the entire trade.The fix
Derive slippage/fee from
amountOutvsminAmountOut(both already in the same native units, no USD conversion involved) instead of the USD ratio, and assign each fee variable to the correct currency.How to test
New test added:
should compute a sane BTC-denominated fee for a real production BTC bridge quote, using the real numbers above. It asserts the BTC-denominated fee can never exceed the BTC amount being bridged, and pinsthe exact before/after values (
1263735nonmain,36nwith this fix) so a future regression fails immediately.