Skip to content

Comments

Update memclob on ClobPair Update.#3306

Merged
Kefancao merged 13 commits intomainfrom
kefan/update-memclob
Jan 14, 2026
Merged

Update memclob on ClobPair Update.#3306
Kefancao merged 13 commits intomainfrom
kefan/update-memclob

Conversation

@Kefancao
Copy link
Contributor

@Kefancao Kefancao commented Jan 12, 2026

Changelist

Currently the ClobPair update doesn't update the memclob, this PR will help do sync the memclob with the ClobPair.

Author/Reviewer Checklist

  • If this PR has changes that result in a different app state given the same prior state and transaction list, manually add the state-breaking label.
  • If the PR has breaking postgres changes to the indexer add the indexer-postgres-breaking label.
  • If this PR isn't state-breaking but has changes that modify behavior in PrepareProposal or ProcessProposal, manually add the label proposal-breaking.
  • If this PR is one of many that implement a specific feature, manually label them all feature:[feature-name].
  • If you wish to for mergify-bot to automatically create a PR to backport your change to a release branch, manually add the label backport/[branch-name].
  • Manually add any of the following labels: refactor, chore, bug.

Summary by CodeRabbit

  • New Features

    • Automatic synchronization of in-memory orderbook state when key ClobPair parameters are updated, preserving valid divisor relationships and preventing inconsistent state.
  • Tests

    • Added comprehensive tests covering validation rules, failure cases, and successful synchronization scenarios to ensure safe state updates.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 12, 2026

📝 Walkthrough

Walkthrough

Adds MemClob.SyncOrderbookState and caller integration: keeper conditionally invokes it when StepBaseQuantums or SubticksPerTick decrease to valid divisors; MemClob validates divisors/non‑increasing constraints and updates the in‑memory orderbook or panics on violations.

Changes

