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

gusin13 · 2023-04-25T11:38:53Z

Description

This PR is part of the optimizations being done to improve babylon-api performance. ChainInfo rpc query is being called multiple times by the explorer apis. I have replaced this method with ChainsInfo which will accept a list of chain ids and will output latest header info of the concerned cz's

Testing

Have added relevant fuzz tests and cli query

vitsalis

Nice!

vitsalis · 2023-04-26T15:12:35Z

test/e2e/configurer/chain/queries.go

@@ -209,17 +209,6 @@ func (n *NodeConfig) QueryCheckpointChains() (*[]string, error) {
 return &chainsResponse.ChainIds, nil
 }

-func (n *NodeConfig) QueryCheckpointChainInfo(chainId string) (*zctypes.ChainInfo, error) {


Why is this method removed?

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?

so to run tests:

first build docker container on branch with make build-docker

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.

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

x/zoneconcierge/keeper/grpc_query.go

vitsalis · 2023-04-26T15:14:43Z

x/zoneconcierge/keeper/grpc_query.go

 }

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


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

sure, fixed.

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

sure, fixed.

vitsalis · 2023-04-26T15:16:21Z

x/zoneconcierge/keeper/grpc_query_test.go

+ var chainIDs []string
+ for _, data := range chainsInfo {
+ chainIDs = append(chainIDs, data.chainID)
+ }


Maybe you could build the chainIDs list in the above for loop.

sure, fixed.

Co-authored-by: Vitalis Salis <VitSalis@gmail.com>

SebastianElvis

This would be a good optimisation! Just some minor comments

SebastianElvis · 2023-04-26T23:18:31Z

proto/babylon/zoneconcierge/v1/query.proto

 option (google.api.http).get =
- "/babylon/zoneconcierge/v1/chain_info/{chain_id}";
+ "/babylon/zoneconcierge/v1/chains_info";


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

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

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.

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

SebastianElvis · 2023-04-26T23:19:31Z

x/zoneconcierge/client/cli/query.go

+ cmd := &cobra.Command{
+ Use: "chains-info <chain-ids>",
+ Short: "retrieve the chain info of given chain ids",
+ Args: cobra.RangeArgs(1, 5),


Why do we put a limit of 5 here? maybe it's not needed

sure, have removed the limit now.

SebastianElvis · 2023-04-26T23:19:58Z

x/zoneconcierge/client/cli/query.go

+ return cmd
+}
+
+func CmdChainsInfo() *cobra.Command {


Thanks for adding this command!

x/zoneconcierge/client/cli/query.go

SebastianElvis · 2023-04-26T23:21:54Z

x/zoneconcierge/keeper/grpc_query.go

 }

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


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

KonradStaniec · 2023-04-27T04:19:55Z

x/zoneconcierge/keeper/grpc_query.go

-// 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) {


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 ?)

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

KonradStaniec · 2023-04-27T04:23:27Z

x/zoneconcierge/keeper/grpc_query.go

- chainInfo, err := k.GetChainInfo(ctx, req.ChainId)
- if err != nil {
- return nil, err
+ chainInfo, err := k.GetChainInfo(ctx, chainID)


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:

either deduplicate chain-ids when recveing requrest

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.

good catch, sure I have implemented 2 now.

Co-authored-by: Runchao Han <me@runchao.rocks>

gusin13 · 2023-04-27T09:49:51Z

@KonradStaniec @vitsalis @SebastianElvis Thanks for the reviews guys, I have addressed above comments. Please feel free to review again :)

KonradStaniec

Looks good !

At some point, we need to figure out how to best paginate those list queries so that caller have nice hints how to get next existing chain-ids, but at this point having limits should be enough 👍

vitsalis

Nice!

gusin13 force-pushed the cz-rpc-chains-info branch from 9513d26 to af12a80 Compare April 26, 2023 05:31

gusin13 changed the title ~~zoneconcierge api: chain info of multiple chain ids~~ feat: support multiple chain ids in zoneconcierge chains info endpoint Apr 26, 2023

