Move memstore hydration initialized flag to memstore #2681
Conversation
WalkthroughThe changes modify the CLOB (Central Limit Order Book) module's keeper implementation, focusing on how the memory store initialization state is managed. Instead of using multiple struct fields to track initialization, the implementation now uses a single atomic boolean field, Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
🔇 Additional comments (8)
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
jayy04
force-pushed
the
jy/memstore-hydration-fix
branch
from
863751f to
1c35383
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
protocol/x/clob/keeper/keeper.go (1)
312-323: Consider using a constant for the initialization flag value.While the implementation is correct, consider defining a constant for the byte value
1used inSetMemstoreInitialized. This would make the code more maintainable and self-documenting.// In types/keys.go +const ( + MemstoreInitializedValue byte = 1 +) // In keeper.go func (k Keeper) SetMemstoreInitialized(ctx sdk.Context) { store := ctx.KVStore(k.memKey) store.Set( []byte(types.KeyMemstoreInitialized), - []byte{1}, + []byte{types.MemstoreInitializedValue}, ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
protocol/x/clob/keeper/keeper.go(4 hunks)protocol/x/clob/types/keys.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test-sim-multi-seed-short
- GitHub Check: test-race
- GitHub Check: Summary
🔇 Additional comments (4)
protocol/x/clob/types/keys.go (1)
82-84: LGTM! Well-placed constant definition.The new constant follows the existing naming conventions and is appropriately placed in the Memstore section.
protocol/x/clob/keeper/keeper.go (3)
56-56: LGTM! Correct retention of the initialized field.The
initializedfield is correctly retained as it serves a different purpose than the memstore initialization flag.
187-191: LGTM! Clear initialization flow.The initialization flow is maintained while properly utilizing the new memstore-based flag system.
256-263: LGTM! Proper initialization guard and flag setting.The initialization check and flag setting ensure atomic initialization and prevent double initialization of the memstore.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
protocol/x/clob/keeper/keeper.go (1)
312-324: Consider adding error handling for store operations.While the implementation is clean and focused, it might be worth adding error handling for store operations to handle potential edge cases.
Consider this implementation:
func (k Keeper) SetMemstoreInitialized(ctx sdk.Context) { store := ctx.KVStore(k.memKey) + if store == nil { + panic("memstore is nil") + } store.Set( []byte(types.KeyMemstoreInitialized), []byte{1}, ) } func (k Keeper) GetMemstoreInitialized(ctx sdk.Context) bool { store := ctx.KVStore(k.memKey) + if store == nil { + panic("memstore is nil") + } return store.Has([]byte(types.KeyMemstoreInitialized)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
protocol/x/clob/keeper/keeper.go(4 hunks)protocol/x/clob/types/keys.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test-race
- GitHub Check: Summary
🔇 Additional comments (4)
protocol/x/clob/types/keys.go (1)
82-84: LGTM!The new constant
KeyMemstoreInitializedis well-placed in the "Memstore" section and follows the established naming conventions. The purpose is clearly documented in the comment.protocol/x/clob/keeper/keeper.go (3)
56-56: LGTM!The
initializedfield usingatomic.Boolensures thread-safe access to the initialization state.
187-189: LGTM!The initialization flow is well-structured with clear separation of concerns.
256-263: LGTM!The initialization check and flag setting in the memstore follow a clear and consistent pattern.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
protocol/x/clob/keeper/keeper_test.go (1)
Line range hint
26-34: Consider adding edge case tests for memstore initialization.While the basic initialization and stateful order count cases are covered, consider adding tests for:
- Initialization with empty state
- Initialization after chain halt/resume
- Concurrent initialization attempts
Would you like me to provide example test implementations for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
protocol/x/clob/keeper/keeper_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: test-sim-multi-seed-short
- GitHub Check: test-sim-import-export
- GitHub Check: test-sim-after-import
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: test-race
- GitHub Check: test-coverage-upload
- GitHub Check: liveness-test
- GitHub Check: container-tests
- GitHub Check: Summary
| require.Panics(t, func() { | ||
| ks.ClobKeeper.InitMemStore(ks.Ctx) | ||
| }) | ||
| require.True(t, ks.ClobKeeper.GetMemstoreInitialized(ks.Ctx)) |
There was a problem hiding this comment.
Test doesn't fully validate "OnlyAllowedOnce" behavior.
The test name suggests it should verify that initialization is only allowed once, but it only checks if the first initialization succeeds. It should also verify that subsequent initialization attempts are properly handled.
Consider updating the test to verify both cases:
func TestInitMemStore_OnlyAllowedOnce(t *testing.T) {
memClob := memclob.NewMemClobPriceTimePriority(false)
ks := keepertest.NewClobKeepersTestContextWithUninitializedMemStore(
t,
memClob,
&mocks.BankKeeper{},
&mocks.IndexerEventManager{})
// First initialization should succeed
ks.ClobKeeper.InitMemStore(ks.Ctx)
require.True(t, ks.ClobKeeper.GetMemstoreInitialized(ks.Ctx))
+ // Second initialization should not be allowed
+ secondCtx := ks.Ctx.WithBlockHeight(ks.Ctx.BlockHeight() + 1)
+ ks.ClobKeeper.InitMemStore(secondCtx)
+
+ // Verify the initialization flag wasn't affected by second attempt
+ require.True(t, ks.ClobKeeper.GetMemstoreInitialized(secondCtx))
}Committable suggestion skipped: line range outside the PR's diff.
protocol/x/clob/keeper/keeper.go
Outdated
| @@ -54,8 +53,7 @@ type ( | |||
| streamingManager streamingtypes.FullNodeStreamingManager | |||
| finalizeBlockEventStager finalizeblock.EventStager[*types.ClobStagedFinalizeBlockEvent] | |||
|
|
|||
| initialized *atomic.Bool | |||
| memStoreInitialized *atomic.Bool | |||
| initialized *atomic.Bool | |||
There was a problem hiding this comment.
nit: rename to inMemStructsInitialized or sth similar to disambiguate with memStoreInitialized
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
Refactor
New Features
Tests
These changes improve the management and testing of memory store initialization, enhancing overall robustness and clarity.