Cohort / File(s) Summary
Interface
protocol/x/clob/types/memclob.go
Add SyncOrderbookState(clobPair ClobPair) to MemClob interface.
MemClob implementation
protocol/x/clob/memclob/memclob.go, protocol/x/clob/memclob/...
Add SyncOrderbookState on MemClobPriceTimePriority: validate orderbook exists, ensure SubticksPerTick and MinOrderBaseQuantums >0, enforce they do not increase and remain divisors of previous values, update in‑memory orderbook; panic on violations.
Keeper integration
protocol/x/clob/keeper/clob_pair.go
Introduce shouldSyncMemClobState flag in UpdateClobPair; call MemClob.SyncOrderbookState(clobPair) when divisors decrease to valid values.
Tests — memclob
protocol/x/clob/memclob/memclob_sync_orderbook_state_test.go
New tests covering panic cases (missing orderbook, zero/increase/invalid divisors) and successful update that mutates orderbook fields.
Tests — keeper
protocol/x/clob/keeper/clob_pair_test.go, protocol/x/clob/keeper/msg_server_update_clob_pair_test.go
Add integration test validating end‑to‑end sync and order placement; test setups now create in‑memory orderbook before updates.
Mocks
protocol/mocks/MemClob.go
Add mock method SyncOrderbookState(clobPair) delegating to _m.Called(clobPair).
Build tags (test)
protocol/app/upgrades/v9.6/upgrade_container_test.go
Add build constraint `//go:build all

Sequence Diagram

sequenceDiagram
    participant Keeper
    participant State as ClobPair State
    participant MemClob

    Keeper->>State: UpdateClobPair (StepBaseQuantums/SubticksPerTick change)
    State-->>Keeper: Updated ClobPair
    alt change qualifies (decrease to valid divisor)
        Keeper->>MemClob: SyncOrderbookState(clobPair)
        MemClob->>MemClob: Ensure orderbook exists
        MemClob->>MemClob: Validate >0 and divisor/non-increase constraints
        MemClob->>MemClob: Update in-memory orderbook params
        MemClob-->>Keeper: return (or panic on violation)
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • jayy04
  • roy-dydx
  • teddyding

Poem

🐰 I hopped through code with ears held high,
Divisors checked beneath the sky.
MemClob syncs, orderbooks align,
Small numbers shrink, the state feels fine.
🍃 A tiny hop — a cleaner line.

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 15.38% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The pull request title clearly and concisely describes the main objective: updating memclob when a ClobPair is updated, which aligns with the primary changes across all modified files.
Description check ✅ Passed The description includes a changelist explaining the purpose but lacks a detailed test plan explaining how the changes were tested beyond the high-level intent statement.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings


📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c9d8fdd and deb33df.

📒 Files selected for processing (1)
  • protocol/app/upgrades/v9.6/upgrade_container_test.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: UnbornAztecKing
Repo: dydxprotocol/v4-chain PR: 2704
File: indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.ts:281-287
Timestamp: 2025-01-27T14:49:32.366Z
Learning: The FilterOrdersBySubaccountId flag in StreamOrderbookUpdatesRequest is handled by passing req.GetFilterOrdersBySubaccountId() to the Subscribe method in protocol/x/clob/keeper/grpc_stream_orderbook.go.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
  • GitHub Check: build
  • GitHub Check: test-race
  • GitHub Check: liveness-test
  • GitHub Check: test-coverage-upload
  • GitHub Check: unit-end-to-end-and-integration
  • GitHub Check: container-tests
  • GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
  • GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
  • GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
  • GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
  • GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
  • GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
  • GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
  • GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
  • GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
  • GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
  • GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
  • GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
  • GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
  • GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
  • GitHub Check: build-and-push-mainnet
  • GitHub Check: benchmark
  • GitHub Check: check-sample-pregenesis-up-to-date
  • GitHub Check: golangci-lint
  • GitHub Check: build-and-push-testnet
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Analyze (go)
  • GitHub Check: Summary
🔇 Additional comments (2)
protocol/app/upgrades/v9.6/upgrade_container_test.go (2)

1-2: LGTM on build constraint.

The //go:build all || container_test constraint correctly gates this container-based test, preventing it from running during regular go test invocations without explicit build tags. This is appropriate for tests that depend on Docker.


32-36: Empty pre/post upgrade functions provide no validation.

These helper functions are empty, meaning the upgrade test only verifies that the upgrade completes without error but doesn't validate any specific state changes. If v9.6 introduces the memclob synchronization behavior mentioned in the PR objectives, consider adding checks to verify the expected state after upgrade.

If no state validation is needed for this upgrade, this is fine as-is.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 Fix all issues with AI agents
In @protocol/x/clob/memclob/memclob.go:
- Around line 533-536: The log call m.clobKeeper.Logger(ctx).Error("PlaceOrder:
validateNewOrder", "order", order.GetOrderTextString()) is a debug artifact and
should not be logged at Error level; either remove the statement entirely or
change it to a debug-level call (e.g., use m.clobKeeper.Logger(ctx).Debug(...)
with the same message and fields) so non-error validation traces do not pollute
production error logs.
- Around line 170-208: The comments and panic messages in SyncOrderbookState are
inconsistent with the function and variable names; update the top comment to
reference SyncOrderbookState (not SyncSubticksPerTick), change the panic that
currently mentions "SyncSubticksPerTick" to "SyncOrderbookState", and correct
the panic text that refers to "stepBaseQuantums" to mention
"minOrderBaseQuantums" (and keep the
ClobPair/SubticksPerTick/MinOrderBaseQuantums wording aligned with variables
like SubticksPerTick and MinOrderBaseQuantums and clobPairId/orderbook to avoid
confusion).
- Around line 1489-1499: The two debug log calls in validateNewOrder are using
Error level and one omits the stepBaseQuantums value; replace or remove them:
either drop the logs or change m.clobKeeper.Logger(ctx).Error(...) to
m.clobKeeper.Logger(ctx).Debug(...) and in the second call include the missing
value by adding "stepBaseQuantums", stepBaseQuantums (or the appropriate
variable/property that holds the step base quantum for the clob pair) alongside
"clobPairId", order.GetClobPairId(); ensure the logged variable name matches the
actual symbol used in the surrounding code.
🧹 Nitpick comments (1)
protocol/x/clob/types/memclob.go (1)

153-155: Consider adding a documentation comment for the new interface method.

Other methods in this interface have context from their implementations, but a brief doc comment here would help clarify the expected behavior, preconditions (e.g., orderbook must exist), and panic conditions for implementers.

📝 Suggested documentation
+	// SyncOrderbookState synchronizes the in-memory orderbook state with the ClobPair.
+	// This updates SubticksPerTick and MinOrderBaseQuantums when they change.
+	// Panics if the orderbook does not exist or if values are invalid.
 	SyncOrderbookState(
 		clobPair ClobPair,
 	)
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f4db85a and d5023ad.

📒 Files selected for processing (6)
  • protocol/mocks/MemClob.go
  • protocol/x/clob/keeper/clob_pair.go
  • protocol/x/clob/keeper/clob_pair_test.go
  • protocol/x/clob/memclob/memclob.go
  • protocol/x/clob/memclob/memclob_sync_orderbook_state_test.go
  • protocol/x/clob/types/memclob.go
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-10-06T20:00:48.549Z
Learnt from: anmolagrawal345
Repo: dydxprotocol/v4-chain PR: 3079
File: protocol/x/clob/types/leverage.go:10-56
Timestamp: 2025-10-06T20:00:48.549Z
Learning: In the leverage update flow for protocol/x/clob, the `ValidateUpdateLeverageMsg` function performs basic validation (non-nil checks, non-zero leverage, unique IDs, valid clob pair existence), while maximum leverage validation against `GetMaxLeverageForPerpetual` is intentionally deferred to the `UpdateLeverage` keeper method.

Applied to files:

  • protocol/x/clob/keeper/clob_pair.go
  • protocol/x/clob/keeper/clob_pair_test.go
  • protocol/x/clob/memclob/memclob.go
📚 Learning: 2024-11-15T16:17:29.092Z
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2551
File: protocol/x/clob/types/expected_keepers.go:86-90
Timestamp: 2024-11-15T16:17:29.092Z
Learning: The function `GetCrossInsuranceFundBalance` in `protocol/x/clob/types/expected_keepers.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.

