fix(#161): live USD-to-payment-asset conversion in verification - #169
Conversation
…cation Replace hardcoded expectedAmountUsd * 1e18 (1 USD = 1 asset) with asset-agnostic pricing: live/cached rate, configurable tolerance, and FALLBACK_USD_PER_ASSET when the price API is unavailable. Closes BuidlZone-Labs#161
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughPayment verification now converts USD amounts using configured payment-asset pricing, applies tolerance to on-chain base-unit thresholds, supports cached live or fallback quotes, and distinguishes pricing configuration failures from unavailable pricing. ChangesPayment pricing verification
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PaymentVerificationService
participant asset-pricing.service
participant PricingAPI
participant BlockchainProvider
PaymentVerificationService->>asset-pricing.service: resolve expected base units
asset-pricing.service->>PricingAPI: fetch USD-per-asset quote
PricingAPI-->>asset-pricing.service: return live quote
asset-pricing.service-->>PaymentVerificationService: return expected and minimum amounts
PaymentVerificationService->>BlockchainProvider: retrieve payment transaction
BlockchainProvider-->>PaymentVerificationService: return on-chain value
PaymentVerificationService-->>PaymentVerificationService: compare value with minimum threshold
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/services/asset-pricing.service.ts (3)
206-240: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPrefer the exact decimal path unconditionally instead of float math with an overflow-only fallback.
whole * factoruses IEEE-754 floats, which aren't exact for most rates/decimals combinations; the tolerance mechanism usually absorbs the resulting rounding noise, but this is fragile for tight tolerances or largedecimals. The safetoFixed-based BigInt conversion already exists as a fallback for the!Number.isFinite(raw)case — using it unconditionally removes the float precision risk with little extra cost.♻️ Proposed simplification
- const whole = amountUsd / usdPerAsset; - if (!Number.isFinite(whole) || whole < 0) { - throw new Error(`USD→asset conversion produced non-finite value`); - } - - // Split integer + fractional parts in base units - const factor = 10 ** decimals; - const raw = whole * factor; - if (!Number.isFinite(raw)) { - // Fallback: BigInt via string with fixed precision - const [i, f = ''] = whole.toFixed(decimals).split('.'); - const frac = (f + '0'.repeat(decimals)).slice(0, decimals); - return BigInt(i) * BigInt(10 ** decimals) + BigInt(frac || '0'); - } - return BigInt(Math.round(raw)); + const whole = amountUsd / usdPerAsset; + if (!Number.isFinite(whole) || whole < 0) { + throw new Error(`USD→asset conversion produced non-finite value`); + } + const [i, f = ''] = whole.toFixed(decimals).split('.'); + const frac = (f + '0'.repeat(decimals)).slice(0, decimals); + return BigInt(i) * BigInt(10 ** decimals) + BigInt(frac || '0');🤖 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 `@src/services/asset-pricing.service.ts` around lines 206 - 240, Update usdToAssetBaseUnits to use the existing toFixed-based BigInt conversion unconditionally after validating whole, removing the factor/raw float-math path and its overflow fallback. Preserve the current decimal truncation/padding behavior and return the exact base-unit BigInt result.
126-149: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo retry/backoff on the live price fetch before falling back.
A single transient network error immediately triggers the fallback path (or a hard failure if no fallback is configured), unlike
fetchTransaction, which useswithRpcRetry. Consider a short retry/backoff here too, so a single blip doesn't unnecessarily degrade to (possibly stale) fallback pricing or reject a valid payment.🤖 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 `@src/services/asset-pricing.service.ts` around lines 126 - 149, Update fetchLiveUsdPerAsset to retry transient fetch or HTTP failures with a short bounded backoff before propagating the error to the fallback path. Reuse the existing retry/backoff utility used by fetchTransaction, if available, while preserving response parsing and immediate handling of invalid price data.
155-198: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache stampede on TTL expiry.
When the cache expires, every concurrent
getUsdPerAssetcall independently callsfetchLiveUsdPerAssetinstead of sharing one in-flight request. Under load this multiplies calls to the price API right at cache-expiry moments, increasing the risk of hitting provider rate limits.♻️ Sketch: coalesce concurrent refreshes
let cache: CacheEntry | null = null; +let inFlight: Promise<AssetPriceQuote> | null = null; export async function getUsdPerAsset( assetOverride?: string, ): Promise<AssetPriceQuote> { ... if (cache && cache.quote.asset === asset && cache.expiresAt > now && cache.quote.usdPerAsset > 0) { return { ...cache.quote, source: 'cache' }; } + if (inFlight) return inFlight; + inFlight = (async () => { try { const quote = await fetchLiveUsdPerAsset(asset, cfg.priceApiUrl); cache = { quote, expiresAt: now + cfg.cacheTtlMs }; return quote; } catch (err) { ... + } finally { + inFlight = null; } + })(); + return inFlight; }🤖 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 `@src/services/asset-pricing.service.ts` around lines 155 - 198, Update getUsdPerAsset to coalesce concurrent cache refreshes by maintaining a shared in-flight fetch promise. After the cache-miss check, reuse that promise when present; otherwise create and store the live fetch operation, update the cache or apply the existing fallback behavior once it settles, and clear the in-flight reference afterward so later expirations can refresh normally.
🤖 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.
Inline comments:
In `@src/services/asset-pricing.service.ts`:
- Around line 98-124: Update parseUsdFromBody to only inspect CoinGecko-shaped
nested prices under the requested asset’s mapped key, using a shared
COINGECKO_ID_MAP extracted from defaultPriceEndpoint. Remove the unrestricted
loop over all object keys, while preserving the direct usdPerAsset, price, and
flat symbol fallbacks.
In `@src/services/paymentVerification.service.ts`:
- Around line 138-163: The pricing-resolution catch in the payment verification
flow must distinguish transient pricing unavailability from permanent
configuration or input failures. Update resolveExpectedPaymentBaseUnits and/or
usdToAssetBaseUnits to expose a distinct error type or classification for
validation/configuration errors, then map those errors to non-retryable
PaymentVerificationError while retaining ServiceUnavailableError only for
genuinely unavailable pricing sources; preserve the existing retry guidance
solely for transient failures.
---
Nitpick comments:
In `@src/services/asset-pricing.service.ts`:
- Around line 206-240: Update usdToAssetBaseUnits to use the existing
toFixed-based BigInt conversion unconditionally after validating whole, removing
the factor/raw float-math path and its overflow fallback. Preserve the current
decimal truncation/padding behavior and return the exact base-unit BigInt
result.
- Around line 126-149: Update fetchLiveUsdPerAsset to retry transient fetch or
HTTP failures with a short bounded backoff before propagating the error to the
fallback path. Reuse the existing retry/backoff utility used by
fetchTransaction, if available, while preserving response parsing and immediate
handling of invalid price data.
- Around line 155-198: Update getUsdPerAsset to coalesce concurrent cache
refreshes by maintaining a shared in-flight fetch promise. After the cache-miss
check, reuse that promise when present; otherwise create and store the live
fetch operation, update the cache or apply the existing fallback behavior once
it settles, and clear the in-flight reference afterward so later expirations can
refresh normally.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: abd62d96-2d0e-4a49-b04b-b45fa0fe86ad
📒 Files selected for processing (4)
.env.examplesrc/services/asset-pricing.service.tssrc/services/paymentVerification.service.tstests/payment-conversion.service.test.ts
CI noteThe red checks on this PR are pre-existing on
|
|
@naninu123 kindly ensure the right changes & resolve the coderabbit review. |
…ath, config vs transient error separation - parseUsdFromBody: only match CoinGecko entry for the requested asset (via shared COINGECKO_ID_MAP), not first object with .usd field. - usdToAssetBaseUnits: always use toFixed→BigInt path (no float * 10**N), removing IEEE-754 precision risk unconditionally. - PricingConfigError: new distinct error class for permanent config/input failures; paymentVerification catch maps it to 422 PaymentVerificationError, while transient network/API errors still get 503 ServiceUnavailableError. - resolveExpectedPaymentBaseUnits: validates expectedAmountUsd + decimals upfront before network call (fail-fast, not per-request 503). Closes BuidlZone-Labs#161
|
@DioChuks All 3 CodeRabbit review items addressed in
Pre-existing |
Alright, thanks for your contribution ❤️ |
2 participants
Summary
PaymentVerificationService.verifytreatedexpectedAmountUsdas if it were already a whole payment-asset amount (expectedAmountUsd * 1e18). Valid payments at real market rates were rejected (or absurd under/over-pays accepted).This PR replaces that hardcoded assumption with an asset-agnostic pricing path:
PRICE_API_URL/ CoinGecko defaultPAYMENT_TOLERANCE_BPS) for quote→confirm driftFALLBACK_USD_PER_ASSETwhen pricing is temporarily unavailablePAYMENT_ASSET,PAYMENT_ASSET_DECIMALS) so XLM/tokens work, not just ETHChanges
src/services/asset-pricing.service.ts— rate fetch, cache, conversion, tolerancesrc/services/paymentVerification.service.ts— value check uses resolved base units + min acceptable.env.example— pricing knobs documentedtests/payment-conversion.service.test.ts— conversion + verify integration (14 tests)Acceptance criteria
Verifier.verify)Test
npx jest tests/payment-conversion.service.test.ts --runInBand --forceExit # 14 passedCloses #161
Summary by CodeRabbit