Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: extend consumer chains query #2172

Merged
merged 21 commits into from
Aug 29, 2024
Merged
Show file tree
Hide file tree
Changes from 20 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
18 changes: 14 additions & 4 deletions proto/interchain_security/ccv/provider/v1/query.proto
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,14 @@
[ (gogoproto.nullable) = false ];
}

message QueryConsumerChainsRequest {}
message QueryConsumerChainsRequest {
// The phase of the consumer chains returned (optional)
// Registered=1|Initialized=2|Launched=3|Stopped=4
ConsumerPhase phase = 1;
// The limit of consumer chains returned (optional)
// default is 100
int32 limit = 2;
}

message QueryConsumerChainsResponse { repeated Chain chains = 1; }

Expand Down Expand Up @@ -233,11 +240,14 @@
repeated string allowlist = 7;
// Corresponds to a list of provider consensus addresses of validators that CANNOT validate the consumer chain.
repeated string denylist = 8;
// The phase the consumer chain (Registered=0|Initialized=1|FailedToLaunch=2|Launched=3|Stopped=4)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// The phase the consumer chain (Registered=0|Initialized=1|FailedToLaunch=2|Launched=3|Stopped=4)
// The phase the consumer chain (Registered=1|Initialized=2|Launched=3|Stopped=4)

ConsumerPhase phase = 9;

Check failure on line 244 in proto/interchain_security/ccv/provider/v1/query.proto

View workflow job for this annotation

GitHub Actions / break-check

Field "9" with name "phase" on message "Chain" changed option "json_name" from "minStake" to "phase".

Check failure on line 244 in proto/interchain_security/ccv/provider/v1/query.proto

View workflow job for this annotation

GitHub Actions / break-check

Field "9" with name "phase" on message "Chain" changed type from "uint64" to "enum".

Check failure on line 244 in proto/interchain_security/ccv/provider/v1/query.proto

View workflow job for this annotation

GitHub Actions / break-check

Field "9" on message "Chain" changed name from "min_stake" to "phase".
// The metadata of the consumer chain
ConsumerMetadata metadata = 10 [(gogoproto.nullable) = false ];

Check failure on line 246 in proto/interchain_security/ccv/provider/v1/query.proto

View workflow job for this annotation

GitHub Actions / break-check

Field "10" with name "metadata" on message "Chain" changed cardinality from "optional with implicit presence" to "optional with explicit presence".

Check failure on line 246 in proto/interchain_security/ccv/provider/v1/query.proto

View workflow job for this annotation

GitHub Actions / break-check

Field "10" with name "metadata" on message "Chain" changed option "json_name" from "allowInactiveVals" to "metadata".

Check failure on line 246 in proto/interchain_security/ccv/provider/v1/query.proto

View workflow job for this annotation

GitHub Actions / break-check

Field "10" with name "metadata" on message "Chain" changed type from "bool" to "message".

Check failure on line 246 in proto/interchain_security/ccv/provider/v1/query.proto

View workflow job for this annotation

GitHub Actions / break-check

Field "10" on message "Chain" changed name from "allow_inactive_vals" to "metadata".
// Corresponds to the minimal amount of (provider chain) stake required to validate on the consumer chain.
uint64 min_stake = 9;
uint64 min_stake = 11;
// Corresponds to whether inactive validators are allowed to validate the consumer chain.
bool allow_inactive_vals = 10;

bool allow_inactive_vals = 12;
}

