Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 88 additions & 8 deletions x/action/v1/keeper/query_list_actions.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition on line 20 specifically checks for an empty pagination Key, but this means the numeric reverse ordering is only applied when pagination Key is not provided. If a client uses cursor-based pagination with a Key from a previous response, they would get lexical ordering instead of numeric ordering, leading to inconsistent results across paginated requests. This could cause confusion when paginating through results.

Suggested change
return pageReq != nil && pageReq.Reverse && len(pageReq.Key) == 0
return pageReq != nil && pageReq.Reverse

Copilot uses AI. Check for mistakes.
}

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
}
})
}
Comment on lines +19 to +48
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The helper functions shouldUseNumericReverseOrdering, parseNumericActionID, sortActionsByNumericID, and paginateActionSlice lack documentation comments explaining their purpose, parameters, and return values. Adding godoc-style comments would improve code maintainability and help other developers understand the special handling for numeric action ID ordering.

Copilot uses AI. Check for mistakes.

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
Comment on lines +61 to +63
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the limit is 0, this function returns all remaining items after the offset. This behavior differs from the standard cosmos-sdk pagination where limit=0 typically uses a default limit value. While this may be intentional, it could lead to unexpected behavior where clients requesting limit=0 receive potentially thousands of results instead of a default page size. Consider using a default limit when limit=0 to prevent accidentally large responses.

Copilot uses AI. Check for mistakes.
}

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 {
Expand Down Expand Up @@ -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)
}
Comment on lines +144 to 154
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loading all actions into memory for reverse pagination creates a scalability concern. On a blockchain with a large number of actions (e.g., thousands or millions), this approach could lead to excessive memory consumption and slow response times. Consider implementing a more efficient solution that doesn't require loading all records, such as using a reverse iterator with the cosmos-sdk's pagination utilities, or document this limitation clearly if it's intentional for a mainnet fix.

Copilot uses AI. Check for mistakes.

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)
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The custom pagination implementation does not set the NextKey field in the PageResponse, which breaks cursor-based pagination for clients that rely on it. The cosmos-sdk's query.FilteredPaginate normally sets NextKey to allow clients to fetch subsequent pages using the Key field. Without NextKey, clients cannot perform cursor-based pagination when reverse ordering is enabled, which is a significant limitation for large result sets.

Copilot uses AI. Check for mistakes.
} 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)
}
Comment on lines 141 to 176
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The numeric reverse ordering fix is only applied when no state or type filtering is used (the else branch starting at line 141). This means that queries with ActionState or ActionType filters will still use the default lexical ordering from FilteredPaginate, even when reverse pagination is requested. This creates inconsistent behavior where reverse pagination ordering depends on whether filters are applied. Consider either applying the same numeric ordering logic to filtered queries or documenting this limitation.

Copilot uses AI. Check for mistakes.
pageRes, err = query.FilteredPaginate(actionStore, req.Pagination, onResult)
}
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to paginate actions: %v", err)
Expand Down
48 changes: 47 additions & 1 deletion x/action/v1/keeper/query_list_actions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Comment on lines 12 to +13
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The import order here deviates from the established codebase convention. In all other keeper test files (query_get_action_test.go, query_list_actions_by_creator_test.go, query_list_actions_by_sn_test.go, query_list_actions_by_block_height_test.go), 'go.uber.org/mock/gomock' is consistently imported before 'github.com/stretchr/testify/require'. Please reorder these imports to maintain consistency with the rest of the codebase.

Suggested change
"github.com/stretchr/testify/require"
"go.uber.org/mock/gomock"
"go.uber.org/mock/gomock"
"github.com/stretchr/testify/require"

Copilot uses AI. Check for mistakes.
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
)
Expand Down Expand Up @@ -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)
}
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test only covers the basic scenario with two numeric action IDs in reverse pagination. Consider adding test cases for edge cases such as: mixing numeric and non-numeric action IDs, actions with identical numeric values but different string representations, very large numeric IDs approaching uint64 limits, and pagination with offset to ensure the numeric ordering is maintained across multiple pages.

