-
Notifications
You must be signed in to change notification settings - Fork 2
fix: harden amount sanity checks for CoW order and swap flows #225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 7 commits
a35cbc8
0f70271
363b035
9e48a66
0db7e43
9750361
27d1303
fe6083a
1f6aacd
8d3743c
87941e0
cb3de63
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ import { toBaseUnit } from '@shapeshiftoss/utils' | |
| import BigNumber from 'bignumber.js' | ||
| import { z } from 'zod' | ||
|
|
||
| import { getSimplePrices } from '../../lib/asset/coingecko' | ||
| import { resolveCowTokenAddress } from '../../lib/composableCow' | ||
| import { COW_VAULT_RELAYER_ADDRESS, prepareCowLimitOrder } from '../../lib/cow' | ||
| import type { CowOrderSigningData } from '../../lib/cow/types' | ||
|
|
@@ -21,13 +22,17 @@ export const createLimitOrderSchema = z.object({ | |
| network: cowSupportedNetworkSchema.describe('Network for the limit order'), | ||
| sellAmount: z | ||
| .string() | ||
| .refine(val => !/^\d{15,}/.test(val.trim()), { | ||
| message: | ||
| 'sellAmount looks like a base-unit value (15+ digits). Use human-readable token amounts (e.g. "230" for 230 ARB, not "230000000000000000000").', | ||
| }) | ||
| .describe( | ||
| 'Amount to sell in TOKEN units, not USD (e.g., "100" for 100 USDC, "0.5" for 0.5 WETH). If the user specified a USD dollar amount, convert to token units first using getAssetPricesTool and mathCalculatorTool.' | ||
| 'Amount to sell in TOKEN units, not USD (e.g., "100" for 100 USDC, "230" for 230 ARB). Never pass base units even if precision is 18 (e.g., not "230000000000000000000"). If the user specified a USD dollar amount, convert to token units first using getAssetPricesTool and mathCalculatorTool.' | ||
| ), | ||
| limitPrice: z | ||
| .string() | ||
| .describe( | ||
| 'How much buyAsset you receive per 1 sellAsset. "sell A when worth X B" → limitPrice=X. Example: "worth 2 USDT" → "2". For percentage-based requests ("sell when up 5%"), compute: currentPricePerToken × (1 + pct/100).' | ||
| 'How much buyAsset you receive per 1 sellAsset. NEVER invert — for sub-dollar tokens (e.g. ARB at $0.50 USD selling for USDC), limitPrice ≈ 0.50, NOT 2. "sell A when worth X B" → limitPrice=X. Example: "worth 2 USDT" → "2". For percentage-based requests ("sell when up 5%"), compute: currentPricePerToken × (1 + pct/100). Use getAssetPrices if uncertain.' | ||
| ), | ||
| expirationHours: z | ||
| .number() | ||
|
|
@@ -93,6 +98,54 @@ export async function executeCreateLimitOrder( | |
| resolveAsset({ symbolOrName: input.buyAsset, network: input.network }, walletContext), | ||
| ]) | ||
|
|
||
| const limitPriceNum = Number(input.limitPrice) | ||
| if (!Number.isFinite(limitPriceNum) || limitPriceNum <= 0) { | ||
| throw new Error(`Invalid limitPrice "${input.limitPrice}". It must be a positive number.`) | ||
| } | ||
|
|
||
| // Sanity-check limitPrice against current market rate to catch LLM inversion/base-unit errors | ||
| const priceResults = await getSimplePrices([sellAsset.assetId, buyAsset.assetId]) | ||
| const sellUsdPrice = Number(priceResults.find(p => p.assetId === sellAsset.assetId)?.price ?? '0') | ||
| const buyUsdPrice = Number(priceResults.find(p => p.assetId === buyAsset.assetId)?.price ?? '0') | ||
| if (sellUsdPrice > 0 && buyUsdPrice > 0) { | ||
| const marketLimitPrice = sellUsdPrice / buyUsdPrice | ||
| const ratio = limitPriceNum / marketLimitPrice | ||
| if (!Number.isFinite(ratio) || ratio <= 0) { | ||
| throw new Error( | ||
| `Invalid limitPrice "${input.limitPrice}" for market comparison. ` + | ||
| `Expected a positive ${buyAsset.symbol}/${sellAsset.symbol} price.` | ||
| ) | ||
| } | ||
| const logRatio = Math.abs(Math.log10(ratio)) | ||
| const isNearUsdPrice = (usdPrice: number) => usdPrice > 0 && Math.abs(limitPriceNum - usdPrice) / usdPrice <= 0.25 | ||
|
|
||
| // Guard likely "USD price leaked into pair price" mistakes. | ||
| // Example: ARB->EUL should be ~0.086 EUL/ARB, but passing 1.39 (EUL USD) is >10x off. | ||
| if (logRatio > 1 && (isNearUsdPrice(sellUsdPrice) || isNearUsdPrice(buyUsdPrice))) { | ||
| throw new Error( | ||
| `limitPrice ${input.limitPrice} appears to be a USD token price, not the pair price. ` + | ||
| `Expected approximately ${marketLimitPrice.toFixed(6)} ${buyAsset.symbol}/${sellAsset.symbol} ` + | ||
| `(1 ${sellAsset.symbol} = X ${buyAsset.symbol}).` | ||
| ) | ||
| } | ||
|
|
||
| if (logRatio > 3) { | ||
| throw new Error( | ||
| `limitPrice ${input.limitPrice} is more than 1000× from the market rate (~${marketLimitPrice.toFixed(6)} ${buyAsset.symbol}/${sellAsset.symbol}). ` + | ||
| `Did you invert the price or pass a base-unit value? For ${sellAsset.symbol} at $${sellUsdPrice} selling for ${buyAsset.symbol}, limitPrice should be ~${marketLimitPrice.toFixed(6)}.` | ||
| ) | ||
| } | ||
| if (logRatio > 1) { | ||
| console.warn('[createLimitOrder] limitPrice sanity check: suspicious deviation', { | ||
| inputLimitPrice: input.limitPrice, | ||
| marketLimitPrice, | ||
| ratio, | ||
| sellAsset: sellAsset.symbol, | ||
| buyAsset: buyAsset.symbol, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // Get numeric chain ID directly from network (Zod schema guarantees valid network) | ||
| const evmChainId = NETWORK_TO_CHAIN_ID[input.network]! | ||
|
|
||
|
|
@@ -198,4 +251,20 @@ IMPORTANT: | |
| - For percentage-based requests ("sell when up X%"), compute limitPrice = currentPricePerToken × (1 + X/100) using getAssetPrices and the maths tool`, | ||
| inputSchema: createLimitOrderSchema, | ||
| execute: executeCreateLimitOrder, | ||
| experimental_toToolResultContent: (result: CreateLimitOrderOutput) => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This callback never runs: |
||
| const llmSigningData: Pick<CowOrderSigningData, 'domain' | 'types' | 'primaryType'> = { | ||
| domain: result.signingData.domain, | ||
| types: result.signingData.types, | ||
| primaryType: result.signingData.primaryType, | ||
| } | ||
| const llmVisible = { | ||
| summary: result.summary, | ||
| signingData: llmSigningData, | ||
| needsApproval: result.needsApproval, | ||
| approvalTx: result.approvalTx, | ||
| approvalTarget: result.approvalTarget, | ||
| trackingUrl: result.trackingUrl, | ||
| } | ||
| return [{ type: 'text' as const, text: JSON.stringify(llmVisible) }] | ||
| }, | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,8 +38,12 @@ export const createTwapSchema = z.object({ | |
| network: cowSupportedNetworkSchema.describe('Network for the TWAP/DCA order'), | ||
| totalAmount: z | ||
| .string() | ||
| .refine(val => !/^\d{15,}/.test(val.trim()), { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This accepts |
||
| message: | ||
| 'totalAmount looks like a base-unit value (15+ digits). Use human-readable token amounts (e.g. "1000" for 1000 USDC, not "1000000000").', | ||
| }) | ||
| .describe( | ||
| 'Total amount to sell in TOKEN units, not USD (e.g., "1000" for 1000 USDC, "0.5" for 0.5 WETH). If the user specified a USD dollar amount, convert to token units first using getAssetPricesTool and mathCalculatorTool.' | ||
| 'Total amount to sell in TOKEN units, not USD (e.g., "1000" for 1000 USDC, "230" for 230 ARB, "0.5" for 0.5 WETH). Never pass base units even if precision is 18 (e.g., not "230000000000000000000"). If the user specified a USD dollar amount, convert to token units first using getAssetPricesTool and mathCalculatorTool.' | ||
| ), | ||
| durationSeconds: z | ||
| .number() | ||
|
|
@@ -271,4 +275,23 @@ IMPORTANT: | |
| - Native tokens (ETH) must be wrapped (WETH) to sell`, | ||
| inputSchema: createTwapSchema, | ||
| execute: executeCreateTwap, | ||
| experimental_toToolResultContent: (result: CreateTwapOutput) => { | ||
| const llmVisible = { | ||
| summary: result.summary, | ||
| safeTransaction: result.safeTransaction, | ||
| needsApproval: result.needsApproval, | ||
| approvalTx: result.approvalTx, | ||
| approvalTarget: result.approvalTarget, | ||
| safeAddress: result.safeAddress, | ||
| orderHash: result.orderHash, | ||
| conditionalOrderParams: result.conditionalOrderParams, | ||
| needsDeposit: result.needsDeposit, | ||
| depositTx: result.depositTx, | ||
| sellTokenAddress: result.sellTokenAddress, | ||
| buyTokenAddress: result.buyTokenAddress, | ||
| durationSeconds: result.durationSeconds, | ||
| warnings: result.warnings, | ||
| } | ||
| return [{ type: 'text' as const, text: JSON.stringify(llmVisible) }] | ||
| }, | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ARB at $0.50 → USDC with
limitPrice="2"passes silently. Cover this inversion, and ask for clarification on suspicious prices while allowing explicitly confirmed future targets.