[TASK-11578] feat: abstract squid route fetching #906
Conversation
Stop using the skd and use the squid API directly, this give us more control and access to all the data that returns squid (for example, we now have access to the fees and don't have to recalculate them ourselves)
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Caution Review failedThe pull request is closed. """ WalkthroughThis update simplifies the token decimals extraction logic in the token actions module and adds a new service module that fetches and optimizes cross-chain token swap routes using the Squid API. The new service includes detailed TypeScript types, route fetching, binary search optimization for amounts, fee aggregation, and USD conversions. Additionally, the Squid API URL constant was changed to be sourced from an environment variable instead of a hardcoded string. Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ Finishing Touches
🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/app/actions/tokens.ts (1)
78-88:⚠️ Potential issueGuard against missing contract match before non-null assertion
json.data.contracts.find(…)!.decimalswill throw ifblockchainIddoesn’t exactly matchchainId(Mobula sometimes uses numeric IDs or differing case).
Add a fallback or explicit error to avoid a runtime crash:-const decimals = json.data.contracts.find((c) => c.blockchainId === chainId)!.decimals +const contractMatch = json.data.contracts.find((c) => c.blockchainId === chainId) +const decimals = + contractMatch?.decimals ?? + json.data.decimals ?? // legacy field + 18 // sensible default
🧹 Nitpick comments (1)
src/services/swap.ts (1)
235-239:maxIterations = 3risks missing an optimal amountWith only three iterations the binary search covers at most 8 values—often insufficient for tight tolerances, especially on wider ranges (e.g., USD mode). Consider increasing or making it configurable.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/services/swap.ts (2)
176-189: Remove unconditional logging & validate env vars (already flagged).
console.dirleaks wallet addresses / API secrets in prod logs, and!does not guard at runtime against missingSQUID_API_URL/SQUID_INTEGRATOR_ID. Make the logs dev-only and throw early if the env vars are absent (see earlier review).
212-214: BigInt → Number cast drops precision (already flagged).
Number(formatUnits(..)) * priceloses accuracy for large 18-dec tokens. Keep everything in big-number arithmetic instead of casting to JS numbers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/swap.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/services/swap.ts (1)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#495
File: src/components/Cashout/Components/Initial.view.tsx:194-198
Timestamp: 2024-10-29T14:44:08.745Z
Learning: Using a fixed 6 decimal places for `floorFixed` is acceptable for token amounts in this codebase, even if tokens have varying decimal places.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/services/swap.ts (4)
210-211: Precision loss converting BigInt → Number
Number(formatUnits(...))drops precision for 18-dec tokens above 2⁵³-1 and can mis-size the tolerance window. Same issue was flagged in previous PRs.
Use big-number arithmetic or a decimal lib instead.
245-247:overagecalculation can overflowNumber
Number(receivedAmount - targetToAmount)risks precision loss for large amounts; the comparison that follows then uses this inaccuracy to decide search bounds. Switch to bigint math or at least a safe-int check.
317-320: Possible scientific-notation string breaksparseUnits
(Number(amount.fromUsd) / fromTokenPrice.price).toString()can yield"1e-7", whichparseUnitsrejects. Format the number withtoFixed(fromTokenPrice.decimals)as done later in the file.
176-183:⚠️ Potential issueValidate env vars before constructing the request URL & headers
process.env.SQUID_API_URL!andprocess.env.SQUID_INTEGRATOR_ID!are asserted non-null, yet no explicit guard exists.
If either is undefined the code silently builds a URL like"undefined/v2/route"or sends an empty integrator id – easy to overlook in prod and hard to diagnose.- const response = await fetchWithSentry(`${process.env.SQUID_API_URL!}/v2/route`, { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - 'x-integrator-id': process.env.SQUID_INTEGRATOR_ID!, - }, + const apiUrl = process.env.SQUID_API_URL + const integratorId = process.env.SQUID_INTEGRATOR_ID + if (!apiUrl || !integratorId) { + throw new Error('SQUID_API_URL or SQUID_INTEGRATOR_ID env var not set') + } + + const response = await fetchWithSentry(`${apiUrl}/v2/route`, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'x-integrator-id': integratorId, + },
🧹 Nitpick comments (2)
src/services/swap.ts (2)
236-244: Binary-search capped to 3 iterations may miss the tolerance windowWith only three requests the search space is reduced by 8× at best; for widely spaced liquidity steps you could exit without ever probing a viable quote.
Consider:
- Increasing
maxIterations, or- Using an exponential probing phase before binary search, or
- Accepting the first quote within
maxOverageirrespective of iteration count.
372-375: CastingamountUsdstrings toNumbermay silently truncate large values
Number(cost.amountUsd)assumes the value is < 2⁵³ and non-scientific notation. For consistency with other bigint-centric code, consider keeping these as strings and using a decimal library (e.g.decimal.js) for the sum.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/swap.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/services/swap.ts (1)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#495
File: src/components/Cashout/Components/Initial.view.tsx:194-198
Timestamp: 2024-10-29T14:44:08.745Z
Learning: Using a fixed 6 decimal places for `floorFixed` is acceptable for token amounts in this codebase, even if tokens have varying decimal places.
Stop using the skd and use the squid API directly, this give us more control and access to all the data that returns squid (for example, we now have access to the fees and don't have to recalculate them ourselves)