fix(x/market): reverse indexes for bids and leases #2040
Conversation
WalkthroughRefactors the market module to use Collection-based IndexedMaps for Orders, Bids, and Leases, adds a v1.2.0 upgrade that migrates legacy KV data into the new schema, increments market ConsensusVersion to 8, and introduces key/index types plus query and keeper rewrites to support indexed storage. Changes
Sequence DiagramsequenceDiagram
participant Chain as Cosmos Chain
participant MM as Module Manager
participant Migrator as v1.2.0 Migrator
participant LegacyKV as Legacy KV Store
participant IM as IndexedMap
Chain->>MM: trigger upgrade (v1.2.0)
MM->>Migrator: RunMigrations(ctx)
Migrator->>LegacyKV: iterate order keys
LegacyKV-->>Migrator: return order objects
Migrator->>IM: Set orders into Orders IndexedMap
IM-->>Migrator: ack
Migrator->>LegacyKV: delete old order keys
Migrator->>LegacyKV: iterate bid keys
LegacyKV-->>Migrator: return bid objects
Migrator->>IM: Set bids into Bids IndexedMap
IM-->>Migrator: ack
Migrator->>LegacyKV: delete old bid keys
Migrator->>LegacyKV: iterate lease keys
LegacyKV-->>Migrator: return lease objects
Migrator->>IM: Set leases into Leases IndexedMap
IM-->>Migrator: ack
Migrator->>LegacyKV: delete old lease keys
Migrator->>MM: return migration result
MM->>Chain: complete upgrade
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@app/types/app.go`:
- Around line 485-488: The mint module subspace is currently commented out
causing app.GetSubspace(minttypes.ModuleName) calls (used during mint/slashing
setup) to panic at startup; fix by uncommenting the mint subspace registration
line so
paramsKeeper.Subspace(minttypes.ModuleName).WithKeyTable(minttypes.ParamKeyTable())
is registered (and ensure the comment after it has a space: "// nolint:
staticcheck // SA1019"), or alternatively remove all calls to
app.GetSubspace(minttypes.ModuleName) from module initialization (mint/slashing
setup) so there is no lookup for an unregistered subspace.
In `@go.mod`:
- Line 3: The go.mod Go version was bumped to 1.25.5; verify and lock the
bytedance/sonic dependency to the reviewed safe release (e.g.,
github.com/bytedance/sonic@v1.14.2), run module maintenance (go get ...@v1.14.2
and go mod tidy / go mod vendor as appropriate) and then run a vulnerability
scan (govulncheck or similar) and go list -m all to confirm no advisories
remain; update go.mod/go.sum accordingly and include a brief commit message
referencing the Go bump and the sonic upgrade.
In `@x/market/keeper/grpc_query.go`:
- Around line 160-169: The GetAccount call inside the iterator callbacks for
assembling QueryBidResponse (when building the bids slice) and the equivalent
for asks is currently returning true on error which silently truncates results;
fix by declaring an outer-scope error variable (e.g., var acctErr error) before
the scan/iteration, inside the callback when k.ekeeper.GetAccount(ctx,
bid.ID.ToEscrowAccountID()) (or the ask equivalent) returns an error assign that
error to acctErr and return true to stop the iteration, then after the
scan/iteration check acctErr and return it (or wrap it) to the caller instead of
returning partial results; be careful not to shadow the outer err variable and
apply the same pattern to both the bids and asks assembly code paths that append
QueryBidResponse/QueryAskResponse and manage limit.
- Around line 30-48: The pagination logic currently only honors offset/limit and
silently ignores key/reverse; add an explicit guard in the Orders (and mirror in
QueryBids and QueryLeases) query handlers: check req.Pagination.Key != nil or
req.Pagination.Reverse == true and return status.Error(codes.InvalidArgument,
"key/reverse pagination not supported") (or similar message) before using
req.Pagination.Limit and iterating states, or alternatively replace the
offset-only loop with sdkquery.FilteredPaginate/util/query pagination utilities
to support key-based pagination; update the checks in the functions that
reference req.Pagination, sdkquery.DefaultLimit, and the state iteration logic
to ensure consistent behavior across Orders/Bids/Leases.
- Around line 89-93: In the Orders/Bids/Leases gRPC query handlers (functions
that build responses with variables orders, bids, leases), don't set
sdkquery.PageResponse.Total to len(...) unconditionally; instead check
req.Pagination != nil && req.Pagination.CountTotal — if true, compute the full
matching count by iterating the underlying store/prefix (or using an existing
count helper) and set PageResponse.Total = uint64(count); if false, skip
counting for efficiency and either omit setting Total or leave it zero. Apply
this change to the three handlers (the blocks that build
&sdkquery.PageResponse{Total: ...}) so Total only reflects the actual total when
req.Pagination.CountTotal is requested and avoid unnecessary work otherwise.
In `@x/market/keeper/keys/lease.go`:
- Around line 8-23: Update the doc comments to correctly reference leases
instead of bids: change the comment for LeasePrimaryKey to indicate "full
composite primary key for a lease in the IndexedMap" and change the comment for
LeasePrimaryKeyCodec to indicate "key codec for LeasePrimaryKey, composed from
stdlib codecs." Edit the comments immediately above the LeasePrimaryKey type
alias and the LeasePrimaryKeyCodec variable.
In `@x/market/keeper/keys/order.go`:
- Around line 9-12: The doc comment for OrderIDToKey incorrectly mentions
"mv1.BidID"; update the comment to accurately describe the function by
referencing mv1.OrderID and that it converts an mv1.OrderID to an
OrderPrimaryKey for use with the IndexedMap (function: OrderIDToKey, types:
mv1.OrderID, OrderPrimaryKey, and call collections.Join4).
🧹 Nitpick comments (4)
upgrades/software/v1.2.0/upgrade.go (1)
49-49: Unnecessaryfmt.Sprintffor static string.The format string contains no verbs, so
fmt.Sprintfis unnecessary overhead.♻️ Suggested fix
- up.log.Info(fmt.Sprintf("all migrations have been completed")) + up.log.Info("all migrations have been completed")x/market/genesis.go (2)
36-36: Consider adding a safety comment for the type assertion.The unchecked type assertion
kpr.(*keeper.Keeper)will panic if the cast fails. While this is likely intentional during genesis initialization (failure should halt the chain), a brief comment documenting this expectation would improve clarity.♻️ Suggested documentation
func InitGenesis(ctx sdk.Context, kpr keeper.IKeeper, data *v1beta5.GenesisState) { + // Type assertion is intentional: genesis init requires the concrete keeper for IndexedMap access concreteKeeper := kpr.(*keeper.Keeper)
72-74: Inconsistent error message for lease existence check.The lease existence error at line 73 uses a hardcoded string
"lease exists"while orders (line 45) and bids (line 59) use error constants from the v1 package (v1.ErrOrderExists,v1.ErrBidExists). The suggested error constantv1.ErrLeaseExistsdoes not currently exist in the market v1 package. To align with the pattern used for orders and bids, you would need to defineErrLeaseExistsin the external v1 package first, then update this error handling to use it with the%wformat specifier.upgrades/software/v1.2.0/market.go (1)
43-43: Unchecked type assertion on store key.The type assertion
skey.(*storetypes.KVStoreKey)will panic if the store key is of a different type. Consider adding error handling or a comment explaining why this is safe.♻️ Suggested defensive check
- ssvc := runtime.NewKVStoreService(skey.(*storetypes.KVStoreKey)) + kvStoreKey, ok := skey.(*storetypes.KVStoreKey) + if !ok { + return fmt.Errorf("expected *storetypes.KVStoreKey, got %T", skey) + } + ssvc := runtime.NewKVStoreService(kvStoreKey)
troian
force-pushed
the
upgrade-v1.2.0
branch
from
6ccf6c3 to
7a2e4f8
Compare
fixes akash-network/support#416 Signed-off-by: Artur Troian <troian@users.noreply.github.com>
troian
force-pushed
the
upgrade-v1.2.0
branch
from
7a2e4f8 to
375fb1c
Compare
1 participant
fixes akash-network/support#416