Suggested change
}
}
func TestKeeper_ListActions_ReversePagination_MixedNumericAndNonNumeric(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)
actions := []types.Action{
{
Creator: "creator-numeric-low",
ActionID: "2",
ActionType: types.ActionTypeCascade,
Metadata: []byte("metadata-2"),
Price: price.String(),
ExpirationTime: 1234567890,
State: types.ActionStateApproved,
BlockHeight: 10,
SuperNodes: []string{"supernode-1"},
},
{
Creator: "creator-numeric-high",
ActionID: "10",
ActionType: types.ActionTypeCascade,
Metadata: []byte("metadata-10"),
Price: price.String(),
ExpirationTime: 1234567891,
State: types.ActionStateApproved,
BlockHeight: 11,
SuperNodes: []string{"supernode-2"},
},
{
Creator: "creator-non-numeric",
ActionID: "abc",
ActionType: types.ActionTypeCascade,
Metadata: []byte("metadata-abc"),
Price: price.String(),
ExpirationTime: 1234567892,
State: types.ActionStateApproved,
BlockHeight: 12,
SuperNodes: []string{"supernode-3"},
},
}
for i := range actions {
require.NoError(t, k.SetAction(ctx, &actions[i]))
}
resp, err := q.ListActions(ctx, &types.QueryListActionsRequest{
Pagination: &query.PageRequest{
Limit: 10,
Reverse: true,
},
})
require.NoError(t, err)
require.Len(t, resp.Actions, 3)
// Ensure all expected IDs are present.
gotIDs := []string{
resp.Actions[0].ActionID,
resp.Actions[1].ActionID,
resp.Actions[2].ActionID,
}
require.ElementsMatch(t, []string{"2", "10", "abc"}, gotIDs)
// Ensure numeric IDs are ordered numerically relative to each other in reverse order.
// That is, "10" must appear before "2" in the result slice.
var idx10, idx2 int = -1, -1
for i, id := range gotIDs {
if id == "10" {
idx10 = i
}
if id == "2" {
idx2 = i
}
}
require.NotEqual(t, -1, idx10)
require.NotEqual(t, -1, idx2)
require.Less(t, idx10, idx2)
}
func TestKeeper_ListActions_ReversePagination_IdenticalNumericDifferentStrings(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)
actionA := types.Action{
Creator: "creator-a",
ActionID: "10",
ActionType: types.ActionTypeCascade,
Metadata: []byte("metadata-10"),
Price: price.String(),
ExpirationTime: 1234567890,
State: types.ActionStateApproved,
BlockHeight: 20,
SuperNodes: []string{"supernode-a"},
}
actionB := types.Action{
Creator: "creator-b",
ActionID: "0010",
ActionType: types.ActionTypeCascade,
Metadata: []byte("metadata-0010"),
Price: price.String(),
ExpirationTime: 1234567891,
State: types.ActionStateApproved,
BlockHeight: 21,
SuperNodes: []string{"supernode-b"},
}
require.NoError(t, k.SetAction(ctx, &actionA))
require.NoError(t, k.SetAction(ctx, &actionB))
resp, err := q.ListActions(ctx, &types.QueryListActionsRequest{
Pagination: &query.PageRequest{
Limit: 10,
Reverse: true,
},
})
require.NoError(t, err)
require.Len(t, resp.Actions, 2)
// Ensure both representations are present and there are no duplicates/missing entries.
gotIDs := []string{
resp.Actions[0].ActionID,
resp.Actions[1].ActionID,
}
require.ElementsMatch(t, []string{"10", "0010"}, gotIDs)
}
func TestKeeper_ListActions_ReversePagination_VeryLargeNumericIDs(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 close to uint64 max value.
actionLow := types.Action{
Creator: "creator-low-large",
ActionID: "18446744073709551610",
ActionType: types.ActionTypeCascade,
Metadata: []byte("metadata-low-large"),
Price: price.String(),
ExpirationTime: 1234567890,
State: types.ActionStateApproved,
BlockHeight: 30,
SuperNodes: []string{"supernode-low"},
}
actionHigh := types.Action{
Creator: "creator-high-large",
ActionID: "18446744073709551615",
ActionType: types.ActionTypeCascade,
Metadata: []byte("metadata-high-large"),
Price: price.String(),
ExpirationTime: 1234567891,
State: types.ActionStateApproved,
BlockHeight: 31,
SuperNodes: []string{"supernode-high"},
}
require.NoError(t, k.SetAction(ctx, &actionLow))
require.NoError(t, k.SetAction(ctx, &actionHigh))
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, "18446744073709551615", resp.Actions[0].ActionID)
}
func TestKeeper_ListActions_ReversePaginationWithOffset_MaintainsNumericOrder(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", "1000"}
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(40 + i),
SuperNodes: []string{"supernode-" + id},
}
require.NoError(t, k.SetAction(ctx, &action))
}
// First page: highest two numeric IDs in reverse order.
firstResp, err := q.ListActions(ctx, &types.QueryListActionsRequest{
Pagination: &query.PageRequest{
Limit: 2,
Offset: 0,
Reverse: true,
},
})
require.NoError(t, err)
require.Len(t, firstResp.Actions, 2)
// Second page: next two numeric IDs in reverse order.
secondResp, err := q.ListActions(ctx, &types.QueryListActionsRequest{
Pagination: &query.PageRequest{
Limit: 2,
Offset: 2,
Reverse: true,
},
})
require.NoError(t, err)
require.Len(t, secondResp.Actions, 2)
// Collect all IDs from both pages.
allIDs := []string{
firstResp.Actions[0].ActionID,
firstResp.Actions[1].ActionID,
secondResp.Actions[0].ActionID,
secondResp.Actions[1].ActionID,
}
// Ensure we saw exactly the four IDs we inserted.
require.ElementsMatch(t, []string{"2", "10", "100", "1000"}, allIDs)
// Ensure global reverse numeric order across both pages.
// The expected reverse numeric order is: 1000, 100, 10, 2.
expectedOrder := []string{"1000", "100", "10", "2"}
// Map id -> index in concatenated results.
index := make(map[string]int)
for i, id := range allIDs {
index[id] = i
}
for i := 0; i < len(expectedOrder)-1; i++ {
curr := expectedOrder[i]
next := expectedOrder[i+1]
require.Less(t, index[curr], index[next])
}
}

Copilot uses AI. Check for mistakes.