Open
Conversation
Contributor
WalkthroughAdds Raindex router trade support across config, validation, ABI/types, mode orchestration, order discovery, profitability utilities, and simulation, including new v6 RouteLeg ABI, raindexArb/raindexRouter config keys, a Raindex router mode, and a RaindexRouterTradeSimulator with tests. Changes
Sequence DiagramsequenceDiagram
participant Solver as RainSolver
participant Mode as RaindexMode
participant Order as OrderManager
participant Router as SushiRouter
participant Sim as RaindexSimulator
participant Chain as Blockchain
Solver->>Mode: findBestRaindexRouterTrade(orderDetails,...)
Mode->>Mode: validate v6 orderbook & raindexArb configured
Mode->>Order: getCounterpartyOrdersAgainstBaseTokens(orderDetails)
Order-->>Mode: Map<token, Pair[]>
Mode->>Router: request route/quotes per base token
Router-->>Mode: route visuals & RP params
Mode->>Mode: estimateProfit(counterparty, quote)
Mode->>Mode: select top candidates
Mode->>Sim: RaindexSimulator.withArgs(candidate)
Sim->>Sim: prepareTradeParams() (encode route leg, build takeOrders)
Sim->>Chain: getEnsureBountyTaskBytecode()
Chain-->>Sim: task bytecode
Sim->>Sim: getCalldata() → encoded arb4(...)
Sim-->>Mode: Prepared transaction
Mode->>Sim: simulate execution
Sim-->>Mode: SimulationResult
Mode->>Solver: return best SimulationResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 21
🤖 Fix all issues with AI agents
In `@src/common/abis/orderbook.ts`:
- Line 124: RouteLeg is missing an "as const" assertion so its literal ABI
string widens to string and breaks type inference when used with
parseAbiParameters (e.g., parseAbiParameters(_v6.RouteLeg)). Fix by adding the
as const assertion to the RouteLeg export (the exported constant named RouteLeg)
so it preserves the literal type; ensure any other occurrences in the _v6 ABI
group follow the same pattern to keep parseAbiParameters strongly typed.
- Around line 150-154: The ABI for Arb.arb4 changed to require four args
(orderBook, startTakeOrders[], exchangeData, task); update all call sites that
invoke arb4 (notably in the router and inter simulate functions) to pass the new
exchangeData parameter between startTakeOrders and task, e.g., modify the calls
in simulate.ts where arb4 is invoked to include an appropriate bytes
exchangeData value (or an empty bytes if no data) as the third argument; also
update any unit/integration tests that invoke arb4 to reflect the new
four-argument signature and adjust expected behavior accordingly.
In `@src/core/modes/index.test.ts`:
- Around line 23-29: Remove the leftover commented-out balancer mock in
index.test.ts (the vi.mock call for "./balancer" and findBestBalancerTrade);
either delete those three commented lines entirely or replace them with a brief
inline comment explaining why balancer support is omitted (e.g., removed/handled
in another PR) so future readers aren’t confused—locate the commented block near
the existing vi.mock("./raindex") mock to edit.
- Line 674: The test title string in the it(...) block "should return only
inter-orderbook trade when orderbook is in raindex router set" is misleading;
change that description to accurately reference the raindex router (for example:
"should return only trades routed via raindex router when orderbook is in
raindex router set") so the test name matches the behavior being asserted in the
test case (modify the it(...) title in src/core/modes/index.test.ts).
- Around line 323-328: The test has a copy-paste bug: the raindexResult constant
is created with type: "interOrderbook" instead of the correct "raindex"; update
the object literal in raindexResult (the Result.ok call named raindexResult) to
set type: "raindex" so the test validates raindex results using the correct type
field.
In `@src/core/modes/raindex/index.test.ts`:
- Line 937: The test case name in the it(...) call currently reads "should
return corrrect profit when order max input is lower counterparty max output"
(note the function symbol is the it(...) in
src/core/modes/raindex/index.test.ts) — fix the typo by renaming the test string
to "should return correct profit when order max input is lower counterparty max
output" so the test description is spelled correctly.
- Around line 23-32: The mocked Token class in the vi.mock for "sushi/currency"
returns a plain object from its constructor (return { ...args }), which violates
the noConstructorReturn rule and can break instanceof checks; change the mock to
either use a factory function that returns a proper Token-like object or mutate
the instance instead (e.g., in the Token constructor use Object.assign(this,
args) or populate properties on this) so the constructor returns the instance
rather than a new plain object, keeping the mock defined under
vi.mock("sushi/currency") and preserving the Token symbol.
- Around line 129-133: Replace incorrect assertions that use
.not.toHaveBeenCalledWith() with no arguments — which only checks for a call
with zero args — with .not.toHaveBeenCalled() to assert the mock was never
invoked; update the occurrences that reference
solver.state.contracts.getAddressesForTrade,
solver.orderManager.getCounterpartyOrdersAgainstBaseTokens, and
solver.state.router.sushi!.tryQuote (and the other similar spots in the same
test file) so each uses .not.toHaveBeenCalled() instead of
.not.toHaveBeenCalledWith().
In `@src/core/modes/raindex/index.ts`:
- Around line 28-38: Update the JSDoc comment in src/core/modes/raindex/index.ts
for the RainSolver method that begins "Tries to find the best raindex routed
trade..." and correct the typo "swaped" to "swapped" so the sentence reads
"...swapped through sushi RP"; ensure the change is applied in the JSDoc block
associated with the RainSolver/raindex function signature.
- Around line 196-204: The shared spanAttributes object is being mutated inside
the .map() callback (see Pair.isV4OrderbookV6 and spanAttributes usage) which
can overwrite or leak error context across counterparties; instead create a
local per-counterparty attributes object (e.g., let localSpanAttrs = {
...spanAttributes } or a new object) inside the map/callback and set
localSpanAttrs["error"] when Pair.isV4OrderbookV6(counterpartyOrderDetails)
fails, then return Result.err({... , spanAttributes: localSpanAttrs, ...}) as
SimulationResult so each error carries its own isolated attributes and the outer
spanAttributes remains unchanged for success paths.
In `@src/core/modes/raindex/simulation.ts`:
- Line 19: Update the top doc comment in src/core/modes/raindex/simulation.ts by
replacing the phrase "inter-orderbook trade" with "raindex router trade" so the
comment accurately describes the arguments for simulating a raindex router
trade; locate the comment that currently reads "Arguments for simulating
inter-orderbook trade" and change only the wording to "Arguments for simulating
raindex router trade".
- Around line 59-66: Edit the JSDoc above the RaindexRouterTradeSimulator class
(which extends TradeSimulatorBase) to fix typos and grammar: change "wwith" to
"with", "a external" to "an external", and ensure the phrase reads "Simulates a
trade between two orders with different IO through an external route" (also
correct "two order" → "two orders" if present); update any other small
grammatical issues in that block so the description is clear and consistent.
In `@src/core/modes/raindex/utils.test.ts`:
- Around line 9-141: Tests for calcCounterpartyInputProfit only use
buyTokenDecimals: 18 so scaleTo18 logic is untested; add a new unit test in
src/core/modes/raindex/utils.test.ts that uses a counterparty with
buyTokenDecimals: 6 (or another non-18 value) and a quote where
amountOut/maxOutput are scaled accordingly, then assert both
counterpartyInputProfit and counterpartyMaxOutput reflect the decimal scaling
(reference calcCounterpartyInputProfit and scaleTo18 to craft expected values).
In `@src/core/modes/raindex/utils.ts`:
- Around line 51-58: The function calcCounterpartyInputToEthPrice can divide by
quote.price which may be 0n; add a guard at the start of
calcCounterpartyInputToEthPrice to return 0n when quote.price === 0n (similar to
the check in calcCounterpartyInputProfit), so parsing outputToEthPrice and the
division (outputEthPrice * ONE18) / quote.price only run when quote.price is
non-zero; reference the function name calcCounterpartyInputToEthPrice and the
symbol quote.price when adding the early-return check.
- Line 17: Rename the misspelled variable counterpatryMaxInput to
counterpartyMaxInput everywhere in this file and update all references (the
declaration and uses at the occurrences you noted) so the identifier matches;
search for counterpatryMaxInput and replace with counterpartyMaxInput to avoid
runtime/reference errors and keep naming consistent with related variables like
maxSushiOutput.
In `@src/order/index.test.ts`:
- Around line 22-26: Tests mutate the mocked config object
BASES_TO_CHECK_TRADES_AGAINST created via vi.mock, and vi.clearAllMocks() does
not revert object property changes, so add a reset in beforeEach to restore the
mock state (e.g., call vi.resetModules() or explicitly reassign
BASES_TO_CHECK_TRADES_AGAINST[1] = [] before each test). Update the beforeEach
in src/order/index.test.ts (where vi.clearAllMocks() is used) to either call
vi.resetModules() and re-import the mock or directly reset the
BASES_TO_CHECK_TRADES_AGAINST[1] property so each test starts with a clean
array.
In `@src/order/index.ts`:
- Around line 531-538: The guard checking tokens can throw if
BASES_TO_CHECK_TRADES_AGAINST[this.state.chainConfig.id] is undefined; update
the condition in the loop that references tkn and sellToken so the .every() call
uses a null-coalescing default (e.g.,
BASES_TO_CHECK_TRADES_AGAINST[this.state.chainConfig.id] ?? []) before calling
.every(), ensuring the expression safely falls back to an empty array when the
chain id is not present.
In `@src/order/pair.test.ts`:
- Around line 463-665: The test title for the case validating identical inputs
with differing outputs is misleading; update the it() description string that
currently reads "should handle edge case where all pairs have same input and
output" (the test using createPair hashes hash1/hash2/hash3 and calling
getOptimalSortedList) to something clearer like "should handle edge case where
all pairs have same input but different outputs" so the test name matches the
assertions against getOptimalSortedList.
In `@src/order/pair.ts`:
- Line 82: The JSDoc for getSortedPairList mentions a non-existent parameter
"sortBy"; remove or update that line so the doc matches the function signature:
either delete the `@param sortBy` entry or replace it with descriptions of the
actual parameters accepted by getSortedPairList (use the exact function name
getSortedPairList to locate the comment) and ensure parameter names/types in the
JSDoc match the current signature.
- Around line 166-200: getOptimalSortedList currently mutates the caller's array
and also filters out pairs with maxOutput === 0n due to a truthiness check; fix
by sorting a shallow copy of the input (e.g., use [...options] before calling
.sort) so callers aren't surprised, and change the filter condition in the
pareto selection to explicitly check for undefined/null (e.g.,
opt.takeOrder.quote?.maxOutput !== undefined && opt.takeOrder.quote.maxOutput >=
0n) or at least opt.takeOrder.quote != null to include 0n results if desired;
also add a short inline comment near the filter to explain that 0n outputs are
intentionally excluded or included per the chosen behavior.
In `@src/state/contracts.test.ts`:
- Line 868: Fix the typo in the test description string inside the it(...) block
named "should return undefined for Raindex tradeType when raindexRab is not
available" by changing "raindexRab" to "raindexArb" so the description reads
"should return undefined for Raindex tradeType when raindexArb is not
available"; locate the it(...) in the tests (symbol: the it(...) with that
description) and update the string only—no logic changes.
| `(${EvaluableV4} evaluable, ${IOV2}[] validInputs, ${IOV2}[] validOutputs, bytes32 nonce, bytes32 secret, bytes meta)` as const; | ||
| export const ClearConfigV2 = | ||
| "(uint256 aliceInputIOIndex, uint256 aliceOutputIOIndex, uint256 bobInputIOIndex, uint256 bobOutputIOIndex, bytes32 aliceBountyVaultId, bytes32 bobBountyVaultId)" as const; | ||
| export const RouteLeg = `(uint8 routeLegType, address destination, bytes data)[]`; |
Contributor
There was a problem hiding this comment.
Missing as const assertion on RouteLeg.
Every other struct definition in _v6 (and _v4/_v5) uses as const to preserve the literal string type, which is required for parseAbiParameters to produce a strongly-typed result. Without it, the type widens to string, and parseAbiParameters(_v6.RouteLeg) at line 262 will lose type inference.
Proposed fix
- export const RouteLeg = `(uint8 routeLegType, address destination, bytes data)[]`;
+ export const RouteLeg = `(uint8 routeLegType, address destination, bytes data)[]` as const;📝 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 const RouteLeg = `(uint8 routeLegType, address destination, bytes data)[]`; | |
| export const RouteLeg = `(uint8 routeLegType, address destination, bytes data)[]` as const; |
🤖 Prompt for AI Agents
In `@src/common/abis/orderbook.ts` at line 124, RouteLeg is missing an "as const"
assertion so its literal ABI string widens to string and breaks type inference
when used with parseAbiParameters (e.g., parseAbiParameters(_v6.RouteLeg)). Fix
by adding the as const assertion to the RouteLeg export (the exported constant
named RouteLeg) so it preserves the literal type; ensure any other occurrences
in the _v6 ABI group follow the same pattern to keep parseAbiParameters strongly
typed.
Comment on lines
150
to
154
| export const Arb = [ | ||
| "function iRouteProcessor() external view returns (address)", | ||
| `function arb5(address orderBook, ${TakeOrdersConfigV5} calldata takeOrders, ${TaskV2} calldata task) external payable`, | ||
| // `function arb4(address orderBook, ${TakeOrdersConfigV5} calldata startTakeOrders, ${TakeOrdersConfigV5} calldata endTakeOrders, bytes calldata exchangeData, ${TaskV2} calldata task) external payable`, | ||
| `function arb4(address orderBook, ${TakeOrdersConfigV5}[] calldata startTakeOrders, bytes calldata exchangeData, ${TaskV2} calldata task) external payable`, | ||
| ] as const; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all usages of arb4 in the codebase
rg -n --type=ts -C3 '\barb4\b' --max-count=100Repository: rainlanguage/rain.solver
Length of output: 8743
🏁 Script executed:
#!/bin/bash
# Search for any references to endTakeOrders to ensure it's been removed from all call sites
rg -n 'endTakeOrders' --type=tsRepository: rainlanguage/rain.solver
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Also check the actual file to confirm the lines mentioned
sed -n '145,160p' src/common/abis/orderbook.tsRepository: rainlanguage/rain.solver
Length of output: 1273
🏁 Script executed:
#!/bin/bash
# Get full context around arb4 calls in router simulate
sed -n '270,280p' src/core/modes/router/simulate.tsRepository: rainlanguage/rain.solver
Length of output: 478
🏁 Script executed:
#!/bin/bash
# Get full context around arb4 calls in inter simulate
sed -n '556,566p' src/core/modes/inter/simulate.tsRepository: rainlanguage/rain.solver
Length of output: 351
🏁 Script executed:
#!/bin/bash
# Verify which ABI versions exist and their arb4 signatures
grep -n "export.*Arb.*=" src/common/abis/orderbook.tsRepository: rainlanguage/rain.solver
Length of output: 593
🏁 Script executed:
#!/bin/bash
# Get more context around inter/simulate.ts arb4 call to see how args is constructed
sed -n '545,565p' src/core/modes/inter/simulate.tsRepository: rainlanguage/rain.solver
Length of output: 788
🏁 Script executed:
#!/bin/bash
# Get the full V5 arb4 ABI definition to confirm the signature
sed -n '150,154p' src/common/abis/orderbook.tsRepository: rainlanguage/rain.solver
Length of output: 472
🏁 Script executed:
#!/bin/bash
# Check router/simulate.ts context for what takeOrdersConfigStruct contains
sed -n '260,282p' src/core/modes/router/simulate.tsRepository: rainlanguage/rain.solver
Length of output: 952
Breaking ABI change to arb4: startTakeOrders is now an array, endTakeOrders is removed, and new exchangeData parameter is required.
This is a significant contract interface change. The new V5 arb4 signature requires four parameters: orderBook, startTakeOrders[], exchangeData, and task. However, call sites in src/core/modes/router/simulate.ts (line 274) and src/core/modes/inter/simulate.ts (line 561) are still passing only three arguments and missing the exchangeData parameter. Update these call sites to include the required exchangeData parameter, and update corresponding tests to reflect the new four-argument signature.
🤖 Prompt for AI Agents
In `@src/common/abis/orderbook.ts` around lines 150 - 154, The ABI for Arb.arb4
changed to require four args (orderBook, startTakeOrders[], exchangeData, task);
update all call sites that invoke arb4 (notably in the router and inter simulate
functions) to pass the new exchangeData parameter between startTakeOrders and
task, e.g., modify the calls in simulate.ts where arb4 is invoked to include an
appropriate bytes exchangeData value (or an empty bytes if no data) as the third
argument; also update any unit/integration tests that invoke arb4 to reflect the
new four-argument signature and adjust expected behavior accordingly.
Comment on lines
+23
to
29
| // vi.mock("./balancer", () => ({ | ||
| // findBestBalancerTrade: vi.fn(), | ||
| // })); | ||
|
|
||
| vi.mock("./raindex", () => ({ | ||
| findBestRaindexRouterTrade: vi.fn(), | ||
| })); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Commented-out balancer mock left behind.
Lines 23–25 contain a commented-out balancer mock. If balancer support was removed or is pending in a separate PR, consider adding a brief comment explaining why, or remove it entirely to avoid confusion.
🤖 Prompt for AI Agents
In `@src/core/modes/index.test.ts` around lines 23 - 29, Remove the leftover
commented-out balancer mock in index.test.ts (the vi.mock call for "./balancer"
and findBestBalancerTrade); either delete those three commented lines entirely
or replace them with a brief inline comment explaining why balancer support is
omitted (e.g., removed/handled in another PR) so future readers aren’t
confused—locate the commented block near the existing vi.mock("./raindex") mock
to edit.
Comment on lines
+323
to
+328
| const raindexResult = Result.ok({ | ||
| type: "interOrderbook", | ||
| spanAttributes: { foundOpp: true }, | ||
| estimatedProfit: 120n, | ||
| oppBlockNumber: 123, | ||
| }); |
Contributor
There was a problem hiding this comment.
Copy-paste bug: raindexResult has type: "interOrderbook" instead of "raindex".
This doesn't cause the test to fail because only the highest estimatedProfit is asserted, but it means the test isn't validating raindex results with the correct type field.
🐛 Proposed fix
const raindexResult = Result.ok({
- type: "interOrderbook",
+ type: "raindex",
spanAttributes: { foundOpp: true },
estimatedProfit: 120n,
oppBlockNumber: 123,
});📝 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
| const raindexResult = Result.ok({ | |
| type: "interOrderbook", | |
| spanAttributes: { foundOpp: true }, | |
| estimatedProfit: 120n, | |
| oppBlockNumber: 123, | |
| }); | |
| const raindexResult = Result.ok({ | |
| type: "raindex", | |
| spanAttributes: { foundOpp: true }, | |
| estimatedProfit: 120n, | |
| oppBlockNumber: 123, | |
| }); |
🤖 Prompt for AI Agents
In `@src/core/modes/index.test.ts` around lines 323 - 328, The test has a
copy-paste bug: the raindexResult constant is created with type:
"interOrderbook" instead of the correct "raindex"; update the object literal in
raindexResult (the Result.ok call named raindexResult) to set type: "raindex" so
the test validates raindex results using the correct type field.
| expect(result.findBestInterOrderbookTrade).toBe(findBestInterOrderbookTrade); | ||
| }); | ||
|
|
||
| it("should return only inter-orderbook trade when orderbook is in raindex router set", () => { |
Contributor
There was a problem hiding this comment.
Test description is incorrect: says "inter-orderbook" but tests "raindex router".
- it("should return only inter-orderbook trade when orderbook is in raindex router set", () => {
+ it("should return only raindex router trade when orderbook is in raindex router set", () => {📝 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
| it("should return only inter-orderbook trade when orderbook is in raindex router set", () => { | |
| it("should return only raindex router trade when orderbook is in raindex router set", () => { |
🤖 Prompt for AI Agents
In `@src/core/modes/index.test.ts` at line 674, The test title string in the
it(...) block "should return only inter-orderbook trade when orderbook is in
raindex router set" is misleading; change that description to accurately
reference the raindex router (for example: "should return only trades routed via
raindex router when orderbook is in raindex router set") so the test name
matches the behavior being asserted in the test case (modify the it(...) title
in src/core/modes/index.test.ts).
Comment on lines
+531
to
+538
| if ( | ||
| tkn === sellToken || | ||
| BASES_TO_CHECK_TRADES_AGAINST[this.state.chainConfig.id].every( | ||
| (baseToken) => baseToken.address.toLowerCase() !== tkn, | ||
| ) | ||
| ) { | ||
| continue; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find where chainConfig is defined and used
rg -n "chainConfig\.id|BASES_TO_CHECK_TRADES_AGAINST" --type=ts -g '!node_modules' -g '!dist' -A 2 -B 2Repository: rainlanguage/rain.solver
Length of output: 11556
🏁 Script executed:
# Check the file in question to understand context
fd "index.ts" --path "src/order" | head -5Repository: rainlanguage/rain.solver
Length of output: 283
🏁 Script executed:
# Look for chainConfig initialization and chain ID validation
rg -n "chainConfig\s*=" --type=ts -g '!node_modules' -g '!dist' -A 3Repository: rainlanguage/rain.solver
Length of output: 1244
🏁 Script executed:
# Search for where BASES_TO_CHECK_TRADES_AGAINST is defined
rg -n "BASES_TO_CHECK_TRADES_AGAINST" --type=ts -g '!node_modules' -g '!dist' -A 5 -B 2Repository: rainlanguage/rain.solver
Length of output: 5330
🏁 Script executed:
# Check chainConfig initialization and validation
cat -n src/state/index.ts | head -250 | tail -150Repository: rainlanguage/rain.solver
Length of output: 6867
🏁 Script executed:
# Search for any explicit chain ID validation or guards
rg -n "chainConfig\.id|ChainId" --type=ts -g '!node_modules' -g '!dist' | grep -i "valid\|check\|support\|assert" | head -20Repository: rainlanguage/rain.solver
Length of output: 1630
🏁 Script executed:
# Look for the getCounterpartyOrdersAgainstBaseTokens method to see the full context
sed -n '515,550p' src/order/index.tsRepository: rainlanguage/rain.solver
Length of output: 1532
🏁 Script executed:
# Check if there are any initialization checks that prevent unsupported chains
rg -n "BASES_TO_CHECK_TRADES_AGAINST\[" --type=ts -g '!node_modules' -g '!dist' -B 5Repository: rainlanguage/rain.solver
Length of output: 555
Add null-coalescing operator to prevent potential TypeError when accessing unsupported chains in BASES_TO_CHECK_TRADES_AGAINST.
If this.state.chainConfig.id is not present in Sushi's BASES_TO_CHECK_TRADES_AGAINST configuration, the code will throw a TypeError when calling .every() on undefined. While chainConfig itself is validated at application initialization, validation only confirms the chain is supported by the app—not that it exists in the external Sushi configuration.
Proposed fix
- BASES_TO_CHECK_TRADES_AGAINST[this.state.chainConfig.id].every(
+ (BASES_TO_CHECK_TRADES_AGAINST[this.state.chainConfig.id] ?? []).every(With this change, chains missing from Sushi's base tokens list will safely skip routing (.every() on [] returns true, triggering continue), which is the correct fallback behavior for unsupported chains.
📝 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 ( | |
| tkn === sellToken || | |
| BASES_TO_CHECK_TRADES_AGAINST[this.state.chainConfig.id].every( | |
| (baseToken) => baseToken.address.toLowerCase() !== tkn, | |
| ) | |
| ) { | |
| continue; | |
| } | |
| if ( | |
| tkn === sellToken || | |
| (BASES_TO_CHECK_TRADES_AGAINST[this.state.chainConfig.id] ?? []).every( | |
| (baseToken) => baseToken.address.toLowerCase() !== tkn, | |
| ) | |
| ) { | |
| continue; | |
| } |
🤖 Prompt for AI Agents
In `@src/order/index.ts` around lines 531 - 538, The guard checking tokens can
throw if BASES_TO_CHECK_TRADES_AGAINST[this.state.chainConfig.id] is undefined;
update the condition in the loop that references tkn and sellToken so the
.every() call uses a null-coalescing default (e.g.,
BASES_TO_CHECK_TRADES_AGAINST[this.state.chainConfig.id] ?? []) before calling
.every(), ensuring the expression safely falls back to an empty array when the
chain id is not present.
Comment on lines
+463
to
+665
| describe("Test getOptimalSortedList function", () => { | ||
| const createPair = (hash: string, quote?: { ratio: bigint; maxOutput: bigint }): Pair => | ||
| ({ | ||
| orderbook: "0xorderbook", | ||
| buyToken: "0xinput", | ||
| sellToken: "0xoutput", | ||
| takeOrder: { | ||
| id: hash, | ||
| quote, | ||
| }, | ||
| }) as any; | ||
|
|
||
| it("should return empty array for empty input", () => { | ||
| const result = getOptimalSortedList([]); | ||
| expect(result).toEqual([]); | ||
| }); | ||
|
|
||
| it("should return single pair when only one option exists", () => { | ||
| const pair = createPair("hash1", { ratio: 10n, maxOutput: 100n }); | ||
| const result = getOptimalSortedList([pair]); | ||
|
|
||
| expect(result).toHaveLength(1); | ||
| expect(result[0]).toBe(pair); | ||
| }); | ||
|
|
||
| it("should filter out pairs without quotes", () => { | ||
| const pairs = [ | ||
| createPair("hash1", { ratio: 10n, maxOutput: 100n }), | ||
| createPair("hash2"), // no quote | ||
| createPair("hash3", { ratio: 5n, maxOutput: 200n }), | ||
| createPair("hash4"), // no quote | ||
| ]; | ||
|
|
||
| const result = getOptimalSortedList(pairs); | ||
|
|
||
| // Should only include pairs with quotes | ||
| expect(result.every((p) => p.takeOrder.quote !== undefined)).toBe(true); | ||
| }); | ||
|
|
||
| it("should return Pareto-optimal options sorted descending by maxOutput", () => { | ||
| // Create pairs with different input/output combinations | ||
| // Pair format: [input (ratio * maxOutput), maxOutput] | ||
| const pairs = [ | ||
| createPair("hash1", { ratio: 2n, maxOutput: 100n }), // input: 200, output: 100 | ||
| createPair("hash2", { ratio: 3n, maxOutput: 150n }), // input: 450, output: 150 (dominated) | ||
| createPair("hash3", { ratio: 1n, maxOutput: 200n }), // input: 200, output: 200 (Pareto) | ||
| createPair("hash4", { ratio: 2n, maxOutput: 250n }), // input: 500, output: 250 (dominated) | ||
| createPair("hash5", { ratio: 1n, maxOutput: 300n }), // input: 300, output: 300 (Pareto) | ||
| ]; | ||
|
|
||
| const result = getOptimalSortedList(pairs); | ||
|
|
||
| // Pareto-optimal set (least input, most output): | ||
| // hash3: input 200, output 200 | ||
| // hash5: input 300, output 300 | ||
| // Sorted descending by maxOutput: hash5, hash3 | ||
| expect(result).toHaveLength(2); | ||
| expect(result[0].takeOrder.id).toBe("hash5"); | ||
| expect(result[0].takeOrder.quote?.maxOutput).toBe(300n); | ||
| expect(result[1].takeOrder.id).toBe("hash3"); | ||
| expect(result[1].takeOrder.quote?.maxOutput).toBe(200n); | ||
| }); | ||
|
|
||
| it("should handle pairs with equal input but different outputs", () => { | ||
| const pairs = [ | ||
| createPair("hash1", { ratio: 2n, maxOutput: 100n }), // input: 200, output: 100 | ||
| createPair("hash2", { ratio: 1n, maxOutput: 200n }), // input: 200, output: 200 (better) | ||
| createPair("hash3", { ratio: 4n, maxOutput: 50n }), // input: 200, output: 50 | ||
| ]; | ||
|
|
||
| const result = getOptimalSortedList(pairs); | ||
|
|
||
| // With same input (200), highest output (200) wins | ||
| expect(result).toHaveLength(1); | ||
| expect(result[0].takeOrder.id).toBe("hash2"); | ||
| expect(result[0].takeOrder.quote?.maxOutput).toBe(200n); | ||
| }); | ||
|
|
||
| it("should handle pairs with equal output but different inputs", () => { | ||
| const pairs = [ | ||
| createPair("hash1", { ratio: 5n, maxOutput: 100n }), // input: 500, output: 100 | ||
| createPair("hash2", { ratio: 3n, maxOutput: 100n }), // input: 300, output: 100 (better) | ||
| createPair("hash3", { ratio: 2n, maxOutput: 100n }), // input: 200, output: 100 (best) | ||
| ]; | ||
|
|
||
| const result = getOptimalSortedList(pairs); | ||
|
|
||
| // With same output (100), lowest input (200) wins | ||
| expect(result).toHaveLength(1); | ||
| expect(result[0].takeOrder.id).toBe("hash3"); | ||
| expect(result[0].takeOrder.quote?.ratio).toBe(2n); | ||
| }); | ||
|
|
||
| it("should correctly identify Pareto frontier with multiple options", () => { | ||
| const pairs = [ | ||
| createPair("hash1", { ratio: 10n, maxOutput: 50n }), // input: 500, output: 50 (dominated) | ||
| createPair("hash2", { ratio: 5n, maxOutput: 100n }), // input: 500, output: 100 (Pareto) | ||
| createPair("hash3", { ratio: 4n, maxOutput: 150n }), // input: 600, output: 150 (Pareto) | ||
| createPair("hash4", { ratio: 6n, maxOutput: 120n }), // input: 720, output: 120 (dominated) | ||
| createPair("hash5", { ratio: 3n, maxOutput: 200n }), // input: 600, output: 200 (Pareto) | ||
| createPair("hash6", { ratio: 2n, maxOutput: 350n }), // input: 700, output: 350 (Pareto) | ||
| ]; | ||
|
|
||
| const result = getOptimalSortedList(pairs); | ||
|
|
||
| // Pareto frontier (sorted by input, then by output): | ||
| // hash2: input 500, output 100 | ||
| // hash5: input 600, output 200 (better output than hash3 with same input) | ||
| // hash6: input 700, output 350 | ||
| expect(result).toHaveLength(3); | ||
|
|
||
| // Should be sorted descending by maxOutput | ||
| expect(result[0].takeOrder.id).toBe("hash6"); | ||
| expect(result[0].takeOrder.quote?.maxOutput).toBe(350n); | ||
|
|
||
| expect(result[1].takeOrder.id).toBe("hash5"); | ||
| expect(result[1].takeOrder.quote?.maxOutput).toBe(200n); | ||
|
|
||
| expect(result[2].takeOrder.id).toBe("hash2"); | ||
| expect(result[2].takeOrder.quote?.maxOutput).toBe(100n); | ||
| }); | ||
|
|
||
| it("should handle all pairs being Pareto-optimal", () => { | ||
| // Each pair has progressively more input and more output | ||
| const pairs = [ | ||
| createPair("hash1", { ratio: 1n, maxOutput: 100n }), // input: 100, output: 100 | ||
| createPair("hash2", { ratio: 1n, maxOutput: 200n }), // input: 200, output: 200 | ||
| createPair("hash3", { ratio: 1n, maxOutput: 300n }), // input: 300, output: 300 | ||
| ]; | ||
|
|
||
| const result = getOptimalSortedList(pairs); | ||
|
|
||
| // All are Pareto-optimal | ||
| expect(result).toHaveLength(3); | ||
|
|
||
| // Sorted descending by maxOutput | ||
| expect(result[0].takeOrder.id).toBe("hash3"); | ||
| expect(result[1].takeOrder.id).toBe("hash2"); | ||
| expect(result[2].takeOrder.id).toBe("hash1"); | ||
| }); | ||
|
|
||
| it("should handle pairs with very small differences", () => { | ||
| const pairs = [ | ||
| createPair("hash1", { ratio: 1n, maxOutput: 100n }), // input: 100, output: 100 | ||
| createPair("hash2", { ratio: 1n, maxOutput: 101n }), // input: 101, output: 101 | ||
| createPair("hash3", { ratio: 1n, maxOutput: 102n }), // input: 102, output: 102 | ||
| ]; | ||
|
|
||
| const result = getOptimalSortedList(pairs); | ||
|
|
||
| // All are Pareto-optimal (each has more input and more output) | ||
| expect(result).toHaveLength(3); | ||
| expect(result[0].takeOrder.quote?.maxOutput).toBe(102n); | ||
| expect(result[1].takeOrder.quote?.maxOutput).toBe(101n); | ||
| expect(result[2].takeOrder.quote?.maxOutput).toBe(100n); | ||
| }); | ||
|
|
||
| it("should handle edge case where all pairs have same input and output", () => { | ||
| const pairs = [ | ||
| createPair("hash1", { ratio: 2n, maxOutput: 50n }), // input: 100, output: 50 | ||
| createPair("hash2", { ratio: 5n, maxOutput: 20n }), // input: 100, output: 20 | ||
| createPair("hash3", { ratio: 1n, maxOutput: 100n }), // input: 100, output: 100 | ||
| ]; | ||
|
|
||
| const result = getOptimalSortedList(pairs); | ||
|
|
||
| // With same input, only the one with highest output is Pareto-optimal | ||
| expect(result).toHaveLength(1); | ||
| expect(result[0].takeOrder.id).toBe("hash3"); | ||
| expect(result[0].takeOrder.quote?.maxOutput).toBe(100n); | ||
| }); | ||
|
|
||
| it("should handle mix of pairs with and without quotes", () => { | ||
| const pairs = [ | ||
| createPair("hash1", { ratio: 2n, maxOutput: 100n }), // input: 200, output: 100 | ||
| createPair("hash2"), // no quote | ||
| createPair("hash3", { ratio: 1n, maxOutput: 200n }), // input: 200, output: 200 (Pareto) | ||
| createPair("hash4"), // no quote | ||
| createPair("hash5", { ratio: 1n, maxOutput: 300n }), // input: 300, output: 300 (Pareto) | ||
| ]; | ||
|
|
||
| const result = getOptimalSortedList(pairs); | ||
|
|
||
| // Only pairs with quotes, and only Pareto-optimal ones | ||
| expect(result).toHaveLength(2); | ||
| expect(result[0].takeOrder.id).toBe("hash5"); | ||
| expect(result[1].takeOrder.id).toBe("hash3"); | ||
| expect(result.every((p) => p.takeOrder.quote !== undefined)).toBe(true); | ||
| }); | ||
|
|
||
| it("should maintain stability for equivalent Pareto options", () => { | ||
| const pairs = [ | ||
| createPair("hash1", { ratio: 2n, maxOutput: 100n }), // input: 200, output: 100 | ||
| createPair("hash2", { ratio: 4n, maxOutput: 50n }), // input: 200, output: 50 | ||
| createPair("hash3", { ratio: 1n, maxOutput: 200n }), // input: 200, output: 200 | ||
| ]; | ||
|
|
||
| const result = getOptimalSortedList(pairs); | ||
|
|
||
| // With same input, only highest output is Pareto-optimal | ||
| expect(result).toHaveLength(1); | ||
| expect(result[0].takeOrder.id).toBe("hash3"); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Thorough test coverage for the Pareto-optimal selection.
The suite covers empty input, single element, no-quote filtering, various Pareto frontier scenarios, ties, mixed quoted/unquoted, and stability — all expectations align with the algorithm's behavior.
One minor nit: the test name on line 620 says "all pairs have same input and output" but the pairs actually share the same input with different outputs. Consider rewording to avoid confusion.
- it("should handle edge case where all pairs have same input and output", () => {
+ it("should handle edge case where all pairs have same input but different outputs", () => {📝 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
| describe("Test getOptimalSortedList function", () => { | |
| const createPair = (hash: string, quote?: { ratio: bigint; maxOutput: bigint }): Pair => | |
| ({ | |
| orderbook: "0xorderbook", | |
| buyToken: "0xinput", | |
| sellToken: "0xoutput", | |
| takeOrder: { | |
| id: hash, | |
| quote, | |
| }, | |
| }) as any; | |
| it("should return empty array for empty input", () => { | |
| const result = getOptimalSortedList([]); | |
| expect(result).toEqual([]); | |
| }); | |
| it("should return single pair when only one option exists", () => { | |
| const pair = createPair("hash1", { ratio: 10n, maxOutput: 100n }); | |
| const result = getOptimalSortedList([pair]); | |
| expect(result).toHaveLength(1); | |
| expect(result[0]).toBe(pair); | |
| }); | |
| it("should filter out pairs without quotes", () => { | |
| const pairs = [ | |
| createPair("hash1", { ratio: 10n, maxOutput: 100n }), | |
| createPair("hash2"), // no quote | |
| createPair("hash3", { ratio: 5n, maxOutput: 200n }), | |
| createPair("hash4"), // no quote | |
| ]; | |
| const result = getOptimalSortedList(pairs); | |
| // Should only include pairs with quotes | |
| expect(result.every((p) => p.takeOrder.quote !== undefined)).toBe(true); | |
| }); | |
| it("should return Pareto-optimal options sorted descending by maxOutput", () => { | |
| // Create pairs with different input/output combinations | |
| // Pair format: [input (ratio * maxOutput), maxOutput] | |
| const pairs = [ | |
| createPair("hash1", { ratio: 2n, maxOutput: 100n }), // input: 200, output: 100 | |
| createPair("hash2", { ratio: 3n, maxOutput: 150n }), // input: 450, output: 150 (dominated) | |
| createPair("hash3", { ratio: 1n, maxOutput: 200n }), // input: 200, output: 200 (Pareto) | |
| createPair("hash4", { ratio: 2n, maxOutput: 250n }), // input: 500, output: 250 (dominated) | |
| createPair("hash5", { ratio: 1n, maxOutput: 300n }), // input: 300, output: 300 (Pareto) | |
| ]; | |
| const result = getOptimalSortedList(pairs); | |
| // Pareto-optimal set (least input, most output): | |
| // hash3: input 200, output 200 | |
| // hash5: input 300, output 300 | |
| // Sorted descending by maxOutput: hash5, hash3 | |
| expect(result).toHaveLength(2); | |
| expect(result[0].takeOrder.id).toBe("hash5"); | |
| expect(result[0].takeOrder.quote?.maxOutput).toBe(300n); | |
| expect(result[1].takeOrder.id).toBe("hash3"); | |
| expect(result[1].takeOrder.quote?.maxOutput).toBe(200n); | |
| }); | |
| it("should handle pairs with equal input but different outputs", () => { | |
| const pairs = [ | |
| createPair("hash1", { ratio: 2n, maxOutput: 100n }), // input: 200, output: 100 | |
| createPair("hash2", { ratio: 1n, maxOutput: 200n }), // input: 200, output: 200 (better) | |
| createPair("hash3", { ratio: 4n, maxOutput: 50n }), // input: 200, output: 50 | |
| ]; | |
| const result = getOptimalSortedList(pairs); | |
| // With same input (200), highest output (200) wins | |
| expect(result).toHaveLength(1); | |
| expect(result[0].takeOrder.id).toBe("hash2"); | |
| expect(result[0].takeOrder.quote?.maxOutput).toBe(200n); | |
| }); | |
| it("should handle pairs with equal output but different inputs", () => { | |
| const pairs = [ | |
| createPair("hash1", { ratio: 5n, maxOutput: 100n }), // input: 500, output: 100 | |
| createPair("hash2", { ratio: 3n, maxOutput: 100n }), // input: 300, output: 100 (better) | |
| createPair("hash3", { ratio: 2n, maxOutput: 100n }), // input: 200, output: 100 (best) | |
| ]; | |
| const result = getOptimalSortedList(pairs); | |
| // With same output (100), lowest input (200) wins | |
| expect(result).toHaveLength(1); | |
| expect(result[0].takeOrder.id).toBe("hash3"); | |
| expect(result[0].takeOrder.quote?.ratio).toBe(2n); | |
| }); | |
| it("should correctly identify Pareto frontier with multiple options", () => { | |
| const pairs = [ | |
| createPair("hash1", { ratio: 10n, maxOutput: 50n }), // input: 500, output: 50 (dominated) | |
| createPair("hash2", { ratio: 5n, maxOutput: 100n }), // input: 500, output: 100 (Pareto) | |
| createPair("hash3", { ratio: 4n, maxOutput: 150n }), // input: 600, output: 150 (Pareto) | |
| createPair("hash4", { ratio: 6n, maxOutput: 120n }), // input: 720, output: 120 (dominated) | |
| createPair("hash5", { ratio: 3n, maxOutput: 200n }), // input: 600, output: 200 (Pareto) | |
| createPair("hash6", { ratio: 2n, maxOutput: 350n }), // input: 700, output: 350 (Pareto) | |
| ]; | |
| const result = getOptimalSortedList(pairs); | |
| // Pareto frontier (sorted by input, then by output): | |
| // hash2: input 500, output 100 | |
| // hash5: input 600, output 200 (better output than hash3 with same input) | |
| // hash6: input 700, output 350 | |
| expect(result).toHaveLength(3); | |
| // Should be sorted descending by maxOutput | |
| expect(result[0].takeOrder.id).toBe("hash6"); | |
| expect(result[0].takeOrder.quote?.maxOutput).toBe(350n); | |
| expect(result[1].takeOrder.id).toBe("hash5"); | |
| expect(result[1].takeOrder.quote?.maxOutput).toBe(200n); | |
| expect(result[2].takeOrder.id).toBe("hash2"); | |
| expect(result[2].takeOrder.quote?.maxOutput).toBe(100n); | |
| }); | |
| it("should handle all pairs being Pareto-optimal", () => { | |
| // Each pair has progressively more input and more output | |
| const pairs = [ | |
| createPair("hash1", { ratio: 1n, maxOutput: 100n }), // input: 100, output: 100 | |
| createPair("hash2", { ratio: 1n, maxOutput: 200n }), // input: 200, output: 200 | |
| createPair("hash3", { ratio: 1n, maxOutput: 300n }), // input: 300, output: 300 | |
| ]; | |
| const result = getOptimalSortedList(pairs); | |
| // All are Pareto-optimal | |
| expect(result).toHaveLength(3); | |
| // Sorted descending by maxOutput | |
| expect(result[0].takeOrder.id).toBe("hash3"); | |
| expect(result[1].takeOrder.id).toBe("hash2"); | |
| expect(result[2].takeOrder.id).toBe("hash1"); | |
| }); | |
| it("should handle pairs with very small differences", () => { | |
| const pairs = [ | |
| createPair("hash1", { ratio: 1n, maxOutput: 100n }), // input: 100, output: 100 | |
| createPair("hash2", { ratio: 1n, maxOutput: 101n }), // input: 101, output: 101 | |
| createPair("hash3", { ratio: 1n, maxOutput: 102n }), // input: 102, output: 102 | |
| ]; | |
| const result = getOptimalSortedList(pairs); | |
| // All are Pareto-optimal (each has more input and more output) | |
| expect(result).toHaveLength(3); | |
| expect(result[0].takeOrder.quote?.maxOutput).toBe(102n); | |
| expect(result[1].takeOrder.quote?.maxOutput).toBe(101n); | |
| expect(result[2].takeOrder.quote?.maxOutput).toBe(100n); | |
| }); | |
| it("should handle edge case where all pairs have same input and output", () => { | |
| const pairs = [ | |
| createPair("hash1", { ratio: 2n, maxOutput: 50n }), // input: 100, output: 50 | |
| createPair("hash2", { ratio: 5n, maxOutput: 20n }), // input: 100, output: 20 | |
| createPair("hash3", { ratio: 1n, maxOutput: 100n }), // input: 100, output: 100 | |
| ]; | |
| const result = getOptimalSortedList(pairs); | |
| // With same input, only the one with highest output is Pareto-optimal | |
| expect(result).toHaveLength(1); | |
| expect(result[0].takeOrder.id).toBe("hash3"); | |
| expect(result[0].takeOrder.quote?.maxOutput).toBe(100n); | |
| }); | |
| it("should handle mix of pairs with and without quotes", () => { | |
| const pairs = [ | |
| createPair("hash1", { ratio: 2n, maxOutput: 100n }), // input: 200, output: 100 | |
| createPair("hash2"), // no quote | |
| createPair("hash3", { ratio: 1n, maxOutput: 200n }), // input: 200, output: 200 (Pareto) | |
| createPair("hash4"), // no quote | |
| createPair("hash5", { ratio: 1n, maxOutput: 300n }), // input: 300, output: 300 (Pareto) | |
| ]; | |
| const result = getOptimalSortedList(pairs); | |
| // Only pairs with quotes, and only Pareto-optimal ones | |
| expect(result).toHaveLength(2); | |
| expect(result[0].takeOrder.id).toBe("hash5"); | |
| expect(result[1].takeOrder.id).toBe("hash3"); | |
| expect(result.every((p) => p.takeOrder.quote !== undefined)).toBe(true); | |
| }); | |
| it("should maintain stability for equivalent Pareto options", () => { | |
| const pairs = [ | |
| createPair("hash1", { ratio: 2n, maxOutput: 100n }), // input: 200, output: 100 | |
| createPair("hash2", { ratio: 4n, maxOutput: 50n }), // input: 200, output: 50 | |
| createPair("hash3", { ratio: 1n, maxOutput: 200n }), // input: 200, output: 200 | |
| ]; | |
| const result = getOptimalSortedList(pairs); | |
| // With same input, only highest output is Pareto-optimal | |
| expect(result).toHaveLength(1); | |
| expect(result[0].takeOrder.id).toBe("hash3"); | |
| }); | |
| describe("Test getOptimalSortedList function", () => { | |
| const createPair = (hash: string, quote?: { ratio: bigint; maxOutput: bigint }): Pair => | |
| ({ | |
| orderbook: "0xorderbook", | |
| buyToken: "0xinput", | |
| sellToken: "0xoutput", | |
| takeOrder: { | |
| id: hash, | |
| quote, | |
| }, | |
| }) as any; | |
| it("should return empty array for empty input", () => { | |
| const result = getOptimalSortedList([]); | |
| expect(result).toEqual([]); | |
| }); | |
| it("should return single pair when only one option exists", () => { | |
| const pair = createPair("hash1", { ratio: 10n, maxOutput: 100n }); | |
| const result = getOptimalSortedList([pair]); | |
| expect(result).toHaveLength(1); | |
| expect(result[0]).toBe(pair); | |
| }); | |
| it("should filter out pairs without quotes", () => { | |
| const pairs = [ | |
| createPair("hash1", { ratio: 10n, maxOutput: 100n }), | |
| createPair("hash2"), // no quote | |
| createPair("hash3", { ratio: 5n, maxOutput: 200n }), | |
| createPair("hash4"), // no quote | |
| ]; | |
| const result = getOptimalSortedList(pairs); | |
| // Should only include pairs with quotes | |
| expect(result.every((p) => p.takeOrder.quote !== undefined)).toBe(true); | |
| }); | |
| it("should return Pareto-optimal options sorted descending by maxOutput", () => { | |
| // Create pairs with different input/output combinations | |
| // Pair format: [input (ratio * maxOutput), maxOutput] | |
| const pairs = [ | |
| createPair("hash1", { ratio: 2n, maxOutput: 100n }), // input: 200, output: 100 | |
| createPair("hash2", { ratio: 3n, maxOutput: 150n }), // input: 450, output: 150 (dominated) | |
| createPair("hash3", { ratio: 1n, maxOutput: 200n }), // input: 200, output: 200 (Pareto) | |
| createPair("hash4", { ratio: 2n, maxOutput: 250n }), // input: 500, output: 250 (dominated) | |
| createPair("hash5", { ratio: 1n, maxOutput: 300n }), // input: 300, output: 300 (Pareto) | |
| ]; | |
| const result = getOptimalSortedList(pairs); | |
| // Pareto-optimal set (least input, most output): | |
| // hash3: input 200, output 200 | |
| // hash5: input 300, output 300 | |
| // Sorted descending by maxOutput: hash5, hash3 | |
| expect(result).toHaveLength(2); | |
| expect(result[0].takeOrder.id).toBe("hash5"); | |
| expect(result[0].takeOrder.quote?.maxOutput).toBe(300n); | |
| expect(result[1].takeOrder.id).toBe("hash3"); | |
| expect(result[1].takeOrder.quote?.maxOutput).toBe(200n); | |
| }); | |
| it("should handle pairs with equal input but different outputs", () => { | |
| const pairs = [ | |
| createPair("hash1", { ratio: 2n, maxOutput: 100n }), // input: 200, output: 100 | |
| createPair("hash2", { ratio: 1n, maxOutput: 200n }), // input: 200, output: 200 (better) | |
| createPair("hash3", { ratio: 4n, maxOutput: 50n }), // input: 200, output: 50 | |
| ]; | |
| const result = getOptimalSortedList(pairs); | |
| // With same input (200), highest output (200) wins | |
| expect(result).toHaveLength(1); | |
| expect(result[0].takeOrder.id).toBe("hash2"); | |
| expect(result[0].takeOrder.quote?.maxOutput).toBe(200n); | |
| }); | |
| it("should handle pairs with equal output but different inputs", () => { | |
| const pairs = [ | |
| createPair("hash1", { ratio: 5n, maxOutput: 100n }), // input: 500, output: 100 | |
| createPair("hash2", { ratio: 3n, maxOutput: 100n }), // input: 300, output: 100 (better) | |
| createPair("hash3", { ratio: 2n, maxOutput: 100n }), // input: 200, output: 100 (best) | |
| ]; | |
| const result = getOptimalSortedList(pairs); | |
| // With same output (100), lowest input (200) wins | |
| expect(result).toHaveLength(1); | |
| expect(result[0].takeOrder.id).toBe("hash3"); | |
| expect(result[0].takeOrder.quote?.ratio).toBe(2n); | |
| }); | |
| it("should correctly identify Pareto frontier with multiple options", () => { | |
| const pairs = [ | |
| createPair("hash1", { ratio: 10n, maxOutput: 50n }), // input: 500, output: 50 (dominated) | |
| createPair("hash2", { ratio: 5n, maxOutput: 100n }), // input: 500, output: 100 (Pareto) | |
| createPair("hash3", { ratio: 4n, maxOutput: 150n }), // input: 600, output: 150 (Pareto) | |
| createPair("hash4", { ratio: 6n, maxOutput: 120n }), // input: 720, output: 120 (dominated) | |
| createPair("hash5", { ratio: 3n, maxOutput: 200n }), // input: 600, output: 200 (Pareto) | |
| createPair("hash6", { ratio: 2n, maxOutput: 350n }), // input: 700, output: 350 (Pareto) | |
| ]; | |
| const result = getOptimalSortedList(pairs); | |
| // Pareto frontier (sorted by input, then by output): | |
| // hash2: input 500, output 100 | |
| // hash5: input 600, output 200 (better output than hash3 with same input) | |
| // hash6: input 700, output 350 | |
| expect(result).toHaveLength(3); | |
| // Should be sorted descending by maxOutput | |
| expect(result[0].takeOrder.id).toBe("hash6"); | |
| expect(result[0].takeOrder.quote?.maxOutput).toBe(350n); | |
| expect(result[1].takeOrder.id).toBe("hash5"); | |
| expect(result[1].takeOrder.quote?.maxOutput).toBe(200n); | |
| expect(result[2].takeOrder.id).toBe("hash2"); | |
| expect(result[2].takeOrder.quote?.maxOutput).toBe(100n); | |
| }); | |
| it("should handle all pairs being Pareto-optimal", () => { | |
| // Each pair has progressively more input and more output | |
| const pairs = [ | |
| createPair("hash1", { ratio: 1n, maxOutput: 100n }), // input: 100, output: 100 | |
| createPair("hash2", { ratio: 1n, maxOutput: 200n }), // input: 200, output: 200 | |
| createPair("hash3", { ratio: 1n, maxOutput: 300n }), // input: 300, output: 300 | |
| ]; | |
| const result = getOptimalSortedList(pairs); | |
| // All are Pareto-optimal | |
| expect(result).toHaveLength(3); | |
| // Sorted descending by maxOutput | |
| expect(result[0].takeOrder.id).toBe("hash3"); | |
| expect(result[1].takeOrder.id).toBe("hash2"); | |
| expect(result[2].takeOrder.id).toBe("hash1"); | |
| }); | |
| it("should handle pairs with very small differences", () => { | |
| const pairs = [ | |
| createPair("hash1", { ratio: 1n, maxOutput: 100n }), // input: 100, output: 100 | |
| createPair("hash2", { ratio: 1n, maxOutput: 101n }), // input: 101, output: 101 | |
| createPair("hash3", { ratio: 1n, maxOutput: 102n }), // input: 102, output: 102 | |
| ]; | |
| const result = getOptimalSortedList(pairs); | |
| // All are Pareto-optimal (each has more input and more output) | |
| expect(result).toHaveLength(3); | |
| expect(result[0].takeOrder.quote?.maxOutput).toBe(102n); | |
| expect(result[1].takeOrder.quote?.maxOutput).toBe(101n); | |
| expect(result[2].takeOrder.quote?.maxOutput).toBe(100n); | |
| }); | |
| it("should handle edge case where all pairs have same input but different outputs", () => { | |
| const pairs = [ | |
| createPair("hash1", { ratio: 2n, maxOutput: 50n }), // input: 100, output: 50 | |
| createPair("hash2", { ratio: 5n, maxOutput: 20n }), // input: 100, output: 20 | |
| createPair("hash3", { ratio: 1n, maxOutput: 100n }), // input: 100, output: 100 | |
| ]; | |
| const result = getOptimalSortedList(pairs); | |
| // With same input, only the one with highest output is Pareto-optimal | |
| expect(result).toHaveLength(1); | |
| expect(result[0].takeOrder.id).toBe("hash3"); | |
| expect(result[0].takeOrder.quote?.maxOutput).toBe(100n); | |
| }); | |
| it("should handle mix of pairs with and without quotes", () => { | |
| const pairs = [ | |
| createPair("hash1", { ratio: 2n, maxOutput: 100n }), // input: 200, output: 100 | |
| createPair("hash2"), // no quote | |
| createPair("hash3", { ratio: 1n, maxOutput: 200n }), // input: 200, output: 200 (Pareto) | |
| createPair("hash4"), // no quote | |
| createPair("hash5", { ratio: 1n, maxOutput: 300n }), // input: 300, output: 300 (Pareto) | |
| ]; | |
| const result = getOptimalSortedList(pairs); | |
| // Only pairs with quotes, and only Pareto-optimal ones | |
| expect(result).toHaveLength(2); | |
| expect(result[0].takeOrder.id).toBe("hash5"); | |
| expect(result[1].takeOrder.id).toBe("hash3"); | |
| expect(result.every((p) => p.takeOrder.quote !== undefined)).toBe(true); | |
| }); | |
| it("should maintain stability for equivalent Pareto options", () => { | |
| const pairs = [ | |
| createPair("hash1", { ratio: 2n, maxOutput: 100n }), // input: 200, output: 100 | |
| createPair("hash2", { ratio: 4n, maxOutput: 50n }), // input: 200, output: 50 | |
| createPair("hash3", { ratio: 1n, maxOutput: 200n }), // input: 200, output: 200 | |
| ]; | |
| const result = getOptimalSortedList(pairs); | |
| // With same input, only highest output is Pareto-optimal | |
| expect(result).toHaveLength(1); | |
| expect(result[0].takeOrder.id).toBe("hash3"); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In `@src/order/pair.test.ts` around lines 463 - 665, The test title for the case
validating identical inputs with differing outputs is misleading; update the
it() description string that currently reads "should handle edge case where all
pairs have same input and output" (the test using createPair hashes
hash1/hash2/hash3 and calling getOptimalSortedList) to something clearer like
"should handle edge case where all pairs have same input but different outputs"
so the test name matches the assertions against getOptimalSortedList.
| * @param output - The output token address to get pairs from | ||
| * @param input - The input token address to get pairs from | ||
| * @param counterpartySource - Determines the type of counterparty orders source to return | ||
| * @param sortBy - Either by ratio first (default) or by max out first |
Contributor
There was a problem hiding this comment.
Stale doc: sortBy parameter does not exist.
The JSDoc comment references a sortBy parameter, but getSortedPairList has no such parameter in its signature (lines 84–92).
Proposed fix
- * `@param` sortBy - Either by ratio first (default) or by max out first📝 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
| * @param sortBy - Either by ratio first (default) or by max out first |
🤖 Prompt for AI Agents
In `@src/order/pair.ts` at line 82, The JSDoc for getSortedPairList mentions a
non-existent parameter "sortBy"; remove or update that line so the doc matches
the function signature: either delete the `@param sortBy` entry or replace it
with descriptions of the actual parameters accepted by getSortedPairList (use
the exact function name getSortedPairList to locate the comment) and ensure
parameter names/types in the JSDoc match the current signature.
Comment on lines
+166
to
+200
| export function getOptimalSortedList(options: Pair[]): Pair[] { | ||
| // sort based on least input and most output | ||
| options.sort((a, b) => { | ||
| if (!a.takeOrder.quote && !b.takeOrder.quote) return 0; | ||
| if (!a.takeOrder.quote) return 1; | ||
| if (!b.takeOrder.quote) return -1; | ||
| const aInput = a.takeOrder.quote.ratio * a.takeOrder.quote.maxOutput; | ||
| const bInput = b.takeOrder.quote.ratio * b.takeOrder.quote.maxOutput; | ||
| if (aInput < bInput) { | ||
| return -1; | ||
| } else if (aInput > bInput) { | ||
| return 1; | ||
| } else { | ||
| // if maxinputs are equal, sort by maxoutput | ||
| if (a.takeOrder.quote.maxOutput < b.takeOrder.quote.maxOutput) { | ||
| return 1; | ||
| } else if (a.takeOrder.quote.maxOutput > b.takeOrder.quote.maxOutput) { | ||
| return -1; | ||
| } else { | ||
| return 0; | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| // pick pareto optimized options | ||
| const paretoOpts: Pair[] = []; | ||
| let maxoutBase = -1n; | ||
| for (const opt of options) { | ||
| if (opt.takeOrder.quote?.maxOutput && opt.takeOrder.quote.maxOutput > maxoutBase) { | ||
| paretoOpts.push(opt); | ||
| maxoutBase = opt.takeOrder.quote.maxOutput; | ||
| } | ||
| } | ||
| return paretoOpts.reverse(); // reverse to be sorted desc based on maxout | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Algorithm looks correct; two observations on the implementation.
-
Mutation of the input array:
options.sort(...)on line 168 sorts in-place, so the caller's array is silently reordered. This is consistent with howsortPairListis used elsewhere in this file (lines 96, 112), but the function namegetOptimalSortedListreads as if it returns a new list. Consider sorting a copy ([...options].sort(...)) to avoid surprising callers, or at least document the mutation. -
maxOutput === 0nis filtered out: On line 194, the truthiness checkopt.takeOrder.quote?.maxOutput && ...means any pair whosemaxOutputis0n(falsy) is silently excluded. This is likely intentional (zero output = no trade), but worth a brief inline comment for clarity.
Optional: avoid mutating the input
export function getOptimalSortedList(options: Pair[]): Pair[] {
// sort based on least input and most output
- options.sort((a, b) => {
+ const sorted = [...options].sort((a, b) => {
...
});
// pick pareto optimized options
const paretoOpts: Pair[] = [];
let maxoutBase = -1n;
- for (const opt of options) {
+ for (const opt of sorted) {🤖 Prompt for AI Agents
In `@src/order/pair.ts` around lines 166 - 200, getOptimalSortedList currently
mutates the caller's array and also filters out pairs with maxOutput === 0n due
to a truthiness check; fix by sorting a shallow copy of the input (e.g., use
[...options] before calling .sort) so callers aren't surprised, and change the
filter condition in the pareto selection to explicitly check for undefined/null
(e.g., opt.takeOrder.quote?.maxOutput !== undefined &&
opt.takeOrder.quote.maxOutput >= 0n) or at least opt.takeOrder.quote != null to
include 0n results if desired; also add a short inline comment near the filter
to explain that 0n outputs are intentionally excluded or included per the chosen
behavior.
| }); | ||
| }); | ||
|
|
||
| it("should return undefined for Raindex tradeType when raindexRab is not available", () => { |
Contributor
There was a problem hiding this comment.
Typo in test description: "raindexRab" → "raindexArb".
- it("should return undefined for Raindex tradeType when raindexRab is not available", () => {
+ it("should return undefined for Raindex tradeType when raindexArb is not available", () => {📝 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
| it("should return undefined for Raindex tradeType when raindexRab is not available", () => { | |
| it("should return undefined for Raindex tradeType when raindexArb is not available", () => { |
🤖 Prompt for AI Agents
In `@src/state/contracts.test.ts` at line 868, Fix the typo in the test
description string inside the it(...) block named "should return undefined for
Raindex tradeType when raindexRab is not available" by changing "raindexRab" to
"raindexArb" so the description reads "should return undefined for Raindex
tradeType when raindexArb is not available"; locate the it(...) in the tests
(symbol: the it(...) with that description) and update the string only—no logic
changes.
This was referenced Feb 12, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/modes/raindex/simulation.test.ts`:
- Around line 299-312: The compose-error test for setTransactionData is missing
an assertion that getCalldata was not invoked; update the "should return error
if getEnsureBountyTaskBytecode fails with compose error" test to include
expect(getCalldataSpy).not.toHaveBeenCalled() after calling
simulator.setTransactionData(preparedParams). This mirrors the parse-error test
and ensures getCalldata (spy referenced as getCalldataSpy) is not called when
getEnsureBountyTaskBytecode (mocked via getEnsureBountyTaskBytecode) resolves
with Result.err, keeping checks around simulator.setTransactionData and the
EnsureBountyTaskError behavior consistent.
- Around line 244-258: The test "should create takeOrders with correct min/max
float values" currently only checks IOIsInput and data; update the assertions
after calling simulator.prepareTradeParams() to verify takeOrders[0] and
takeOrders[1] include the numeric properties minimumIO, maximumIO, and
maximumIORatio and that they equal the expected values (either the known
constants used to construct the simulator or values available on the simulator
fixture/result); specifically add assertions for
result.value.takeOrders[0].minimumIO, .maximumIO, .maximumIORatio and the same
for takeOrders[1], comparing against the correct expected numbers from the test
setup (or rename the test if you prefer to keep only IOIsInput/data checks).
In `@src/core/modes/raindex/simulation.ts`:
- Around line 78-92: Remove the commented-out destructured bindings from the
this.tradeArgs destructuring so only actual used properties are listed;
specifically edit the destructuring block that defines orderDetails,
counterpartyOrderDetails, maximumInputFixed, blockNumber,
counterpartyInputToEthPrice, counterpartyOutputToEthPrice, quote, rpParams, and
routeVisual and delete the commented items `type`, `solver`, `signer`, and
`profit` to eliminate dead code and keep the destructure clean.
- Around line 136-137: The code accesses
tradeArgs.solver.state.chainConfig.routeProcessors["4"] without guarding for
missing entries, which can make destination undefined and cause
encodeAbiParameters to throw; update the logic where destination is set
(referencing routeProcessors and destination in the route leg construction
inside the function that builds route legs in simulation.ts) to check for a
present routeProcessors["4"] and, if missing, return a SimulationHaltReason (or
similar typed halt) instead of proceeding; ensure you use the same typed halt
used for addresses checks, e.g., create and return a SimulationHaltReason
indicating a missing route processor for chain id "4" so encodeAbiParameters
never receives undefined.
- Around line 159-165: The TakeOrdersConfigTypeV5 construction is using
orderDetails.buyTokenDecimals for IO sizing which is misleading for the
counterparty take order; change the decimals passed to minFloat(...) and
maxFloat(...) to use this.tradeArgs.counterpartyOrderDetails.sellTokenDecimals
(i.e., replace references to this.tradeArgs.orderDetails.buyTokenDecimals for
minimumIO and maximumIO) so the IO bounds reflect the counterparty's sell token
denomination while leaving maximumIORatio, orders, data and IOIsInput unchanged.
- Around line 50-57: The RaindexRouterTradePreparedParams.type defines an
optional price that prepareTradeParams() never sets but trySimulateTrade()
passes to estimateProfit(...), so either set price inside prepareTradeParams()
or stop using it; fix by computing and assigning a bigint price in
prepareTradeParams() (e.g., derive from prepared takeOrders / minimumExpected /
rawtx or from the same pricing logic used elsewhere) and return it on the
RaindexRouterTradePreparedParams object, then keep the trySimulateTrade() call
to this.estimateProfit(prepareParamsResult.value.price) safe; alternatively, if
you cannot compute a meaningful price, remove the price field from
RaindexRouterTradePreparedParams and update trySimulateTrade() to call
estimateProfit with a computed value or skip passing undefined to
estimateProfit. Ensure changes reference RaindexRouterTradePreparedParams,
prepareTradeParams(), trySimulateTrade(), and estimateProfit().
---
Duplicate comments:
In `@src/core/modes/raindex/simulation.ts`:
- Around line 59-66: Fix the typos in the class JSDoc for
RaindexRouterTradeSimulator (which extends TradeSimulatorBase): change "wwith"
to "with" and "a external" to "an external" and ensure the first sentence reads
clearly (e.g., "Simulates a trade between two orders with different IO through
an external route"). Keep the rest of the docblock intact and consistent with
the class purpose and bullet points.
- Line 19: Update the doc comment that currently reads "Arguments for simulating
inter-orderbook trade" to instead read "Arguments for simulating raindex router
trade" so the documentation accurately describes the feature; locate the comment
string "Arguments for simulating inter-orderbook trade" in
src/core/modes/raindex/simulation.ts (immediately above the related
arguments/interface) and replace "inter-orderbook trade" with "raindex router
trade".
Comment on lines
+244
to
+258
| it("should create takeOrders with correct min/max float values", async () => { | ||
| const result = await simulator.prepareTradeParams(); | ||
| assert(result.isOk()); | ||
|
|
||
| const takeOrders = result.value.takeOrders; | ||
| expect(takeOrders).toHaveLength(2); | ||
|
|
||
| // First order (main order) | ||
| expect(takeOrders[0].IOIsInput).toBe(false); | ||
| expect(takeOrders[0].data).toBe("0x"); | ||
|
|
||
| // Second order (counterparty) | ||
| expect(takeOrders[1].IOIsInput).toBe(false); | ||
| expect(takeOrders[1].data).toBe("0x"); | ||
| }); |
Contributor
There was a problem hiding this comment.
Test name claims to verify min/max float values but only checks IOIsInput and data.
The test description says "should create takeOrders with correct min/max float values" yet the assertions never check minimumIO, maximumIO, or maximumIORatio. Either add the missing assertions or rename the test to accurately describe what is actually verified.
✅ Add missing assertions
const takeOrders = result.value.takeOrders;
expect(takeOrders).toHaveLength(2);
// First order (main order)
+ expect(takeOrders[0].minimumIO).toBe(minFloat(tradeArgs.orderDetails.sellTokenDecimals));
+ expect(takeOrders[0].maximumIO).toBe(maxFloat(tradeArgs.orderDetails.sellTokenDecimals));
+ expect(takeOrders[0].maximumIORatio).toBe(maxFloat(18));
expect(takeOrders[0].IOIsInput).toBe(false);
expect(takeOrders[0].data).toBe("0x");
// Second order (counterparty)
+ expect(takeOrders[1].minimumIO).toBe(minFloat(tradeArgs.orderDetails.buyTokenDecimals));
+ expect(takeOrders[1].maximumIO).toBe(maxFloat(tradeArgs.orderDetails.buyTokenDecimals));
+ expect(takeOrders[1].maximumIORatio).toBe(maxFloat(18));
expect(takeOrders[1].IOIsInput).toBe(false);
expect(takeOrders[1].data).toBe("0x");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/modes/raindex/simulation.test.ts` around lines 244 - 258, The test
"should create takeOrders with correct min/max float values" currently only
checks IOIsInput and data; update the assertions after calling
simulator.prepareTradeParams() to verify takeOrders[0] and takeOrders[1] include
the numeric properties minimumIO, maximumIO, and maximumIORatio and that they
equal the expected values (either the known constants used to construct the
simulator or values available on the simulator fixture/result); specifically add
assertions for result.value.takeOrders[0].minimumIO, .maximumIO, .maximumIORatio
and the same for takeOrders[1], comparing against the correct expected numbers
from the test setup (or rename the test if you prefer to keep only
IOIsInput/data checks).
Comment on lines
+299
to
+312
| it("should return error if getEnsureBountyTaskBytecode fails with compose error", async () => { | ||
| const error = new EnsureBountyTaskError( | ||
| "compose error", | ||
| EnsureBountyTaskErrorType.ComposeError, | ||
| ); | ||
| (getEnsureBountyTaskBytecode as Mock).mockResolvedValueOnce(Result.err(error)); | ||
|
|
||
| const result = await simulator.setTransactionData(preparedParams); | ||
| assert(result.isErr()); | ||
| expect(result.error.type).toBe(TradeType.Raindex); | ||
| expect(result.error.reason).toBe(SimulationHaltReason.FailedToGetTaskBytecode); | ||
| expect(result.error.spanAttributes["isNodeError"]).toBe(false); | ||
| expect(result.error.spanAttributes["duration"]).toBeGreaterThan(0); | ||
| }); |
Contributor
There was a problem hiding this comment.
Missing assertion that getCalldata is not called on compose error.
The parse error test (line 294) explicitly asserts expect(getCalldataSpy).not.toHaveBeenCalled(). The compose error test omits this assertion, creating an inconsistency. Add it for parity and to guard against regressions.
✅ Proposed fix
const result = await simulator.setTransactionData(preparedParams);
assert(result.isErr());
expect(result.error.type).toBe(TradeType.Raindex);
expect(result.error.reason).toBe(SimulationHaltReason.FailedToGetTaskBytecode);
expect(result.error.spanAttributes["isNodeError"]).toBe(false);
expect(result.error.spanAttributes["duration"]).toBeGreaterThan(0);
+ const getCalldataSpy = vi.spyOn(simulator, "getCalldata");
+ expect(getCalldataSpy).not.toHaveBeenCalled();
+ getCalldataSpy.mockRestore();📝 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
| it("should return error if getEnsureBountyTaskBytecode fails with compose error", async () => { | |
| const error = new EnsureBountyTaskError( | |
| "compose error", | |
| EnsureBountyTaskErrorType.ComposeError, | |
| ); | |
| (getEnsureBountyTaskBytecode as Mock).mockResolvedValueOnce(Result.err(error)); | |
| const result = await simulator.setTransactionData(preparedParams); | |
| assert(result.isErr()); | |
| expect(result.error.type).toBe(TradeType.Raindex); | |
| expect(result.error.reason).toBe(SimulationHaltReason.FailedToGetTaskBytecode); | |
| expect(result.error.spanAttributes["isNodeError"]).toBe(false); | |
| expect(result.error.spanAttributes["duration"]).toBeGreaterThan(0); | |
| }); | |
| it("should return error if getEnsureBountyTaskBytecode fails with compose error", async () => { | |
| const error = new EnsureBountyTaskError( | |
| "compose error", | |
| EnsureBountyTaskErrorType.ComposeError, | |
| ); | |
| (getEnsureBountyTaskBytecode as Mock).mockResolvedValueOnce(Result.err(error)); | |
| const getCalldataSpy = vi.spyOn(simulator, "getCalldata"); | |
| const result = await simulator.setTransactionData(preparedParams); | |
| assert(result.isErr()); | |
| expect(result.error.type).toBe(TradeType.Raindex); | |
| expect(result.error.reason).toBe(SimulationHaltReason.FailedToGetTaskBytecode); | |
| expect(result.error.spanAttributes["isNodeError"]).toBe(false); | |
| expect(result.error.spanAttributes["duration"]).toBeGreaterThan(0); | |
| expect(getCalldataSpy).not.toHaveBeenCalled(); | |
| getCalldataSpy.mockRestore(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/modes/raindex/simulation.test.ts` around lines 299 - 312, The
compose-error test for setTransactionData is missing an assertion that
getCalldata was not invoked; update the "should return error if
getEnsureBountyTaskBytecode fails with compose error" test to include
expect(getCalldataSpy).not.toHaveBeenCalled() after calling
simulator.setTransactionData(preparedParams). This mirrors the parse-error test
and ensures getCalldata (spy referenced as getCalldataSpy) is not called when
getEnsureBountyTaskBytecode (mocked via getEnsureBountyTaskBytecode) resolves
with Result.err, keeping checks around simulator.setTransactionData and the
EnsureBountyTaskError behavior consistent.
Comment on lines
+50
to
+57
| export type RaindexRouterTradePreparedParams = { | ||
| type: TradeType.Raindex; | ||
| rawtx: RawTransaction; | ||
| takeOrders: TakeOrdersConfigTypeV5[]; | ||
| minimumExpected: bigint; | ||
| exchangeData: `0x${string}`; | ||
| price?: bigint; | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usages of .price in relation to RaindexRouterTradePreparedParams
rg -n "\.price" --type ts src/core/modes/raindex/ -A 2 -B 2Repository: rainlanguage/rain.solver
Length of output: 1378
🏁 Script executed:
#!/bin/bash
# Also search for price field across the entire codebase to understand its usage pattern
rg -n "price\s*[:?]" --type ts src/core/modes/raindex/ -A 1 -B 1Repository: rainlanguage/rain.solver
Length of output: 3559
🏁 Script executed:
#!/bin/bash
# Look at the prepareTradeParams and setTransactionData functions
rg -n "prepareTradeParams|setTransactionData" --type ts src/core/modes/raindex/ -A 10Repository: rainlanguage/rain.solver
Length of output: 13019
🏁 Script executed:
#!/bin/bash
# Get the full prepareTradeParams implementation to see what it returns
sed -n '75,180p' src/core/modes/raindex/simulation.tsRepository: rainlanguage/rain.solver
Length of output: 4384
🏁 Script executed:
#!/bin/bash
# Search for any code that reads params.price or similar patterns on RaindexRouterTradePreparedParams
rg -n "params\.price|tradeParams\.price|prepared.*\.price|\.price\s*=" --type ts -A 2 -B 2 src/core/modes/raindex/Repository: rainlanguage/rain.solver
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Check the index.ts file to see how prepareTradeParams is called and if price is set
rg -n "prepareTradeParams|RaindexRouterTradePreparedParams" --type ts src/core/modes/raindex/index.ts -A 5 -B 5Repository: rainlanguage/rain.solver
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Search more broadly for any reference to the price field on prepared params or trade params
rg -n "\.price" --type ts src/core/modes/raindex/ | grep -v "quote\.price"Repository: rainlanguage/rain.solver
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Search for how prepareTradeParams result is used
rg -n "prepareTradeParams" --type ts -A 10 | head -100Repository: rainlanguage/rain.solver
Length of output: 7709
🏁 Script executed:
#!/bin/bash
# Look at the index.ts file directly to see if price is set when calling prepareTradeParams
cat src/core/modes/raindex/index.ts | head -200Repository: rainlanguage/rain.solver
Length of output: 7137
🏁 Script executed:
#!/bin/bash
# Search for any assignment of price to the prepared params after prepareTradeParams is called
rg -n "prepareParamsResult|preparedParams|tradeParams" --type ts -A 5 src/core/modes/simulator.ts | head -50Repository: rainlanguage/rain.solver
Length of output: 2236
🏁 Script executed:
#!/bin/bash
# Look at the full simulator.ts to see if price is ever set on the params after prepareTradeParams
cat src/core/modes/simulator.ts | head -150Repository: rainlanguage/rain.solver
Length of output: 6242
🏁 Script executed:
#!/bin/bash
# Search for any other places where RaindexRouterTradePreparedParams might have price set
rg -n "price.*=" --type ts src/core/modes/raindex/ | grep -v "quote\.price\|gasPrice\|inputToEthPrice\|outputToEthPrice"Repository: rainlanguage/rain.solver
Length of output: 416
price field in RaindexRouterTradePreparedParams is never assigned but is used at line 146 of simulator.ts.
prepareTradeParams() does not populate price in its return object, yet trySimulateTrade() calls this.estimateProfit(prepareParamsResult.value.price) with a non-null assertion, causing undefined to be passed. Either populate price in prepareTradeParams() or remove the field and its usage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/modes/raindex/simulation.ts` around lines 50 - 57, The
RaindexRouterTradePreparedParams.type defines an optional price that
prepareTradeParams() never sets but trySimulateTrade() passes to
estimateProfit(...), so either set price inside prepareTradeParams() or stop
using it; fix by computing and assigning a bigint price in prepareTradeParams()
(e.g., derive from prepared takeOrders / minimumExpected / rawtx or from the
same pricing logic used elsewhere) and return it on the
RaindexRouterTradePreparedParams object, then keep the trySimulateTrade() call
to this.estimateProfit(prepareParamsResult.value.price) safe; alternatively, if
you cannot compute a meaningful price, remove the price field from
RaindexRouterTradePreparedParams and update trySimulateTrade() to call
estimateProfit with a computed value or skip passing undefined to
estimateProfit. Ensure changes reference RaindexRouterTradePreparedParams,
prepareTradeParams(), trySimulateTrade(), and estimateProfit().
Comment on lines
+78
to
+92
| const { | ||
| orderDetails, | ||
| counterpartyOrderDetails, | ||
| maximumInputFixed, | ||
| blockNumber, | ||
| // type, | ||
| // solver, | ||
| // signer, | ||
| counterpartyInputToEthPrice, | ||
| counterpartyOutputToEthPrice, | ||
| quote, | ||
| // profit, | ||
| rpParams, | ||
| routeVisual, | ||
| } = this.tradeArgs; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove commented-out destructured bindings.
type, solver, signer, and profit are commented-out rather than simply omitted from the destructuring. Dead code like this should be removed entirely.
♻️ Proposed cleanup
const {
orderDetails,
counterpartyOrderDetails,
maximumInputFixed,
blockNumber,
- // type,
- // solver,
- // signer,
counterpartyInputToEthPrice,
counterpartyOutputToEthPrice,
quote,
- // profit,
rpParams,
routeVisual,
} = this.tradeArgs;📝 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
| const { | |
| orderDetails, | |
| counterpartyOrderDetails, | |
| maximumInputFixed, | |
| blockNumber, | |
| // type, | |
| // solver, | |
| // signer, | |
| counterpartyInputToEthPrice, | |
| counterpartyOutputToEthPrice, | |
| quote, | |
| // profit, | |
| rpParams, | |
| routeVisual, | |
| } = this.tradeArgs; | |
| const { | |
| orderDetails, | |
| counterpartyOrderDetails, | |
| maximumInputFixed, | |
| blockNumber, | |
| counterpartyInputToEthPrice, | |
| counterpartyOutputToEthPrice, | |
| quote, | |
| rpParams, | |
| routeVisual, | |
| } = this.tradeArgs; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/modes/raindex/simulation.ts` around lines 78 - 92, Remove the
commented-out destructured bindings from the this.tradeArgs destructuring so
only actual used properties are listed; specifically edit the destructuring
block that defines orderDetails, counterpartyOrderDetails, maximumInputFixed,
blockNumber, counterpartyInputToEthPrice, counterpartyOutputToEthPrice, quote,
rpParams, and routeVisual and delete the commented items `type`, `solver`,
`signer`, and `profit` to eliminate dead code and keep the destructure clean.
Comment on lines
+136
to
+137
| routeLegType: RouteLegType.SUSHI, | ||
| destination: this.tradeArgs.solver.state.chainConfig.routeProcessors["4"], |
Contributor
There was a problem hiding this comment.
No null guard for routeProcessors["4"].
chainConfig.routeProcessors["4"] is accessed without checking if the key exists. If a supported chain omits this entry, destination in the route leg becomes undefined, and encodeAbiParameters will either throw or silently encode an invalid address. Unlike the addresses check below (which returns a typed SimulationHaltReason), this path produces an unhandled runtime error.
🛡️ Proposed guard
+ const routeProcessor = this.tradeArgs.solver.state.chainConfig.routeProcessors["4"];
+ if (!routeProcessor) {
+ this.spanAttributes["error"] = "routeProcessors['4'] not configured for this chain";
+ this.spanAttributes["duration"] = performance.now() - this.startTime;
+ return Result.err({
+ type: TradeType.Raindex,
+ spanAttributes: this.spanAttributes,
+ reason: SimulationHaltReason.UndefinedTradeDestinationAddress,
+ });
+ }
// Single RouteLeg
const legs = [
{
routeLegType: RouteLegType.SUSHI,
- destination: this.tradeArgs.solver.state.chainConfig.routeProcessors["4"],
+ destination: routeProcessor,📝 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
| routeLegType: RouteLegType.SUSHI, | |
| destination: this.tradeArgs.solver.state.chainConfig.routeProcessors["4"], | |
| const routeProcessor = this.tradeArgs.solver.state.chainConfig.routeProcessors["4"]; | |
| if (!routeProcessor) { | |
| this.spanAttributes["error"] = "routeProcessors['4'] not configured for this chain"; | |
| this.spanAttributes["duration"] = performance.now() - this.startTime; | |
| return Result.err({ | |
| type: TradeType.Raindex, | |
| spanAttributes: this.spanAttributes, | |
| reason: SimulationHaltReason.UndefinedTradeDestinationAddress, | |
| }); | |
| } | |
| // Single RouteLeg | |
| const legs = [ | |
| { | |
| routeLegType: RouteLegType.SUSHI, | |
| destination: routeProcessor, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/modes/raindex/simulation.ts` around lines 136 - 137, The code
accesses tradeArgs.solver.state.chainConfig.routeProcessors["4"] without
guarding for missing entries, which can make destination undefined and cause
encodeAbiParameters to throw; update the logic where destination is set
(referencing routeProcessors and destination in the route leg construction
inside the function that builds route legs in simulation.ts) to check for a
present routeProcessors["4"] and, if missing, return a SimulationHaltReason (or
similar typed halt) instead of proceeding; ensure you use the same typed halt
used for addresses checks, e.g., create and return a SimulationHaltReason
indicating a missing route processor for chain id "4" so encodeAbiParameters
never receives undefined.
Comment on lines
+159
to
+165
| minimumIO: minFloat(this.tradeArgs.orderDetails.buyTokenDecimals), | ||
| maximumIO: maxFloat(this.tradeArgs.orderDetails.buyTokenDecimals), | ||
| maximumIORatio: maxFloat(18), | ||
| orders: [this.tradeArgs.counterpartyOrderDetails.takeOrder.struct], | ||
| data: "0x", | ||
| IOIsInput: false, | ||
| }, |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use counterpartyOrderDetails.sellTokenDecimals for the counterparty TakeOrdersConfigTypeV5.
In the route A/B (order) → B/C (sushi) → C/A (order), the counterparty order's IOIsInput=false IO is denominated in its sell token (A). While orderDetails.buyToken and counterpartyOrderDetails.sellToken happen to be the same token (A), using orderDetails.buyTokenDecimals here is misleading. Explicitly referencing counterpartyOrderDetails.sellTokenDecimals makes the intent clear and would remain correct if the token relationship ever changes.
♻️ Proposed fix
{
- minimumIO: minFloat(this.tradeArgs.orderDetails.buyTokenDecimals),
- maximumIO: maxFloat(this.tradeArgs.orderDetails.buyTokenDecimals),
+ minimumIO: minFloat(this.tradeArgs.counterpartyOrderDetails.sellTokenDecimals),
+ maximumIO: maxFloat(this.tradeArgs.counterpartyOrderDetails.sellTokenDecimals),
maximumIORatio: maxFloat(18),
orders: [this.tradeArgs.counterpartyOrderDetails.takeOrder.struct],
data: "0x",
IOIsInput: false,
},📝 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
| minimumIO: minFloat(this.tradeArgs.orderDetails.buyTokenDecimals), | |
| maximumIO: maxFloat(this.tradeArgs.orderDetails.buyTokenDecimals), | |
| maximumIORatio: maxFloat(18), | |
| orders: [this.tradeArgs.counterpartyOrderDetails.takeOrder.struct], | |
| data: "0x", | |
| IOIsInput: false, | |
| }, | |
| minimumIO: minFloat(this.tradeArgs.counterpartyOrderDetails.sellTokenDecimals), | |
| maximumIO: maxFloat(this.tradeArgs.counterpartyOrderDetails.sellTokenDecimals), | |
| maximumIORatio: maxFloat(18), | |
| orders: [this.tradeArgs.counterpartyOrderDetails.takeOrder.struct], | |
| data: "0x", | |
| IOIsInput: false, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/modes/raindex/simulation.ts` around lines 159 - 165, The
TakeOrdersConfigTypeV5 construction is using orderDetails.buyTokenDecimals for
IO sizing which is misleading for the counterparty take order; change the
decimals passed to minFloat(...) and maxFloat(...) to use
this.tradeArgs.counterpartyOrderDetails.sellTokenDecimals (i.e., replace
references to this.tradeArgs.orderDetails.buyTokenDecimals for minimumIO and
maximumIO) so the IO bounds reflect the counterparty's sell token denomination
while leaving maximumIORatio, orders, data and IOIsInput unchanged.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Motivation
Caution
Do NOT merge before #427
This PR implements solving for routed Raindex orders through Raindex router arb contract:
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Tests
Types/State