gusin13 changed the title ~~feat: support multiple chain ids in zoneconcierge chains info endpoint~~ feat: zoneconcierge: support multiple chain ids in chains info endpoint Apr 26, 2023

gusin13 changed the title ~~feat: zoneconcierge: support multiple chain ids in chains info endpoint~~ feat: support multiple chain ids in zoneconcierge chains info api Apr 26, 2023

gusin13 added 9 commits April 26, 2023 18:41

new proto files

cfd10a0

accept repeated str

e3e9913

add query cmd

0e563f2

logic

b076f45

swag file

5b43df7

add cli query for /babylon/zoneconcierge/v1/chain_info/{chain_id}

f5121d4

extend chain info api

c058d25

update swag

08d18f1

typo

e1ee110

gusin13 force-pushed the cz-rpc-chains-info branch from 8fe040b to e1ee110 Compare April 26, 2023 13:13

remove from e2e dir

2be6a46

gusin13 marked this pull request as ready for review April 26, 2023 13:40

gusin13 requested review from SebastianElvis, KonradStaniec and vitsalis April 26, 2023 13:40

vitsalis reviewed Apr 26, 2023

View reviewed changes

gusin13 and others added 3 commits April 27, 2023 00:46

update docstring

f9cf919

Co-authored-by: Vitalis Salis <VitSalis@gmail.com>

check if chain id empty

fba823f

make chain id slice in loop

6af0216

SebastianElvis approved these changes Apr 26, 2023

View reviewed changes

KonradStaniec reviewed Apr 27, 2023

View reviewed changes

gusin13 and others added 5 commits April 27, 2023 10:54

ctx suggestion

7fcfa35

Co-authored-by: Runchao Han <me@runchao.rocks>

don't perform any query if chain empty

70dead9

arb args

65f945b

add check for max query and dups

8da5b99

improve comments

e36eaf6

gusin13 added 3 commits April 27, 2023 14:40

add e2e test

fa511e9

update protover

beecb9e

revert back to 12.0

39a9297

KonradStaniec approved these changes Apr 28, 2023

View reviewed changes

vitsalis approved these changes Apr 28, 2023

View reviewed changes

gusin13 merged commit 6f14271 into dev Apr 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gusin13 commented Apr 25, 2023 •

edited

Loading

vitsalis left a comment

vitsalis Apr 26, 2023

gusin13 Apr 26, 2023 •

edited

Loading

KonradStaniec Apr 27, 2023

gusin13 Apr 27, 2023 •

edited

Loading

vitsalis Apr 26, 2023

gusin13 Apr 26, 2023

SebastianElvis Apr 26, 2023

gusin13 Apr 27, 2023

vitsalis Apr 26, 2023

gusin13 Apr 26, 2023

SebastianElvis left a comment

SebastianElvis Apr 26, 2023

gusin13 Apr 27, 2023

gusin13 Apr 27, 2023

SebastianElvis Apr 27, 2023

SebastianElvis Apr 26, 2023

gusin13 Apr 27, 2023

SebastianElvis Apr 26, 2023

SebastianElvis Apr 26, 2023

KonradStaniec Apr 27, 2023

gusin13 Apr 27, 2023

KonradStaniec Apr 27, 2023

gusin13 Apr 27, 2023

gusin13 commented Apr 27, 2023

KonradStaniec left a comment

vitsalis left a comment

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

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

Conversation

gusin13 commented Apr 25, 2023 • edited Loading

Description

Testing

vitsalis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gusin13 Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gusin13 Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianElvis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gusin13 commented Apr 27, 2023

KonradStaniec left a comment

Choose a reason for hiding this comment

vitsalis left a comment

Choose a reason for hiding this comment

gusin13 commented Apr 25, 2023 •

edited

Loading

gusin13 Apr 26, 2023 •

edited

Loading

gusin13 Apr 27, 2023 •

edited

Loading