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.

Sign up for GitHub

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

Jump to bottom

fix!: update consumer chain queries #2246

Merged
merged 3 commits into from
Sep 11, 2024
Merged

fix!: update consumer chain queries #2246

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5b8d0f0
fix!: update consumer chain queries
MSalopek Sep 10, 2024
d040bc1
apply review comments
MSalopek Sep 10, 2024
a771293
fix err handling and gosec reports
MSalopek Sep 11, 2024
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
26 changes: 15 additions & 11 deletions proto/interchain_security/ccv/provider/v1/query.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import "tendermint/crypto/keys.proto";
import "cosmos_proto/cosmos.proto";
import "cosmos/staking/v1beta1/staking.proto";
import "cosmos/base/query/v1beta1/pagination.proto";

service Query {
// ConsumerGenesis queries the genesis state needed to start a consumer chain
Expand All @@ -28,7 +29,7 @@
rpc QueryConsumerChains(QueryConsumerChainsRequest)
returns (QueryConsumerChainsResponse) {
option (google.api.http).get =
"/interchain_security/ccv/provider/consumer_chains/{phase}/{limit}";
"/interchain_security/ccv/provider/consumer_chains/{phase}";
}

// QueryValidatorConsumerAddr queries the address
Expand Down Expand Up @@ -158,12 +159,14 @@
// The phase of the consumer chains returned (optional)
// Registered=1|Initialized=2|Launched=3|Stopped=4|Deleted=5
ConsumerPhase phase = 1;
// The limit of consumer chains returned (optional)
// default is 100
int32 limit = 2;

cosmos.base.query.v1beta1.PageRequest pagination = 2;

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

View workflow job for this annotation

GitHub Actions / break-check 

Field "2" with name "pagination" on message "QueryConsumerChainsRequest" changed cardinality from "optional with implicit presence" to "optional with explicit presence".

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

View workflow job for this annotation

GitHub Actions / break-check 

Field "2" with name "pagination" on message "QueryConsumerChainsRequest" changed option "json_name" from "limit" to "pagination".

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

View workflow job for this annotation

GitHub Actions / break-check 

Field "2" with name "pagination" on message "QueryConsumerChainsRequest" changed type from "int32" to "message".

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

View workflow job for this annotation

GitHub Actions / break-check 

Field "2" on message "QueryConsumerChainsRequest" changed name from "limit" to "pagination".
}

message QueryConsumerChainsResponse { repeated Chain chains = 1; }
message QueryConsumerChainsResponse {
repeated Chain chains = 1;
cosmos.base.query.v1beta1.PageResponse pagination = 2;
}

message Chain {
string chain_id = 1;
Expand Down Expand Up @@ -372,10 +375,11 @@
}

message QueryConsumerChainResponse {
string chain_id = 1;
string owner_address = 2;
string phase = 3;
ConsumerMetadata metadata = 4 [ (gogoproto.nullable) = false ];
ConsumerInitializationParameters init_params = 5;
PowerShapingParameters power_shaping_params = 6;
string consumer_id = 1;

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

View workflow job for this annotation

GitHub Actions / break-check 

Field "1" with name "consumer_id" on message "QueryConsumerChainResponse" changed option "json_name" from "chainId" to "consumerId".

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

View workflow job for this annotation

GitHub Actions / break-check 

Field "1" on message "QueryConsumerChainResponse" changed name from "chain_id" to "consumer_id".
Copy link
Contributor

Choose a reason for hiding this comment

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

The consumer_id seems to be part of the QueryConsumerChainRequest. If so, what's the point of returning it as part the response?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that we can validate on the FE that we got the chain_id for the consumer_id we requested. But, more importantly so that we don't have to keep extra context (e.g coming from URL params) about which chain we're dealing with.

E.g. previously we had to read the URL param for consumer_id for any operation we were doing:

forge/consumer-id/123

So we had to "remember" the 123. This way we look up an object and treat is as the source of truth and do all operations required by fetching the consumer_id from the returned object.

Does that make sense?

string chain_id = 2;

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

View workflow job for this annotation

GitHub Actions / break-check 

Field "2" with name "chain_id" on message "QueryConsumerChainResponse" changed option "json_name" from "ownerAddress" to "chainId".

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

View workflow job for this annotation

GitHub Actions / break-check 

Field "2" on message "QueryConsumerChainResponse" changed name from "owner_address" to "chain_id".
string owner_address = 3;

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

View workflow job for this annotation

GitHub Actions / break-check 

Field "3" with name "owner_address" on message "QueryConsumerChainResponse" changed option "json_name" from "phase" to "ownerAddress".

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

View workflow job for this annotation

GitHub Actions / break-check 

Field "3" on message "QueryConsumerChainResponse" changed name from "phase" to "owner_address".
string phase = 4;
ConsumerMetadata metadata = 5 [ (gogoproto.nullable) = false ];
ConsumerInitializationParameters init_params = 6;
PowerShapingParameters power_shaping_params = 7;
}
2 changes: 1 addition & 1 deletion x/ccv/provider/client/cli/query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ func CmdConsumerChains() *cobra.Command {
if err != nil {
return err
}
req.Limit = int32(limit)
req.Pagination.Limit = uint64(limit)
}

