fix(x/market): reverse indexes for bids and leases#2040
Open
Conversation
WalkthroughThis pull request upgrades the Akash market module from consensus version 7 to 8 by migrating from KVStore-based storage to IndexedMap-based storage, introducing a comprehensive data migration system and new query implementation using index-based iteration instead of key-based pagination. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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
2 times, most recently
from
7a2e4f8 to
375fb1c
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
375fb1c to
955d89e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
x/market/genesis.go (1)
35-37: Avoid concrete type assertion in InitGenesis.
Line 36 assumeskeeper.IKeeperis always*keeper.Keeper, which can panic in tests/mocks. Consider extending the interface to exposeOrders/Bids/Leasesor at least guard the cast with a clearer error.♻️ Minimal guard (if interface expansion isn’t feasible yet)
- k := kpr.(*keeper.Keeper) + k, ok := kpr.(*keeper.Keeper) + if !ok { + panic("market InitGenesis requires *keeper.Keeper") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/market/genesis.go` around lines 35 - 37, InitGenesis currently does a concrete type assertion k := kpr.(*keeper.Keeper) which can panic; instead either expand keeper.IKeeper to expose the needed methods/properties (e.g., Orders, Bids, Leases) and use those methods in InitGenesis, or replace the hard cast with a safe assertion and clear error handling: perform kConcrete, ok := kpr.(*keeper.Keeper); if !ok { panic or return with a descriptive error message referencing InitGenesis and keeper.IKeeper } so tests/mocks don't crash unexpectedly.upgrades/software/v1.2.0/market.go (1)
69-124: Prefer error-returning Unmarshal in migration loops to avoid panics.
MustUnmarshalwill panic on corrupted data and bypass theerrorreturn path. UsingUnmarshallets the upgrade fail gracefully with a wrapped error. Apply the same pattern for orders, bids, and leases.♻️ Suggested change (pattern for orders; repeat for bids/leases)
- var order mtypes.Order - cdc.MustUnmarshal(oiter.Value(), &order) + var order mtypes.Order + if err := cdc.Unmarshal(oiter.Value(), &order); err != nil { + return fmt.Errorf("failed to unmarshal order: %w", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upgrades/software/v1.2.0/market.go` around lines 69 - 124, Replace panic-prone cdc.MustUnmarshal calls in the migration loops with error-returning cdc.Unmarshal and handle failures by returning a wrapped error; specifically, in the orders loop replace cdc.MustUnmarshal(oiter.Value(), &order) with a cdc.Unmarshal call and if it returns err return fmt.Errorf("failed to unmarshal order at key %s: %w", oiter.Key(), err), do the same for bids (replace cdc.MustUnmarshal(biter.Value(), &bid) and return fmt.Errorf("failed to unmarshal bid at key %s: %w", biter.Key(), err)) and leases (replace cdc.MustUnmarshal(liter.Value(), &lease) and return fmt.Errorf("failed to unmarshal lease at key %s: %w", liter.Key(), err)); keep all existing Set/Delete logic and counts intact.x/market/keeper/keeper.go (1)
557-561: Consider handling iterator close errors consistently.The function panics on iteration errors but discards the
iter.Close()error. While this rarely fails in practice, for consistency with the defensive error handling elsewhere, consider at minimum logging the error.♻️ Optional: Log iterator close errors
for ; iter.Valid(); iter.Next() { count++ } - _ = iter.Close() + if err := iter.Close(); err != nil { + // Log or handle close error if needed + _ = err + } }Alternatively, if you want to match the panic pattern:
- _ = iter.Close() + if err := iter.Close(); err != nil { + panic(fmt.Sprintf("BidCountForOrder iterator close failed: %v", err)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/market/keeper/keeper.go` around lines 557 - 561, The iterator close error is discarded after the loop (iter.Close()); update the block to check the returned error and handle it consistently with other defensive code paths—either log the error using the keeper's logger (e.g., k.Logger or k.Logger(ctx).Error/Info) including context like the iteration target and current count, or raise a panic if you prefer the existing panic-on-iteration-error pattern; ensure you reference iter.Close() and the surrounding loop that increments count so the change is applied in the same scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@upgrades/software/v1.2.0/market.go`:
- Around line 69-124: Replace panic-prone cdc.MustUnmarshal calls in the
migration loops with error-returning cdc.Unmarshal and handle failures by
returning a wrapped error; specifically, in the orders loop replace
cdc.MustUnmarshal(oiter.Value(), &order) with a cdc.Unmarshal call and if it
returns err return fmt.Errorf("failed to unmarshal order at key %s: %w",
oiter.Key(), err), do the same for bids (replace
cdc.MustUnmarshal(biter.Value(), &bid) and return fmt.Errorf("failed to
unmarshal bid at key %s: %w", biter.Key(), err)) and leases (replace
cdc.MustUnmarshal(liter.Value(), &lease) and return fmt.Errorf("failed to
unmarshal lease at key %s: %w", liter.Key(), err)); keep all existing Set/Delete
logic and counts intact.
In `@x/market/genesis.go`:
- Around line 35-37: InitGenesis currently does a concrete type assertion k :=
kpr.(*keeper.Keeper) which can panic; instead either expand keeper.IKeeper to
expose the needed methods/properties (e.g., Orders, Bids, Leases) and use those
methods in InitGenesis, or replace the hard cast with a safe assertion and clear
error handling: perform kConcrete, ok := kpr.(*keeper.Keeper); if !ok { panic or
return with a descriptive error message referencing InitGenesis and
keeper.IKeeper } so tests/mocks don't crash unexpectedly.
In `@x/market/keeper/keeper.go`:
- Around line 557-561: The iterator close error is discarded after the loop
(iter.Close()); update the block to check the returned error and handle it
consistently with other defensive code paths—either log the error using the
keeper's logger (e.g., k.Logger or k.Logger(ctx).Error/Info) including context
like the iteration target and current count, or raise a panic if you prefer the
existing panic-on-iteration-error pattern; ensure you reference iter.Close() and
the surrounding loop that increments count so the change is applied in the same
scope.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes akash-network/support#416