Applied to files:

  • protocol/x/clob/keeper/clob_pair.go
  • protocol/mocks/MemClob.go
  • protocol/x/clob/keeper/clob_pair_test.go
  • protocol/x/clob/memclob/memclob.go
  • protocol/x/clob/memclob/memclob_sync_orderbook_state_test.go
📚 Learning: 2024-06-10T19:25:08.720Z
Learnt from: jonfung-dydx
Repo: dydxprotocol/v4-chain PR: 1009
File: protocol/x/clob/memclob/memclob_place_order_test.go:4108-4108
Timestamp: 2024-06-10T19:25:08.720Z
Learning: The `assertPlaceOrderOffchainMessages` function in `memclob_test_util.go` indeed requires `ctx` as a parameter for its operations, contrary to the initial comment made.

Applied to files:

  • protocol/mocks/MemClob.go
  • protocol/x/clob/types/memclob.go
  • protocol/x/clob/keeper/clob_pair_test.go
  • protocol/x/clob/memclob/memclob.go
  • protocol/x/clob/memclob/memclob_sync_orderbook_state_test.go
📚 Learning: 2025-01-27T14:49:32.366Z
Learnt from: UnbornAztecKing
Repo: dydxprotocol/v4-chain PR: 2704
File: indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.ts:281-287
Timestamp: 2025-01-27T14:49:32.366Z
Learning: The FilterOrdersBySubaccountId flag in StreamOrderbookUpdatesRequest is handled by passing req.GetFilterOrdersBySubaccountId() to the Subscribe method in protocol/x/clob/keeper/grpc_stream_orderbook.go.

Applied to files:

  • protocol/x/clob/types/memclob.go
