taking Price Impact into account for partial trade size#433
taking Price Impact into account for partial trade size#433
Price Impact into account for partial trade size#433Conversation
WalkthroughThe PR extends the router infrastructure to include optional route information in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/state/index.ts (1)
337-345: 🧹 Nitpick | 🔵 TrivialThe synthetic
orderDetailsis a code smell.
{ takeOrder: { quote: { ratio: 0n } } } as anyworks becauseabsolutemode ignoresratio, but this couples the callsite to an implementation detail offindLargestTradeSize. A dedicated overload or a distinct method signature for the absolute-mode case would be cleaner.
🤖 Fix all issues with AI agents
In `@src/router/sushi/index.test.ts`:
- Around line 1026-1167: Add an edge-case test that verifies
calculateEffectivePrice handles priceImpact >= 1 without throwing (e.g.,
priceImpact: 1.5) by creating a route with amountOutBI and maximumInput and
asserting the call does not throw and returns parseUnits("0", 18) (or the
defined minimum/clamped value) — target the calculateEffectivePrice function and
use parseUnits for inputs and expected output so the test ensures the guard in
index.ts prevents negative/invalid strings passed to parseUnits.
In `@src/router/sushi/index.ts`:
- Around line 556-571: In calculateEffectivePrice, guard against
route.priceImpact outside [0,1] before computing the multiplier: validate or
clamp route.priceImpact to the 0..1 range (e.g. let impact = Math.min(1,
Math.max(0, route.priceImpact))) and use that clamped value in the parseUnits
call instead of route.priceImpact; keep references to calculatePrice18,
parseUnits, ONE18 and MultiRoute.priceImpact so you locate the code, and ensure
the function returns a non-negative, logically valid bigint price.
In `@src/router/types.ts`:
- Line 22: Fix the typo in the comment text in src/router/types.ts: change
"Default price imapct tolerance used for getting unit market price." to "Default
price impact tolerance used for getting unit market price." — locate the comment
containing "Default price imapct tolerance" and update the word "imapct" to
"impact" so the documentation string is correct.
- Line 26: The DEFAULT_PRICE_IMPACT_TOLERANCE constant is set to 2.5 (250%) but
the code/tests use a 0–1 fractional scale; change the exported constant
DEFAULT_PRICE_IMPACT_TOLERANCE from 2.5 to 0.025 (2.5% expressed as a fraction)
and keep the as const typing so callers using priceImpact comparisons (e.g., any
logic referencing DEFAULT_PRICE_IMPACT_TOLERANCE) correctly treat it as a 0–1
tolerance.
| describe("calculateEffectivePrice", () => { | ||
| const mockFromToken = new Token({ | ||
| address: "0x1111111111111111111111111111111111111111", | ||
| decimals: 18, | ||
| symbol: "TOKEN1", | ||
| chainId: 1, | ||
| name: "Token 1", | ||
| }); | ||
|
|
||
| const mockToToken = new Token({ | ||
| address: "0x2222222222222222222222222222222222222222", | ||
| decimals: 6, | ||
| symbol: "TOKEN2", | ||
| chainId: 1, | ||
| name: "Token 2", | ||
| }); | ||
|
|
||
| it("should return the base price when priceImpact is undefined", () => { | ||
| const maximumInput = parseUnits("100", 18); | ||
| const route: MultiRoute = { | ||
| status: RouteStatus.Success, | ||
| amountOutBI: parseUnits("200", 6), | ||
| priceImpact: undefined, | ||
| } as any as MultiRoute; | ||
|
|
||
| const result = calculateEffectivePrice(maximumInput, route, mockFromToken, mockToToken); | ||
|
|
||
| // Expected price: (200 * 10^6 * 10^18) / (100 * 10^18) = 2 * 10^6 = 2000000 | ||
| // Scaled to 18 decimals: 2 * 10^18 | ||
| expect(result).toBe(parseUnits("2", 18)); | ||
| }); | ||
|
|
||
| it("should apply price impact when priceImpact is defined", () => { | ||
| const maximumInput = parseUnits("100", 18); | ||
| const route: MultiRoute = { | ||
| status: RouteStatus.Success, | ||
| amountOutBI: parseUnits("200", 6), | ||
| priceImpact: 0.05, // 5% price impact | ||
| } as any as MultiRoute; | ||
|
|
||
| const result = calculateEffectivePrice(maximumInput, route, mockFromToken, mockToToken); | ||
|
|
||
| // Base price: 2 * 10^18 | ||
| // With 5% impact: 2 * 0.95 = 1.9 * 10^18 | ||
| expect(result).toBe(parseUnits("1.9", 18)); | ||
| }); | ||
|
|
||
| it("should handle zero price impact", () => { | ||
| const maximumInput = parseUnits("100", 18); | ||
| const route: MultiRoute = { | ||
| status: RouteStatus.Success, | ||
| amountOutBI: parseUnits("150", 6), | ||
| priceImpact: 0, | ||
| } as any as MultiRoute; | ||
|
|
||
| const result = calculateEffectivePrice(maximumInput, route, mockFromToken, mockToToken); | ||
|
|
||
| // Price: 1.5 * 10^18, no impact | ||
| expect(result).toBe(parseUnits("1.5", 18)); | ||
| }); | ||
|
|
||
| it("should handle small price impact correctly", () => { | ||
| const maximumInput = parseUnits("1000", 18); | ||
| const route: MultiRoute = { | ||
| status: RouteStatus.Success, | ||
| amountOutBI: parseUnits("1000", 6), | ||
| priceImpact: 0.001, // 0.1% price impact | ||
| } as any as MultiRoute; | ||
|
|
||
| const result = calculateEffectivePrice(maximumInput, route, mockFromToken, mockToToken); | ||
|
|
||
| // Base price: 1 * 10^18 | ||
| // With 0.1% impact: 1 * 0.999 = 0.999 * 10^18 | ||
| expect(result).toBe(parseUnits("0.999", 18)); | ||
| }); | ||
|
|
||
| it("should handle very small amounts", () => { | ||
| const maximumInput = parseUnits("0.001", 18); // 1e-3 | ||
| const route: MultiRoute = { | ||
| status: RouteStatus.Success, | ||
| amountOutBI: parseUnits("0.002", 6), // 2e-3 | ||
| priceImpact: 0.1, // 10% price impact | ||
| } as any as MultiRoute; | ||
|
|
||
| const result = calculateEffectivePrice(maximumInput, route, mockFromToken, mockToToken); | ||
|
|
||
| // Base price: 2 * 10^18 | ||
| // With 10% impact: 2 * 0.9 = 1.8 * 10^18 | ||
| expect(result).toBe(parseUnits("1.8", 18)); | ||
| }); | ||
|
|
||
| it("should handle different token decimals", () => { | ||
| const token8Decimals: Token = { | ||
| ...mockFromToken, | ||
| decimals: 8, | ||
| } as Token; | ||
|
|
||
| const maximumInput = parseUnits("50", 8); | ||
| const route: MultiRoute = { | ||
| status: RouteStatus.Success, | ||
| amountOutBI: parseUnits("100", 6), | ||
| priceImpact: 0.02, // 2% price impact | ||
| } as any as MultiRoute; | ||
|
|
||
| const result = calculateEffectivePrice(maximumInput, route, token8Decimals, mockToToken); | ||
|
|
||
| // Base price: 2 * 10^18 | ||
| // With 2% impact: 2 * 0.98 = 1.96 * 10^18 | ||
| expect(result).toBe(parseUnits("1.96", 18)); | ||
| }); | ||
|
|
||
| it("should handle high price impact", () => { | ||
| const maximumInput = parseUnits("1000", 18); | ||
| const route: MultiRoute = { | ||
| status: RouteStatus.Success, | ||
| amountOutBI: parseUnits("500", 6), | ||
| priceImpact: 0.5, // 50% price impact | ||
| } as any as MultiRoute; | ||
|
|
||
| const result = calculateEffectivePrice(maximumInput, route, mockFromToken, mockToToken); | ||
|
|
||
| // Base price: 0.5 * 10^18 | ||
| // With 50% impact: 0.5 * 0.5 = 0.25 * 10^18 | ||
| expect(result).toBe(parseUnits("0.25", 18)); | ||
| }); | ||
|
|
||
| it("should handle very small price impact (scientific notation)", () => { | ||
| const maximumInput = parseUnits("10000", 18); | ||
| const route: MultiRoute = { | ||
| status: RouteStatus.Success, | ||
| amountOutBI: parseUnits("10000", 6), | ||
| priceImpact: 1e-20, // extremely small impact | ||
| } as any as MultiRoute; | ||
|
|
||
| const result = calculateEffectivePrice(maximumInput, route, mockFromToken, mockToToken); | ||
|
|
||
| // Base price: 1 * 10^18 | ||
| // With negligible impact, should be very close to base price | ||
| expect(result).toBeGreaterThan(parseUnits("0.999999999999999999", 18)); | ||
| expect(result).toBeLessThanOrEqual(parseUnits("1", 18)); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good test coverage for calculateEffectivePrice.
The tests thoroughly cover various scenarios. Consider adding an edge-case test for priceImpact >= 1 (e.g., 1.5) to validate the guard suggested in src/router/sushi/index.ts, since parseUnits would throw on the resulting negative string.
🤖 Prompt for AI Agents
In `@src/router/sushi/index.test.ts` around lines 1026 - 1167, Add an edge-case
test that verifies calculateEffectivePrice handles priceImpact >= 1 without
throwing (e.g., priceImpact: 1.5) by creating a route with amountOutBI and
maximumInput and asserting the call does not throw and returns parseUnits("0",
18) (or the defined minimum/clamped value) — target the calculateEffectivePrice
function and use parseUnits for inputs and expected output so the test ensures
the guard in index.ts prevents negative/invalid strings passed to parseUnits.
| export function calculateEffectivePrice( | ||
| maximumInput: bigint, | ||
| route: MultiRoute, | ||
| fromToken: Token, | ||
| toToken: Token, | ||
| ): bigint { | ||
| const price = calculatePrice18( | ||
| maximumInput, | ||
| route.amountOutBI, | ||
| fromToken.decimals, | ||
| toToken.decimals, | ||
| ); | ||
| if (typeof route.priceImpact === "undefined") { | ||
| return price; | ||
| } | ||
| return (price * parseUnits((1 - route.priceImpact).toFixed(12), 18)) / ONE18; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
viem parseUnits negative number string behavior documentation
💡 Result:
parseUnits does accept negative decimal strings (e.g. "-1.5"). Internally it detects a leading -, parses/rounds the absolute value the same way as for positives, then re-applies the minus sign and returns a negative bigint. [2]
So, for example (behavior implied by implementation):
parseUnits('-1', 18)→-1000000000000000000nparseUnits('-1.234', 2)→-123n(fraction is rounded/truncated to the given decimals) [2]
The official docs describe parseUnits as multiplying a string number by 10**exponent, but don’t explicitly call out negative input. [1]
Sources:
[1] Viem docs – parseUnits https://viem.sh/docs/utilities/parseUnits
[2] Viem implementation (parseUnits.js) https://app.unpkg.com/viem%402.0.6/files/_cjs/utils/unit/parseUnits.js
🏁 Script executed:
# First, let's examine the actual code around lines 556-571
cat -n src/router/sushi/index.ts | sed -n '550,575p'Repository: rainlanguage/rain.solver
Length of output: 748
🏁 Script executed:
# Check how priceImpact is validated or bounded in the codebase
rg "priceImpact" -B 2 -A 2 --max-count 20Repository: rainlanguage/rain.solver
Length of output: 4513
The concern about parseUnits throwing is incorrect, but defensive bounds-checking would still be prudent.
parseUnits from viem does accept negative decimal strings and returns a negative bigint rather than throwing. However, if route.priceImpact exceeds 1, the calculation (1 - route.priceImpact) still produces a negative value, leading to a negative or zero effective price—a logically invalid result for a price calculation.
While this scenario is unlikely (tests only show priceImpact values between 0 and 1), adding explicit bounds-checking would prevent nonsensical outcomes:
- return (price * parseUnits((1 - route.priceImpact).toFixed(12), 18)) / ONE18;
+ const impactMultiplier = Math.max(0, 1 - route.priceImpact);
+ return (price * parseUnits(impactMultiplier.toFixed(12), 18)) / ONE18;📝 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.
| export function calculateEffectivePrice( | |
| maximumInput: bigint, | |
| route: MultiRoute, | |
| fromToken: Token, | |
| toToken: Token, | |
| ): bigint { | |
| const price = calculatePrice18( | |
| maximumInput, | |
| route.amountOutBI, | |
| fromToken.decimals, | |
| toToken.decimals, | |
| ); | |
| if (typeof route.priceImpact === "undefined") { | |
| return price; | |
| } | |
| return (price * parseUnits((1 - route.priceImpact).toFixed(12), 18)) / ONE18; | |
| export function calculateEffectivePrice( | |
| maximumInput: bigint, | |
| route: MultiRoute, | |
| fromToken: Token, | |
| toToken: Token, | |
| ): bigint { | |
| const price = calculatePrice18( | |
| maximumInput, | |
| route.amountOutBI, | |
| fromToken.decimals, | |
| toToken.decimals, | |
| ); | |
| if (typeof route.priceImpact === "undefined") { | |
| return price; | |
| } | |
| const impactMultiplier = Math.max(0, 1 - route.priceImpact); | |
| return (price * parseUnits(impactMultiplier.toFixed(12), 18)) / ONE18; | |
| } |
🤖 Prompt for AI Agents
In `@src/router/sushi/index.ts` around lines 556 - 571, In
calculateEffectivePrice, guard against route.priceImpact outside [0,1] before
computing the multiplier: validate or clamp route.priceImpact to the 0..1 range
(e.g. let impact = Math.min(1, Math.max(0, route.priceImpact))) and use that
clamped value in the parseUnits call instead of route.priceImpact; keep
references to calculatePrice18, parseUnits, ONE18 and MultiRoute.priceImpact so
you locate the code, and ensure the function returns a non-negative, logically
valid bigint price.
| } from "../order"; | ||
|
|
||
| /* | ||
| * Default price imapct tolerance used for getting unit market price. |
There was a problem hiding this comment.
Typo: "imapct" → "impact"
- * Default price imapct tolerance used for getting unit market price.
+ * Default price impact tolerance used for getting unit market price.📝 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.
| * Default price imapct tolerance used for getting unit market price. | |
| * Default price impact tolerance used for getting unit market price. |
🤖 Prompt for AI Agents
In `@src/router/types.ts` at line 22, Fix the typo in the comment text in
src/router/types.ts: change "Default price imapct tolerance used for getting
unit market price." to "Default price impact tolerance used for getting unit
market price." — locate the comment containing "Default price imapct tolerance"
and update the word "imapct" to "impact" so the documentation string is correct.
| * this is not used for actual trade size finding in simulations but | ||
| * only for getting market price for unit token. | ||
| */ | ||
| export const DEFAULT_PRICE_IMPACT_TOLERANCE = 2.5 as const; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how priceImpact is used/compared throughout the codebase
rg -n "priceImpact" --type=ts -C3Repository: rainlanguage/rain.solver
Length of output: 6810
Change DEFAULT_PRICE_IMPACT_TOLERANCE from 2.5 to 0.025.
priceImpact uses a 0–1 fractional scale as confirmed by the tests (e.g., 0.05 = 5%, 0.1 = 10%). A tolerance of 2.5 means 250% and effectively disables tolerance checking. The intended value for a 2.5% tolerance is 0.025.
🤖 Prompt for AI Agents
In `@src/router/types.ts` at line 26, The DEFAULT_PRICE_IMPACT_TOLERANCE constant
is set to 2.5 (250%) but the code/tests use a 0–1 fractional scale; change the
exported constant DEFAULT_PRICE_IMPACT_TOLERANCE from 2.5 to 0.025 (2.5%
expressed as a fraction) and keep the as const typing so callers using
priceImpact comparisons (e.g., any logic referencing
DEFAULT_PRICE_IMPACT_TOLERANCE) correctly treat it as a 0–1 tolerance.
1 participant
Motivation
This PR enables accounting for sushi's swap Price Impact for finding the partial trade size.
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Bug Fixes
Tests