res, err := queryClient.QueryConsumerChains(cmd.Context(), req)
Expand Down
48 changes: 25 additions & 23 deletions x/ccv/provider/keeper/grpc_query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,18 @@ package keeper
import (
"bytes"
"context"
"encoding/binary"
"fmt"
"sort"

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

errorsmod "cosmossdk.io/errors"
"cosmossdk.io/store/prefix"

sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/query"

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

ctx := sdk.UnwrapSDKContext(goCtx)
var chains []*types.Chain

consumerIds := []string{}
for _, consumerID := range k.GetAllConsumerIds(ctx) {
phase := k.GetConsumerPhase(ctx, consumerID)
if req.Phase != types.CONSUMER_PHASE_UNSPECIFIED && req.Phase != phase {
// ignore consumer chain
continue
store := ctx.KVStore(k.storeKey)
storePrefix := types.ConsumerIdToPhaseKeyPrefix()
consumerPhaseStore := prefix.NewStore(store, []byte{storePrefix})
pageRes, err := query.Paginate(consumerPhaseStore, req.Pagination, func(key, value []byte) error {
consumerId, err := types.ParseStringIdWithLenKey(storePrefix, append([]byte{storePrefix}, key...))
if err != nil {
status.Error(codes.Internal, err.Error())
}
consumerIds = append(consumerIds, consumerID)
}

// set limit to default value
limit := 100
if req.Limit != 0 {
// update limit if specified
limit = int(req.Limit)
}
if len(consumerIds) > limit {
consumerIds = consumerIds[:limit]
}
phase := types.ConsumerPhase(binary.BigEndian.Uint32(value))
if req.Phase != types.CONSUMER_PHASE_UNSPECIFIED && req.Phase != phase {
return nil
}

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

if err != nil {
return nil, status.Error(codes.Internal, err.Error())
}

return &types.QueryConsumerChainsResponse{Chains: chains}, nil
return &types.QueryConsumerChainsResponse{Chains: chains, Pagination: pageRes}, nil
}

// GetConsumerChain returns a Chain data structure with all the necessary fields
Expand Down Expand Up @@ -588,6 +589,7 @@ func (k Keeper) QueryConsumerChain(goCtx context.Context, req *types.QueryConsum

return &types.QueryConsumerChainResponse{
ChainId: chainId,
ConsumerId: consumerId,
OwnerAddress: ownerAddress,
Phase: phase.String(),
Metadata: metadata,
Expand Down
15 changes: 11 additions & 4 deletions x/ccv/provider/keeper/grpc_query_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (

"github.com/cometbft/cometbft/proto/tendermint/crypto"

sdkquery "github.com/cosmos/cosmos-sdk/types/query"
cryptotestutil "github.com/cosmos/interchain-security/v6/testutil/crypto"
testkeeper "github.com/cosmos/interchain-security/v6/testutil/keeper"
"github.com/cosmos/interchain-security/v6/x/ccv/provider/keeper"
Expand Down Expand Up @@ -558,6 +559,7 @@ func TestQueryConsumerChain(t *testing.T) {

expRes := types.QueryConsumerChainResponse{
ChainId: chainId,
ConsumerId: consumerId,
OwnerAddress: providerKeeper.GetAuthority(),
Metadata: types.ConsumerMetadata{Name: chainId},
Phase: types.CONSUMER_PHASE_REGISTERED.String(),
Expand Down Expand Up @@ -663,7 +665,7 @@ func TestQueryConsumerChains(t *testing.T) {
name string
setup func(ctx sdk.Context, pk keeper.Keeper)
phase_filter types.ConsumerPhase
limit int32
limit uint64
expConsumers []*types.Chain
}{
{
Expand All @@ -675,7 +677,7 @@ func TestQueryConsumerChains(t *testing.T) {
name: "expect an amount of consumer equal to the limit",
setup: func(ctx sdk.Context, pk keeper.Keeper) {},
expConsumers: consumers[:3],
limit: int32(3),
limit: 3,
},
{
name: "expect registered consumers when phase filter is set to Registered",
Expand Down Expand Up @@ -720,14 +722,19 @@ func TestQueryConsumerChains(t *testing.T) {
tc.setup(ctx, pk)
req := types.QueryConsumerChainsRequest{
Phase: tc.phase_filter,
Limit: tc.limit,
Pagination: &sdkquery.PageRequest{
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we test this, but would be nice to have a test that uses the PageResponse to construct the new PageRequest and verify that everything works as expected.

Limit: tc.limit,
},
}
expectedResponse := types.QueryConsumerChainsResponse{
Chains: tc.expConsumers,
}
res, err := pk.QueryConsumerChains(ctx, &req)
require.NoError(t, err)
require.Equal(t, &expectedResponse, res, tc.name)
require.Equal(t, expectedResponse.GetChains(), res.GetChains(), tc.name)
if tc.limit != 0 {
require.Len(t, res.GetChains(), int(tc.limit), tc.name)
}
})
}
}
9 changes: 7 additions & 2 deletions x/ccv/provider/types/keys.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -685,6 +685,11 @@ func ConsumerIdToPhaseKey(consumerId string) []byte {
return StringIdWithLenKey(mustGetKeyPrefix(ConsumerIdToPhaseKeyName), consumerId)
}

// ConsumerIdToPhaseKeyPrefix returns the key prefix used to iterate over all the consumer ids and their phases.
Copy link
Contributor

Choose a reason for hiding this comment

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

In line 685 (above) do return return StringIdWithLenKey(ConsumerIdToPhaseKeyPrefix(), consumerId) instead of return StringIdWithLenKey(mustGetKeyPrefix(ConsumerIdToPhaseKeyName), consumerId).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't get it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got it

Copy link
Contributor

Choose a reason for hiding this comment

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

See @mpoke comment here:

mustGetKeyPrefix(ConsumerIdToRegistrationRecordKeyName). The idea is to have only one method calling mustGetKeyPrefix for a given key and for all these methods to be part of the UTs so that we make sure the panic in mustGetKeyPrefix is not triggered.

func ConsumerIdToPhaseKeyPrefix() byte {
return mustGetKeyPrefix(ConsumerIdToPhaseKeyName)
}

// ConsumerIdToRemovalTimeKeyPrefix returns the key prefix for storing the removal times of consumer chains
// that are about to be removed
func ConsumerIdToRemovalTimeKeyPrefix() byte {
Expand Down Expand Up @@ -813,8 +818,8 @@ func StringIdWithLenKey(prefix byte, stringId string) []byte {
func ParseStringIdWithLenKey(prefix byte, bz []byte) (string, error) {
expectedPrefix := []byte{prefix}
prefixL := len(expectedPrefix)
if prefix := bz[:prefixL]; !bytes.Equal(prefix, expectedPrefix) {
return "", fmt.Errorf("invalid prefix; expected: %X, got: %X", expectedPrefix, prefix)
if prefixBz := bz[:prefixL]; !bytes.Equal(prefixBz, expectedPrefix) {
return "", fmt.Errorf("invalid prefix; expected: %X, got: %X, input %X -- len %d", expectedPrefix, prefixBz, prefix, prefixL)
}
stringIdL := sdk.BigEndianToUint64(bz[prefixL : prefixL+8])
stringId := string(bz[prefixL+8 : prefixL+8+int(stringIdL)])
Expand Down
Loading
Loading