v6 orderbook #427
Open
v6 orderbook #427
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughAdds comprehensive V6 orderbook version support across configuration, ABI definitions, type system, and trade simulation layers. Introduces OrderbookVersions and SubgraphVersions enums for version tracking, extends contract resolution pipelines to handle v6 addresses, and updates simulators to route trades through appropriate V6-specific contract ABIs and configuration paths. Changes
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
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/modes/router/simulate.ts (1)
62-62: Update documentation to reflect V6 support.The comment states the simulator "supports both V4 and V5 orderbook (order v3 and v4) types" but the implementation now also includes V6 orderbook support.
📝 Suggested documentation update
- * The simulator supports both V4 and V5 orderbook (order v3 and v4) types. + * The simulator supports V4, V5, and V6 orderbook (order v3 and v4) types.src/subgraph/index.ts (1)
146-165: Consider defaulting version insidefetchSubgraphOrders.
Since the version is derived from the URL anyway, allowing the method to resolve it when omitted reduces the chance of mismatched tagging by external callers.♻️ Suggested adjustment
- async fetchSubgraphOrders(url: string, version: SubgraphVersions): Promise<SgOrder[]> { + async fetchSubgraphOrders(url: string, version?: SubgraphVersions): Promise<SgOrder[]> { const result: SgOrder[] = []; let skip = 0; let timestamp = Date.now(); for (;;) { timestamp = Date.now(); const res = await axios.post( url, { query: getQueryPaginated(skip, this.filters), }, { headers, timeout: this.requestTimeout }, ); if (res?.data?.data?.orders) { const orders: SgOrder[] = res.data.data.orders; - if (version === SubgraphVersions.V6) { + const resolvedVersion = version ?? this.getSubgraphVersion(url); + if (resolvedVersion === SubgraphVersions.V6) { orders.forEach((v) => (v.__version = SubgraphVersions.V6)); } else { orders.forEach((v) => (v.__version = SubgraphVersions.OLD_V)); }
🤖 Fix all issues with AI agents
In `@src/common/abis/orderbook.ts`:
- Around line 103-154: The TakeOrdersConfigV5 ABI struct fields in namespace _v6
(symbol TakeOrdersConfigV5) don't match the TypeScript type
(TakeOrdersConfigTypeV5) — update the ABI string for TakeOrdersConfigV5 to use
minimumIO, maximumIO, maximumIORatio, add the bool IOIsInput field, then orders
and data (i.e. `(${Float} minimumIO, ${Float} maximumIO, ${Float}
maximumIORatio, bool IOIsInput, ${TakeOrderConfigV4}[] orders, bytes data)`) so
calldata encoding for takeOrders4 and arb5 will align with the TypeScript types.
In `@src/core/modes/inter/simulate.test.ts`:
- Around line 638-691: The test description is wrong: update the it(...)
description that currently reads "should return v4 config" to accurately say
"should return v5 config" (the test is exercising getTakeOrdersConfigV5 and
asserting V5-specific fields like minimumIO, maximumIO, maximumIORatio,
IOIsInput), i.e., locate the it block with the string literal "should return v4
config" and change it to reference V5 so the test name matches the behavior
being tested.
In `@src/core/modes/inter/simulate.ts`:
- Around line 323-350: The method getTakeOrdersConfigV5 currently has a
docstring mentioning "V4" which is incorrect; update the comment to describe V5
semantics and parameters: change the header to reference TakeOrdersConfigTypeV5
and V5 order/counterparty parameters, and adjust any parameter descriptions
(orderDetails: PairV4 -> V5-specific wording, counterpartyOrderDetails: Pair) to
reflect this method builds a V5 take-orders config including encodedFN; ensure
the summary, param descriptions, and return type mention V5
(TakeOrdersConfigTypeV5) and that data is ABI-encoded with orderbook and
encodedFN.
In `@src/order/sync.ts`:
- Around line 18-19: The yield currently sets version: res.__version
unconditionally which can overwrite an existing event.order.__version; change
the assignment to guard against undefined by using res.__version when it's
defined, otherwise fall back to the existing event.order.__version (e.g.
version: res.__version !== undefined ? res.__version : event?.order?.__version).
Update both occurrences where the yield/object uses version (the one with yield
{ event, url, timestamp: Number(res.timestamp), version: res.__version } and the
similar occurrence around lines 79-80) so you only overwrite when the upstream
provides a __version.
In `@src/order/types/v4.ts`:
- Around line 85-93: The comment above TakeOrdersConfigTypeV5 is misleading
because it says "version 6 orderbook" while the type name indicates this is the
V5 config shape; update the comment or name so they align — e.g., change the
comment to "Represents the V5 take orders configuration used by the version 6
orderbook (config schema v5)" or rename TakeOrdersConfigTypeV5 to
TakeOrdersConfigTypeV6 if the intent is a V6 config; adjust any references to
the type accordingly.
In `@src/router/types.ts`:
- Around line 235-244: The JSDoc incorrectly states "Creates a new
TakeOrdersConfigTypeV4" while the implementation constructs
TakeOrdersConfigTypeV5; update the documentation to reference
TakeOrdersConfigTypeV5 (and any other V4 mentions) so it matches the produced
type. Locate the JSDoc block documenting the method that returns
TakeOrdersConfigTypeV5 and replace "V4" with "V5", and adjust any param text if
it still incorrectly refers to v4 order objects to accurately say v5 where
applicable.
In `@src/subgraph/types.ts`:
- Around line 103-106: The enum member OLD_V in SubgraphVersions is ambiguous;
rename OLD_V to a clearer identifier such as LEGACY or PRE_V6 in the
SubgraphVersions enum declaration and update all references/usages of
SubgraphVersions.OLD_V across the codebase (imports, switch cases, comparisons,
and tests) to the new name to preserve behavior and ensure type safety.
Comment on lines
+103
to
+154
| export namespace _v6 { | ||
| // structs | ||
| export const Float = "bytes32" as const; | ||
| export const IOV2 = `(address token, bytes32 vaultId)` as const; | ||
| export const EvaluableV4 = `(address interpreter, address store, bytes bytecode)` as const; | ||
| export const SignedContextV1 = "(address signer, bytes32[] context, bytes signature)" as const; | ||
| export const TaskV2 = `(${EvaluableV4} evaluable, ${SignedContextV1}[] signedContext)` as const; | ||
| export const ClearStateChangeV2 = | ||
| `(${Float} aliceOutput, ${Float} bobOutput, ${Float} aliceInput, ${Float} bobInput)` as const; | ||
| export const OrderV4 = | ||
| `(address owner, ${EvaluableV4} evaluable, ${IOV2}[] validInputs, ${IOV2}[] validOutputs, bytes32 nonce)` as const; | ||
| export const TakeOrderConfigV4 = | ||
| `(${OrderV4} order, uint256 inputIOIndex, uint256 outputIOIndex, ${SignedContextV1}[] signedContext)` as const; | ||
| export const QuoteV2 = | ||
| `(${OrderV4} order, uint256 inputIOIndex, uint256 outputIOIndex, ${SignedContextV1}[] signedContext)` as const; | ||
| export const TakeOrdersConfigV5 = | ||
| `(${Float} minimumInput, ${Float} maximumInput, ${Float} maximumIORatio, ${TakeOrderConfigV4}[] orders, bytes data)` as const; | ||
| export const OrderConfigV4 = | ||
| `(${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; | ||
|
|
||
| // signatures | ||
| export const Orderbook = [ | ||
| `event OrderNotFound(address sender, address owner, bytes32 orderHash)` as const, | ||
| `event AddOrderV3(address sender, bytes32 orderHash, ${OrderV4} order)` as const, | ||
| `event OrderZeroAmount(address sender, address owner, bytes32 orderHash)` as const, | ||
| `event RemoveOrderV3(address sender, bytes32 orderHash, ${OrderV4} order)` as const, | ||
| `event AfterClearV2(address sender, ${ClearStateChangeV2} clearStateChange)` as const, | ||
| `event OrderExceedsMaxRatio(address sender, address owner, bytes32 orderHash)` as const, | ||
| `event DepositV2(address sender, address token, bytes32 vaultId, uint256 depositAmountUint256)` as const, | ||
| `event ClearV3(address sender, ${OrderV4} alice, ${OrderV4} bob, ${ClearConfigV2} clearConfig)` as const, | ||
| `event TakeOrderV3(address sender, ${TakeOrderConfigV4} config, ${Float} input, ${Float} output)` as const, | ||
| `event WithdrawV2(address sender, address token, bytes32 vaultId, ${Float} targetAmount, ${Float} withdrawAmount, uint256 withdrawAmountUint256)` as const, | ||
| `function entask2(${TaskV2}[] calldata tasks) external` as const, | ||
| `function orderExists(bytes32 orderHash) external view returns (bool exists)` as const, | ||
| `function vaultBalance2(address owner, address token, bytes32 vaultId) external view returns (${Float} balance)` as const, | ||
| `function deposit3(address token, bytes32 vaultId, ${Float} depositAmount, ${TaskV2}[] calldata tasks) external` as const, | ||
| `function withdraw3(address token, bytes32 vaultId, ${Float} targetAmount, ${TaskV2}[] calldata tasks) external` as const, | ||
| `function removeOrder3(${OrderV4} calldata order, ${TaskV2}[] calldata tasks) external returns (bool stateChanged)` as const, | ||
| `function addOrder4(${OrderConfigV4} calldata config, ${TaskV2}[] calldata tasks) external returns (bool stateChanged)` as const, | ||
| `function quote2(${QuoteV2} calldata quoteConfig) external view returns (bool exists, ${Float} outputMax, ${Float} ioRatio)` as const, | ||
| `function takeOrders4(${TakeOrdersConfigV5} calldata config) external returns (${Float} totalTakerInput, ${Float} totalTakerOutput)` as const, | ||
| `function clear3(${OrderV4} memory alice, ${OrderV4} memory bob, ${ClearConfigV2} calldata clearConfig, ${SignedContextV1}[] memory aliceSignedContext, ${SignedContextV1}[] memory bobSignedContext) external` as const, | ||
| "function multicall(bytes[] calldata data) external returns (bytes[] memory results)", | ||
| ] as const; | ||
| 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`, | ||
| ] as const; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for Solidity sources and ABI files related to V6 orderbook/arb
find . -type f \( -name "*.sol" -o -name "*.json" \) | head -30Repository: rainlanguage/rain.solver
Length of output: 1021
🏁 Script executed:
# Search for V6-related content in Solidity and ABI files
rg -l "addOrder4|takeOrders4|arb5" -g "*.sol" -g "*.json" -g "*.ts" | head -20Repository: rainlanguage/rain.solver
Length of output: 240
🏁 Script executed:
# Look for OrderBook contract definitions
rg -l "contract.*OrderBook|contract.*Orderbook" -g "*.sol" | head -10Repository: rainlanguage/rain.solver
Length of output: 50
🏁 Script executed:
# Search for specific function definitions in the codebase
rg "function\s+(addOrder4|takeOrders4|arb5)" -A 3 -g "*.sol"Repository: rainlanguage/rain.solver
Length of output: 50
🏁 Script executed:
# Search for V6 usage throughout the codebase
rg "_v6" -nRepository: rainlanguage/rain.solver
Length of output: 1617
🏁 Script executed:
# Check the OrderbookAbi definition and its exports
rg "OrderbookAbi" -A 10 -B 2 -n | head -50Repository: rainlanguage/rain.solver
Length of output: 2123
🏁 Script executed:
# Look at the complete orderbook.ts file structure
cat -n src/common/abis/orderbook.ts | grep -E "^[[:space:]]*[0-9]+(.*namespace|.*export const|.*V[0-9])" | head -40Repository: rainlanguage/rain.solver
Length of output: 3707
🏁 Script executed:
# Search for any reference to addOrder4 or takeOrders4 outside orderbook.ts
rg "addOrder4|takeOrders4" -n --type ts --type tsxRepository: rainlanguage/rain.solver
Length of output: 95
🏁 Script executed:
# Search for addOrder4 and takeOrders4 in TypeScript files only
rg "addOrder4|takeOrders4" -n --type tsRepository: rainlanguage/rain.solver
Length of output: 674
🏁 Script executed:
# Check if V6 is actually being used in the codebase
rg "OrderbookAbi\.V6" -n --type tsRepository: rainlanguage/rain.solver
Length of output: 50
🏁 Script executed:
# Look for test files that might reference V6
find . -type f -name "*.test.ts" | xargs grep -l "V6\|_v6" 2>/dev/nullRepository: rainlanguage/rain.solver
Length of output: 230
🏁 Script executed:
# Get the full _v6 namespace definition to inspect the signatures
sed -n '103,154p' src/common/abis/orderbook.tsRepository: rainlanguage/rain.solver
Length of output: 4704
🏁 Script executed:
# Check the V6 ABI export section in detail
sed -n '249,293p' src/common/abis/orderbook.tsRepository: rainlanguage/rain.solver
Length of output: 1868
🏁 Script executed:
# Check which Orderbook ABI version is being used in simulate.ts
sed -n '500,530p' src/core/modes/inter/simulate.tsRepository: rainlanguage/rain.solver
Length of output: 1262
🏁 Script executed:
# Check the full context of takeOrders4 usage in simulate.ts
sed -n '500,550p' src/core/modes/inter/simulate.ts | cat -nRepository: rainlanguage/rain.solver
Length of output: 2432
🏁 Script executed:
# Check for any imports or version selection logic in simulate.ts
head -100 src/core/modes/inter/simulate.ts | grep -E "import|OrderbookAbi|version"Repository: rainlanguage/rain.solver
Length of output: 718
🏁 Script executed:
# Look for where the Orderbook version is determined/set
rg "OrderbookAbi\." src/core/modes/inter/simulate.ts -A 2 -B 2 | head -40Repository: rainlanguage/rain.solver
Length of output: 50
🏁 Script executed:
# Check if V6 is actually exported from orderbook.ts
rg "export.*V6" src/common/abis/orderbook.tsRepository: rainlanguage/rain.solver
Length of output: 93
🏁 Script executed:
# Look at the exact TakeOrdersConfigV5 struct definition and compare with usage
sed -n '115,120p' src/common/abis/orderbook.tsRepository: rainlanguage/rain.solver
Length of output: 540
🏁 Script executed:
# Check the actual args structure being passed in simulate.ts more carefully
sed -n '505,530p' src/core/modes/inter/simulate.tsRepository: rainlanguage/rain.solver
Length of output: 1036
🏁 Script executed:
# Search for any type definitions related to TakeOrdersConfigV5 in the codebase
rg "TakeOrdersConfigV5|TakeOrdersConfigType" -A 5 -B 2 --type ts | head -80Repository: rainlanguage/rain.solver
Length of output: 4612
🏁 Script executed:
# Check if there's a mapping or transformation of field names somewhere
rg "minimumIO|maximumIO|IOIsInput" -B 3 -A 3 --type ts | head -60Repository: rainlanguage/rain.solver
Length of output: 3707
🏁 Script executed:
# Look for TakeOrdersConfigTypeV5 type definition
rg "interface TakeOrdersConfigTypeV5|type TakeOrdersConfigTypeV5" -A 10 --type tsRepository: rainlanguage/rain.solver
Length of output: 592
🏁 Script executed:
# Check if there's a mismatch between ABI definition and actual struct usage
sed -n '115,120p' src/common/abis/orderbook.ts && echo "---ABI Definition Above---" && rg "TakeOrdersConfigTypeV5 = " -A 7 src/router/types.tsRepository: rainlanguage/rain.solver
Length of output: 885
🏁 Script executed:
# Get the complete TakeOrdersConfigTypeV5 type definition from order/index.ts or where it's defined
rg -l "TakeOrdersConfigTypeV5" --type ts | xargs cat | grep -A 15 "type TakeOrdersConfigTypeV5\|interface TakeOrdersConfigTypeV5"Repository: rainlanguage/rain.solver
Length of output: 467
🏁 Script executed:
# Check the order/index.ts file for type definitions
find . -path "./src/order/*" -name "*.ts" | head -10Repository: rainlanguage/rain.solver
Length of output: 312
🏁 Script executed:
# Verify the exact ABI string definition once more to be absolutely sure
sed -n '103,154p' src/common/abis/orderbook.ts | grep -A 1 "TakeOrdersConfigV5"Repository: rainlanguage/rain.solver
Length of output: 980
🏁 Script executed:
# Check how encodeFunctionData is being called with the mismatched struct
sed -n '510,530p' src/core/modes/inter/simulate.tsRepository: rainlanguage/rain.solver
Length of output: 852
🏁 Script executed:
# Look for any other V5/V6 structs that might have similar field name mismatches
rg "minimumInput|maximumInput" src/order/types/ --type tsRepository: rainlanguage/rain.solver
Length of output: 388
🏁 Script executed:
# Check if the ABI definitions for V4 and V5 have the correct field names for comparison
sed -n '1,102p' src/common/abis/orderbook.ts | tail -30Repository: rainlanguage/rain.solver
Length of output: 3030
🏁 Script executed:
# Double-check the V5 namespace to see the pattern for comparison
sed -n '50,102p' src/common/abis/orderbook.ts | grep -E "export const (TakeOrdersConfigV4|TaskV2|OrderV4)" -A 2Repository: rainlanguage/rain.solver
Length of output: 706
🏁 Script executed:
# Verify if the struct encoding works at runtime by checking test files
rg "takeOrders4" src/core/modes/inter/simulate.test.ts -B 5 -A 10 | head -40Repository: rainlanguage/rain.solver
Length of output: 1538
Fix V6 ABI struct field names to match TypeScript type definitions.
The TakeOrdersConfigV5 struct in the V6 namespace has a critical mismatch: the ABI string defines fields as minimumInput and maximumInput, but the TypeScript type (TakeOrdersConfigTypeV5 in src/order/types/v4.ts) uses minimumIO, maximumIO, and includes an IOIsInput boolean field. When encodeFunctionData attempts to encode the struct for takeOrders4() and arb5() calls, it will fail to match ABI field names to the provided arguments, breaking calldata encoding at runtime.
Update line 119 to:
export const TakeOrdersConfigV5 =
`(${Float} minimumIO, ${Float} maximumIO, ${Float} maximumIORatio, bool IOIsInput, ${TakeOrderConfigV4}[] orders, bytes data)` as const;
🤖 Prompt for AI Agents
In `@src/common/abis/orderbook.ts` around lines 103 - 154, The TakeOrdersConfigV5
ABI struct fields in namespace _v6 (symbol TakeOrdersConfigV5) don't match the
TypeScript type (TakeOrdersConfigTypeV5) — update the ABI string for
TakeOrdersConfigV5 to use minimumIO, maximumIO, maximumIORatio, add the bool
IOIsInput field, then orders and data (i.e. `(${Float} minimumIO, ${Float}
maximumIO, ${Float} maximumIORatio, bool IOIsInput, ${TakeOrderConfigV4}[]
orders, bytes data)`) so calldata encoding for takeOrders4 and arb5 will align
with the TypeScript types.
Comment on lines
+638
to
+691
| describe("Test getTakeOrdersConfigV5 method", () => { | ||
| let minFloatSpy: any; | ||
| let maxFloatSpy: any; | ||
| let toFloatSpy: any; | ||
|
|
||
| beforeEach(() => { | ||
| if (minFloatSpy) { | ||
| minFloatSpy.mockRestore(); | ||
| maxFloatSpy.mockRestore(); | ||
| toFloatSpy.mockRestore(); | ||
| } | ||
| minFloatSpy = vi.spyOn(common, "minFloat"); | ||
| maxFloatSpy = vi.spyOn(common, "maxFloat"); | ||
| toFloatSpy = vi.spyOn(common, "toFloat"); | ||
| }); | ||
|
|
||
| it("should return v4 config", () => { | ||
| (encodeAbiParameters as Mock).mockReturnValue("0xdata"); | ||
| (minFloatSpy as Mock).mockReturnValueOnce("0xmin"); | ||
| (maxFloatSpy as Mock).mockReturnValueOnce("0xmax1").mockReturnValueOnce("0xmax2"); | ||
| const result = simulator.getTakeOrdersConfigV5( | ||
| simulator.tradeArgs.orderDetails as any, | ||
| simulator.tradeArgs.counterpartyOrderDetails, | ||
| "0x1234", | ||
| ); | ||
| expect(result).toEqual({ | ||
| minimumIO: "0xmin", | ||
| maximumIO: "0xmax1", | ||
| maximumIORatio: "0xmax2", | ||
| IOIsInput: true, | ||
| orders: [simulator.tradeArgs.orderDetails.takeOrder.struct], | ||
| data: "0xdata", | ||
| }); | ||
| expect(minFloatSpy).toHaveBeenCalledTimes(1); | ||
| expect(minFloatSpy).toHaveBeenCalledWith( | ||
| simulator.tradeArgs.orderDetails.sellTokenDecimals, | ||
| ); | ||
| expect(maxFloatSpy).toHaveBeenCalledTimes(2); | ||
| expect(maxFloatSpy).toHaveBeenNthCalledWith( | ||
| 1, | ||
| simulator.tradeArgs.orderDetails.sellTokenDecimals, | ||
| ); | ||
| expect(maxFloatSpy).toHaveBeenNthCalledWith(2, 18); | ||
| expect(encodeAbiParameters).toHaveBeenCalledTimes(1); | ||
| expect(encodeAbiParameters).toHaveBeenCalledWith( | ||
| [{ type: "address" }, { type: "address" }, { type: "bytes" }], | ||
| [ | ||
| simulator.tradeArgs.counterpartyOrderDetails.orderbook as `0x${string}`, | ||
| simulator.tradeArgs.counterpartyOrderDetails.orderbook as `0x${string}`, | ||
| "0x1234", | ||
| ], | ||
| ); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Fix test description: says "v4 config" but tests V5.
Line 654 describes the test as "should return v4 config" but it's testing getTakeOrdersConfigV5 and expects V5 fields (minimumIO, maximumIO, IOIsInput).
📝 Proposed fix
- it("should return v4 config", () => {
+ it("should return v5 config", () => {📝 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 getTakeOrdersConfigV5 method", () => { | |
| let minFloatSpy: any; | |
| let maxFloatSpy: any; | |
| let toFloatSpy: any; | |
| beforeEach(() => { | |
| if (minFloatSpy) { | |
| minFloatSpy.mockRestore(); | |
| maxFloatSpy.mockRestore(); | |
| toFloatSpy.mockRestore(); | |
| } | |
| minFloatSpy = vi.spyOn(common, "minFloat"); | |
| maxFloatSpy = vi.spyOn(common, "maxFloat"); | |
| toFloatSpy = vi.spyOn(common, "toFloat"); | |
| }); | |
| it("should return v4 config", () => { | |
| (encodeAbiParameters as Mock).mockReturnValue("0xdata"); | |
| (minFloatSpy as Mock).mockReturnValueOnce("0xmin"); | |
| (maxFloatSpy as Mock).mockReturnValueOnce("0xmax1").mockReturnValueOnce("0xmax2"); | |
| const result = simulator.getTakeOrdersConfigV5( | |
| simulator.tradeArgs.orderDetails as any, | |
| simulator.tradeArgs.counterpartyOrderDetails, | |
| "0x1234", | |
| ); | |
| expect(result).toEqual({ | |
| minimumIO: "0xmin", | |
| maximumIO: "0xmax1", | |
| maximumIORatio: "0xmax2", | |
| IOIsInput: true, | |
| orders: [simulator.tradeArgs.orderDetails.takeOrder.struct], | |
| data: "0xdata", | |
| }); | |
| expect(minFloatSpy).toHaveBeenCalledTimes(1); | |
| expect(minFloatSpy).toHaveBeenCalledWith( | |
| simulator.tradeArgs.orderDetails.sellTokenDecimals, | |
| ); | |
| expect(maxFloatSpy).toHaveBeenCalledTimes(2); | |
| expect(maxFloatSpy).toHaveBeenNthCalledWith( | |
| 1, | |
| simulator.tradeArgs.orderDetails.sellTokenDecimals, | |
| ); | |
| expect(maxFloatSpy).toHaveBeenNthCalledWith(2, 18); | |
| expect(encodeAbiParameters).toHaveBeenCalledTimes(1); | |
| expect(encodeAbiParameters).toHaveBeenCalledWith( | |
| [{ type: "address" }, { type: "address" }, { type: "bytes" }], | |
| [ | |
| simulator.tradeArgs.counterpartyOrderDetails.orderbook as `0x${string}`, | |
| simulator.tradeArgs.counterpartyOrderDetails.orderbook as `0x${string}`, | |
| "0x1234", | |
| ], | |
| ); | |
| }); | |
| }); | |
| describe("Test getTakeOrdersConfigV5 method", () => { | |
| let minFloatSpy: any; | |
| let maxFloatSpy: any; | |
| let toFloatSpy: any; | |
| beforeEach(() => { | |
| if (minFloatSpy) { | |
| minFloatSpy.mockRestore(); | |
| maxFloatSpy.mockRestore(); | |
| toFloatSpy.mockRestore(); | |
| } | |
| minFloatSpy = vi.spyOn(common, "minFloat"); | |
| maxFloatSpy = vi.spyOn(common, "maxFloat"); | |
| toFloatSpy = vi.spyOn(common, "toFloat"); | |
| }); | |
| it("should return v5 config", () => { | |
| (encodeAbiParameters as Mock).mockReturnValue("0xdata"); | |
| (minFloatSpy as Mock).mockReturnValueOnce("0xmin"); | |
| (maxFloatSpy as Mock).mockReturnValueOnce("0xmax1").mockReturnValueOnce("0xmax2"); | |
| const result = simulator.getTakeOrdersConfigV5( | |
| simulator.tradeArgs.orderDetails as any, | |
| simulator.tradeArgs.counterpartyOrderDetails, | |
| "0x1234", | |
| ); | |
| expect(result).toEqual({ | |
| minimumIO: "0xmin", | |
| maximumIO: "0xmax1", | |
| maximumIORatio: "0xmax2", | |
| IOIsInput: true, | |
| orders: [simulator.tradeArgs.orderDetails.takeOrder.struct], | |
| data: "0xdata", | |
| }); | |
| expect(minFloatSpy).toHaveBeenCalledTimes(1); | |
| expect(minFloatSpy).toHaveBeenCalledWith( | |
| simulator.tradeArgs.orderDetails.sellTokenDecimals, | |
| ); | |
| expect(maxFloatSpy).toHaveBeenCalledTimes(2); | |
| expect(maxFloatSpy).toHaveBeenNthCalledWith( | |
| 1, | |
| simulator.tradeArgs.orderDetails.sellTokenDecimals, | |
| ); | |
| expect(maxFloatSpy).toHaveBeenNthCalledWith(2, 18); | |
| expect(encodeAbiParameters).toHaveBeenCalledTimes(1); | |
| expect(encodeAbiParameters).toHaveBeenCalledWith( | |
| [{ type: "address" }, { type: "address" }, { type: "bytes" }], | |
| [ | |
| simulator.tradeArgs.counterpartyOrderDetails.orderbook as `0x${string}`, | |
| simulator.tradeArgs.counterpartyOrderDetails.orderbook as `0x${string}`, | |
| "0x1234", | |
| ], | |
| ); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In `@src/core/modes/inter/simulate.test.ts` around lines 638 - 691, The test
description is wrong: update the it(...) description that currently reads
"should return v4 config" to accurately say "should return v5 config" (the test
is exercising getTakeOrdersConfigV5 and asserting V5-specific fields like
minimumIO, maximumIO, maximumIORatio, IOIsInput), i.e., locate the it block with
the string literal "should return v4 config" and change it to reference V5 so
the test name matches the behavior being tested.
Comment on lines
+323
to
+350
| /** | ||
| * Creates a new TakeOrdersConfigTypeV4 based on the v4 order and counterparty order | ||
| * @param orderDetails - The order pair v4 to create the config for | ||
| * @param counterpartyOrderDetails - Counterparty order | ||
| * @param maximumInputFixed - The trade maximum input | ||
| */ | ||
| getTakeOrdersConfigV5( | ||
| orderDetails: PairV4, | ||
| counterpartyOrderDetails: Pair, | ||
| encodedFN: `0x${string}`, | ||
| ): TakeOrdersConfigTypeV5 { | ||
| const takeOrdersConfigStruct: TakeOrdersConfigTypeV5 = { | ||
| minimumIO: minFloat(orderDetails.sellTokenDecimals), | ||
| maximumIO: maxFloat(orderDetails.sellTokenDecimals), | ||
| maximumIORatio: maxFloat(18), | ||
| IOIsInput: true, | ||
| orders: [orderDetails.takeOrder.struct], | ||
| data: encodeAbiParameters( | ||
| [{ type: "address" }, { type: "address" }, { type: "bytes" }], | ||
| [ | ||
| counterpartyOrderDetails.orderbook as `0x${string}`, | ||
| counterpartyOrderDetails.orderbook as `0x${string}`, | ||
| encodedFN, | ||
| ], | ||
| ), | ||
| }; | ||
| return takeOrdersConfigStruct; | ||
| } |
Contributor
There was a problem hiding this comment.
Docstring still refers to V4.
This block is V5-specific; the comment should be updated to avoid confusion.
📝 Suggested doc fix
- * Creates a new TakeOrdersConfigTypeV4 based on the v4 order and counterparty order
+ * Creates a new TakeOrdersConfigTypeV5 based on the v4 order and counterparty order📝 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
| /** | |
| * Creates a new TakeOrdersConfigTypeV4 based on the v4 order and counterparty order | |
| * @param orderDetails - The order pair v4 to create the config for | |
| * @param counterpartyOrderDetails - Counterparty order | |
| * @param maximumInputFixed - The trade maximum input | |
| */ | |
| getTakeOrdersConfigV5( | |
| orderDetails: PairV4, | |
| counterpartyOrderDetails: Pair, | |
| encodedFN: `0x${string}`, | |
| ): TakeOrdersConfigTypeV5 { | |
| const takeOrdersConfigStruct: TakeOrdersConfigTypeV5 = { | |
| minimumIO: minFloat(orderDetails.sellTokenDecimals), | |
| maximumIO: maxFloat(orderDetails.sellTokenDecimals), | |
| maximumIORatio: maxFloat(18), | |
| IOIsInput: true, | |
| orders: [orderDetails.takeOrder.struct], | |
| data: encodeAbiParameters( | |
| [{ type: "address" }, { type: "address" }, { type: "bytes" }], | |
| [ | |
| counterpartyOrderDetails.orderbook as `0x${string}`, | |
| counterpartyOrderDetails.orderbook as `0x${string}`, | |
| encodedFN, | |
| ], | |
| ), | |
| }; | |
| return takeOrdersConfigStruct; | |
| } | |
| /** | |
| * Creates a new TakeOrdersConfigTypeV5 based on the v4 order and counterparty order | |
| * `@param` orderDetails - The order pair v4 to create the config for | |
| * `@param` counterpartyOrderDetails - Counterparty order | |
| * `@param` maximumInputFixed - The trade maximum input | |
| */ | |
| getTakeOrdersConfigV5( | |
| orderDetails: PairV4, | |
| counterpartyOrderDetails: Pair, | |
| encodedFN: `0x${string}`, | |
| ): TakeOrdersConfigTypeV5 { | |
| const takeOrdersConfigStruct: TakeOrdersConfigTypeV5 = { | |
| minimumIO: minFloat(orderDetails.sellTokenDecimals), | |
| maximumIO: maxFloat(orderDetails.sellTokenDecimals), | |
| maximumIORatio: maxFloat(18), | |
| IOIsInput: true, | |
| orders: [orderDetails.takeOrder.struct], | |
| data: encodeAbiParameters( | |
| [{ type: "address" }, { type: "address" }, { type: "bytes" }], | |
| [ | |
| counterpartyOrderDetails.orderbook as `0x${string}`, | |
| counterpartyOrderDetails.orderbook as `0x${string}`, | |
| encodedFN, | |
| ], | |
| ), | |
| }; | |
| return takeOrdersConfigStruct; | |
| } |
🤖 Prompt for AI Agents
In `@src/core/modes/inter/simulate.ts` around lines 323 - 350, The method
getTakeOrdersConfigV5 currently has a docstring mentioning "V4" which is
incorrect; update the comment to describe V5 semantics and parameters: change
the header to reference TakeOrdersConfigTypeV5 and V5 order/counterparty
parameters, and adjust any parameter descriptions (orderDetails: PairV4 ->
V5-specific wording, counterpartyOrderDetails: Pair) to reflect this method
builds a V5 take-orders config including encodedFN; ensure the summary, param
descriptions, and return type mention V5 (TakeOrdersConfigTypeV5) and that data
is ABI-encoded with orderbook and encodedFN.
Comment on lines
+18
to
19
| yield { event, url, timestamp: Number(res.timestamp), version: res.__version }; | ||
| } |
Contributor
There was a problem hiding this comment.
Guard __version assignment when upstream omits it.
If res.__version is undefined (older subgraphs or partial data), this overwrites any existing event.order.__version and can break versioned routing. Prefer a guard (or explicit default) before assignment.
🔧 Suggested fix
- event.order.__version = version;
+ if (version !== undefined) {
+ event.order.__version = version;
+ }Also applies to: 79-80
🤖 Prompt for AI Agents
In `@src/order/sync.ts` around lines 18 - 19, The yield currently sets version:
res.__version unconditionally which can overwrite an existing
event.order.__version; change the assignment to guard against undefined by using
res.__version when it's defined, otherwise fall back to the existing
event.order.__version (e.g. version: res.__version !== undefined ? res.__version
: event?.order?.__version). Update both occurrences where the yield/object uses
version (the one with yield { event, url, timestamp: Number(res.timestamp),
version: res.__version } and the similar occurrence around lines 79-80) so you
only overwrite when the upstream provides a __version.
Comment on lines
+85
to
+93
| /** Represents the take orders configuration structure for version 6 orderbook */ | ||
| export type TakeOrdersConfigTypeV5 = { | ||
| minimumIO: `0x${string}`; | ||
| maximumIO: `0x${string}`; | ||
| maximumIORatio: `0x${string}`; | ||
| IOIsInput: boolean; | ||
| orders: TakeOrderV4[]; | ||
| data: `0x${string}`; | ||
| }; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify naming: type is TakeOrdersConfigTypeV5 but comment says "version 6 orderbook".
The comment indicates this is for "version 6 orderbook" but the type is named TakeOrdersConfigTypeV5. This likely reflects that the config structure version (V5) differs from the orderbook version (V6). Consider updating the comment to clarify this relationship to avoid confusion.
-/** Represents the take orders configuration structure for version 6 orderbook */
+/** Represents the take orders configuration structure (V5 format) for version 6 orderbook */📝 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
| /** Represents the take orders configuration structure for version 6 orderbook */ | |
| export type TakeOrdersConfigTypeV5 = { | |
| minimumIO: `0x${string}`; | |
| maximumIO: `0x${string}`; | |
| maximumIORatio: `0x${string}`; | |
| IOIsInput: boolean; | |
| orders: TakeOrderV4[]; | |
| data: `0x${string}`; | |
| }; | |
| /** Represents the take orders configuration structure (V5 format) for version 6 orderbook */ | |
| export type TakeOrdersConfigTypeV5 = { | |
| minimumIO: `0x${string}`; | |
| maximumIO: `0x${string}`; | |
| maximumIORatio: `0x${string}`; | |
| IOIsInput: boolean; | |
| orders: TakeOrderV4[]; | |
| data: `0x${string}`; | |
| }; |
🤖 Prompt for AI Agents
In `@src/order/types/v4.ts` around lines 85 - 93, The comment above
TakeOrdersConfigTypeV5 is misleading because it says "version 6 orderbook" while
the type name indicates this is the V5 config shape; update the comment or name
so they align — e.g., change the comment to "Represents the V5 take orders
configuration used by the version 6 orderbook (config schema v5)" or rename
TakeOrdersConfigTypeV5 to TakeOrdersConfigTypeV6 if the intent is a V6 config;
adjust any references to the type accordingly.
Comment on lines
+235
to
+244
| /** | ||
| * Creates a new TakeOrdersConfigTypeV4 based on the given v4 order and other parameters | ||
| * This is the default implementation, but child classes can override it if needed. | ||
| * @param order The order pair v4 to create the config for | ||
| * @param maximumInput The maximum input amount | ||
| * @param price The current market price for input amount | ||
| * @param data The TakeOrdersConfig data | ||
| * @param maxRatio Whether to use the maximum IO ratio | ||
| * @param isPartial Whether the trade is a partial fill | ||
| */ |
Contributor
There was a problem hiding this comment.
Fix copy-paste error in JSDoc: says V4 but creates V5.
The documentation states "Creates a new TakeOrdersConfigTypeV4" but the method creates TakeOrdersConfigTypeV5.
📝 Proposed fix
/**
- * Creates a new TakeOrdersConfigTypeV4 based on the given v4 order and other parameters
+ * Creates a new TakeOrdersConfigTypeV5 based on the given v4 order and other parameters
* This is the default implementation, but child classes can override it if needed.
* `@param` order The order pair v4 to create the config for
* `@param` maximumInput The maximum input amount
* `@param` price The current market price for input amount
* `@param` data The TakeOrdersConfig data
* `@param` maxRatio Whether to use the maximum IO ratio
* `@param` isPartial Whether the trade is a partial fill
*/📝 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
| /** | |
| * Creates a new TakeOrdersConfigTypeV4 based on the given v4 order and other parameters | |
| * This is the default implementation, but child classes can override it if needed. | |
| * @param order The order pair v4 to create the config for | |
| * @param maximumInput The maximum input amount | |
| * @param price The current market price for input amount | |
| * @param data The TakeOrdersConfig data | |
| * @param maxRatio Whether to use the maximum IO ratio | |
| * @param isPartial Whether the trade is a partial fill | |
| */ | |
| /** | |
| * Creates a new TakeOrdersConfigTypeV5 based on the given v4 order and other parameters | |
| * This is the default implementation, but child classes can override it if needed. | |
| * `@param` order The order pair v4 to create the config for | |
| * `@param` maximumInput The maximum input amount | |
| * `@param` price The current market price for input amount | |
| * `@param` data The TakeOrdersConfig data | |
| * `@param` maxRatio Whether to use the maximum IO ratio | |
| * `@param` isPartial Whether the trade is a partial fill | |
| */ |
🤖 Prompt for AI Agents
In `@src/router/types.ts` around lines 235 - 244, The JSDoc incorrectly states
"Creates a new TakeOrdersConfigTypeV4" while the implementation constructs
TakeOrdersConfigTypeV5; update the documentation to reference
TakeOrdersConfigTypeV5 (and any other V4 mentions) so it matches the produced
type. Locate the JSDoc block documenting the method that returns
TakeOrdersConfigTypeV5 and replace "V4" with "V5", and adjust any param text if
it still incorrectly refers to v4 order objects to accurately say v5 where
applicable.
Comment on lines
+103
to
+106
| export enum SubgraphVersions { | ||
| OLD_V = "old", | ||
| V6 = "v6", | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider more descriptive naming for OLD_V enum member.
The SubgraphVersions enum implementation is correct, but OLD_V is somewhat ambiguous about which versions it represents. If it covers V4 and V5, a more explicit name like LEGACY or PRE_V6 might improve clarity for future maintainers.
💡 Alternative naming suggestion
export enum SubgraphVersions {
- OLD_V = "old",
+ LEGACY = "legacy", // Covers V4/V5 orderbook versions
V6 = "v6",
}🤖 Prompt for AI Agents
In `@src/subgraph/types.ts` around lines 103 - 106, The enum member OLD_V in
SubgraphVersions is ambiguous; rename OLD_V to a clearer identifier such as
LEGACY or PRE_V6 in the SubgraphVersions enum declaration and update all
references/usages of SubgraphVersions.OLD_V across the codebase (imports, switch
cases, comparisons, and tests) to the new name to preserve behavior and ensure
type safety.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Updates
✏️ Tip: You can customize this high-level summary in your review settings.