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

feat: support multiple chain ids in zoneconcierge chains info api #362

Merged
merged 21 commits into from
Apr 28, 2023
Merged

feat: support multiple chain ids in zoneconcierge chains info api #362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
cfd10a0
new proto files
gusin13 Apr 25, 2023
e3e9913
accept repeated str
gusin13 Apr 25, 2023
0e563f2
add query cmd
gusin13 Apr 25, 2023
b076f45
logic
gusin13 Apr 25, 2023
5b43df7
swag file
gusin13 Apr 25, 2023
f5121d4
add cli query for /babylon/zoneconcierge/v1/chain_info/{chain_id}
gusin13 Apr 26, 2023
c058d25
extend chain info api
gusin13 Apr 26, 2023
08d18f1
update swag
gusin13 Apr 26, 2023
e1ee110
typo
gusin13 Apr 26, 2023
2be6a46
remove from e2e dir
gusin13 Apr 26, 2023
f9cf919
update docstring
gusin13 Apr 26, 2023
fba823f
check if chain id empty
gusin13 Apr 26, 2023
6af0216
make chain id slice in loop
gusin13 Apr 26, 2023
7fcfa35
ctx suggestion
gusin13 Apr 27, 2023
70dead9
don't perform any query if chain empty
gusin13 Apr 27, 2023
65f945b
arb args
gusin13 Apr 27, 2023
8da5b99
add check for max query and dups
gusin13 Apr 27, 2023
e36eaf6
improve comments
gusin13 Apr 27, 2023
fa511e9
add e2e test
gusin13 Apr 27, 2023
beecb9e
update protover
gusin13 Apr 27, 2023
39a9297
revert back to 12.0
gusin13 Apr 27, 2023
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
2,397 changes: 1,015 additions & 1,382 deletions client/docs/swagger-ui/swagger.yaml
View file
Open in desktop

Large diffs are not rendered by default.

