Skip to content

taking Price Impact into account for partial trade size #433

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.

Sign up for GitHub

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

Open
rouzwelt wants to merge 1 commit into
base: master
Choose a base branch
from
+214 −21
Open

taking Price Impact into account for partial trade size #433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/router/router.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@ import { Pair } from "../order";
import { SushiRouter } from "./sushi";
import { Token } from "sushi/currency";
import { Err, Result } from "../common";
import { MultiRoute } from "sushi/tines";
import { StabullRouter } from "./stabull";
import { LiquidityProviders } from "sushi";
import { BalancerRouter } from "./balancer";
import { Account, Chain, PublicClient, Transport, parseUnits } from "viem";
import { StabullRouterError, StabullRouterErrorType } from "./stabull/error";
import {
TradeParamsType,
GetTradeParamsArgs,
Expand All @@ -20,8 +23,6 @@ import {
BalancerRouterErrorType,
RainSolverRouterErrorType,
} from "./error";
import { StabullRouter } from "./stabull";
import { StabullRouterError, StabullRouterErrorType } from "./stabull/error";

export type RainSolverRouterConfig = {
/** The chain id of the operating chain */
Expand Down Expand Up @@ -139,7 +140,7 @@ export class RainSolverRouter extends RainSolverRouterBase {
*/
async getMarketPrice(
params: RainSolverRouterQuoteParams,
): Promise<Result<{ price: string }, RainSolverRouterError>> {
): Promise<Result<{ price: string; route?: MultiRoute }, RainSolverRouterError>> {
const key = `${params.fromToken.address.toLowerCase()}-${params.toToken.address.toLowerCase}`;
let value = this.cache.get(key);
if (typeof value === "number") {
Expand Down Expand Up @@ -170,7 +171,7 @@ export class RainSolverRouter extends RainSolverRouterBase {
if (results.every((res) => !res?.isOk())) {
return Result.err(getError("Failed to get market price", results));
}
return results[0] as Result<{ price: string }, RainSolverRouterError>;
return results[0] as Result<{ price: string; route?: MultiRoute }, RainSolverRouterError>;
}

/**
Expand Down
150 changes: 147 additions & 3 deletions src/router/sushi/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
import { ONE18 } from "../../math";
import { Order, OrderbookVersions, TakeOrdersConfigType } from "../../order";
import { RouteLeg } from "sushi/tines";
import { Token } from "sushi/currency";
import { SharedState } from "../../state";
import { Dispair, Result } from "../../common";
import { maxUint256, PublicClient } from "viem";
import { calculateEffectivePrice } from "./index";
import { RouterType, RouteStatus } from "../types";
import { RouteLeg, MultiRoute } from "sushi/tines";
import { maxUint256, PublicClient, parseUnits } from "viem";
import { SushiRouterError, SushiRouterErrorType } from "./error";
import { LiquidityProviders, RainDataFetcher, Router } from "sushi";
import { describe, it, expect, vi, beforeEach, Mock, assert } from "vitest";
import { Order, OrderbookVersions, TakeOrdersConfigType } from "../../order";
import { SushiRouter, SushiQuoteParams, ExcludedLiquidityProviders } from ".";

// mock the sushi dependencies
Expand Down Expand Up @@ -1021,3 +1022,146 @@ describe("test SushiRouter methods", () => {
});
});
});

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));
});
});
Comment on lines +1026 to +1167
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

60 changes: 48 additions & 12 deletions src/router/sushi/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,24 @@ import { MultiRoute, RouteLeg } from "sushi/tines";
import { BlackListSet, poolFilter } from "./blacklist";
import { TakeOrdersConfigType } from "../../order/types";
import { SushiRouterError, SushiRouterErrorType } from "./error";
import { calculatePrice18, scaleFrom18, scaleTo18 } from "../../math";
import { calculatePrice18, ONE18, scaleFrom18, scaleTo18 } from "../../math";
import { ChainId, LiquidityProviders, PoolCode, RainDataFetcher, Router } from "sushi";
import { Chain, Account, Transport, formatUnits, PublicClient, encodeAbiParameters } from "viem";
import {
Chain,
Account,
Transport,
parseUnits,
formatUnits,
PublicClient,
encodeAbiParameters,
} from "viem";
import {
RouterType,
RouteStatus,
GetTradeParamsArgs,
RainSolverRouterBase,
RainSolverRouterQuoteParams,
DEFAULT_PRICE_IMPACT_TOLERANCE,
} from "../types";
import {
ROUTE_PROCESSOR_3_ADDRESS,
Expand Down Expand Up @@ -153,7 +162,7 @@ export class SushiRouter extends RainSolverRouterBase {
*/
async getMarketPrice(
params: SushiQuoteParams,
): Promise<Result<{ price: string }, SushiRouterError>> {
): Promise<Result<{ price: string; route?: MultiRoute }, SushiRouterError>> {
// return early if from and to tokens are the same
if (params.fromToken.address.toLowerCase() === params.toToken.address.toLowerCase()) {
return Result.ok({ price: "1" });
Expand All @@ -163,7 +172,10 @@ export class SushiRouter extends RainSolverRouterBase {
if (quoteResult.isErr()) {
return Result.err(quoteResult.error);
}
return Result.ok({ price: formatUnits(quoteResult.value.price, 18) });
return Result.ok({
price: formatUnits(quoteResult.value.price, 18),
route: quoteResult.value.route.route,
});
}

/**
Expand Down Expand Up @@ -508,17 +520,23 @@ export class SushiRouter extends RainSolverRouterBase {
if (route.status == "NoWay") {
maximumInput = maximumInput - initAmount / 2n ** i;
} else if (absolute) {
result.unshift(maxInput18);
maximumInput = maximumInput + initAmount / 2n ** i;
if (
typeof route.priceImpact === "undefined" ||
route.priceImpact < DEFAULT_PRICE_IMPACT_TOLERANCE
) {
result.unshift(maxInput18);
maximumInput = maximumInput + initAmount / 2n ** i;
} else {
maximumInput = maximumInput - initAmount / 2n ** i;
}
} else {
const price = calculatePrice18(
const effectivePrice = calculateEffectivePrice(
maximumInput,
route.amountOutBI,
fromToken.decimals,
toToken.decimals,
route,
fromToken,
toToken,
);

if (price < ratio) {
if (effectivePrice < ratio) {
maximumInput = maximumInput - initAmount / 2n ** i;
} else {
result.unshift(maxInput18);
Expand All @@ -534,3 +552,21 @@ export class SushiRouter extends RainSolverRouterBase {
}
}
}

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;
Comment on lines +556 to +571
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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) → -1000000000000000000n
  • parseUnits('-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 20

Repository: 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.

Suggested change
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.

}
7 changes: 7 additions & 0 deletions src/router/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@ import {
TakeOrdersConfigTypeV5,
} from "../order";

/*
* Default price imapct tolerance used for getting unit market price.
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
* 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;
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check how priceImpact is used/compared throughout the codebase
rg -n "priceImpact" --type=ts -C3

Repository: 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.


/** Represents the different router types */
export enum RouterType {
/** The Sushi router (RainDataFetcher) */
Expand Down
9 changes: 7 additions & 2 deletions src/state/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { GasManager } from "../gas";
import { ChainId } from "sushi/chain";
import { AppOptions } from "../config";
import { Token } from "sushi/currency";
import { BalancerRouter } from "../router";
import { BalancerRouter, DEFAULT_PRICE_IMPACT_TOLERANCE } from "../router";
import { LiquidityProviders } from "sushi";
import { SolverContracts } from "./contracts";
import { SushiRouter } from "../router/sushi";
Expand Down Expand Up @@ -326,7 +326,12 @@ export class SharedState {
sushiRouteType: this.appOptions.route,
skipFetch: !!skipFetch,
});
if (result.isOk()) {
if (
result.isOk() &&
(!result.value.route ||
typeof result.value.route.priceImpact === "undefined" ||
result.value.route.priceImpact <= DEFAULT_PRICE_IMPACT_TOLERANCE)
) {
return result;
}
const partialAmountIn = this.router.findLargestTradeSize(
Expand Down
Loading