skip balance tracking for vaultless vaults orderbook v6 #432
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
WalkthroughThe PR introduces version-awareness to vault handling in the orderbook system. OrderbookVersions parameter is propagated through vault update calls and conditional logic is applied for V6 orderbooks, particularly for vaultless configurations identified by ZERO_BYTES_32 constant. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 |
hardyjosh
approved these changes
Feb 4, 2026
![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: 4
🤖 Fix all issues with AI agents
In `@src/core/process/round.ts`:
- Around line 186-194: The condition redundantly checks
orderDetails.sellTokenVaultBalance <= 0n in both branches; refactor the if in
round.ts to first check sellTokenVaultBalance <= 0n and then, if needed,
evaluate the V6-specific vaultId condition
(orderDetails.takeOrder.struct.outputIOIndex and its validOutputs[].vaultId
compared to ZERO_BYTES_32) only when orderDetails.orderbookVersion ===
OrderbookVersions.V6, so the common guard is factored out and the logic remains
equivalent.
In `@src/order/index.test.ts`:
- Around line 1268-1300: The test currently checks the spy with
expect(spy).not.toHaveBeenCalledWith(), which only asserts no call with zero
args; change this to assert the spy was never called by replacing that line with
expect(spy).not.toHaveBeenCalled() (or expect(spy).toHaveBeenCalledTimes(0)) so
the test truly verifies common.normalizeFloat was not invoked when calling
orderManager.updateVault(..., OrderbookVersions.V6); keep the spy setup and
spy.mockRestore() as-is.
In `@src/order/sync.test.ts`:
- Around line 86-91: Add a test that covers the V6 subgraph branch by creating a
mock transaction result where you set res.__version = SubgraphVersions.V6 (reuse
the existing Deposit/Withdrawal/Clear/TakeOrder test pattern), run the same sync
logic that triggers mockUpdateVault, and assert mockUpdateVault was called with
OrderbookVersions.V6 as the final argument; reference the existing
mockUpdateVault, SubgraphVersions, and OrderbookVersions symbols to
modify/extend the test so the last parameter expectation checks for
OrderbookVersions.V6.
In `@src/order/sync.ts`:
- Line 38: The code passes an invalid "" cast as any into updateVault's
orderbookVersion parameter (seen where it computes version ===
SubgraphVersions.V6 ? OrderbookVersions.V6 : ("" as any)), which bypasses type
safety; update updateVault(orderbookVersion: OrderbookVersions) usage by either
making orderbookVersion optional in updateVault's declaration
(orderbookVersion?: OrderbookVersions) and handle undefined inside updateVault,
or replace the fallback with a concrete enum member (e.g., OrderbookVersions.V1
or a clearly named OrderbookVersions.Unknown) and handle that case; apply the
same fix to the other occurrences (the branches at the other two calls in
sync.ts).
Comment on lines
+186
to
+194
| if ( | ||
| (orderDetails.orderbookVersion !== OrderbookVersions.V6 && | ||
| orderDetails.sellTokenVaultBalance <= 0n) || | ||
| (orderDetails.orderbookVersion === OrderbookVersions.V6 && | ||
| orderDetails.takeOrder.struct.order.validOutputs[ | ||
| orderDetails.takeOrder.struct.outputIOIndex | ||
| ].vaultId !== ZERO_BYTES_32 && | ||
| orderDetails.sellTokenVaultBalance <= 0n) | ||
| ) { |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: the condition can be simplified by factoring out the common <= 0n check.
Both branches share the sellTokenVaultBalance <= 0n guard.
Simplified condition
- if (
- (orderDetails.orderbookVersion !== OrderbookVersions.V6 &&
- orderDetails.sellTokenVaultBalance <= 0n) ||
- (orderDetails.orderbookVersion === OrderbookVersions.V6 &&
- orderDetails.takeOrder.struct.order.validOutputs[
- orderDetails.takeOrder.struct.outputIOIndex
- ].vaultId !== ZERO_BYTES_32 &&
- orderDetails.sellTokenVaultBalance <= 0n)
- ) {
+ const isVaultless =
+ orderDetails.orderbookVersion === OrderbookVersions.V6 &&
+ orderDetails.takeOrder.struct.order.validOutputs[
+ orderDetails.takeOrder.struct.outputIOIndex
+ ].vaultId === ZERO_BYTES_32;
+ if (!isVaultless && orderDetails.sellTokenVaultBalance <= 0n) {🤖 Prompt for AI Agents
In `@src/core/process/round.ts` around lines 186 - 194, The condition redundantly
checks orderDetails.sellTokenVaultBalance <= 0n in both branches; refactor the
if in round.ts to first check sellTokenVaultBalance <= 0n and then, if needed,
evaluate the V6-specific vaultId condition
(orderDetails.takeOrder.struct.outputIOIndex and its validOutputs[].vaultId
compared to ZERO_BYTES_32) only when orderDetails.orderbookVersion ===
OrderbookVersions.V6, so the common guard is factored out and the logic remains
equivalent.
Comment on lines
+1268
to
+1300
| it("should set 0 vault balance for vaultless orderbook v6", () => { | ||
| const orderbook = "0xorderbook1"; | ||
| const owner = "0xowner1"; | ||
| const token = { | ||
| address: "0xtoken1", | ||
| symbol: "TOKEN1", | ||
| decimals: 18, | ||
| }; | ||
| const vaultId = 0n; | ||
| const balance = "0xffffffee00000000000000000000000000000000000000000000000000000001"; // ignored | ||
| const spy = vi.spyOn(common, "normalizeFloat"); | ||
|
|
||
| orderManager.updateVault(orderbook, owner, token, vaultId, balance, OrderbookVersions.V6); | ||
|
|
||
| expect(spy).not.toHaveBeenCalledWith(); | ||
|
|
||
| const orderbookMap = orderManager.ownerTokenVaultMap.get(orderbook); | ||
| expect(orderbookMap).toBeDefined(); | ||
|
|
||
| const ownerMap = orderbookMap?.get(owner); | ||
| expect(ownerMap).toBeDefined(); | ||
|
|
||
| const tokenMap = ownerMap?.get(token.address); | ||
| expect(tokenMap).toBeDefined(); | ||
|
|
||
| const vault = tokenMap?.get(vaultId); | ||
| expect(vault).toBeDefined(); | ||
| expect(vault?.id).toBe(vaultId); | ||
| expect(vault?.balance).toBe(0n); // skip tracking, always 0 | ||
| expect(vault?.token).toEqual(token); | ||
|
|
||
| spy.mockRestore(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Bug: expect(spy).not.toHaveBeenCalledWith() does not assert the spy was never called.
On line 1282, .not.toHaveBeenCalledWith() with no arguments only asserts the spy was not called with zero arguments — it does not assert that normalizeFloat was never invoked. The intent here is to verify that balance normalization is skipped entirely for vaultless V6 vaults.
Fix the assertion
- expect(spy).not.toHaveBeenCalledWith();
+ expect(spy).not.toHaveBeenCalled();📝 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 set 0 vault balance for vaultless orderbook v6", () => { | |
| const orderbook = "0xorderbook1"; | |
| const owner = "0xowner1"; | |
| const token = { | |
| address: "0xtoken1", | |
| symbol: "TOKEN1", | |
| decimals: 18, | |
| }; | |
| const vaultId = 0n; | |
| const balance = "0xffffffee00000000000000000000000000000000000000000000000000000001"; // ignored | |
| const spy = vi.spyOn(common, "normalizeFloat"); | |
| orderManager.updateVault(orderbook, owner, token, vaultId, balance, OrderbookVersions.V6); | |
| expect(spy).not.toHaveBeenCalledWith(); | |
| const orderbookMap = orderManager.ownerTokenVaultMap.get(orderbook); | |
| expect(orderbookMap).toBeDefined(); | |
| const ownerMap = orderbookMap?.get(owner); | |
| expect(ownerMap).toBeDefined(); | |
| const tokenMap = ownerMap?.get(token.address); | |
| expect(tokenMap).toBeDefined(); | |
| const vault = tokenMap?.get(vaultId); | |
| expect(vault).toBeDefined(); | |
| expect(vault?.id).toBe(vaultId); | |
| expect(vault?.balance).toBe(0n); // skip tracking, always 0 | |
| expect(vault?.token).toEqual(token); | |
| spy.mockRestore(); | |
| }); | |
| it("should set 0 vault balance for vaultless orderbook v6", () => { | |
| const orderbook = "0xorderbook1"; | |
| const owner = "0xowner1"; | |
| const token = { | |
| address: "0xtoken1", | |
| symbol: "TOKEN1", | |
| decimals: 18, | |
| }; | |
| const vaultId = 0n; | |
| const balance = "0xffffffee00000000000000000000000000000000000000000000000000000001"; // ignored | |
| const spy = vi.spyOn(common, "normalizeFloat"); | |
| orderManager.updateVault(orderbook, owner, token, vaultId, balance, OrderbookVersions.V6); | |
| expect(spy).not.toHaveBeenCalled(); | |
| const orderbookMap = orderManager.ownerTokenVaultMap.get(orderbook); | |
| expect(orderbookMap).toBeDefined(); | |
| const ownerMap = orderbookMap?.get(owner); | |
| expect(ownerMap).toBeDefined(); | |
| const tokenMap = ownerMap?.get(token.address); | |
| expect(tokenMap).toBeDefined(); | |
| const vault = tokenMap?.get(vaultId); | |
| expect(vault).toBeDefined(); | |
| expect(vault?.id).toBe(vaultId); | |
| expect(vault?.balance).toBe(0n); // skip tracking, always 0 | |
| expect(vault?.token).toEqual(token); | |
| spy.mockRestore(); | |
| }); |
🤖 Prompt for AI Agents
In `@src/order/index.test.ts` around lines 1268 - 1300, The test currently checks
the spy with expect(spy).not.toHaveBeenCalledWith(), which only asserts no call
with zero args; change this to assert the spy was never called by replacing that
line with expect(spy).not.toHaveBeenCalled() (or
expect(spy).toHaveBeenCalledTimes(0)) so the test truly verifies
common.normalizeFloat was not invoked when calling orderManager.updateVault(...,
OrderbookVersions.V6); keep the spy setup and spy.mockRestore() as-is.
Comment on lines
86
to
91
| 123n, | ||
| "1000000000000000000", | ||
| "", | ||
| ); | ||
| expect(mockUpdateVault).toHaveBeenCalledTimes(1); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing test coverage for the V6 subgraph version path in vault-related events.
All Deposit/Withdrawal/Clear/TakeOrder tests pass "" as the expected orderbookVersion since none of the mock transactions set __version to SubgraphVersions.V6. Consider adding at least one test that sets res.__version = SubgraphVersions.V6 and asserts that mockUpdateVault is called with OrderbookVersions.V6 as the last argument.
🤖 Prompt for AI Agents
In `@src/order/sync.test.ts` around lines 86 - 91, Add a test that covers the V6
subgraph branch by creating a mock transaction result where you set
res.__version = SubgraphVersions.V6 (reuse the existing
Deposit/Withdrawal/Clear/TakeOrder test pattern), run the same sync logic that
triggers mockUpdateVault, and assert mockUpdateVault was called with
OrderbookVersions.V6 as the final argument; reference the existing
mockUpdateVault, SubgraphVersions, and OrderbookVersions symbols to
modify/extend the test so the last parameter expectation checks for
OrderbookVersions.V6.
| }, | ||
| BigInt(event.vault.vaultId), | ||
| event.vault.balance, | ||
| version === SubgraphVersions.V6 ? OrderbookVersions.V6 : ("" as any), |
Contributor
There was a problem hiding this comment.
"" as any bypasses type safety and is fragile.
The updateVault signature declares orderbookVersion: OrderbookVersions, but the empty string "" is not a valid enum member. This compiles only because of the as any cast. If future code adds version-specific branches (e.g., for V4 or V5), the empty string will silently fall through.
Consider either making the parameter optional (orderbookVersion?: OrderbookVersions) or mapping non-V6 versions to a concrete enum value. This same pattern repeats on lines 54 and 66.
Option A: Make the parameter optional in updateVault
In src/order/index.ts:
updateVault(
orderbook: string,
owner: string,
token: TokenDetails,
vaultId: bigint,
balance: string | bigint,
- orderbookVersion: OrderbookVersions,
+ orderbookVersion?: OrderbookVersions,
) {Then in src/order/sync.ts:
- version === SubgraphVersions.V6 ? OrderbookVersions.V6 : ("" as any),
+ version === SubgraphVersions.V6 ? OrderbookVersions.V6 : undefined,🤖 Prompt for AI Agents
In `@src/order/sync.ts` at line 38, The code passes an invalid "" cast as any into
updateVault's orderbookVersion parameter (seen where it computes version ===
SubgraphVersions.V6 ? OrderbookVersions.V6 : ("" as any)), which bypasses type
safety; update updateVault(orderbookVersion: OrderbookVersions) usage by either
making orderbookVersion optional in updateVault's declaration
(orderbookVersion?: OrderbookVersions) and handle undefined inside updateVault,
or replace the fallback with a concrete enum member (e.g., OrderbookVersions.V1
or a clearly named OrderbookVersions.Unknown) and handle that case; apply the
same fix to the other occurrences (the branches at the other two calls in
sync.ts).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests