x/ibc: gRPC query service #6466

fedekunze · 2020-06-18T10:28:46Z

Description

TODO:

Connection
Channel
IBC transfer
Tests

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

lgtm-com · 2020-06-18T10:37:41Z

This pull request introduces 2 alerts when merging e82f809 into 257354d - view on LGTM.com

new alerts:

2 for Useless assignment to local variable

…ibc-grpc

lgtm-com · 2020-06-24T20:43:46Z

This pull request introduces 4 alerts when merging 447b0e5 into cb6c552 - view on LGTM.com

new alerts:

4 for Useless assignment to local variable

…ibc-grpc

lgtm-com · 2020-06-25T13:27:57Z

This pull request introduces 4 alerts when merging 20c6b7a into dcd5781 - view on LGTM.com

new alerts:

4 for Useless assignment to local variable

lgtm-com · 2020-06-26T11:03:28Z

This pull request introduces 4 alerts when merging 2296bb7 into 7c0fa69 - view on LGTM.com

new alerts:

4 for Useless assignment to local variable

lgtm-com · 2020-06-26T16:09:28Z

This pull request introduces 4 alerts when merging 774185e into 98a3645 - view on LGTM.com

new alerts:

4 for Useless assignment to local variable

…ibc-grpc

codecov · 2020-06-29T08:49:47Z

Codecov Report

Merging #6466 into master will increase coverage by 0.21%.
The diff coverage is 69.31%.

@@            Coverage Diff             @@
##           master    #6466      +/-   ##
==========================================
+ Coverage   57.30%   57.52%   +0.21%     
==========================================
  Files         491      493       +2     
  Lines       29470    29539      +69     
==========================================
+ Hits        16889    16991     +102     
+ Misses      11372    11336      -36     
- Partials     1209     1212       +3

…o fedekunze/ibc-grpc

tac0turtle · 2020-07-06T09:14:02Z

remove buf file plz

proto/ibc/channel/channel.proto

proto/ibc/channel/query.proto

colin-axner

left lots of suggestions, I'm open ears to doing some of the suggested changes in a followup pr since I think there will still need some refactor of cli stuff to be done. Some of my suggestions got outdated as I understood the entire pr, I'm going to leave them, but comment and resolve with the answer to them in case other reviewers have the same q

x/ibc-transfer/client/cli/cli.go

proto/ibc/connection/query.proto

x/ibc/03-connection/client/cli/query.go

x/ibc/04-channel/keeper/grpc_query_test.go

colin-axner · 2020-07-06T11:01:19Z

x/ibc/04-channel/keeper/grpc_query_test.go

