feat(public-api): evm validation, on-chain allowance, partner code timeout#12283
feat(public-api): evm validation, on-chain allowance, partner code timeout#12283kaladinlight merged 6 commits intodevelopfrom
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add optional VITE_<CHAIN>_NODE_URL env vars for first-class EVM chains, consumed by @shapeshiftoss/contracts viem clients. When unset, clients fall back to bundled public RPC URLs. Adds @shapeshiftoss/contracts and viem as workspace deps. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Reject EVM sell requests without sendAddress or on unsupported chains - buildApprovalInfo now reads the ERC-20 allowance on-chain via the viem client and only flags approval as required when allowance < sellAmount - Replace non-null assertions on affiliateBps with DEFAULT_AFFILIATE_BPS fallback for unattributed swaps Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 49 minutes and 20 seconds. ⌛ 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 (1)
📝 WalkthroughWalkthroughThe changes add EVM node configuration support, integrate the viem library for on-chain interactions, and implement dynamic ERC-20 allowance checking for quote operations. They also replace non-null assertions with fallback values in quote and rates endpoints and add timeout handling to the auth middleware. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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. Comment |
kaladinlight
changed the title
kaladinlight
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/public-api/src/routes/quote/utils.ts (1)
43-69: Well-implemented on-chain allowance check.The logic correctly:
- Gates the check with
CHAIN_NAMESPACE.Evm,isToken(), and presence ofallowanceContract- Uses viem's
readContractwitherc20Abifor the allowance read- Compares against
BigIntfor precisionOne minor defensive consideration:
viemClientByChainId[step.sellAsset.chainId]at line 57 assumes the client exists. WhilegetQuote.tsvalidates this upstream forsellAsset.chainId, adding a guard here would makebuildApprovalInfosafer if called from other contexts in the future.🛡️ Optional: Add defensive client check
if (!needsAllowanceCheck) return { isRequired: false, spender: '' } const spender = step.allowanceContract const client = viemClientByChainId[step.sellAsset.chainId] + + if (!client) return { isRequired: false, spender: '' } const allowance = await client.readContract({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/public-api/src/routes/quote/utils.ts` around lines 43 - 69, In buildApprovalInfo, defensively check that viemClientByChainId[step.sellAsset.chainId] exists before using it: retrieve the client into a local (e.g., const client = viemClientByChainId[step.sellAsset.chainId]) and if falsy return a safe default like { isRequired: false, spender: '' } (or optionally throw a descriptive error) so the function does not assume a client is always present when called from other contexts; keep the rest of the allowance logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/public-api/src/middleware/auth.ts`:
- Around line 31-32: The catch block that currently swallows all errors and
returns null should instead log the failure with structured context before
returning null: inside the catch in the partner-resolution function (e.g., the
catch in resolvePartner / fetch partner block in middleware/auth.ts) capture the
caught error and call the module's logger (processLogger / logger) with a
structured message including partner code, endpoint URL, configured timeout, and
error.name and error.message (and error.stack if available), then return null as
before; ensure the log uses key/value fields for each piece of context to
satisfy the "ALWAYS log errors" guideline.
---
Nitpick comments:
In `@packages/public-api/src/routes/quote/utils.ts`:
- Around line 43-69: In buildApprovalInfo, defensively check that
viemClientByChainId[step.sellAsset.chainId] exists before using it: retrieve the
client into a local (e.g., const client =
viemClientByChainId[step.sellAsset.chainId]) and if falsy return a safe default
like { isRequired: false, spender: '' } (or optionally throw a descriptive
error) so the function does not assume a client is always present when called
from other contexts; keep the rest of the allowance logic unchanged.
🪄 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: cc137006-1559-4b22-aa73-53acd8ce607f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
packages/public-api/.env.examplepackages/public-api/package.jsonpackages/public-api/src/env.tspackages/public-api/src/middleware/auth.tspackages/public-api/src/routes/quote/getQuote.tspackages/public-api/src/routes/quote/utils.tspackages/public-api/src/routes/rates/getRates.ts
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Description
Several independent improvements to
public-api's quote/rates flow. Split into three commits:fix(public-api): timeout partner code resolution after 5s— add anAbortControllerwith a 5s timeout to the partner-code lookup againstswap-service, so slow/hung responses can't stall every incoming request.feat(public-api): support optional EVM node URL env vars— add optionalVITE_<CHAIN>_NODE_URLenv vars for first-class EVM chains (ETH, BSC, AVAX, ARB, OP, GNO, POLY, BASE). These are consumed by@shapeshiftoss/contractsviem clients viaprocess.env.VITE_*_NODE_URL. When unset, the clients fall back to the public RPCs bundled in@shapeshiftoss/contracts. Adds@shapeshiftoss/contractsandviemas workspace deps so we can reuse the client factory instead of reimplementing it.feat(public-api): validate EVM inputs and check on-chain allowance—sendAddress(400MISSING_SEND_ADDRESS) or on unsupported chains (400UNSUPPORTED_CHAIN) instead of crashing downstream.buildApprovalInfonow reads the ERC-20 allowance on-chain via the viem client and only flags approval as required whenallowance < sellAmount. Previously we returnedisRequired: truefor every ERC-20 quote regardless of existing allowance, forcing the widget to always prompt for approval even when one was already in place.affiliateBps!non-null assertions with aDEFAULT_AFFILIATE_BPSfallback so unattributed swaps don't explode at runtime.Why
swap-servicepartner lookups were propagating into request latency with no bound.public-apineeds its own transport to sell assets that aren't hitting a SwapServer private RPC; exposing the bundled public RPCs via@shapeshiftoss/contractskeeps env-var overhead minimal and gives us working clients out of the box for all supported EVM chains.Issue (if applicable)
closes #
Risk
Medium — changes the contract of
/quotefor EVM sells (now requiressendAddress), adds a network call per ERC-20 quote (on-chain allowance read), and introduces a new transport path (public RPCs) when private node URLs are unset.All EVM swaps routed through
public-api. Non-EVM chains are unaffected.Testing
Engineering
pnpm run type-checkpassespnpm run lintpasses/quotewith EVM sell + nosendAddress→ 400MISSING_SEND_ADDRESS/quotewith an unsupported EVM chain → 400UNSUPPORTED_CHAIN/quotewith an ERC-20 sell where allowance is already sufficient →approval.isRequired: false/quotewith an ERC-20 sell where allowance is insufficient →approval.isRequired: true, correctspender/quotewith native EVM sell →approval.isRequired: falseswap-servicepartner endpoint does not stall requests past ~5sVITE_<CHAIN>_NODE_URL→ viem client still works via public RPC fallbackOperations
Screenshots (if applicable)
Summary by CodeRabbit
New Features
Bug Fixes
Chores