From f992907d1a780c1bec62b566c3eedb786ca9c683 Mon Sep 17 00:00:00 2001 From: Matee Ullah Malik Date: Mon, 16 Feb 2026 13:43:57 +0500 Subject: [PATCH 1/4] fix ordering in list-action query --- x/action/v1/keeper/query_list_actions.go | 96 +++++++++++++++++-- x/action/v1/keeper/query_list_actions_test.go | 48 +++++++++- 2 files changed, 135 insertions(+), 9 deletions(-) diff --git a/x/action/v1/keeper/query_list_actions.go b/x/action/v1/keeper/query_list_actions.go index 4e5de75c..37dc1266 100644 --- a/x/action/v1/keeper/query_list_actions.go +++ b/x/action/v1/keeper/query_list_actions.go @@ -2,6 +2,8 @@ package keeper import ( "context" + "sort" + "strconv" "github.com/LumeraProtocol/lumera/x/action/v1/types" actiontypes "github.com/LumeraProtocol/lumera/x/action/v1/types" @@ -14,6 +16,64 @@ import ( "google.golang.org/grpc/status" ) +func shouldUseNumericReverseOrdering(pageReq *query.PageRequest) bool { + return pageReq != nil && pageReq.Reverse && len(pageReq.Key) == 0 +} + +func parseNumericActionID(actionID string) (uint64, bool) { + parsed, err := strconv.ParseUint(actionID, 10, 64) + if err != nil { + return 0, false + } + return parsed, true +} + +func sortActionsByNumericID(actions []*types.Action) { + sort.SliceStable(actions, func(i, j int) bool { + leftNumericID, leftIsNumeric := parseNumericActionID(actions[i].ActionID) + rightNumericID, rightIsNumeric := parseNumericActionID(actions[j].ActionID) + + switch { + case leftIsNumeric && rightIsNumeric: + if leftNumericID == rightNumericID { + return actions[i].ActionID < actions[j].ActionID + } + return leftNumericID < rightNumericID + case leftIsNumeric != rightIsNumeric: + return leftIsNumeric + default: + return actions[i].ActionID < actions[j].ActionID + } + }) +} + +func paginateActionSlice(actions []*types.Action, pageReq *query.PageRequest) ([]*types.Action, *query.PageResponse) { + if pageReq == nil { + return actions, &query.PageResponse{} + } + + total := uint64(len(actions)) + offset := pageReq.Offset + if offset > total { + offset = total + } + + limit := pageReq.Limit + if limit == 0 || offset+limit > total { + limit = total - offset + } + + end := offset + limit + page := actions[int(offset):int(end)] + + pageRes := &query.PageResponse{} + if pageReq.CountTotal { + pageRes.Total = total + } + + return page, pageRes +} + // ListActions returns a list of actions, optionally filtered by type and state func (q queryServer) ListActions(goCtx context.Context, req *types.QueryListActionsRequest) (*types.QueryListActionsResponse, error) { if req == nil { @@ -81,19 +141,39 @@ func (q queryServer) ListActions(goCtx context.Context, req *types.QueryListActi } else { actionStore := prefix.NewStore(storeAdapter, []byte(ActionKeyPrefix)) - onResult := func(key, value []byte, accumulate bool) (bool, error) { - var act actiontypes.Action - if err := q.k.cdc.Unmarshal(value, &act); err != nil { - return false, err - } + if shouldUseNumericReverseOrdering(req.Pagination) { + iter := actionStore.Iterator(nil, nil) + defer iter.Close() - if accumulate { + for ; iter.Valid(); iter.Next() { + var act actiontypes.Action + if unmarshalErr := q.k.cdc.Unmarshal(iter.Value(), &act); unmarshalErr != nil { + return nil, status.Errorf(codes.Internal, "failed to unmarshal action: %v", unmarshalErr) + } actions = append(actions, &act) } - return true, nil + sortActionsByNumericID(actions) + for i, j := 0, len(actions)-1; i < j; i, j = i+1, j-1 { + actions[i], actions[j] = actions[j], actions[i] + } + + actions, pageRes = paginateActionSlice(actions, req.Pagination) + } else { + onResult := func(key, value []byte, accumulate bool) (bool, error) { + var act actiontypes.Action + if err := q.k.cdc.Unmarshal(value, &act); err != nil { + return false, err + } + + if accumulate { + actions = append(actions, &act) + } + + return true, nil + } + pageRes, err = query.FilteredPaginate(actionStore, req.Pagination, onResult) } - pageRes, err = query.FilteredPaginate(actionStore, req.Pagination, onResult) } if err != nil { return nil, status.Errorf(codes.Internal, "failed to paginate actions: %v", err) diff --git a/x/action/v1/keeper/query_list_actions_test.go b/x/action/v1/keeper/query_list_actions_test.go index c92f28fc..e0bf1fe8 100644 --- a/x/action/v1/keeper/query_list_actions_test.go +++ b/x/action/v1/keeper/query_list_actions_test.go @@ -9,8 +9,8 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/types/query" - "go.uber.org/mock/gomock" "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) @@ -172,3 +172,49 @@ func TestKeeper_ListActions(t *testing.T) { }) } } + +func TestKeeper_ListActions_ReversePaginationUsesNumericActionIDOrder(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + k, ctx := keepertest.ActionKeeper(t, ctrl) + q := keeper.NewQueryServerImpl(k) + price := sdk.NewInt64Coin("stake", 100) + + // Reproduces the mainnet boundary where lexical ordering would place 99999 before 123611. + actionLowLexical := types.Action{ + Creator: "creator-low", + ActionID: "99999", + ActionType: types.ActionTypeCascade, + Metadata: []byte("metadata-low"), + Price: price.String(), + ExpirationTime: 1234567891, + State: types.ActionStateApproved, + BlockHeight: 100, + SuperNodes: []string{"supernode-1"}, + } + actionHighNumeric := types.Action{ + Creator: "creator-high", + ActionID: "123611", + ActionType: types.ActionTypeCascade, + Metadata: []byte("metadata-high"), + Price: price.String(), + ExpirationTime: 1234567892, + State: types.ActionStateApproved, + BlockHeight: 101, + SuperNodes: []string{"supernode-2"}, + } + + require.NoError(t, k.SetAction(ctx, &actionLowLexical)) + require.NoError(t, k.SetAction(ctx, &actionHighNumeric)) + + resp, err := q.ListActions(ctx, &types.QueryListActionsRequest{ + Pagination: &query.PageRequest{ + Limit: 1, + Reverse: true, + }, + }) + require.NoError(t, err) + require.Len(t, resp.Actions, 1) + require.Equal(t, "123611", resp.Actions[0].ActionID) +} From 71b8aa3f25db988500f4ab1265c128256ce03bf0 Mon Sep 17 00:00:00 2001 From: Matee Ullah Malik Date: Tue, 3 Mar 2026 15:55:50 +0500 Subject: [PATCH 2/4] fix(action): address PR pagination review findings --- x/action/v1/keeper/query_list_actions.go | 162 +++++++++++++---- x/action/v1/keeper/query_list_actions_test.go | 164 +++++++++++++++++- 2 files changed, 294 insertions(+), 32 deletions(-) diff --git a/x/action/v1/keeper/query_list_actions.go b/x/action/v1/keeper/query_list_actions.go index 37dc1266..fbf02eaf 100644 --- a/x/action/v1/keeper/query_list_actions.go +++ b/x/action/v1/keeper/query_list_actions.go @@ -2,6 +2,8 @@ package keeper import ( "context" + "encoding/binary" + "fmt" "sort" "strconv" @@ -16,10 +18,13 @@ import ( "google.golang.org/grpc/status" ) +// shouldUseNumericReverseOrdering returns true when reverse pagination should use +// numeric action ID ordering instead of lexical store-key ordering. func shouldUseNumericReverseOrdering(pageReq *query.PageRequest) bool { - return pageReq != nil && pageReq.Reverse && len(pageReq.Key) == 0 + return pageReq != nil && pageReq.Reverse } +// parseNumericActionID parses action IDs that are strictly base-10 uint64 values. func parseNumericActionID(actionID string) (uint64, bool) { parsed, err := strconv.ParseUint(actionID, 10, 64) if err != nil { @@ -28,6 +33,8 @@ func parseNumericActionID(actionID string) (uint64, bool) { return parsed, true } +// sortActionsByNumericID sorts actions by ActionID using numeric ordering when +// possible, falling back to lexical ordering for non-numeric IDs. func sortActionsByNumericID(actions []*types.Action) { sort.SliceStable(actions, func(i, j int) bool { leftNumericID, leftIsNumeric := parseNumericActionID(actions[i].ActionID) @@ -47,20 +54,51 @@ func sortActionsByNumericID(actions []*types.Action) { }) } -func paginateActionSlice(actions []*types.Action, pageReq *query.PageRequest) ([]*types.Action, *query.PageResponse) { +// decodeActionPaginationOffset decodes an opaque pagination key into an offset. +func decodeActionPaginationOffset(key []byte) (uint64, error) { + if len(key) != 8 { + return 0, fmt.Errorf("invalid key length %d", len(key)) + } + return binary.BigEndian.Uint64(key), nil +} + +// encodeActionPaginationOffset encodes an offset as an opaque pagination key. +func encodeActionPaginationOffset(offset uint64) []byte { + key := make([]byte, 8) + binary.BigEndian.PutUint64(key, offset) + return key +} + +// paginateActionSlice paginates an already materialized action slice and returns +// a PageResponse compatible with cursor- and offset-based pagination. +func paginateActionSlice(actions []*types.Action, pageReq *query.PageRequest) ([]*types.Action, *query.PageResponse, error) { if pageReq == nil { - return actions, &query.PageResponse{} + return actions, &query.PageResponse{}, nil } total := uint64(len(actions)) offset := pageReq.Offset + + if len(pageReq.Key) > 0 { + decodedOffset, err := decodeActionPaginationOffset(pageReq.Key) + if err != nil { + return nil, nil, status.Error(codes.InvalidArgument, "invalid pagination key") + } + offset = decodedOffset + } + if offset > total { offset = total } limit := pageReq.Limit - if limit == 0 || offset+limit > total { - limit = total - offset + if limit == 0 { + limit = query.DefaultLimit + } + + remaining := total - offset + if limit > remaining { + limit = remaining } end := offset + limit @@ -70,8 +108,11 @@ func paginateActionSlice(actions []*types.Action, pageReq *query.PageRequest) ([ if pageReq.CountTotal { pageRes.Total = total } + if end < total { + pageRes.NextKey = encodeActionPaginationOffset(end) + } - return page, pageRes + return page, pageRes, nil } // ListActions returns a list of actions, optionally filtered by type and state @@ -97,51 +138,107 @@ func (q queryServer) ListActions(goCtx context.Context, req *types.QueryListActi statePrefix := []byte(ActionByStatePrefix + types.ActionState(req.ActionState).String() + "/") indexStore := prefix.NewStore(storeAdapter, statePrefix) - onResult := func(key, _ []byte, accumulate bool) (bool, error) { - actionID := string(key) - act, found := q.k.GetActionByID(ctx, actionID) - if !found { - // Stale index entry; skip without counting - return false, nil + if shouldUseNumericReverseOrdering(req.Pagination) { + // Numeric reverse ordering cannot be derived from lexical KV iteration, so + // we materialize the matched set, sort it numerically, then paginate. + iter := indexStore.Iterator(nil, nil) + defer iter.Close() + + for ; iter.Valid(); iter.Next() { + actionID := string(iter.Key()) + act, found := q.k.GetActionByID(ctx, actionID) + if !found { + // Stale index entry; skip without counting + continue + } + + if req.ActionType != types.ActionTypeUnspecified && act.ActionType != actiontypes.ActionType(req.ActionType) { + continue + } + + actions = append(actions, act) } - if req.ActionType != types.ActionTypeUnspecified && act.ActionType != actiontypes.ActionType(req.ActionType) { - return false, nil + sortActionsByNumericID(actions) + for i, j := 0, len(actions)-1; i < j; i, j = i+1, j-1 { + actions[i], actions[j] = actions[j], actions[i] } - if accumulate { - actions = append(actions, act) + actions, pageRes, err = paginateActionSlice(actions, req.Pagination) + } else { + onResult := func(key, _ []byte, accumulate bool) (bool, error) { + actionID := string(key) + act, found := q.k.GetActionByID(ctx, actionID) + if !found { + // Stale index entry; skip without counting + return false, nil + } + + if req.ActionType != types.ActionTypeUnspecified && act.ActionType != actiontypes.ActionType(req.ActionType) { + return false, nil + } + + if accumulate { + actions = append(actions, act) + } + + return true, nil } - return true, nil + pageRes, err = query.FilteredPaginate(indexStore, req.Pagination, onResult) } - - pageRes, err = query.FilteredPaginate(indexStore, req.Pagination, onResult) } else if useTypeIndex { // When filtering only by type, use the type index typePrefix := []byte(ActionByTypePrefix + types.ActionType(req.ActionType).String() + "/") indexStore := prefix.NewStore(storeAdapter, typePrefix) - onResult := func(key, _ []byte, accumulate bool) (bool, error) { - actionID := string(key) - act, found := q.k.GetActionByID(ctx, actionID) - if !found { - // Stale index entry; skip - return false, nil - } + if shouldUseNumericReverseOrdering(req.Pagination) { + // Numeric reverse ordering cannot be derived from lexical KV iteration, so + // we materialize the matched set, sort it numerically, then paginate. + iter := indexStore.Iterator(nil, nil) + defer iter.Close() + + for ; iter.Valid(); iter.Next() { + actionID := string(iter.Key()) + act, found := q.k.GetActionByID(ctx, actionID) + if !found { + // Stale index entry; skip + continue + } - if accumulate { actions = append(actions, act) } - return true, nil - } + sortActionsByNumericID(actions) + for i, j := 0, len(actions)-1; i < j; i, j = i+1, j-1 { + actions[i], actions[j] = actions[j], actions[i] + } + + actions, pageRes, err = paginateActionSlice(actions, req.Pagination) + } else { + onResult := func(key, _ []byte, accumulate bool) (bool, error) { + actionID := string(key) + act, found := q.k.GetActionByID(ctx, actionID) + if !found { + // Stale index entry; skip + return false, nil + } + + if accumulate { + actions = append(actions, act) + } - pageRes, err = query.FilteredPaginate(indexStore, req.Pagination, onResult) + return true, nil + } + + pageRes, err = query.FilteredPaginate(indexStore, req.Pagination, onResult) + } } else { actionStore := prefix.NewStore(storeAdapter, []byte(ActionKeyPrefix)) if shouldUseNumericReverseOrdering(req.Pagination) { + // Numeric reverse ordering cannot be derived from lexical KV iteration, so + // we materialize the matched set, sort it numerically, then paginate. iter := actionStore.Iterator(nil, nil) defer iter.Close() @@ -158,7 +255,7 @@ func (q queryServer) ListActions(goCtx context.Context, req *types.QueryListActi actions[i], actions[j] = actions[j], actions[i] } - actions, pageRes = paginateActionSlice(actions, req.Pagination) + actions, pageRes, err = paginateActionSlice(actions, req.Pagination) } else { onResult := func(key, value []byte, accumulate bool) (bool, error) { var act actiontypes.Action @@ -176,6 +273,9 @@ func (q queryServer) ListActions(goCtx context.Context, req *types.QueryListActi } } if err != nil { + if st, ok := status.FromError(err); ok && st.Code() != codes.Unknown { + return nil, err + } return nil, status.Errorf(codes.Internal, "failed to paginate actions: %v", err) } diff --git a/x/action/v1/keeper/query_list_actions_test.go b/x/action/v1/keeper/query_list_actions_test.go index e0bf1fe8..d9e7f423 100644 --- a/x/action/v1/keeper/query_list_actions_test.go +++ b/x/action/v1/keeper/query_list_actions_test.go @@ -1,6 +1,7 @@ package keeper_test import ( + "fmt" "testing" keepertest "github.com/LumeraProtocol/lumera/testutil/keeper" @@ -9,8 +10,8 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/types/query" - "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" + "github.com/stretchr/testify/require" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) @@ -218,3 +219,164 @@ func TestKeeper_ListActions_ReversePaginationUsesNumericActionIDOrder(t *testing require.Len(t, resp.Actions, 1) require.Equal(t, "123611", resp.Actions[0].ActionID) } + +func TestKeeper_ListActions_ReversePaginationCursorMaintainsNumericOrder(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + k, ctx := keepertest.ActionKeeper(t, ctrl) + q := keeper.NewQueryServerImpl(k) + price := sdk.NewInt64Coin("stake", 100) + + ids := []string{"2", "10", "100"} + for i, id := range ids { + action := types.Action{ + Creator: "creator-" + id, + ActionID: id, + ActionType: types.ActionTypeCascade, + Metadata: []byte("metadata-" + id), + Price: price.String(), + ExpirationTime: int64(1234567890 + i), + State: types.ActionStateApproved, + BlockHeight: int64(100 + i), + SuperNodes: []string{"supernode-" + id}, + } + require.NoError(t, k.SetAction(ctx, &action)) + } + + firstPage, err := q.ListActions(ctx, &types.QueryListActionsRequest{ + Pagination: &query.PageRequest{ + Limit: 1, + Reverse: true, + }, + }) + require.NoError(t, err) + require.Len(t, firstPage.Actions, 1) + require.Equal(t, "100", firstPage.Actions[0].ActionID) + require.NotEmpty(t, firstPage.Pagination.NextKey) + + secondPage, err := q.ListActions(ctx, &types.QueryListActionsRequest{ + Pagination: &query.PageRequest{ + Key: firstPage.Pagination.NextKey, + Limit: 1, + Reverse: true, + }, + }) + require.NoError(t, err) + require.Len(t, secondPage.Actions, 1) + require.Equal(t, "10", secondPage.Actions[0].ActionID) +} + +func TestKeeper_ListActions_ReversePaginationWithTypeFilterUsesNumericOrder(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + k, ctx := keepertest.ActionKeeper(t, ctrl) + q := keeper.NewQueryServerImpl(k) + price := sdk.NewInt64Coin("stake", 100) + + actionLowLexical := types.Action{ + Creator: "creator-low", + ActionID: "99999", + ActionType: types.ActionTypeCascade, + Metadata: []byte("metadata-low"), + Price: price.String(), + ExpirationTime: 1234567891, + State: types.ActionStateApproved, + BlockHeight: 100, + SuperNodes: []string{"supernode-1"}, + } + actionHighNumeric := types.Action{ + Creator: "creator-high", + ActionID: "123611", + ActionType: types.ActionTypeCascade, + Metadata: []byte("metadata-high"), + Price: price.String(), + ExpirationTime: 1234567892, + State: types.ActionStateApproved, + BlockHeight: 101, + SuperNodes: []string{"supernode-2"}, + } + actionDifferentType := types.Action{ + Creator: "creator-other-type", + ActionID: "999999", + ActionType: types.ActionTypeSense, + Metadata: []byte("metadata-other-type"), + Price: price.String(), + ExpirationTime: 1234567893, + State: types.ActionStateApproved, + BlockHeight: 102, + SuperNodes: []string{"supernode-3"}, + } + + require.NoError(t, k.SetAction(ctx, &actionLowLexical)) + require.NoError(t, k.SetAction(ctx, &actionHighNumeric)) + require.NoError(t, k.SetAction(ctx, &actionDifferentType)) + + resp, err := q.ListActions(ctx, &types.QueryListActionsRequest{ + ActionType: types.ActionTypeCascade, + Pagination: &query.PageRequest{ + Limit: 1, + Reverse: true, + }, + }) + require.NoError(t, err) + require.Len(t, resp.Actions, 1) + require.Equal(t, "123611", resp.Actions[0].ActionID) +} + +func TestKeeper_ListActions_ReversePaginationInvalidCursorKey(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + k, ctx := keepertest.ActionKeeper(t, ctrl) + q := keeper.NewQueryServerImpl(k) + + resp, err := q.ListActions(ctx, &types.QueryListActionsRequest{ + Pagination: &query.PageRequest{ + Key: []byte("bad-key"), + Reverse: true, + }, + }) + require.Nil(t, resp) + require.Error(t, err) + st, ok := status.FromError(err) + require.True(t, ok) + require.Equal(t, codes.InvalidArgument, st.Code()) +} + +func TestKeeper_ListActions_ReversePaginationZeroLimitUsesDefaultLimit(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + k, ctx := keepertest.ActionKeeper(t, ctrl) + q := keeper.NewQueryServerImpl(k) + price := sdk.NewInt64Coin("stake", 100) + + entries := query.DefaultLimit + 1 + for i := 1; i <= entries; i++ { + id := fmt.Sprintf("%d", i) + action := types.Action{ + Creator: "creator-" + id, + ActionID: id, + ActionType: types.ActionTypeCascade, + Metadata: []byte("metadata-" + id), + Price: price.String(), + ExpirationTime: int64(1234567890 + i), + State: types.ActionStateApproved, + BlockHeight: int64(200 + i), + SuperNodes: []string{"supernode-" + id}, + } + require.NoError(t, k.SetAction(ctx, &action)) + } + + resp, err := q.ListActions(ctx, &types.QueryListActionsRequest{ + Pagination: &query.PageRequest{ + Limit: 0, + Reverse: true, + }, + }) + require.NoError(t, err) + require.Len(t, resp.Actions, int(query.DefaultLimit)) + require.NotEmpty(t, resp.Pagination.NextKey) +} From 176c9743e8bea994b0d42637452d095dce3445ae Mon Sep 17 00:00:00 2001 From: Matee Ullah Malik Date: Tue, 3 Mar 2026 16:04:01 +0500 Subject: [PATCH 3/4] refactor(action): dedupe reverse materialization in ListActions --- x/action/v1/keeper/query_list_actions.go | 122 +++++++++++++---------- 1 file changed, 67 insertions(+), 55 deletions(-) diff --git a/x/action/v1/keeper/query_list_actions.go b/x/action/v1/keeper/query_list_actions.go index fbf02eaf..ef5f400a 100644 --- a/x/action/v1/keeper/query_list_actions.go +++ b/x/action/v1/keeper/query_list_actions.go @@ -54,6 +54,61 @@ func sortActionsByNumericID(actions []*types.Action) { }) } +// applyNumericReverseOrderingAndPaginate applies numeric ActionID ordering in +// descending order and then paginates the resulting slice. +func applyNumericReverseOrderingAndPaginate(actions []*types.Action, pageReq *query.PageRequest) ([]*types.Action, *query.PageResponse, error) { + sortActionsByNumericID(actions) + for i, j := 0, len(actions)-1; i < j; i, j = i+1, j-1 { + actions[i], actions[j] = actions[j], actions[i] + } + + return paginateActionSlice(actions, pageReq) +} + +// collectActionsFromIDIndexStore loads actions by ID from an index store whose keys +// are action IDs. Stale index entries are ignored. +func (q queryServer) collectActionsFromIDIndexStore( + ctx sdk.Context, + indexStore prefix.Store, + actionTypeFilter types.ActionType, +) ([]*types.Action, error) { + actions := make([]*types.Action, 0) + iter := indexStore.Iterator(nil, nil) + defer iter.Close() + + for ; iter.Valid(); iter.Next() { + actionID := string(iter.Key()) + act, found := q.k.GetActionByID(ctx, actionID) + if !found { + continue + } + if actionTypeFilter != types.ActionTypeUnspecified && act.ActionType != actiontypes.ActionType(actionTypeFilter) { + continue + } + + actions = append(actions, act) + } + + return actions, nil +} + +// collectActionsFromPrimaryStore loads all actions from the primary action store. +func (q queryServer) collectActionsFromPrimaryStore(actionStore prefix.Store) ([]*types.Action, error) { + actions := make([]*types.Action, 0) + iter := actionStore.Iterator(nil, nil) + defer iter.Close() + + for ; iter.Valid(); iter.Next() { + var act actiontypes.Action + if unmarshalErr := q.k.cdc.Unmarshal(iter.Value(), &act); unmarshalErr != nil { + return nil, status.Errorf(codes.Internal, "failed to unmarshal action: %v", unmarshalErr) + } + actions = append(actions, &act) + } + + return actions, nil +} + // decodeActionPaginationOffset decodes an opaque pagination key into an offset. func decodeActionPaginationOffset(key []byte) (uint64, error) { if len(key) != 8 { @@ -141,30 +196,12 @@ func (q queryServer) ListActions(goCtx context.Context, req *types.QueryListActi if shouldUseNumericReverseOrdering(req.Pagination) { // Numeric reverse ordering cannot be derived from lexical KV iteration, so // we materialize the matched set, sort it numerically, then paginate. - iter := indexStore.Iterator(nil, nil) - defer iter.Close() - - for ; iter.Valid(); iter.Next() { - actionID := string(iter.Key()) - act, found := q.k.GetActionByID(ctx, actionID) - if !found { - // Stale index entry; skip without counting - continue - } - - if req.ActionType != types.ActionTypeUnspecified && act.ActionType != actiontypes.ActionType(req.ActionType) { - continue - } - - actions = append(actions, act) + actions, err = q.collectActionsFromIDIndexStore(ctx, indexStore, req.ActionType) + if err != nil { + return nil, err } - sortActionsByNumericID(actions) - for i, j := 0, len(actions)-1; i < j; i, j = i+1, j-1 { - actions[i], actions[j] = actions[j], actions[i] - } - - actions, pageRes, err = paginateActionSlice(actions, req.Pagination) + actions, pageRes, err = applyNumericReverseOrderingAndPaginate(actions, req.Pagination) } else { onResult := func(key, _ []byte, accumulate bool) (bool, error) { actionID := string(key) @@ -195,26 +232,12 @@ func (q queryServer) ListActions(goCtx context.Context, req *types.QueryListActi if shouldUseNumericReverseOrdering(req.Pagination) { // Numeric reverse ordering cannot be derived from lexical KV iteration, so // we materialize the matched set, sort it numerically, then paginate. - iter := indexStore.Iterator(nil, nil) - defer iter.Close() - - for ; iter.Valid(); iter.Next() { - actionID := string(iter.Key()) - act, found := q.k.GetActionByID(ctx, actionID) - if !found { - // Stale index entry; skip - continue - } - - actions = append(actions, act) - } - - sortActionsByNumericID(actions) - for i, j := 0, len(actions)-1; i < j; i, j = i+1, j-1 { - actions[i], actions[j] = actions[j], actions[i] + actions, err = q.collectActionsFromIDIndexStore(ctx, indexStore, types.ActionTypeUnspecified) + if err != nil { + return nil, err } - actions, pageRes, err = paginateActionSlice(actions, req.Pagination) + actions, pageRes, err = applyNumericReverseOrderingAndPaginate(actions, req.Pagination) } else { onResult := func(key, _ []byte, accumulate bool) (bool, error) { actionID := string(key) @@ -239,23 +262,12 @@ func (q queryServer) ListActions(goCtx context.Context, req *types.QueryListActi if shouldUseNumericReverseOrdering(req.Pagination) { // Numeric reverse ordering cannot be derived from lexical KV iteration, so // we materialize the matched set, sort it numerically, then paginate. - iter := actionStore.Iterator(nil, nil) - defer iter.Close() - - for ; iter.Valid(); iter.Next() { - var act actiontypes.Action - if unmarshalErr := q.k.cdc.Unmarshal(iter.Value(), &act); unmarshalErr != nil { - return nil, status.Errorf(codes.Internal, "failed to unmarshal action: %v", unmarshalErr) - } - actions = append(actions, &act) - } - - sortActionsByNumericID(actions) - for i, j := 0, len(actions)-1; i < j; i, j = i+1, j-1 { - actions[i], actions[j] = actions[j], actions[i] + actions, err = q.collectActionsFromPrimaryStore(actionStore) + if err != nil { + return nil, err } - actions, pageRes, err = paginateActionSlice(actions, req.Pagination) + actions, pageRes, err = applyNumericReverseOrderingAndPaginate(actions, req.Pagination) } else { onResult := func(key, value []byte, accumulate bool) (bool, error) { var act actiontypes.Action From 0e4b2a569d5f013f765e3ba6b81544b7bccaf6ca Mon Sep 17 00:00:00 2001 From: Matee Ullah Malik Date: Tue, 3 Mar 2026 16:15:52 +0500 Subject: [PATCH 4/4] fix(action): reject offset+key in reverse pagination --- x/action/v1/keeper/query_list_actions.go | 3 +++ x/action/v1/keeper/query_list_actions_test.go | 22 +++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/x/action/v1/keeper/query_list_actions.go b/x/action/v1/keeper/query_list_actions.go index ef5f400a..a9601ecd 100644 --- a/x/action/v1/keeper/query_list_actions.go +++ b/x/action/v1/keeper/query_list_actions.go @@ -130,6 +130,9 @@ func paginateActionSlice(actions []*types.Action, pageReq *query.PageRequest) ([ if pageReq == nil { return actions, &query.PageResponse{}, nil } + if pageReq.Offset > 0 && pageReq.Key != nil { + return nil, nil, status.Error(codes.InvalidArgument, "paginate: invalid request, either offset or key is expected, got both") + } total := uint64(len(actions)) offset := pageReq.Offset diff --git a/x/action/v1/keeper/query_list_actions_test.go b/x/action/v1/keeper/query_list_actions_test.go index d9e7f423..f0400432 100644 --- a/x/action/v1/keeper/query_list_actions_test.go +++ b/x/action/v1/keeper/query_list_actions_test.go @@ -345,6 +345,28 @@ func TestKeeper_ListActions_ReversePaginationInvalidCursorKey(t *testing.T) { require.Equal(t, codes.InvalidArgument, st.Code()) } +func TestKeeper_ListActions_ReversePaginationRejectsOffsetAndKey(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + k, ctx := keepertest.ActionKeeper(t, ctrl) + q := keeper.NewQueryServerImpl(k) + + resp, err := q.ListActions(ctx, &types.QueryListActionsRequest{ + Pagination: &query.PageRequest{ + Key: []byte{0, 0, 0, 0, 0, 0, 0, 1}, + Offset: 1, + Reverse: true, + }, + }) + require.Nil(t, resp) + require.Error(t, err) + st, ok := status.FromError(err) + require.True(t, ok) + require.Equal(t, codes.InvalidArgument, st.Code()) + require.Contains(t, st.Message(), "either offset or key is expected, got both") +} + func TestKeeper_ListActions_ReversePaginationZeroLimitUsesDefaultLimit(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish()