Skip to content

fix(#2): [$40 BOUNTY] [Go] Add order book cancellation depth tests #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Notime02 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(#2): [$40 BOUNTY] [Go] Add order book cancellation depth tests #11

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

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions market/orderbook/orderbook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
// orderbook_test.go

package orderbook

import (
"testing"
)

func TestCancelBid(t *testing.T) {
ob := NewOrderBook()
orderID := ob.AddBid(100, 1)

@coderabbitai coderabbitai Bot Jun 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

AddBid is asserted with incompatible return types (compile blocker).

Line 11 treats AddBid as returning an order ID, but Line 41 treats the same call as returning an error. Both cannot be true for one method signature, so this test file won’t compile as written.

Also applies to: 41-44

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@market/orderbook/orderbook_test.go` at line 11, The AddBid method is being
used inconsistently throughout the test file. On line 11, orderID is assigned
directly from AddBid as if it returns only an order ID, but on lines 41-44 it
appears to be treated as returning an error. Check the actual return signature
of the AddBid method in the orderbook implementation and then adjust all usages
in the test file to match the correct signature consistently. If AddBid returns
both an order ID and an error, update line 11 to handle both return values. If
it returns only an order ID or only an error, fix lines 41-44 accordingly to
match that signature.

ob.CancelOrder(orderID)

@coderabbitai coderabbitai Bot Jun 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Assert CancelOrder success before checking state.

Both tests ignore the returned error from CancelOrder; a failed cancel can produce misleading state assertions. Check err first and fail fast.

Also applies to: 22-22

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@market/orderbook/orderbook_test.go` at line 12, The CancelOrder method calls
at both locations are not checking the returned error before proceeding with
state assertions. Capture the error returned from each CancelOrder call, assert
that it is nil using a test assertion (fail the test immediately if an error
occurred), and only then proceed with the subsequent state validation. This
ensures that any cancellation failures are caught before misleading assertions
on the orderbook state.


if _, exists := ob.bids[orderID]; exists {
t.Fatalf("expected bid to be removed, but it still exists")
Comment on lines +14 to +15

@coderabbitai coderabbitai Bot Jun 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

ob.bids/ob.asks are accessed with incompatible key/index assumptions.

The tests index ob.bids by orderID (Lines 14/24) and by integer index (Line 55). A Go map/slice cannot support both in one type. Also, cancellation in the provided implementation removes order IDs from ob.orders and price levels from bid/ask levels, so these assertions are validating the wrong internal contract.

Also applies to: 24-25, 55-56

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@market/orderbook/orderbook_test.go` around lines 14 - 15, The tests are
accessing ob.bids and ob.asks inconsistently: using orderID as a key in some
assertions (lines 14, 24) and integer indices in others (line 55), which is
incompatible since Go maps and slices cannot use the same access pattern.
Additionally, the test assertions are checking the wrong contract - instead of
verifying that an order exists or doesn't exist directly in ob.bids, verify the
actual implementation behavior by checking that the order is removed from
ob.orders and that price levels in the bid/ask data structures are updated
accordingly. Update all assertions to consistently validate the correct internal
state changes that result from order cancellation.

}
}

func TestCancelAsk(t *testing.T) {
ob := NewOrderBook()
orderID := ob.AddAsk(100, 1)
ob.CancelOrder(orderID)

if _, exists := ob.asks[orderID]; exists {
t.Fatalf("expected ask to be removed, but it still exists")
}
}

func TestCancelUnknownOrder(t *testing.T) {
ob := NewOrderBook()
err := ob.CancelOrder("unknownID")

if err != ErrOrderNotFound {
t.Fatalf("expected ErrOrderNotFound, got %v", err)
}
}

func TestClosedBookRejectsOperations(t *testing.T) {
ob := NewOrderBook()
ob.Close()
err := ob.AddBid(100, 1)

if err != ErrBookClosed {
t.Fatalf("expected ErrBookClosed, got %v", err)
}
}

func TestSnapshotImmutability(t *testing.T) {
ob := NewOrderBook()
ob.AddBid(100, 1)
snapshot := ob.Snapshot()

snapshot.Bids[0].Price = 200 // Attempt to mutate snapshot

if ob.bids[1].Price == 200 {
t.Fatalf("expected internal bids to be immutable")
}
}