migration routers#3336
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3336 +/- ##
==========================================
- Coverage 59.25% 59.07% -0.18%
==========================================
Files 2110 2106 -4
Lines 174181 173350 -831
==========================================
- Hits 103210 102409 -801
+ Misses 62044 61990 -54
- Partials 8927 8951 +24
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 009bf03. Configure here.
| if err != nil { | ||
| return nil, fmt.Errorf("buildMemiavlOnlyRouter: %w", err) | ||
| } | ||
| return router, nil |
There was a problem hiding this comment.
Missing thread-safe wrapping for two write modes
Medium Severity
The MemiavlOnly and FlatKVOnly cases in BuildRouter return their routers without wrapping in NewThreadSafeRouter, while every other case does. The README explicitly states "BuildRouter wraps every router it returns in NewThreadSafeRouter" and callers relying on this documented guarantee will have no external synchronization for these two modes, risking data races when Read and ApplyChangeSets are called concurrently.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 009bf03. Configure here.
| // Module names must be unique; NewRoute's validation rules apply. The | ||
| // returned Route may be passed to NewModuleRouter alongside other | ||
| // Routes to compose multi-database setups. | ||
| func (t *TestOnlyDualWriteRouter) BuildRoute(moduleNames ...string) (*Route, error) { // TODO LLM you need to write a unit test for this |
There was a problem hiding this comment.
Stale LLM instruction left in source code
Low Severity
The comment // TODO LLM you need to write a unit test for this on BuildRoute is an accidentally committed prompt/instruction to an LLM. The unit tests it requests already exist extensively in dual_write_router_test.go (13+ test functions covering BuildRoute), making this TODO stale and confusing.
Reviewed by Cursor Bugbot for commit 009bf03. Configure here.
| // Module names must be unique; NewRoute's validation rules apply. The | ||
| // returned Route may be passed to NewModuleRouter alongside other | ||
| // Routes to compose multi-database setups. | ||
| func (t *TestOnlyDualWriteRouter) BuildRoute(moduleNames ...string) (*Route, error) { // TODO LLM you need to write a unit test for this |
| } | ||
|
|
||
| // Sends evm/ traffic to both memIAVL and flatKV. | ||
| memiavlEvmRoute, err := routeToMemIAVL(memIAVL) |
There was a problem hiding this comment.
routeToMemIAVL(memIAVL) with no module args does not send evm traffic anywhere
| @@ -0,0 +1,1059 @@ | |||
| package migration | |||
There was a problem hiding this comment.
not blocking: 1000+ lines testing file is a little to big. consider splitting by mode (memiavl_only_test.go, migrate_evm_test.go, evm_migrated_test.go, ...).
2 participants
Describe your changes and provide context
Use the migration router primitives to construct the migration workflows we will need.
Use the following link to view the README in rendered form. Suggested read prior to reviewing code. https://github.com/sei-protocol/sei-chain/blob/cjl/migration-integration/sei-db/state_db/sc/migration/README.md
Testing performed to validate your change
unit tests
Note
Medium Risk
Adds new routing/migration composition for splitting reads/writes across
memIAVLandflatKV, including a dual-write path; mistakes here could misroute state writes or migration bookkeeping.Overview
Introduces a
BuildRouterentrypoint that constructs end-to-end routing topologies for each migration/steady-state mode (MemiavlOnly,MigrateEVM,EVMMigrated,MigrateAllButBank,AllMigratedButBank,MigrateBank,FlatKVOnly, andTestOnlyDualWrite), wiringModuleRouter+MigrationManager+ backend-specific routes.Adds a test-only
TestOnlyDualWriteRouterthat mirrors write batches to a secondary backend while serving reads/iteration/proofs from the primary, and integrates it as a newWriteMode.Adds extensive unit/integration-style tests that simulate block workloads (including restart/resume) to validate key placement, migration metadata behavior, and invariants across all modes, plus focused tests for the dual-write router’s fanout and error-wrapping behavior.
Reviewed by Cursor Bugbot for commit 009bf03. Bugbot is set up for automated code reviews on this repo. Configure here.