15 changes: 7 additions & 8 deletions proto/babylon/zoneconcierge/v1/query.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ service Query {
option (google.api.http).get = "/babylon/zoneconcierge/v1/chains";
}
// ChainInfo queries the latest info of a chain in Babylon's view
rpc ChainInfo(QueryChainInfoRequest) returns (QueryChainInfoResponse) {
rpc ChainsInfo(QueryChainsInfoRequest) returns (QueryChainsInfoResponse) {
option (google.api.http).get =
"/babylon/zoneconcierge/v1/chain_info/{chain_id}";
"/babylon/zoneconcierge/v1/chains_info";
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to take a list of strings via the URL? Tbh this api is helpful for manually checking the liveness of checkpoints

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I think it's already being passed that way in current setup. So the GET call would translate to /babylon/zoneconcierge/v1/chains_info?chain_ids=1&chain_ids=2

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 thought about this again, I couldn't test with multiple chain ids so I am not very clear if protobuf repeated strings would translate to HTTP get query params or body.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I could not think of any straightforward way either, nor could I find any practice for such APIs. So probably let's stick to your current change

}
// EpochChainInfo queries the latest info of a chain in a given epoch of
// Babylon's view
Expand Down Expand Up @@ -88,13 +88,12 @@ message QueryChainListResponse {
cosmos.base.query.v1beta1.PageResponse pagination = 2;
}

// QueryChainInfoRequest is request type for the Query/ChainInfo RPC method.
message QueryChainInfoRequest { string chain_id = 1; }
// QueryChainsInfoRequest is request type for the Query/ChainsInfo RPC method.
message QueryChainsInfoRequest { repeated string chain_ids = 1; }

// QueryChainInfoResponse is response type for the Query/ChainInfo RPC method.
message QueryChainInfoResponse {
// chain_info is the info of the CZ
babylon.zoneconcierge.v1.ChainInfo chain_info = 1;
// QueryChainsInfoResponse is response type for the Query/ChainsInfo RPC method.
message QueryChainsInfoResponse {
repeated babylon.zoneconcierge.v1.ChainInfo chains_info = 1;
}

// QueryEpochChainInfoRequest is request type for the Query/EpochChainInfo RPC
Expand Down
9 changes: 4 additions & 5 deletions test/e2e/configurer/chain/queries.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,15 +209,14 @@ func (n *NodeConfig) QueryCheckpointChains() (*[]string, error) {
return &chainsResponse.ChainIds, nil
}

func (n *NodeConfig) QueryCheckpointChainInfo(chainId string) (*zctypes.ChainInfo, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this method removed?

Copy link
Contributor Author

@gusin13 gusin13 Apr 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I couldn't find its usage in any of the existing e2e tests so removed it.

@KonradStaniec I am not sure how to run e2e tests properly locally.
I was getting this warning: no tests to run. Can you guide how to setup and add a new test in the suite?

Copy link
Contributor

Choose a reason for hiding this comment

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

so to run tests:

  1. first build docker container on branch with make build-docker
  2. then run make test-e2e
    that should be it.

To add new tests, proper place is e2e_test.go file. One thing to note is that tests there are stateful i.e one needs to be aware what what is current state after previous tests.

Copy link
Contributor Author

@gusin13 gusin13 Apr 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @KonradStaniec, I have added a relevant assertion in the TestIbcCheckpointing test now.

infoPath := fmt.Sprintf("/babylon/zoneconcierge/v1/chain_info/%s", chainId)
bz, err := n.QueryGRPCGateway(infoPath)
func (n *NodeConfig) QueryCheckpointChainsInfo(chainID string) ([]*zctypes.ChainInfo, error) {
bz, err := n.QueryGRPCGateway("/babylon/zoneconcierge/v1/chains_info", "chain_ids", chainID)
require.NoError(n.t, err)
var infoResponse zctypes.QueryChainInfoResponse
var infoResponse zctypes.QueryChainsInfoResponse
if err := util.Cdc.UnmarshalJSON(bz, &infoResponse); err != nil {
return nil, err
}
return infoResponse.ChainInfo, nil
return infoResponse.ChainsInfo, nil
}

func (n *NodeConfig) QueryCurrentEpoch() (uint64, error) {
Expand Down
5 changes: 5 additions & 0 deletions test/e2e/e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ func (s *IntegrationTestSuite) TestIbcCheckpointing() {
nonValidatorNode, err := chainA.GetNodeAtIndex(2)
s.NoError(err)

// Query checkpoint chain info for opposing chain
chainInfo, err := nonValidatorNode.QueryCheckpointChainsInfo(initialization.ChainBID)
s.NoError(err)
s.Equal(chainInfo[0].ChainId, initialization.ChainBID)

// Finalize epoch 1,2,3 , as first headers of opposing chain are in epoch 3
nonValidatorNode.FinalizeSealedEpochs(1, 3)

Expand Down
27 changes: 24 additions & 3 deletions x/zoneconcierge/client/cli/query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,11 @@ package cli

import (
"fmt"
// "strings"

"github.com/cosmos/cosmos-sdk/client/flags"
"github.com/spf13/cobra"

"github.com/cosmos/cosmos-sdk/client"
// "github.com/cosmos/cosmos-sdk/client/flags"
// sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/babylonchain/babylon/x/zoneconcierge/types"
)
Expand All @@ -24,5 +22,28 @@ func GetQueryCmd(queryRoute string) *cobra.Command {
RunE: client.ValidateCmd,
}

cmd.AddCommand(CmdChainsInfo())
return cmd
}

func CmdChainsInfo() *cobra.Command {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding this command!

cmd := &cobra.Command{
Use: "chains-info <chain-ids>",
Short: "retrieves the latest info for a list of chains with given IDs",
Args: cobra.ArbitraryArgs,
RunE: func(cmd *cobra.Command, args []string) error {
clientCtx := client.GetClientContextFromCmd(cmd)
queryClient := types.NewQueryClient(clientCtx)
req := types.QueryChainsInfoRequest{ChainIds: args}
resp, err := queryClient.ChainsInfo(cmd.Context(), &req)
if err != nil {
return err
}

return clientCtx.PrintProto(resp)
},
}

flags.AddQueryFlagsToCmd(cmd)
return cmd
}
45 changes: 35 additions & 10 deletions x/zoneconcierge/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 (
"context"

"github.com/babylonchain/babylon/x/zoneconcierge/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/query"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"

"github.com/babylonchain/babylon/x/zoneconcierge/types"
)

var _ types.QueryServer = Keeper{}

const maxQueryChainsInfoLimit = 100

func (k Keeper) ChainList(c context.Context, req *types.QueryChainListRequest) (*types.QueryChainListResponse, error) {
if req == nil {
return nil, status.Error(codes.InvalidArgument, "invalid request")
Expand All @@ -37,24 +40,46 @@ func (k Keeper) ChainList(c context.Context, req *types.QueryChainListRequest) (
return resp, nil
}

// ChainInfo returns the latest info of a chain with given ID
func (k Keeper) ChainInfo(c context.Context, req *types.QueryChainInfoRequest) (*types.QueryChainInfoResponse, error) {
// ChainsInfo returns the latest info for a list of chains with given IDs
func (k Keeper) ChainsInfo(c context.Context, req *types.QueryChainsInfoRequest) (*types.QueryChainsInfoResponse, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually requests which returns lists should be paginated. Here it is a bit tricky as we provide list of chain-ids and return list of chain info responses. In perfect world we would need to put some kinda of limit on number of chain-ids requested and tie it somehow to pagination. The limit is needed to avoid someone querying node for 1M of chain-ids, and exhausting node memory when it will try to read 1M ChainInfo objects into memory.

Maybe for not just put limit in place ? (50 or 100 chain ids ?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, good point. I have hardcoded a max limit of 100 now.

if req == nil {
return nil, status.Error(codes.InvalidArgument, "invalid request")
}

if len(req.ChainId) == 0 {
return nil, status.Error(codes.InvalidArgument, "chain ID cannot be empty")
if len(req.ChainIds) == 0 {
return nil, status.Error(codes.InvalidArgument, "chain IDs cannot be empty")
}

if len(req.ChainIds) > maxQueryChainsInfoLimit {
return nil, status.Errorf(codes.InvalidArgument, "cannot query more than %d chains", maxQueryChainsInfoLimit)
}

encountered := map[string]bool{}
for _, chainID := range req.ChainIds {
if len(chainID) == 0 {
return nil, status.Error(codes.InvalidArgument, "chain ID cannot be empty")
}

// check for duplicates and return error on first duplicate found
if encountered[chainID] {
return nil, status.Errorf(codes.InvalidArgument, "duplicate chain ID %s", chainID)
} else {
encountered[chainID] = true
}
}

ctx := sdk.UnwrapSDKContext(c)
var chainsInfo []*types.ChainInfo
for _, chainID := range req.ChainIds {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe also check if a chain ID is an empty string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, fixed.

Copy link
Member

Choose a reason for hiding this comment

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

Could we move the check after L49? so that as long as there is an empty chain ID then no query will be performed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, fixed.

chainInfo, err := k.GetChainInfo(ctx, chainID)
Copy link
Contributor

Choose a reason for hiding this comment

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

we are missing handling of the case when caller puts duplicated chain-id in the request list, imo it makes no sense to return duplicated chain-info results.

There two ways on handling that:

  1. either deduplicate chain-ids when recveing requrest
  2. or return error in case of first duplicate happening.

I think I would go for 2 as then usually duplicates in such case are callers error, and it would be good to inform caller about the error which happened.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, sure I have implemented 2 now.

if err != nil {
return nil, err
}

// find the chain info of this epoch
chainInfo, err := k.GetChainInfo(ctx, req.ChainId)
if err != nil {
return nil, err
chainsInfo = append(chainsInfo, chainInfo)
}
resp := &types.QueryChainInfoResponse{ChainInfo: chainInfo}

resp := &types.QueryChainsInfoResponse{ChainsInfo: chainsInfo}
return resp, nil
}

Expand Down
56 changes: 40 additions & 16 deletions x/zoneconcierge/keeper/grpc_query_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,19 @@ import (
"math/rand"
"testing"

"github.com/babylonchain/babylon/testutil/datagen"
testkeeper "github.com/babylonchain/babylon/testutil/keeper"
btcctypes "github.com/babylonchain/babylon/x/btccheckpoint/types"
checkpointingtypes "github.com/babylonchain/babylon/x/checkpointing/types"
zctypes "github.com/babylonchain/babylon/x/zoneconcierge/types"
tmcrypto "github.com/cometbft/cometbft/proto/tendermint/crypto"
tmrpctypes "github.com/cometbft/cometbft/rpc/core/types"
tmtypes "github.com/cometbft/cometbft/types"
"github.com/cosmos/cosmos-sdk/types/query"
ibctmtypes "github.com/cosmos/ibc-go/v7/modules/light-clients/07-tendermint"
"github.com/golang/mock/gomock"
"github.com/stretchr/testify/require"

"github.com/babylonchain/babylon/testutil/datagen"
testkeeper "github.com/babylonchain/babylon/testutil/keeper"
btcctypes "github.com/babylonchain/babylon/x/btccheckpoint/types"
checkpointingtypes "github.com/babylonchain/babylon/x/checkpointing/types"
zctypes "github.com/babylonchain/babylon/x/zoneconcierge/types"
)

func FuzzChainList(f *testing.F) {
Expand Down Expand Up @@ -65,29 +66,52 @@ func FuzzChainList(f *testing.F) {
})
}

func FuzzChainInfo(f *testing.F) {
func FuzzChainsInfo(f *testing.F) {
datagen.AddRandomSeedsToFuzzer(f, 10)
type chainInfo struct {
chainID string
numHeaders uint64
numForkHeaders uint64
}

f.Fuzz(func(t *testing.T, seed int64) {
r := rand.New(rand.NewSource(seed))

_, babylonChain, czChain, babylonApp := SetupTest(t)
_, babylonChain, _, babylonApp := SetupTest(t)
zcKeeper := babylonApp.ZoneConciergeKeeper

ctx := babylonChain.GetContext()
hooks := zcKeeper.Hooks()

// invoke the hook a random number of times to simulate a random number of blocks
numHeaders := datagen.RandomInt(r, 100) + 1
numForkHeaders := datagen.RandomInt(r, 10) + 1
SimulateHeadersAndForksViaHook(ctx, r, hooks, czChain.ChainID, 0, numHeaders, numForkHeaders)
var (
chainsInfo []chainInfo
chainIDs []string
)
numChains := datagen.RandomInt(r, 100) + 1
for i := uint64(0); i < numChains; i++ {
chainID := datagen.GenRandomHexStr(r, 30)
numHeaders := datagen.RandomInt(r, 100) + 1
numForkHeaders := datagen.RandomInt(r, 10) + 1
SimulateHeadersAndForksViaHook(ctx, r, hooks, chainID, 0, numHeaders, numForkHeaders)

chainIDs = append(chainIDs, chainID)
chainsInfo = append(chainsInfo, chainInfo{
chainID: chainID,
numHeaders: numHeaders,
numForkHeaders: numForkHeaders,
})
}

// check if the chain info of is recorded or not
resp, err := zcKeeper.ChainInfo(ctx, &zctypes.QueryChainInfoRequest{ChainId: czChain.ChainID})
resp, err := zcKeeper.ChainsInfo(ctx, &zctypes.QueryChainsInfoRequest{
ChainIds: chainIDs,
})
require.NoError(t, err)
chainInfo := resp.ChainInfo
require.Equal(t, numHeaders-1, chainInfo.LatestHeader.Height)
require.Equal(t, numForkHeaders, uint64(len(chainInfo.LatestForks.Headers)))

for i, data := range resp.ChainsInfo {
require.Equal(t, chainsInfo[i].chainID, data.ChainId)
require.Equal(t, chainsInfo[i].numHeaders-1, data.LatestHeader.Height)
require.Equal(t, chainsInfo[i].numForkHeaders, uint64(len(data.LatestForks.Headers)))
}
})
}

Expand Down
Loading