refactor(swap-service): per-unit sellAssetUsd + verified affiliate fee USD - #35
Merged
Merged
Conversation
…cing The legacy sellAmountUsd column held a total (sellAmount × price), while buyAssetUsd and affiliateAssetUsd held per-unit prices — forcing back-derivation of the sell-side price in fee math. Renaming the column to sellAssetUsd and storing the per-unit price drops the asymmetry and simplifies resolveActualFeeUsd. calculateFeeForSwap now derives volume from the verified on-chain sell amount rather than the user-declared sell amount, and getAffiliateStats reuses calculateFeeForSwap + getAffiliateFeeRate so it honors the actual verified fee when available and applies shapeshift's cut consistently with the rest of the fee paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bridges old-schema dumps (sellAmountUsd) to the new per-unit semantics (sellAssetUsd = sellAmountUsd / sellAmount) so existing rows can be reapplied after a DB reset. Two phases: export reads via raw SQL to bypass the now-stale generated client; import re-inserts via the new client. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR refactors USD pricing calculations for swaps by decoupling Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 47 minutes.Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/swap-service/src/swaps/utils.ts (1)
124-138:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThird-asset fee routes still fall back to estimated USD.
Relay can now persist
affiliateAssetUsdfor a fee asset that's neither the sell nor buy asset, but this branch still hard-returnsnullbecause it has no precision forswap.affiliateFeeAssetId. That meanscalculateFeeForSwap()reverts to the bps-based estimate instead of the verified fee for those swaps.Please persist or resolve the fee asset precision alongside
affiliateFeeAssetIdand use it here before merging; otherwise the verified-fee stats stay inaccurate for those routes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/swap-service/src/swaps/utils.ts` around lines 124 - 138, The branch that handles a third-party fee asset must use a persisted or resolved precision instead of forcing precision = null; update calculateFeeForSwap() (the block referencing swap.affiliateFeeAssetId, affiliateAssetUsd and bnOrZero(...).div(...).times(...)) to first look for a stored precision field (e.g., swap.affiliateAssetPrecision) and use it when present, otherwise resolve the asset precision via the existing asset registry/metadata lookup (e.g., getAssetPrecision(assetId) or the same source used for sellAsset/buyAsset precision); only return null if priceUsd or the resolved precision cannot be obtained after these steps. Ensure the division uses the resolved precision before multiplying by priceUsd so verified fees are computed for third-asset routes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/swap-service/src/verification/swap-verification.service.ts`:
- Around line 203-209: The async block computing actualAffiliateFeeUsd should
not call getAssetPriceUsd when actualAffiliateFeeAssetId is missing; update the
IIFE so after checking equality with swap.sellAsset.assetId and
swap.buyAsset.assetId you explicitly bail out if actualAffiliateFeeAssetId is
falsy (e.g., null/undefined/empty) and return undefined (or null) instead of
calling getAssetPriceUsd(''), otherwise call
getAssetPriceUsd(actualAffiliateFeeAssetId) as before; reference
variables/functions: actualAffiliateFeeAssetId, actualAffiliateFeeUsd,
getAssetPriceUsd, swap.sellAsset.assetId, swap.buyAsset.assetId.
In `@scripts/referral-rewards.ts`:
- Around line 84-86: The current math uses parseFloat on
swap.sellAmountCryptoBaseUnit which can overflow JS number precision; replace
this with the big-number utility computeSellAmountUsd() from
apps/swap-service/src/swaps/utils.ts (or replicate its BigNumber-based logic) to
compute volumeUsd safely. Locate the usage around the sellAsset/sellAmount
calculation in scripts/referral-rewards.ts (references: swap.sellAsset,
swap.sellAmountCryptoBaseUnit, sellAmount, volumeUsd) and call
computeSellAmountUsd(swap) (or the same BigNumber path) to obtain a precise USD
volume instead of dividing by 10**precision and using parseFloat. Ensure
types/returns are handled where volumeUsd is consumed.
---
Outside diff comments:
In `@apps/swap-service/src/swaps/utils.ts`:
- Around line 124-138: The branch that handles a third-party fee asset must use
a persisted or resolved precision instead of forcing precision = null; update
calculateFeeForSwap() (the block referencing swap.affiliateFeeAssetId,
affiliateAssetUsd and bnOrZero(...).div(...).times(...)) to first look for a
stored precision field (e.g., swap.affiliateAssetPrecision) and use it when
present, otherwise resolve the asset precision via the existing asset
registry/metadata lookup (e.g., getAssetPrecision(assetId) or the same source
used for sellAsset/buyAsset precision); only return null if priceUsd or the
resolved precision cannot be obtained after these steps. Ensure the division
uses the resolved precision before multiplying by priceUsd so verified fees are
computed for third-asset routes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 889dfaff-f580-4775-a800-3da85d13bd90
📒 Files selected for processing (13)
apps/swap-service/src/affiliate/affiliate.controller.tsapps/swap-service/src/affiliate/affiliate.service.tsapps/swap-service/src/swaps/swaps.service.tsapps/swap-service/src/swaps/types.tsapps/swap-service/src/swaps/utils.tsapps/swap-service/src/utils/affiliateFeeAsset.tsapps/swap-service/src/verification/__tests__/fixtures/near/swap.tsapps/swap-service/src/verification/__tests__/fixtures/relay/swap.tsapps/swap-service/src/verification/swap-verification.service.tspackages/shared-types/src/index.tsprisma/migrations/0_init_swap_service/migration.sqlprisma/schema/swap-service.prismascripts/referral-rewards.ts
The Relay verifier left actualAffiliateFeeUsd as a TODO and the create path papered over it by hardcoding USDC-on-Base as Relay's fee asset (fixed_base strategy). Now the Relay verifier resolves the fee asset's USD price using sellAssetUsd / buyAssetUsd when they match, otherwise fetches it, and reconcileSwap persists the result into affiliateAssetUsd so fee aggregation can use it. - shared-types: add actualAffiliateFeeUsd to SwapVerificationResult. - verification: compute and emit actualAffiliateFeeUsd in the Relay verifier path. - swaps.service: write verificationResult.actualAffiliateFeeUsd to affiliateAssetUsd during reconcile. - affiliateFeeAsset: drop the fixed_base strategy and the hardcoded USDC-on-Base address; Relay now uses 'none' since the fee asset is determined post-verification. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kaladinlight
force-pushed
the
refactor/sell-asset-usd-pricing
branch
from
May 1, 2026 00:07
6d7c68e to
601db2e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Reshapes USD pricing on swaps so all three asset prices (sell / buy / affiliate fee) are stored per-unit and resolved consistently across the create and verification paths.
sellAmountUsd→sellAssetUsdon theSwapmodel and stores the per-unit price (sellAmountUsd / sellAmount) so it matchesbuyAssetUsd/affiliateAssetUsd. Drops the back-derivation inresolveActualFeeUsd.calculateFeeForSwapnow derives volume from the verifier's on-chain sell amount rather than the user-declared amount, andgetAffiliateStatsreusescalculateFeeForSwap+getAffiliateFeeRateso it honors the verified fee when present and applies ShapeShift's cut consistently with the other fee paths.actualAffiliateFeeUsdin the Relay verifier (was a TODO) and persists it intoaffiliateAssetUsdduringreconcileSwap. Drops thefixed_basestrategy inaffiliateFeeAsset.tsand the hardcoded USDC-on-Base address — Relay's fee asset is determined post-verification, so it'snoneat create time.scripts/migrate-sell-asset-usd.tshelper that exports legacy rows via raw SQL and re-imports them under the new schema, computingsellAssetUsdfrom the legacysellAmountUsd.Testing
sellAssetUsdvalues matchsellAmountUsd / sellAmount.affiliateAssetUsdis populated after reconciliation (sell-asset, buy-asset, and unrelated fee-asset cases)./swaps/affiliate-fees/:addressand/swaps/referral-fees/:codeand confirm the totals match the verified-fee path (no regression vs. the volume-derived path when verification is present).getAffiliateStatsreturns numbers consistent with the other fee endpoints for the same affiliate.Summary by CodeRabbit
New Features
Improvements
Bug Fixes