📚 Learning: 2024-11-15T16:00:11.304Z
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2551
File: protocol/x/subaccounts/keeper/subaccount.go:852-865
Timestamp: 2024-11-15T16:00:11.304Z
Learning: The function `GetCrossInsuranceFundBalance` in `protocol/x/subaccounts/keeper/subaccount.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.

Applied to files:

  • protocol/x/clob/keeper/clob_pair_test.go
📚 Learning: 2025-04-15T16:58:46.335Z
Learnt from: anmolagrawal345
Repo: dydxprotocol/v4-chain PR: 2780
File: protocol/x/clob/keeper/twap_order_state_test.go:191-209
Timestamp: 2025-04-15T16:58:46.335Z
Learning: For TWAP orders, message validation in message_place_order.go handles edge cases with comprehensive checks for parameters like durations and intervals, making it unnecessary to test invalid parameters in twap_order_state_test.go which only receives pre-validated inputs.

Applied to files:

  • protocol/x/clob/keeper/clob_pair_test.go
  • protocol/x/clob/memclob/memclob.go
  • protocol/x/clob/memclob/memclob_sync_orderbook_state_test.go
📚 Learning: 2025-04-15T16:58:46.335Z
Learnt from: anmolagrawal345
Repo: dydxprotocol/v4-chain PR: 2780
File: protocol/x/clob/keeper/twap_order_state_test.go:191-209
Timestamp: 2025-04-15T16:58:46.335Z
Learning: Invalid TWAP order parameters (like durations and intervals) are already handled by validations in message_place_order.go, so twap_order_state.go can assume all inputs have already been validated.

Applied to files:

  • protocol/x/clob/memclob/memclob.go
📚 Learning: 2025-04-10T19:30:51.917Z
Learnt from: anmolagrawal345
Repo: dydxprotocol/v4-chain PR: 2780
File: indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/order.ts:718-763
Timestamp: 2025-04-10T19:30:51.917Z
Learning: The validation for TWAP parameters is handled in the ValidateBasic method of MsgPlaceOrder in protocol/x/clob/types/message_place_order.go, which ensures that the interval divides the duration and enforces other constraints like min/max values.

Applied to files:

  • protocol/x/clob/memclob/memclob.go
🧬 Code graph analysis (3)
protocol/x/clob/keeper/clob_pair.go (1)
protocol/x/clob/types/memclob.go (1)
  • MemClob (13-156)
protocol/x/clob/keeper/clob_pair_test.go (5)
protocol/x/clob/memclob/memclob.go (1)
  • NewMemClobPriceTimePriority (57-66)
protocol/testutil/keeper/clob.go (1)
  • NewClobKeepersTestContext (61-73)
protocol/testutil/constants/clob_pair.go (1)
  • ClobPair_Btc (21-32)
protocol/x/clob/types/constants.go (1)
  • ShortBlockWindow (9-9)
protocol/x/clob/types/order.pb.go (7)
  • Order (637-684)
  • Order (688-688)
  • Order (689-691)
  • Order (841-846)
  • OrderId (146-172)
  • OrderId (176-176)
  • OrderId (177-179)
protocol/x/clob/memclob/memclob_sync_orderbook_state_test.go (3)
protocol/x/clob/types/orderbook.go (1)
  • SubticksPerTick (27-27)
protocol/x/clob/memclob/memclob.go (1)
  • NewMemClobPriceTimePriority (57-66)
protocol/x/clob/keeper/clob_pair_test.go (1)
  • TestSyncOrderbookState_SucceedsAndUpdatesValues (1210-1255)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
  • GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
  • GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
  • GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
  • GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
  • GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
  • GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
  • GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
  • GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
  • GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
  • GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
  • GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
  • GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
  • GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
  • GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
  • GitHub Check: check-sample-pregenesis-up-to-date
  • GitHub Check: golangci-lint
  • GitHub Check: test-race
  • GitHub Check: liveness-test
  • GitHub Check: build
  • GitHub Check: unit-end-to-end-and-integration
  • GitHub Check: test-coverage-upload
  • GitHub Check: build-and-push-mainnet
  • GitHub Check: container-tests
  • GitHub Check: benchmark
  • GitHub Check: build-and-push-testnet
  • GitHub Check: Analyze (go)
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Summary
🔇 Additional comments (4)
protocol/mocks/MemClob.go (1)

611-615: LGTM!

The auto-generated mock method correctly implements the interface signature and follows the standard mockery pattern used throughout the file.

protocol/x/clob/keeper/clob_pair.go (1)

653-656: LGTM! The sync is correctly placed after the state update.

The memclob synchronization is triggered only after the ClobPair is successfully persisted to state, ensuring consistency between state and memory. The syncMemClobState flag correctly tracks when either StepBaseQuantums or SubticksPerTick changes to a valid divisor value.

Note that the validation in UpdateClobPair (Lines 599-627) is duplicated in memclob.SyncOrderbookState. While this provides defense-in-depth, the memclob validation will panic rather than return an error. Since the keeper validates first, the memclob should never panic in normal operation.

protocol/x/clob/memclob/memclob_sync_orderbook_state_test.go (1)

1-116: LGTM! Comprehensive test coverage for SyncOrderbookState.

The test file provides excellent coverage of all validation paths:

  • Missing orderbook → panic
  • Zero values → panic
  • Increased values → panic
  • Non-divisor values → panic
  • Valid updates → success with correct state changes

The helper function newPerpClobPair is clean and reduces test boilerplate.

protocol/x/clob/keeper/clob_pair_test.go (1)

1210-1255: LGTM! Good end-to-end integration test.

The test correctly validates the synchronization flow:

  1. Creates a ClobPair with default values (5/5)
  2. Updates to valid smaller divisors (1/1)
  3. Verifies memclob sync by placing an order that would only succeed with the new parameters

Comment on lines 170 to 208
// SyncSubticksPerTick syncs the subticks per tick and step base quantums for the memclob with the ClobPair state.
// This is used to ensure the existing memclob has the up to date params after modifying the ClobPair
// subticks per tick in state.
func (m *MemClobPriceTimePriority) SyncOrderbookState(
clobPair types.ClobPair,
) {
clobPairId := clobPair.GetClobPairId()
orderbook, exists := m.orderbooks[clobPairId]
if !exists {
panic(fmt.Sprintf("SyncSubticksPerTick: Orderbook for ClobPair ID %d does not exist", clobPairId))
}

subticksPerTick := clobPair.GetClobPairSubticksPerTick()
if subticksPerTick == 0 {
panic("subticksPerTick must be greater than zero")
}
// if subticksPerTick is increased or not a divisor of the previous subticksPerTick, then we're screwed
// since this is registered in state.
if subticksPerTick > orderbook.SubticksPerTick ||
orderbook.SubticksPerTick%subticksPerTick != 0 {
panic(fmt.Sprintf("clob %d SubticksPerTick increased and/or not a divisor of the previous",
clobPairId))
}

minOrderBaseQuantums := clobPair.GetClobPairMinOrderBaseQuantums()
if minOrderBaseQuantums == 0 {
panic("minOrderBaseQuantums must be greater than zero")
}
// if minOrderBaseQuantums is increased or not a divisor of the previous minOrderBaseQuantums, then we're screwed
// since this is registered in state.
if minOrderBaseQuantums > orderbook.MinOrderBaseQuantums ||
orderbook.MinOrderBaseQuantums%minOrderBaseQuantums != 0 {
panic(fmt.Sprintf("clob %d stepBaseQuantums increased and/or not a divisor of the previous",
clobPairId))
}

orderbook.SubticksPerTick = subticksPerTick
orderbook.MinOrderBaseQuantums = minOrderBaseQuantums
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Fix naming inconsistencies in comments and panic messages.

  1. Line 170: The comment says "SyncSubticksPerTick" but the method is named SyncOrderbookState
  2. Line 179: Panic message references "SyncSubticksPerTick" instead of "SyncOrderbookState"
  3. Line 202: Panic message says "stepBaseQuantums" but the variable is minOrderBaseQuantums
🔧 Proposed fixes
-// SyncSubticksPerTick syncs the subticks per tick and step base quantums for the memclob with the ClobPair state.
-// This is used to ensure the existing memclob has the up to date params after modifying the ClobPair
-// subticks per tick in state.
+// SyncOrderbookState syncs the subticks per tick and min order base quantums for the memclob with the ClobPair state.
+// This is used to ensure the existing memclob has the up to date params after modifying the ClobPair in state.
 func (m *MemClobPriceTimePriority) SyncOrderbookState(
 	clobPair types.ClobPair,
 ) {
 	clobPairId := clobPair.GetClobPairId()
 	orderbook, exists := m.orderbooks[clobPairId]
 	if !exists {
-		panic(fmt.Sprintf("SyncSubticksPerTick: Orderbook for ClobPair ID %d does not exist", clobPairId))
+		panic(fmt.Sprintf("SyncOrderbookState: Orderbook for ClobPair ID %d does not exist", clobPairId))
 	}
 	// if minOrderBaseQuantums is increased or not a divisor of the previous minOrderBaseQuantums, then we're screwed
 	// since this is registered in state.
 	if minOrderBaseQuantums > orderbook.MinOrderBaseQuantums ||
 		orderbook.MinOrderBaseQuantums%minOrderBaseQuantums != 0 {
-		panic(fmt.Sprintf("clob %d stepBaseQuantums increased and/or not a divisor of the previous",
+		panic(fmt.Sprintf("clob %d minOrderBaseQuantums increased and/or not a divisor of the previous",
 			clobPairId))
 	}
🤖 Prompt for AI Agents
In @protocol/x/clob/memclob/memclob.go around lines 170 - 208, The comments and
panic messages in SyncOrderbookState are inconsistent with the function and
variable names; update the top comment to reference SyncOrderbookState (not
SyncSubticksPerTick), change the panic that currently mentions
"SyncSubticksPerTick" to "SyncOrderbookState", and correct the panic text that
refers to "stepBaseQuantums" to mention "minOrderBaseQuantums" (and keep the
ClobPair/SubticksPerTick/MinOrderBaseQuantums wording aligned with variables
like SubticksPerTick and MinOrderBaseQuantums and clobPairId/orderbook to avoid
confusion).

Comment on lines 533 to 536
m.clobKeeper.Logger(ctx).Error(
"PlaceOrder: validateNewOrder",
"order", order.GetOrderTextString(),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Remove or downgrade debug logging from Error level.

These log statements appear to be debugging artifacts. Using Error level for non-error conditions will pollute production logs and make it harder to identify actual errors.

🔧 Recommended fix

Either remove these logs entirely or change to Debug level:

-	m.clobKeeper.Logger(ctx).Error(
-		"PlaceOrder: validateNewOrder",
-		"order", order.GetOrderTextString(),
-	)
+	m.clobKeeper.Logger(ctx).Debug(
+		"PlaceOrder: validateNewOrder",
+		"order", order.GetOrderTextString(),
+	)

Or simply remove if this was added for temporary debugging:

-	m.clobKeeper.Logger(ctx).Error(
-		"PlaceOrder: validateNewOrder",
-		"order", order.GetOrderTextString(),
-	)
📝 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
m.clobKeeper.Logger(ctx).Error(
"PlaceOrder: validateNewOrder",
"order", order.GetOrderTextString(),
)
m.clobKeeper.Logger(ctx).Debug(
"PlaceOrder: validateNewOrder",
"order", order.GetOrderTextString(),
)
🤖 Prompt for AI Agents
In @protocol/x/clob/memclob/memclob.go around lines 533 - 536, The log call
m.clobKeeper.Logger(ctx).Error("PlaceOrder: validateNewOrder", "order",
order.GetOrderTextString()) is a debug artifact and should not be logged at
Error level; either remove the statement entirely or change it to a debug-level
call (e.g., use m.clobKeeper.Logger(ctx).Debug(...) with the same message and
fields) so non-error validation traces do not pollute production error logs.

Comment on lines 1489 to 1499
m.clobKeeper.Logger(ctx).Error(
"validateNewOrder: checked order remaining amount",
"remainingAmount", remainingAmount,
"hasRemainingAmount", hasRemainingAmount,
"orderbook.MinOrderBaseQuantums", orderbook.MinOrderBaseQuantums,
"orderId", orderId.String(),
)
m.clobKeeper.Logger(ctx).Error(
"validateNewOrder: ClobPair stepBaseQuantums",
"clobPairId", order.GetClobPairId(),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Remove or downgrade debug logging and fix incomplete log statement.

  1. These log statements use Error level for debug information, which is inappropriate for production
  2. Line 1496-1499 is missing the actual stepBaseQuantums value - it only logs clobPairId
🔧 Recommended fix

Remove the debug logging or downgrade to appropriate level:

-	m.clobKeeper.Logger(ctx).Error(
-		"validateNewOrder: checked order remaining amount",
-		"remainingAmount", remainingAmount,
-		"hasRemainingAmount", hasRemainingAmount,
-		"orderbook.MinOrderBaseQuantums", orderbook.MinOrderBaseQuantums,
-		"orderId", orderId.String(),
-	)
-	m.clobKeeper.Logger(ctx).Error(
-		"validateNewOrder: ClobPair stepBaseQuantums",
-		"clobPairId", order.GetClobPairId(),
-	)

If logging is needed for debugging, use Debug level and include the missing value:

+	m.clobKeeper.Logger(ctx).Debug(
+		"validateNewOrder: checked order remaining amount",
+		"remainingAmount", remainingAmount,
+		"hasRemainingAmount", hasRemainingAmount,
+		"orderbook.MinOrderBaseQuantums", orderbook.MinOrderBaseQuantums,
+		"orderId", orderId.String(),
+	)
📝 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
m.clobKeeper.Logger(ctx).Error(
"validateNewOrder: checked order remaining amount",
"remainingAmount", remainingAmount,
"hasRemainingAmount", hasRemainingAmount,
"orderbook.MinOrderBaseQuantums", orderbook.MinOrderBaseQuantums,
"orderId", orderId.String(),
)
m.clobKeeper.Logger(ctx).Error(
"validateNewOrder: ClobPair stepBaseQuantums",
"clobPairId", order.GetClobPairId(),
)
// Debug logging removed - these were inappropriate Error-level statements
🤖 Prompt for AI Agents
In @protocol/x/clob/memclob/memclob.go around lines 1489 - 1499, The two debug
log calls in validateNewOrder are using Error level and one omits the
stepBaseQuantums value; replace or remove them: either drop the logs or change
m.clobKeeper.Logger(ctx).Error(...) to m.clobKeeper.Logger(ctx).Debug(...) and
in the second call include the missing value by adding "stepBaseQuantums",
stepBaseQuantums (or the appropriate variable/property that holds the step base
quantum for the clob pair) alongside "clobPairId", order.GetClobPairId(); ensure
the logged variable name matches the actual symbol used in the surrounding code.

@Kefancao Kefancao merged commit 1d6e6ba into main Jan 14, 2026
42 of 46 checks passed
@Kefancao Kefancao deleted the kefan/update-memclob branch January 14, 2026 20:08
@Kefancao
Copy link
Contributor Author

@mergify backport release/protocol/v9.x

@mergify
Copy link
Contributor

mergify bot commented Jan 14, 2026

backport release/protocol/v9.x

✅ Backports have been created

Details

mergify bot pushed a commit that referenced this pull request Jan 14, 2026
Kefancao added a commit that referenced this pull request Jan 14, 2026
Co-authored-by: Kefan Cao <76069770+Kefancao@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

2 participants