[ENG-900] [Turnkey] Add safe slippageTolerance. #3118
Conversation
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
WalkthroughCompute slippage tolerance at runtime (min of USDC absolute cap and percentage cap), remove a top-level slippage constant and an extraneous log, add Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant SkipHelper
participant RouteService
Caller->>SkipHelper: request route (chainId, amount, denom)
SkipHelper->>RouteService: resolve route -> routeResult (estimatedAmountOut)
SkipHelper->>SkipHelper: expectedAmountOut = estimatedAmountOut
SkipHelper->>SkipHelper: call getSlippageTolerancePercent(expectedAmountOut)
Note right of SkipHelper #dff0d8: compute min(USDC_cap/expectedOut, percent_cap)
SkipHelper-->>Caller: return messages payload including slippageTolerancePercent
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
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: 5
🧹 Nitpick comments (1)
indexer/services/comlink/src/helpers/skip-helper.ts (1)
15-15: Remove unnecessary lodash max importAfter the above fixes, max is no longer needed. Keep min or inline native Math.min as desired.
-import { max, min } from 'lodash'; +import { min } from 'lodash';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
indexer/services/comlink/src/helpers/skip-helper.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
indexer/services/comlink/src/helpers/skip-helper.ts (2)
indexer/services/comlink/src/lib/smart-contract-constants.ts (5)
usdcAddressByChainId(12-20)dydxChainId(10-10)ethDenomByChainId(22-27)ETH_USDC_QUANTUM(31-31)ETH_WEI_QUANTUM(32-32)indexer/services/comlink/src/helpers/alchemy-helpers.ts (1)
getETHPrice(309-342)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- GitHub Check: call-build-ecs-service-roundtable / (roundtable) Check docker image build
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: check-build-bazooka
- GitHub Check: call-build-ecs-service-ender / (ender) Check docker image build
- GitHub Check: check-build-auxo
- GitHub Check: lint
- GitHub Check: run_command
- GitHub Check: test / run_command
- GitHub Check: build-and-push-testnet
- GitHub Check: build-and-push-mainnet
- GitHub Check: Summary
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
Comment on lines
62
to
64
| if (amount.startsWith('0x')) { | ||
| amountToUse = parseInt(amount, 16).toString(); | ||
| } |
There was a problem hiding this comment.
Hex amount parsing loses precision for large values
parseInt on 256-bit hex can overflow Number. Use BigInt for exact conversion.
- if (amount.startsWith('0x')) {
- amountToUse = parseInt(amount, 16).toString();
- }
+ if (amount.startsWith('0x')) {
+ amountToUse = BigInt(amount).toString(); // exact conversion for large values
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (amount.startsWith('0x')) { | |
| amountToUse = parseInt(amount, 16).toString(); | |
| } | |
| if (amount.startsWith('0x')) { | |
| amountToUse = BigInt(amount).toString(); // exact conversion for large values | |
| } |
🤖 Prompt for AI Agents
In indexer/services/comlink/src/helpers/skip-helper.ts around lines 62 to 64,
the code uses parseInt(amount, 16) which can overflow for 256-bit hex values;
replace parseInt with BigInt by parsing the hex string with BigInt (e.g.
BigInt(amount) or BigInt('0x...')) and then call toString() to produce an exact
decimal string; ensure you handle the optional "0x" prefix consistently and
maintain the same variable assignment to amountToUse.
There was a problem hiding this comment.
The amountToUse will have enough precision since we limit the amount beforehand.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all callers of getSkipCallData to verify if limitAmount is called first
rg -nP --type=ts -C10 'getSkipCallData\s*\(' -g '!**/skip-helper.ts' -g '!**/__tests__/**'
Length of output: 2413
Comment on lines
93
to
108
| trueEstimateAmountOut = await estimateAmountOutSwap( | ||
| routeResult?.sourceAssetChainId, | ||
| routeResult?.amountIn, | ||
| routeResult?.sourceAssetDenom, | ||
| ); | ||
| toleranceMet = verifySkipEstimation(routeResult.estimatedAmountOut, trueEstimateAmountOut); | ||
| // estimatedAmountOut from skip needs to be appropriate. | ||
| if (!toleranceMet) { | ||
| logger.warning({ | ||
| at: 'skip-helper#getSkipCallData', | ||
| message: 'estimated amount out is less than our tolerance. Retrying...', | ||
| estimatedAmountOut: routeResult.estimatedAmountOut, | ||
| actualAmount: trueEstimateAmountOut, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Verify “true” estimate only for ETH and fix logger level; consider backoff
- Only run market-rate verification for ETH inputs; else use Skip’s estimate.
- logger.warning → logger.warn.
- Optional: add jitter/backoff between retries.
- trueEstimateAmountOut = await estimateAmountOutSwap(
- routeResult?.sourceAssetChainId,
- routeResult?.amountIn,
- routeResult?.sourceAssetDenom,
- );
- toleranceMet = verifySkipEstimation(routeResult.estimatedAmountOut, trueEstimateAmountOut);
+ if (sourceAssetDenom.toLowerCase() === ethDenomByChainId[chainId]?.toLowerCase()) {
+ trueEstimateAmountOut = await estimateAmountOutSwap(
+ routeResult.sourceAssetChainId,
+ routeResult.amountIn,
+ routeResult.sourceAssetDenom,
+ );
+ } else {
+ // For non-ETH, defer to Skip's estimate.
+ trueEstimateAmountOut = routeResult.estimatedAmountOut;
+ }
+ toleranceMet = verifySkipEstimation(routeResult.estimatedAmountOut, trueEstimateAmountOut);
@@
- logger.warning({
+ logger.warn({
at: 'skip-helper#getSkipCallData',
message: 'estimated amount out is less than our tolerance. Retrying...',
estimatedAmountOut: routeResult.estimatedAmountOut,
actualAmount: trueEstimateAmountOut,
});Optional backoff snippet (illustrative, not required):
// await new Promise(r => setTimeout(r, 200 * (i + 1))); // linear backoff🤖 Prompt for AI Agents
In indexer/services/comlink/src/helpers/skip-helper.ts around lines 93 to 108,
the code currently always recalculates a "true" market-rate estimate and uses
logger.warning; update it to only perform the market-rate verification when the
input asset is ETH (check sourceAssetDenom or chain-specific ETH identifier),
otherwise accept Skip's estimatedAmountOut without re-estimation, change
logger.warning to logger.warn, and (optionally) implement a small jitter/backoff
between retry attempts to avoid tight retry loops.
Comment on lines
146
to
151
| const expectedAmountOut = max([routeResult.estimatedAmountOut, trueEstimateAmountOut]); | ||
| const slippageTolerancePercent = min([ | ||
| 100 / parseInt(expectedAmountOut!, 10), | ||
| parseFloat(config.SKIP_SLIPPAGE_TOLERANCE_PERCENTAGE), | ||
| ])!.toString(); | ||
| const response = await messages({ |
There was a problem hiding this comment.
Numeric max on string amounts and wrong slippage scaling
- max on strings compares lexicographically → wrong pick.
- Slippage for $100 anchor must account for USDC quanta and percent scaling.
- const expectedAmountOut = max([routeResult.estimatedAmountOut, trueEstimateAmountOut]);
- const slippageTolerancePercent = min([
- 100 / parseInt(expectedAmountOut!, 10),
- parseFloat(config.SKIP_SLIPPAGE_TOLERANCE_PERCENTAGE),
- ])!.toString();
+ const estimatedOut = BigInt(routeResult.estimatedAmountOut);
+ const trueOut = BigInt(trueEstimateAmountOut!);
+ const expectedAmountOutBig = estimatedOut > trueOut ? estimatedOut : trueOut;
+ const expectedAmountOut = expectedAmountOutBig.toString();
+
+ // Anchor slippage to max($100, 0.1% of expected)
+ const hundredUsdMicro = BigInt(100) * BigInt(ETH_USDC_QUANTUM);
+ const tenthPercentDelta = expectedAmountOutBig / BigInt(1000); // 0.1%
+ const maxDelta = hundredUsdMicro > tenthPercentDelta ? hundredUsdMicro : tenthPercentDelta;
+ // Convert to percent (be mindful of Number limits; typical ranges are safe)
+ const anchorPercent = Number(maxDelta) / Number(expectedAmountOutBig) * 100;
+ const slippageTolerancePercent = Math.min(
+ anchorPercent,
+ parseFloat(config.SKIP_SLIPPAGE_TOLERANCE_PERCENTAGE),
+ ).toString();
@@
- amountOut: expectedAmountOut,
+ amountOut: expectedAmountOut,Also applies to: 154-154
🤖 Prompt for AI Agents
In indexer/services/comlink/src/helpers/skip-helper.ts around lines 146-151
(also applies to line 154), the code is using max/min on string amounts which
performs lexicographic comparison and incorrectly computes slippage because it
ignores token quanta/decimals; convert the string amounts to a numeric/bignumber
type (e.g., BigNumber, bigint, or Decimal) before comparing so you pick the true
numeric max between routeResult.estimatedAmountOut and trueEstimateAmountOut,
then compute slippage scaling using the anchor token's decimals (e.g., scale
expectedAmountOut to token quanta: expectedAmountOutScaled = expectedAmountOut *
10^anchorDecimals) and derive slippageTolerancePercent as the smaller of
configured percent and (100 * tokenQuanta / expectedAmountOutScaled) or
equivalent formula in the chosen numeric type; ensure all operations use the
same numeric type and handle parse errors/nulls consistently.
Comment on lines
431
to
441
| chainId: string, | ||
| amount: string, | ||
| sourceAssetDenom: string, | ||
| ): Promise<string> { | ||
| if (sourceAssetDenom === ethDenomByChainId[chainId]) { | ||
| const ethPrice = await getETHPrice(); | ||
| // ethPrice multiplied by the amount will give us the rough amount in usdc. | ||
| return (ethPrice * parseInt(amount, 10) * (ETH_USDC_QUANTUM / ETH_WEI_QUANTUM)).toString(); | ||
| } | ||
| return (parseInt(amount, 10) * ETH_USDC_QUANTUM).toString(); | ||
| } |
There was a problem hiding this comment.
estimateAmountOutSwap: unit bug (non‑ETH) and float precision; return integer micro‑USDC
- For ETH, compute USDC micro using BigInt with price in micro‑USD.
- For USDC inputs, return amount as-is (already micro‑USDC).
- Avoid parseInt/float math to prevent precision loss.
export async function estimateAmountOutSwap(
chainId: string,
amount: string,
sourceAssetDenom: string,
): Promise<string> {
if (sourceAssetDenom === ethDenomByChainId[chainId]) {
- const ethPrice = await getETHPrice();
- // ethPrice multiplied by the amount will give us the rough amount in usdc.
- return (ethPrice * parseInt(amount, 10) * (ETH_USDC_QUANTUM / ETH_WEI_QUANTUM)).toString();
+ const ethPrice = await getETHPrice();
+ const amountWei = BigInt(amount);
+ const priceMicroUsd = BigInt(Math.floor(ethPrice * 1_000_000)); // $ in micro units
+ // (wei * microUSD_per_ETH) / wei_per_ETH => micro‑USDC (integer)
+ const outMicroUsdc = (amountWei * priceMicroUsd) / BigInt(ETH_WEI_QUANTUM);
+ return outMicroUsdc.toString();
}
- return (parseInt(amount, 10) * ETH_USDC_QUANTUM).toString();
+ // If the input is already USDC on source chain, amount is micro‑USDC
+ if (
+ Object.values(usdcAddressByChainId).some(
+ (x) => x.toLowerCase() === sourceAssetDenom.toLowerCase(),
+ )
+ ) {
+ return amount;
+ }
+ // Fallback: do not scale unknown assets; caller should gate usage to ETH.
+ return amount;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| chainId: string, | |
| amount: string, | |
| sourceAssetDenom: string, | |
| ): Promise<string> { | |
| if (sourceAssetDenom === ethDenomByChainId[chainId]) { | |
| const ethPrice = await getETHPrice(); | |
| // ethPrice multiplied by the amount will give us the rough amount in usdc. | |
| return (ethPrice * parseInt(amount, 10) * (ETH_USDC_QUANTUM / ETH_WEI_QUANTUM)).toString(); | |
| } | |
| return (parseInt(amount, 10) * ETH_USDC_QUANTUM).toString(); | |
| } | |
| export async function estimateAmountOutSwap( | |
| chainId: string, | |
| amount: string, | |
| sourceAssetDenom: string, | |
| ): Promise<string> { | |
| if (sourceAssetDenom === ethDenomByChainId[chainId]) { | |
| const ethPrice = await getETHPrice(); | |
| const amountWei = BigInt(amount); | |
| const priceMicroUsd = BigInt(Math.floor(ethPrice * 1_000_000)); // $ in micro units | |
| // (wei * microUSD_per_ETH) / wei_per_ETH => micro-USDC (integer) | |
| const outMicroUsdc = (amountWei * priceMicroUsd) / BigInt(ETH_WEI_QUANTUM); | |
| return outMicroUsdc.toString(); | |
| } | |
| // If the input is already USDC on source chain, amount is micro-USDC | |
| if ( | |
| Object.values(usdcAddressByChainId).some( | |
| (x) => x.toLowerCase() === sourceAssetDenom.toLowerCase(), | |
| ) | |
| ) { | |
| return amount; | |
| } | |
| // Fallback: do not scale unknown assets; caller should gate usage to ETH. | |
| return amount; | |
| } |
🤖 Prompt for AI Agents
indexer/services/comlink/src/helpers/skip-helper.ts around lines 431 to 441: the
current implementation uses parseInt and floating math which causes precision
loss and incorrectly handles non‑ETH (USDC) inputs; change logic to use BigInt
throughout and price expressed in micro‑USD (integer) so all math is
integer-only, for ETH compute micro‑USDC as BigInt(amountInWei) *
BigInt(ethPriceInMicroUSD) * BigInt(ETH_USDC_QUANTUM) / BigInt(ETH_WEI_QUANTUM)
and for USDC (sourceAssetDenom === USDC denom) simply return the input amount
unchanged (already micro‑USDC); remove parseInt/float operations and ensure the
function returns a stringified BigInt result.
Comment on lines
450
to
455
| export function verifySkipEstimation(skipEstimateAmount: string, actualAmount: string): boolean { | ||
| const skipEstimateAmountNum = parseInt(skipEstimateAmount, 10); | ||
| const actualAmountNum = parseInt(actualAmount, 10); | ||
| const anchor = max([actualAmountNum * (1 - 100 / actualAmountNum), 0.999 * actualAmountNum]); | ||
| return skipEstimateAmountNum >= anchor!; | ||
| } |
There was a problem hiding this comment.
verifySkipEstimation: $100 anchor not scaled; float math and lodash max
Use BigInt and scale $100 to micro‑USDC. Allow max($100, 0.1% of amount).
-export function verifySkipEstimation(skipEstimateAmount: string, actualAmount: string): boolean {
- const skipEstimateAmountNum = parseInt(skipEstimateAmount, 10);
- const actualAmountNum = parseInt(actualAmount, 10);
- const anchor = max([actualAmountNum * (1 - 100 / actualAmountNum), 0.999 * actualAmountNum]);
- return skipEstimateAmountNum >= anchor!;
-}
+export function verifySkipEstimation(skipEstimateAmount: string, actualAmount: string): boolean {
+ const skip = BigInt(skipEstimateAmount);
+ const actual = BigInt(actualAmount);
+ if (actual === BigInt(0)) return false;
+ const hundredUsd = BigInt(100) * BigInt(ETH_USDC_QUANTUM); // $100 in micro‑USDC
+ const tenthPercent = actual / BigInt(1000); // 0.1%
+ const maxDelta = hundredUsd > tenthPercent ? hundredUsd : tenthPercent;
+ const anchor = actual > maxDelta ? actual - maxDelta : BigInt(0);
+ return skip >= anchor;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function verifySkipEstimation(skipEstimateAmount: string, actualAmount: string): boolean { | |
| const skipEstimateAmountNum = parseInt(skipEstimateAmount, 10); | |
| const actualAmountNum = parseInt(actualAmount, 10); | |
| const anchor = max([actualAmountNum * (1 - 100 / actualAmountNum), 0.999 * actualAmountNum]); | |
| return skipEstimateAmountNum >= anchor!; | |
| } | |
| export function verifySkipEstimation(skipEstimateAmount: string, actualAmount: string): boolean { | |
| const skip = BigInt(skipEstimateAmount); | |
| const actual = BigInt(actualAmount); | |
| if (actual === BigInt(0)) return false; | |
| // $100 in micro-USDC | |
| const hundredUsd = BigInt(100) * BigInt(ETH_USDC_QUANTUM); | |
| // 0.1% of the actual amount | |
| const tenthPercent = actual / BigInt(1000); | |
| // pick the larger delta | |
| const maxDelta = hundredUsd > tenthPercent ? hundredUsd : tenthPercent; | |
| // anchor = actual minus that delta, floor at zero | |
| const anchor = actual > maxDelta ? actual - maxDelta : BigInt(0); | |
| return skip >= anchor; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
indexer/services/comlink/src/helpers/skip-helper.ts (1)
392-400: Consider BigInt for consistency and handle null assertion safely.While
limitAmountensures amounts stay within safe bounds, usingBigIntwould be more robust and consistent with other numeric operations in the codebase (e.g., lines 364, 374, 389).Additionally, the
!null assertion onmin()could fail if both values areNaNor undefined. Add a safety check:-export function getSlippageTolerancePercent(estAmountOut: string): string { - return min([ - (100 * (config.SKIP_SLIPPAGE_TOLERANCE_USDC * ETH_USDC_QUANTUM)) / parseInt(estAmountOut, 10), - parseFloat(config.SKIP_SLIPPAGE_TOLERANCE_PERCENTAGE), - ])!.toString(); -} +export function getSlippageTolerancePercent(estAmountOut: string): string { + const estAmountOutNum = BigInt(estAmountOut); + const anchorPercent = Number( + (BigInt(100) * BigInt(config.SKIP_SLIPPAGE_TOLERANCE_USDC) * BigInt(ETH_USDC_QUANTUM)) / estAmountOutNum + ); + const result = min([ + anchorPercent, + parseFloat(config.SKIP_SLIPPAGE_TOLERANCE_PERCENTAGE), + ]); + if (result === undefined) { + throw new Error('Failed to calculate slippage tolerance'); + } + return result.toString(); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
indexer/services/comlink/__tests__/helpers/skip-helper.test.ts(3 hunks)indexer/services/comlink/src/helpers/skip-helper.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- indexer/services/comlink/tests/helpers/skip-helper.test.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Kefancao
PR: dydxprotocol/v4-chain#3118
File: indexer/services/comlink/__tests__/helpers/skip-helper.test.ts:9-32
Timestamp: 2025-10-15T15:26:24.856Z
Learning: In the skip-helper slippage tolerance implementation for dYdX ETH swaps, `min()` is intentionally used to select the smaller (tighter) slippage tolerance percentage between `SKIP_SLIPPAGE_TOLERANCE_USDC * ETH_USDC_QUANTUM / estimatedAmountOut` and `SKIP_SLIPPAGE_TOLERANCE_PERCENTAGE`. This ensures maximum protection by choosing the most conservative slippage bound, as smaller slippage tolerance means less potential loss.
📚 Learning: 2025-10-15T15:26:24.856Z
Learnt from: Kefancao
PR: dydxprotocol/v4-chain#3118
File: indexer/services/comlink/__tests__/helpers/skip-helper.test.ts:9-32
Timestamp: 2025-10-15T15:26:24.856Z
Learning: In the skip-helper slippage tolerance implementation for dYdX ETH swaps, `min()` is intentionally used to select the smaller (tighter) slippage tolerance percentage between `SKIP_SLIPPAGE_TOLERANCE_USDC * ETH_USDC_QUANTUM / estimatedAmountOut` and `SKIP_SLIPPAGE_TOLERANCE_PERCENTAGE`. This ensures maximum protection by choosing the most conservative slippage bound, as smaller slippage tolerance means less potential loss.
Applied to files:
indexer/services/comlink/src/helpers/skip-helper.ts
🧬 Code graph analysis (1)
indexer/services/comlink/src/helpers/skip-helper.ts (1)
indexer/services/comlink/src/lib/smart-contract-constants.ts (1)
ETH_USDC_QUANTUM(31-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: build-and-push-mainnet
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: call-build-ecs-service-roundtable / (roundtable) Check docker image build
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: call-build-ecs-service-ender / (ender) Check docker image build
- GitHub Check: check-build-bazooka
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: check-build-auxo
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: test / run_command
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build-and-push-testnet
- GitHub Check: Analyze (go)
- GitHub Check: run_command
- GitHub Check: lint
- GitHub Check: Summary
Comment on lines
+115
to
+117
| // acceptable slippage is smallest of SKIP_SLIPPAGE_TOLERANCE_USDC (Default $100) divided | ||
| // by the estimatedAmountOut or the SKIP_SLIPPAGE_TOLERANCE_PERCENTAGE. | ||
| const slippageTolerancePercent = getSlippageTolerancePercent(routeResult.estimatedAmountOut); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Clarify: Implementation differs from PR specification.
The comment describes the current implementation (min($100/amount, maxPercent)), but the PR description states slippage should be "anchored to max($100, 0.001 * amount)", which translates to:
slippageDelta = max($100_in_micro_USDC, 0.001 * estimatedAmountOut)
slippagePercent = (slippageDelta / estimatedAmountOut) * 100
For a $200,000 amount:
- Current implementation:
min($100/$200k, maxPercent)= ~0.05% - PR specification:
max($100, $200)/$200k= 0.1%
These produce different results. Based on learnings, min() was chosen intentionally for "maximum protection," but this contradicts the stated PR objective. Please either update the PR description to match the implementation, or update the implementation to match the specification.
Based on learnings.
🏁 Script executed:
#!/bin/bash
set -e
rg -n "function getSlippageTolerancePercent" -C5 indexer/services/comlink/src/helpers/skip-helper.tsLength of output: 520
Align slippage tolerance formula with PR specification
getSlippageTolerancePercent (lines 395–400) currently computes
min(
SKIP_SLIPPAGE_TOLERANCE_USDC/estimatedAmountOut,
SKIP_SLIPPAGE_TOLERANCE_PERCENTAGE
)
but the PR description specifies
max(SKIP_SLIPPAGE_TOLERANCE_USDC, 0.001 * estimatedAmountOut) / estimatedAmountOut
Please update the implementation or the PR description to ensure they match the intended formula.
🤖 Prompt for AI Agents
In indexer/services/comlink/src/helpers/skip-helper.ts around lines 115 to 117,
the slippage tolerance calculation uses getSlippageTolerancePercent which
currently returns min(SKIP_SLIPPAGE_TOLERANCE_USDC/estimatedAmountOut,
SKIP_SLIPPAGE_TOLERANCE_PERCENTAGE) but the PR intends
max(SKIP_SLIPPAGE_TOLERANCE_USDC, 0.001 * estimatedAmountOut) /
estimatedAmountOut; update getSlippageTolerancePercent to compute
max(SKIP_SLIPPAGE_TOLERANCE_USDC, 0.001 * estimatedAmountOut) divided by
estimatedAmountOut (and clamp to 0..SKIP_SLIPPAGE_TOLERANCE_PERCENTAGE if
needed), or if the original min-based behavior is desired, update the PR
description to match the implemented formula.
shrenujb
reviewed
Oct 17, 2025
| TURNKEY_API_SENDER_PUBLIC_KEY: 'test-public-key', | ||
| TURNKEY_API_SENDER_PRIVATE_KEY: 'test-private-key', | ||
| SKIP_SLIPPAGE_TOLERANCE_USDC: 100, | ||
| SKIP_SLIPPAGE_TOLERANCE_PERCENTAGE: '0.1', // 10% |
There was a problem hiding this comment.
Is this 10% or 10bps? 10% seems like a lot?
shrenujb
reviewed
Oct 17, 2025
| APPROVAL_ENABLED: parseBoolean({ default: true }), | ||
| // largest amount we will tolerate to swap in usdc. | ||
| MAXIMUM_BRIDGE_AMOUNT_USDC: parseInteger({ default: 99_900 }), | ||
| MAXIMUM_BRIDGE_AMOUNT_USDC: parseInteger({ default: 999_000 }), |
There was a problem hiding this comment.
Why are we increasing this?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Changelist
Verify against the amount skip quotes for eth swaps and set the amount to be the higher of the two. Additionally, specify a tolerance percent that is anchored to
max($100, 0.001 * amount).Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Chores
Tests