chore: allow ics27 to select and return default JSON encoded metadata #1550

damiannolan · 2022-06-16T14:00:11Z

Description

Allow interchain accounts to accept an empty channel version and select a default.

Adding version return to OnChanOpenInit cb handler in controller
Updating test cases

closes: #1551

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
Review Codecov Report in the comment section below once CI passes

…:cosmos/ibc-go into damian/1477-implement-payee-distribution

…ases

Co-authored-by: Charly <charly@interchain.berlin>

codecov-commenter · 2022-06-16T14:11:03Z

Codecov Report

Merging #1550 (bf866c8) into damian/1477-implement-payee-distribution (f8928f6) will decrease coverage by 0.10%.
The diff coverage is 41.93%.

@@                             Coverage Diff                              @@
##           damian/1477-implement-payee-distribution    #1550      +/-   ##
============================================================================
- Coverage                                     80.29%   80.19%   -0.11%     
============================================================================
  Files                                           166      166              
  Lines                                         12304    12322      +18     
============================================================================
+ Hits                                           9880     9882       +2     
- Misses                                         1959     1974      +15     
- Partials                                        465      466       +1

Impacted Files	Coverage Δ
...ules/apps/27-interchain-accounts/types/metadata.go	`75.70% <0.00%> (-7.81%)`	⬇️
...interchain-accounts/controller/keeper/handshake.go	`78.78% <61.11%> (-7.66%)`	⬇️
...7-interchain-accounts/controller/ibc_middleware.go	`83.63% <100.00%> (+0.30%)`	⬆️

chatton

Looks great! Just had a few questions/comments 👍

chatton · 2022-06-22T10:16:13Z

modules/apps/27-interchain-accounts/controller/keeper/handshake.go

 		}
 	}

-	return nil
+	return string(icatypes.ModuleCdc.MustMarshalJSON(&metadata)), nil


just to make sure I understand, if this panics this is handled at the sdk level right?

Yep, its caught by the grpc recovery interceptor (essentially a middleware handler in grpc land)

modules/apps/27-interchain-accounts/controller/keeper/handshake.go

chatton · 2022-06-22T10:17:54Z

modules/apps/27-interchain-accounts/controller/keeper/handshake.go

@@ -28,26 +28,35 @@ func (k Keeper) OnChanOpenInit(
 	chanCap *capabilitytypes.Capability,
 	counterparty channeltypes.Counterparty,
 	version string,
-) error {
+) (string, error) {


what is the implication of changing an OnChanOpenInit signature like this? Are we free to do this without causing any issues?

In this instance yes, the OnChanOpenInit callback was already updated to include a second version return arg. The calling func in controller/ibc_middleware.go already contains this function signature which is the entrypoint of the module handshake essentially.

In other situations we may need to more careful changing function sigs but this doesn't really cause any issues.

to add to that: in general, we would also update the ibc spec for changes to these fn sigs. But like @damiannolan said the calling func already has this signature.

chatton · 2022-06-22T10:18:58Z

modules/apps/27-interchain-accounts/controller/ibc_middleware.go

@@ -50,7 +50,8 @@ func (im IBCMiddleware) OnChanOpenInit(
 		return "", types.ErrControllerSubModuleDisabled
 	}

-	if err := im.keeper.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version); err != nil {
+	version, err := im.keeper.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version)


nice readability improvement 🥇

chatton · 2022-06-22T10:20:07Z

modules/apps/27-interchain-accounts/controller/keeper/handshake.go

+	} else {
+		if err := icatypes.ModuleCdc.UnmarshalJSON([]byte(version), &metadata); err != nil {


[nit] rather than having nested if/else clauses it might be cleaner to have a helper function to return the metadata (using early returns)

I took a look at this and feel like there's not a massive difference making a helper function. You'd either need to return a new metadata type or take a pointer and a bunch of other args too... I'm pretty indifferent about it

modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go

seantking

LGTM! Nice work.

Should we test this out manually as well?

damiannolan · 2022-06-22T14:28:21Z

LGTM! Nice work.

Should we test this out manually as well?

Yup, already tested this manually as part of #396 QA scenarios.

charleenfei · 2022-06-23T09:44:12Z

modules/apps/27-interchain-accounts/controller/keeper/handshake.go

-		return sdkerrors.Wrapf(icatypes.ErrUnknownDataType, "cannot unmarshal ICS-27 interchain accounts metadata")
+	if strings.TrimSpace(version) == "" {
+		connection, err := k.channelKeeper.GetConnection(ctx, connectionHops[0])
+		if err != nil {


just a super tiny nit but you have the err statement in the second else clause in the ; err != nil setup, would be nice if they were both like this or both the other way just as a tiny remark on style

But we need to use the connection below outside the scope of the returned error.
Generally we use the below for function return sigs with one single error return:

if err := invokeFunc(); err != nil { // handle err }

If we use the following then we can't use the connection below, outside the scope of the conditional:

if connection, err := k.channelKeeper.GetConnection(ctx, connectionHops[0]); err != nil { // connection and err are only available at this level of scope }

ah yes, that makes sense 🙈

you should technically be able to if you put the usage in an else clause, but I personally prefer the way you have it now rather than adding additional else clauses!

charleenfei · 2022-06-23T09:59:15Z

modules/apps/27-interchain-accounts/controller/keeper/handshake.go

-	if err := icatypes.ModuleCdc.UnmarshalJSON([]byte(version), &metadata); err != nil {
-		return sdkerrors.Wrapf(icatypes.ErrUnknownDataType, "cannot unmarshal ICS-27 interchain accounts metadata")
+	if strings.TrimSpace(version) == "" {
+		connection, err := k.channelKeeper.GetConnection(ctx, connectionHops[0])


why is connectionHops hardcoded to the first value of the array?

The first value is the source chain connection end AFAIK

ConnectionHops is an ordered list of connection ids, so using connectionHops[0] here takes the first connection hop through which packets travel. There's a bunch of defensive checks in ibc core ensuring that this is always of length one at the moment. So on source chain connectionHops will contain the connection ID of self, in this instance its the controller chain. And likewise, the counterparty handlers would have a connectionHops array containing the connection of themself (host). Obviously in future with multi hop the semantics will change and ibc core will have to adapt for it. But due to the ordering, we can safely assume that the 0 index element is self.

Channel spec says:

The connectionHops stores the list of connection identifiers, in order, along which packets sent on this channel will travel. At the moment this list must be of length 1. In the future multi-hop channels may be supported.

crodriguezvega

Awesome work, @damiannolan!

I just left a suggestion to potentially remove some duplicate code...

crodriguezvega · 2022-06-23T14:05:05Z

modules/apps/27-interchain-accounts/types/metadata.go

 		ControllerConnectionId: controllerConnectionID,
 		HostConnectionId:       hostConnectionID,
 		Encoding:               EncodingProtobuf,
 		TxType:                 TxTypeSDKMultiMsg,
+		Version:                Version,
 	}


Maybe you can replace now this whole object initialization with a call to the NewDefaultMetadata function above?

damiannolan and others added 13 commits June 10, 2022 17:53

adding distribution logic with new registered payees

20d1003

updating ack test cases

c25ca5f

removing commented out test

2d8d25f

adding additional testcase to TestOnAcknowledgementPacket

3941f55

Merge branch 'main' into damian/1477-implement-payee-distribution

ae9fef6

cleaning up distribution logic and adapting tests

3247100

updating timeout test cases

44c7293

Merge branch 'damian/1477-implement-payee-distribution' of github.com…

0f3f132

…:cosmos/ibc-go into damian/1477-implement-payee-distribution

updating timeout logic and adding application callback failure test c…

64dab2c

…ases

Merge branch 'main' into damian/1477-implement-payee-distribution

76d0eef

Apply suggestions from code review

af201e0

Co-authored-by: Charly <charly@interchain.berlin>

Merge branch 'main' into damian/1477-implement-payee-distribution

f8928f6

WIP handling empty version string for ics27

bf866c8

damiannolan mentioned this pull request Jun 16, 2022

Allow interchain accounts to select a default metadata version #1551

Closed

3 tasks

Base automatically changed from damian/1477-implement-payee-distribution to main June 17, 2022 13:40

damiannolan added 5 commits June 20, 2022 10:09

Merge branch 'main' into damian/handle-ica-empty-version

909f2e5

updating testcases

ffdfc56

Merge branch 'main' into damian/handle-ica-empty-version

8975dd2

adding test with auth module modifying channel version

9a50acc

Merge branch 'main' into damian/handle-ica-empty-version

31e7455

damiannolan marked this pull request as ready for review June 22, 2022 09:26

damiannolan requested review from seantking, colin-axner and AdityaSripal as code owners June 22, 2022 09:26

damiannolan changed the title ~~[WIP] Allow ics27 to select and return default JSON encoded metadata~~ Allow ics27 to select and return default JSON encoded metadata Jun 22, 2022

damiannolan changed the title ~~Allow ics27 to select and return default JSON encoded metadata~~ chore: allow ics27 to select and return default JSON encoded metadata Jun 22, 2022

damiannolan requested review from charleenfei, chatton and crodriguezvega June 22, 2022 10:09

chatton reviewed Jun 22, 2022

View reviewed changes

seantking reviewed Jun 22, 2022

View reviewed changes

modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go Show resolved Hide resolved

seantking approved these changes Jun 22, 2022

View reviewed changes

adding inline comments re. ignored channel version from auth module

e078e9d

charleenfei reviewed Jun 23, 2022

View reviewed changes

crodriguezvega approved these changes Jun 23, 2022

View reviewed changes

damiannolan added 2 commits June 23, 2022 16:20

updating with deduplication suggestion

f08c217

Merge branch 'main' into damian/handle-ica-empty-version

d9cbb0e

damiannolan enabled auto-merge (squash) June 23, 2022 14:30

damiannolan merged commit 8ffa912 into main Jun 23, 2022

damiannolan deleted the damian/handle-ica-empty-version branch June 23, 2022 14:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: allow ics27 to select and return default JSON encoded metadata #1550

chore: allow ics27 to select and return default JSON encoded metadata #1550

damiannolan commented Jun 16, 2022 •

edited

Loading

codecov-commenter commented Jun 16, 2022

chatton left a comment

chatton Jun 22, 2022

damiannolan Jun 22, 2022

chatton Jun 22, 2022

damiannolan Jun 22, 2022

seantking Jun 22, 2022

chatton Jun 22, 2022

chatton Jun 22, 2022

damiannolan Jun 22, 2022

seantking left a comment

damiannolan commented Jun 22, 2022

charleenfei Jun 23, 2022

damiannolan Jun 23, 2022

charleenfei Jun 23, 2022

chatton Jun 23, 2022

charleenfei Jun 23, 2022

seantking Jun 23, 2022 •

edited

Loading

damiannolan Jun 23, 2022

crodriguezvega left a comment

crodriguezvega Jun 23, 2022

damiannolan Jun 23, 2022

		} else {
		if err := icatypes.ModuleCdc.UnmarshalJSON([]byte(version), &metadata); err != nil {

chore: allow ics27 to select and return default JSON encoded metadata #1550

chore: allow ics27 to select and return default JSON encoded metadata #1550

Conversation

damiannolan commented Jun 16, 2022 • edited Loading

Description

codecov-commenter commented Jun 16, 2022

Codecov Report

chatton 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

seantking left a comment

Choose a reason for hiding this comment

damiannolan commented Jun 22, 2022

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

seantking Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crodriguezvega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damiannolan commented Jun 16, 2022 •

edited

Loading

seantking Jun 23, 2022 •

edited

Loading