message QueryValidatorConsumerAddrRequest {
Expand Down
22 changes: 20 additions & 2 deletions x/ccv/provider/client/cli/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package cli

import (
"fmt"
"strconv"
"strings"

"github.com/spf13/cobra"
Expand Down Expand Up @@ -75,9 +76,9 @@ func CmdConsumerGenesis() *cobra.Command {

func CmdConsumerChains() *cobra.Command {
cmd := &cobra.Command{
Use: "list-consumer-chains",
Use: "list-consumer-chains [phase] [limit]",
Short: "Query active consumer chains for provider chain.",
Args: cobra.ExactArgs(0),
Args: cobra.MaximumNArgs(2),
RunE: func(cmd *cobra.Command, args []string) (err error) {
clientCtx, err := client.GetClientQueryContext(cmd)
if err != nil {
Expand All @@ -86,6 +87,23 @@ func CmdConsumerChains() *cobra.Command {
queryClient := types.NewQueryClient(clientCtx)

req := &types.QueryConsumerChainsRequest{}

if args[0] != "" {
phase, err := strconv.ParseInt(args[0], 10, 32)
if err != nil {
return err
}
req.Phase = types.ConsumerPhase(phase)
Fixed Show fixed Hide fixed
}
Comment on lines +94 to +100
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix potential index out of range error.

Accessing args[0] without checking its presence can lead to an index out of range error. Add a check to ensure the argument is present before accessing it.

- if args[0] != "" {
+ if len(args) > 0 && args[0] != "" {

Apply this diff to fix the issue.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if args[0] != "" {
phase, err := strconv.ParseInt(args[0], 10, 32)
if err != nil {
return err
}
req.Phase = types.ConsumerPhase(phase)
}
if len(args) > 0 && args[0] != "" {
phase, err := strconv.ParseInt(args[0], 10, 32)
if err != nil {
return err
}
req.Phase = types.ConsumerPhase(phase)
}


if args[1] != "" {
limit, err := strconv.ParseInt(args[1], 10, 32)
if err != nil {
return err
}
req.Limit = int32(limit)
Fixed Show fixed Hide fixed
}
Comment on lines +102 to +108
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix potential index out of range error.

Accessing args[1] without checking its presence can lead to an index out of range error. Add a check to ensure the argument is present before accessing it.

- if args[1] != "" {
+ if len(args) > 1 && args[1] != "" {

Apply this diff to fix the issue.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if args[1] != "" {
limit, err := strconv.ParseInt(args[1], 10, 32)
if err != nil {
return err
}
req.Limit = int32(limit)
}
if len(args) > 1 && args[1] != "" {
limit, err := strconv.ParseInt(args[1], 10, 32)
if err != nil {
return err
}
req.Limit = int32(limit)
}


res, err := queryClient.QueryConsumerChains(cmd.Context(), req)
if err != nil {
return err
Expand Down
74 changes: 58 additions & 16 deletions x/ccv/provider/keeper/grpc_query.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,15 @@ import (
"context"
"fmt"
"sort"
"strconv"

"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"

errorsmod "cosmossdk.io/errors"

"cosmossdk.io/math"
sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/cosmos/interchain-security/v5/x/ccv/provider/types"
ccvtypes "github.com/cosmos/interchain-security/v5/x/ccv/types"
)
Expand Down Expand Up @@ -51,13 +52,51 @@ func (k Keeper) QueryConsumerChains(goCtx context.Context, req *types.QueryConsu

ctx := sdk.UnwrapSDKContext(goCtx)

chains := []*types.Chain{}
for _, chainID := range k.GetAllRegisteredConsumerIds(ctx) {
c, err := k.GetConsumerChain(ctx, chainID)
consumerIds := []string{}
phaseFilter := req.Phase

// if the phase filter is set Launched get consumer from the state directly
if phaseFilter == types.ConsumerPhase_CONSUMER_PHASE_LAUNCHED {
consumerIds = append(consumerIds, k.GetAllRegisteredConsumerIds(ctx)...)
// otherwise iterate over all the consumer using the last unused consumer Id
} else {
firstUnusedConsumerId, ok := k.GetConsumerId(ctx)
if !ok {
return &types.QueryConsumerChainsResponse{}, nil
}
for i := uint64(0); i < firstUnusedConsumerId; i++ {
// if the phase filter is set, verify that the consumer has the same phase
if phaseFilter != types.ConsumerPhase_CONSUMER_PHASE_UNSPECIFIED {
p := k.GetConsumerPhase(ctx, strconv.FormatInt(int64(i), 10))
if p == types.ConsumerPhase_CONSUMER_PHASE_UNSPECIFIED {
return nil, status.Error(codes.Internal, fmt.Sprintf("cannot retrieve phase for consumer id: %d", i))
}
if p != phaseFilter {
continue
}
}

consumerIds = append(consumerIds, strconv.FormatInt(int64(i), 10))
}
}

// set limit to default value
limit := 100
if req.Limit != 0 {
// update limit if specified
limit = int(req.Limit)
}

chains := make([]*types.Chain, math.Min(len(consumerIds), limit))
for i, cID := range consumerIds {
if i == limit {
break
}
c, err := k.GetConsumerChain(ctx, cID)
if err != nil {
return nil, status.Error(codes.Internal, err.Error())
}
chains = append(chains, &c)
chains[i] = &c
}

return &types.QueryConsumerChainsResponse{Chains: chains}, nil
Expand All @@ -70,11 +109,7 @@ func (k Keeper) GetConsumerChain(ctx sdk.Context, consumerId string) (types.Chai
return types.Chain{}, fmt.Errorf("cannot find chainID for consumer (%s)", consumerId)
}

clientID, found := k.GetConsumerClientId(ctx, consumerId)
if !found {
return types.Chain{}, fmt.Errorf("cannot find clientID for consumer (%s)", consumerId)
}

clientID, _ := k.GetConsumerClientId(ctx, consumerId)
topN := k.GetTopN(ctx, consumerId)

// Get the minimal power in the top N for the consumer chain
Expand All @@ -84,9 +119,10 @@ func (k Keeper) GetConsumerChain(ctx sdk.Context, consumerId string) (types.Chai
minPowerInTopN = -1
}

validatorSetCap := k.GetValidatorSetCap(ctx, consumerId)

validatorsPowerCap := k.GetValidatorsPowerCap(ctx, consumerId)
phase := k.GetConsumerPhase(ctx, consumerId)
if phase == types.ConsumerPhase_CONSUMER_PHASE_UNSPECIFIED {
return types.Chain{}, fmt.Errorf("cannot find phase for consumer (%s)", consumerId)
}

allowlist := k.GetAllowList(ctx, consumerId)
strAllowlist := make([]string, len(allowlist))
Expand All @@ -100,19 +136,25 @@ func (k Keeper) GetConsumerChain(ctx sdk.Context, consumerId string) (types.Chai
strDenylist[i] = addr.String()
}

allowInactiveVals := k.AllowsInactiveValidators(ctx, consumerId)
metadata, err := k.GetConsumerMetadata(ctx, consumerId)
if err != nil {
return types.Chain{}, fmt.Errorf("cannot get metadata for consumer (%s): %w", consumerId, err)
}

allowInactiveVals := k.AllowsInactiveValidators(ctx, consumerId)
minStake := k.GetMinStake(ctx, consumerId)

return types.Chain{
ChainId: chainID,
ClientId: clientID,
Top_N: topN,
MinPowerInTop_N: minPowerInTopN,
ValidatorSetCap: validatorSetCap,
ValidatorsPowerCap: validatorsPowerCap,
ValidatorSetCap: k.GetValidatorSetCap(ctx, consumerId),
ValidatorsPowerCap: k.GetValidatorsPowerCap(ctx, consumerId),
Allowlist: strAllowlist,
Denylist: strDenylist,
Phase: phase,
Metadata: metadata,
AllowInactiveVals: allowInactiveVals,
MinStake: minStake,
}, nil
Expand Down
129 changes: 127 additions & 2 deletions x/ccv/provider/keeper/grpc_query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ import (
"bytes"
"fmt"
"sort"
"strconv"
"testing"
"time"

"github.com/golang/mock/gomock"
"github.com/stretchr/testify/require"
Expand All @@ -18,6 +20,7 @@ import (

cryptotestutil "github.com/cosmos/interchain-security/v5/testutil/crypto"
testkeeper "github.com/cosmos/interchain-security/v5/testutil/keeper"
"github.com/cosmos/interchain-security/v5/x/ccv/provider/keeper"
"github.com/cosmos/interchain-security/v5/x/ccv/provider/types"
ccvtypes "github.com/cosmos/interchain-security/v5/x/ccv/types"
)
Expand Down Expand Up @@ -427,6 +430,8 @@ func TestGetConsumerChain(t *testing.T) {
math.NewInt(300),
}

metadataLists := []types.ConsumerMetadata{}

expectedGetAllOrder := []types.Chain{}
for i, consumerID := range consumerIDs {
pk.SetConsumerChainId(ctx, consumerID, chainIDs[i])
Expand Down Expand Up @@ -455,6 +460,12 @@ func TestGetConsumerChain(t *testing.T) {
strDenylist[j] = addr.String()
}

metadataLists = append(metadataLists, types.ConsumerMetadata{Name: chainIDs[i]})
pk.SetConsumerMetadata(ctx, consumerID, metadataLists[i])

phase := types.ConsumerPhase(int32(i + 1))
pk.SetConsumerPhase(ctx, consumerID, phase)

expectedGetAllOrder = append(expectedGetAllOrder,
types.Chain{
ChainId: chainIDs[i],
Expand All @@ -465,13 +476,15 @@ func TestGetConsumerChain(t *testing.T) {
ValidatorsPowerCap: validatorPowerCaps[i],
Allowlist: strAllowlist,
Denylist: strDenylist,
Phase: phase,
Metadata: metadataLists[i],
AllowInactiveVals: allowInactiveVals[i],
MinStake: minStakes[i].Uint64(),
})
}

for i, chainID := range pk.GetAllActiveConsumerIds(ctx) {
c, err := pk.GetConsumerChain(ctx, chainID)
for i, cId := range consumerIDs {
c, err := pk.GetConsumerChain(ctx, cId)
require.NoError(t, err)
require.Equal(t, expectedGetAllOrder[i], c)
}
Expand Down Expand Up @@ -562,3 +575,115 @@ func TestQueryConsumerIdFromClientId(t *testing.T) {
require.NoError(t, err)
require.Equal(t, expectedConsumerId, res.ConsumerId)
}

func TestQueryConsumerChains(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename to TestQueryConsumerChains and remove redundant comment.

Consider renaming the function to TestQueryConsumerChains and removing any redundant comments.

pk, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
defer ctrl.Finish()

consumerNum := 4
consumerIds := make([]string, consumerNum)
consumers := make([]*types.Chain, consumerNum)

// expect no error and returned chains
res, err := pk.QueryConsumerChains(ctx, &types.QueryConsumerChainsRequest{})
require.NoError(t, err)
require.Len(t, res.Chains, 0)

// create four consumer chains in different phase
for i := 0; i < consumerNum; i++ {
cID := pk.FetchAndIncrementConsumerId(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Clarify the reason for skipping the first consumer ID.

The reason for skipping the first consumer ID is not clear. Consider adding a comment to explain this logic.

chainID := "consumer-" + strconv.Itoa(i)
c := types.Chain{
ChainId: chainID,
MinPowerInTop_N: -1,
ValidatorsPowerCap: 0,
ValidatorSetCap: 0,
Allowlist: []string{},
Denylist: []string{},
Phase: types.ConsumerPhase(i + 1),
Metadata: types.ConsumerMetadata{Name: chainID},
}
pk.SetConsumerPhase(ctx, cID, c.Phase)
pk.SetConsumerMetadata(ctx, cID, c.Metadata)
pk.SetConsumerChainId(ctx, cID, chainID)

consumerIds[i] = cID
consumers[i] = &c
}

testCases := []struct {
name string
setup func(ctx sdk.Context, pk keeper.Keeper)
phase_filter types.ConsumerPhase
limit int32
expConsumers []*types.Chain
}{
{
name: "expect all consumers when phase filter isn't set",
setup: func(ctx sdk.Context, pk keeper.Keeper) {},
expConsumers: consumers,
},
{
name: "expect an amount of consumer equal to the limit",
setup: func(ctx sdk.Context, pk keeper.Keeper) {},
expConsumers: consumers[:3],
limit: int32(3),
},
{
name: "expect registered consumers when phase filter is set to Registered",
setup: func(ctx sdk.Context, pk keeper.Keeper) {
consumers[0].Phase = types.ConsumerPhase_CONSUMER_PHASE_REGISTERED
pk.SetConsumerPhase(ctx, consumerIds[0], types.ConsumerPhase_CONSUMER_PHASE_REGISTERED)
},
phase_filter: types.ConsumerPhase_CONSUMER_PHASE_REGISTERED,
expConsumers: consumers[0:1],
},
{
name: "expect initialized consumers when phase is set to Initialized",
setup: func(ctx sdk.Context, pk keeper.Keeper) {
consumers[1].Phase = types.ConsumerPhase_CONSUMER_PHASE_INITIALIZED
err := pk.AppendConsumerToBeLaunchedOnSpawnTime(ctx, consumerIds[1], time.Now())
require.NoError(t, err)
pk.SetConsumerPhase(ctx, consumerIds[1], types.ConsumerPhase_CONSUMER_PHASE_INITIALIZED)
},
phase_filter: types.ConsumerPhase_CONSUMER_PHASE_INITIALIZED,
expConsumers: consumers[1:2],
},
{
name: "expect launched consumers when phase is set to Launched",
setup: func(ctx sdk.Context, pk keeper.Keeper) {
consumers[2].Phase = types.ConsumerPhase_CONSUMER_PHASE_LAUNCHED
consumers[2].ClientId = "ClientID"
pk.SetConsumerClientId(ctx, consumerIds[2], consumers[2].ClientId)
pk.SetConsumerPhase(ctx, consumerIds[2], types.ConsumerPhase_CONSUMER_PHASE_LAUNCHED)
},
phase_filter: types.ConsumerPhase_CONSUMER_PHASE_LAUNCHED,
expConsumers: consumers[2:3],
},
{
name: "expect stopped consumers when phase is set to Stopped",
setup: func(ctx sdk.Context, pk keeper.Keeper) {
consumers[3].Phase = types.ConsumerPhase_CONSUMER_PHASE_STOPPED
pk.SetConsumerPhase(ctx, consumerIds[3], types.ConsumerPhase_CONSUMER_PHASE_STOPPED)
},
phase_filter: types.ConsumerPhase_CONSUMER_PHASE_STOPPED,
expConsumers: consumers[3:],
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
tc.setup(ctx, pk)
req := types.QueryConsumerChainsRequest{
Phase: tc.phase_filter,
Limit: tc.limit,
}
expectedResponse := types.QueryConsumerChainsResponse{
Chains: tc.expConsumers,
}
res, err := pk.QueryConsumerChains(ctx, &req)
require.NoError(t, err)
require.Equal(t, &expectedResponse, res)
})
}
}
3 changes: 2 additions & 1 deletion x/ccv/provider/keeper/hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@ package keeper

import (
"context"
"cosmossdk.io/math"
"fmt"

"cosmossdk.io/math"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkgov "github.com/cosmos/cosmos-sdk/x/gov/types"
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
Expand Down
Loading
Loading