+			func() {
+				_, _, _, _, channelA, _ := suite.coordinator.Setup(suite.chainA, suite.chainB)
+				ack := types.NewPacketAckCommitment(channelA.PortID, channelA.ID, 1, []byte("hash"))
+				suite.chainA.App.IBCKeeper.ChannelKeeper.SetPacketAcknowledgement(suite.chainA.GetContext(), channelA.PortID, channelA.ID, 1, ack.Hash)


So I think UnrelayedPackets might not be working as intended. Basically we want a query function that can indicate if given a list of packet commitment sequences:

does the packet commitment need to be relayed? (no ack exists)

does the ack need to be relayed? (ack exists and packet commitment exists)

looking at this test, an ack is set, but no packet commitment is created for the corresponding ack (ack has been relayed, packet cycle complete), so the sequences returned should be empty.

It's probably best to split the unrelayed packets into two funcs to avoid confusion or just add a bool in the request.

It's probably best to split the unrelayed packets into two funcs to avoid confusion or just add a bool in the request.

Do you want to split the command as well?

yea that sounds good, a bool flag could work as well. Whichever is easier

bool flag might be easier:
gaia q ibc channel unrelayed-packets --acknowledgements true would return all unrelayed acknowledgements

x/ibc/04-channel/client/cli/query.go

Co-authored-by: colin axner <[email protected]>

…o fedekunze/ibc-grpc

colin-axner · 2020-07-06T14:02:38Z

x/ibc/04-channel/client/cli/query.go

-			seqStrs := strings.Split(args[2], ",")
-
-			seqs := make([]uint64, len(seqStrs))
+			seqSlice, err := cmd.Flags().GetInt64Slice(flagSequences)


ah I meant if the sequence flag is unused that the default behaviour is to query for the packet commitments and supply them as the sequence arg. This can be done in a followup pr though

anilcse

loogs good @fedekunze , added a few comments

proto/ibc/channel/channel.proto

x/ibc-transfer/module.go

anilcse · 2020-07-06T15:43:44Z

x/ibc/03-connection/client/cli/query.go


-			page, _ := cmd.Flags().GetInt(flags.FlagPage)
-			limit, _ := cmd.Flags().GetInt(flags.FlagLimit)
+			page, err := cmd.Flags().GetInt(flags.FlagPage)


Page is confusing, we might need to introduce Offset flag

Suggested change

page, err := cmd.Flags().GetInt(flags.FlagPage)

offset, err := cmd.Flags().GetInt(flags.FlagOffset)

FlagOffset is not defined. We should also add a FlagPaginationKey 👍

anilcse · 2020-07-06T15:50:16Z

x/ibc/03-connection/keeper/grpc_query.go

+}
+
+// Connections implements the Query/Connections gRPC method
+func (q Keeper) Connections(c context.Context, req *types.QueryConnectionsRequest) (*types.QueryConnectionsResponse, error) {


Do we have any filters for Connections? May be would be good to introduce them.

I agree, not in this PR tho 👍

x/ibc/03-connection/keeper/grpc_query_test.go

…ibc-grpc

x/ibc: gRPC query service

e82f809

fedekunze added x/ibc labels Jun 18, 2020

fedekunze added this to the IBC 1.0 milestone Jun 18, 2020

fedekunze added 13 commits June 18, 2020 12:48

fixes

ed53b70

fix conflicts with master

8d90819

connection updates

825e2de

register channel query service

db0b4f4

update clients

d3591a8

change proofs to bytes

a726403

implement additional channel grpc queries

7ffcc34

unrelayed packets query

b8c6e37

Merge branch 'master' of github.com:cosmos/cosmos-sdk into fedekunze/…

ec234f3

…ibc-grpc

Merge branch 'master' of github.com:cosmos/cosmos-sdk into fedekunze/…

151c860

…ibc-grpc

query.proto files

327e440

move next sequence recv query to channel client

759ae9c

update REST

447b0e5

Merge branch 'master' of github.com:cosmos/cosmos-sdk into fedekunze/…

20c6b7a

…ibc-grpc

wip test

2296bb7

add missing cli queries

774185e

fedekunze added 4 commits June 29, 2020 10:20

install the right tools

3b78b73

fixes

4ba2fad

Merge branch 'master' of github.com:cosmos/cosmos-sdk into fedekunze/…

6158d22

…ibc-grpc

build

2c84778

fedekunze added the R4R label Jul 6, 2020

fedekunze requested review from colin-axner, sahith-narahari, anilcse, atheeshp and cwgoes July 6, 2020 08:45

fedekunze added 2 commits July 6, 2020 10:46

typo

733be58

Merge branch 'fedekunze/ibc-grpc' of github.com:cosmos/cosmos-sdk int…

4e2f2f7

…o fedekunze/ibc-grpc

tac0turtle reviewed Jul 6, 2020

View reviewed changes

proto/ibc/channel/channel.proto Show resolved Hide resolved

remove buf

50153a1

tac0turtle reviewed Jul 6, 2020

View reviewed changes

proto/ibc/channel/query.proto Show resolved Hide resolved

colin-axner reviewed Jul 6, 2020

View reviewed changes

fedekunze and others added 6 commits July 6, 2020 14:27

Update x/ibc/03-connection/keeper/grpc_query_test.go

acf1fc1

Co-authored-by: colin axner <[email protected]>

address comments from review

cef7a7a

Merge branch 'fedekunze/ibc-grpc' of github.com:cosmos/cosmos-sdk int…

13b3ccf

…o fedekunze/ibc-grpc

fix tests

358e670

unrelayed packet sequences flag

b790969

fix conflicts

bbe7a88

colin-axner reviewed Jul 6, 2020

View reviewed changes

anilcse approved these changes Jul 6, 2020

View reviewed changes

fedekunze added 3 commits July 6, 2020 21:17

Merge branch 'master' of github.com:cosmos/cosmos-sdk into fedekunze/…

f1da016

…ibc-grpc

remove ClientsConnections query

ca11ce3

fix

491607a

fedekunze mentioned this pull request Jul 6, 2020

Update unrelayed packets query logic #6615

Closed

4 tasks

fedekunze merged commit f35e3b2 into master Jul 6, 2020

fedekunze deleted the fedekunze/ibc-grpc branch July 6, 2020 19:35

fedekunze mentioned this pull request Jul 6, 2020

Add missing pagination flags #6617

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x/ibc: gRPC query service #6466

x/ibc: gRPC query service #6466

fedekunze commented Jun 18, 2020 •

edited

Loading

lgtm-com bot commented Jun 18, 2020

lgtm-com bot commented Jun 24, 2020

lgtm-com bot commented Jun 25, 2020

lgtm-com bot commented Jun 26, 2020

lgtm-com bot commented Jun 26, 2020

codecov bot commented Jun 29, 2020 •

edited

Loading

tac0turtle commented Jul 6, 2020

colin-axner left a comment

colin-axner Jul 6, 2020

fedekunze Jul 6, 2020

colin-axner Jul 6, 2020

colin-axner Jul 6, 2020

colin-axner Jul 6, 2020

anilcse left a comment

anilcse Jul 6, 2020

fedekunze Jul 6, 2020

anilcse Jul 6, 2020

fedekunze Jul 6, 2020

	page, err := cmd.Flags().GetInt(flags.FlagPage)
	offset, err := cmd.Flags().GetInt(flags.FlagOffset)

x/ibc: gRPC query service #6466

x/ibc: gRPC query service #6466

Conversation

fedekunze commented Jun 18, 2020 • edited Loading

Description

lgtm-com bot commented Jun 18, 2020

lgtm-com bot commented Jun 24, 2020

lgtm-com bot commented Jun 25, 2020

lgtm-com bot commented Jun 26, 2020

lgtm-com bot commented Jun 26, 2020

codecov bot commented Jun 29, 2020 • edited Loading

Codecov Report

tac0turtle commented Jul 6, 2020

colin-axner 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

anilcse 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

fedekunze commented Jun 18, 2020 •

edited

Loading

codecov bot commented Jun 29, 2020 •

edited

Loading