Standardize connection versioning + channel versioning docs #6640

colin-axner · 2020-07-08T09:27:06Z

Description

closes: #5846

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

colin-axner · 2020-07-08T09:57:55Z

Should we add some way for application modules to register their own versions? Like I develop an application module and import 1.0 of TAO, but wanna include version X and Y for my app module. I guess this should be done on genesis?

fedekunze · 2020-07-08T10:04:25Z

but wanna include version X and Y for my app module

channel versions? are they your own app channel versions or the counterparty's channel version? I guess adding a parameter makes sense for the latter

…istering

colin-axner · 2020-07-08T10:43:52Z

are they your own app channel versions or the counterparty's channel version?

they are the application versions being negotiated on during the channel handshake (version string stored in channel). I am referring to your own application version.

I think I have a general idea of how to approach it. I'm going to add in calls to ibc-transfer which registers its supported version in InitGenesis and will update channel handshake negotiation to match connection handshake negotiation expect using its own supported versions.

colin-axner · 2020-07-08T11:02:46Z

oh nvm @fedekunze I realized this is simpler than connection versioning and I don't need to add too much extra since the channel version and counterparty version selection are happening off chain by the caller (application module). I just need to add in OpenAck verification that the selected feature set is entirely supported.

codecov · 2020-07-08T12:16:45Z

Codecov Report

Merging #6640 into master will increase coverage by 5.66%.
The diff coverage is 60.88%.

@@            Coverage Diff             @@
##           master    #6640      +/-   ##
==========================================
+ Coverage   55.60%   61.26%   +5.66%     
==========================================
  Files         457      385      -72     
  Lines       27440    23682    -3758     
==========================================
- Hits        15257    14509     -748     
+ Misses      11083     8127    -2956     
+ Partials     1100     1046      -54

colin-axner · 2020-07-08T12:14:15Z

x/ibc/04-channel/types/channel.go

@@ -13,9 +13,6 @@ var (
 	_ exported.CounterpartyI = (*Counterparty)(nil)
 )

-// DefaultChannelVersion defines the default channel version used during handshake.
-const DefaultChannelVersion = "1.0.0"


there is no "channel version" only application versions for the associated channel.

colin-axner · 2020-07-08T12:16:59Z

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

@@ -238,6 +238,11 @@ func (k Keeper) ChanOpenAck(
 		panic("cannot find connection")
 	}

+	// verify that chainB's proposed version identifier is supported by chainA
+	if err := connectiontypes.VerifyProposedVersion(channel.Version, counterpartyVersion, nil); err != nil {


note: no support for disallowing nil feature sets. this is a little more tricky for channel version negotiation. I think it would require creating a channel keeper with the application version set inside a keeper field (perhaps a map). this seems like too much overhead for functionality that hasn't actually been requested yet

I completely forgot there are ChanOpen callbacks. Applications can just throw an error on any version identifier/feature set they disagree with on ChanOpenAck.

colin-axner · 2020-07-08T12:20:28Z

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

@@ -52,7 +53,7 @@ func NewChannelOpenInitCmd() *cobra.Command {
 		},
 	}
 	cmd.Flags().Bool(FlagOrdered, true, "Pass flag for opening ordered channels")
-	cmd.Flags().String(FlagIBCVersion, types.DefaultChannelVersion, "supported IBC version")


this maybe should just be an argument, since the default depends on what application you are creating a channel for, follow up pr since there is a todo for fixing counterparty version as well

colin-axner · 2020-07-08T15:39:58Z

I'm realizing now the addition to the ChanOpenAck handshake is unnecessary because of the ChanOpenAck callback (forgot this even existed). I assume I should remove that (and the tests for them, since this now unnecessary)?

I guess this pr should just be adding the nil feature set addition for connection versions. I don't see any need to impose any restrictions on the application level version. I don't think it needs to follow the same scheme as connections since the negotiation happens off chain really and by the callbacks

colin-axner · 2020-07-10T16:43:42Z

Plans for this pr:

revert back to using strings as versions
parse the string into a standardized version string at the connection level that will match an ICS spec
update channel docs to discuss how version negotiation is to be handled
update ibc docs to give concrete examples of how apps could do their own versioning

x/ibc/03-connection/types/version.go

…ersions renaming

AdityaSripal

utACK

cwgoes

utACK though see one minor comment for docs clarity

cwgoes · 2020-07-13T20:18:38Z

x/ibc/spec/01_concepts.md

+During `ChanOpenInit`, a version string is passed in and set in party A's 
+channel state.
+
+During `ChanOpenTry`, a version string and counterparty version string are 


Maybe worth noting that which chain is the counterparty switches between this and the next paragraph, it's a bit confusing to read, might be helpful to label them A/B

ethanfrey · 2020-07-14T09:54:21Z

@colin-axner I just saw the notification on this PR, sorry for the late response.

The docs look great and I agree with the design.

A large part of this PR updates the connection versioning which is good.

As far as I can see no changes were made to channel versioning except the docs. ie. I see no implementation of the connection versioning in the ibc-transfer module when looking at the diff. I assumed to find these new ChannelInit etc callbacks handled there somewere to see and example that matches the docs.

On the other hand, if this PR does implementation for connection versioing and just docs for channel versioning (which the title implies), then please don't close #5846 yet. I assume closing that one will include a demo implementation of the spec on ibc-transfer at least. This does definitely close #5079 however

colin-axner · 2020-07-14T10:07:01Z

@ethanfrey Channel Versioning happened to be already implemented for the required functionality.

channel handshake callbacks already existed
ICS20 already did basic string matching in these callbacks, see OnChanOpenInit

Since channel versioning is intended to be very abstract and left to application developers no modifications were needed for the handshake calls.

ethanfrey · 2020-07-14T12:24:24Z

Okay, so this was just formalizing what was used for the string there.

And how apps could use that to make a more complex one.

I will try out a more complex workflow then

update connection versions with feature set flag

4e0f898

colin-axner added the x/ibc label Jul 8, 2020

colin-axner mentioned this pull request Jul 8, 2020

x/ibc: refactor CLI #6639

Merged

9 tasks

make connection version modular to support channel versioning and reg…

c7b7e42

…istering

colin-axner added 5 commits July 8, 2020 13:59

revert IBCVersion renaming, add channel versioning logic

7dd7d56

merge master

b69d9c5

fix channel version flag

4b8989c

remove unnecessary godoc

17bed09

remove unused func

7176684

colin-axner commented Jul 8, 2020

View reviewed changes

colin-axner and others added 3 commits July 8, 2020 14:22

fix lint and version test

1baea76

add test and fix error

83a54e9

Merge branch 'master' into colin/5846-channel-versioning

35a6e3e

colin-axner marked this pull request as ready for review July 8, 2020 15:31

colin-axner requested review from aaronc, alessio, alexanderbez, clevinson, fedekunze and jgimeno as code owners July 8, 2020 15:31

colin-axner requested review from cwgoes and AdityaSripal July 8, 2020 15:31

colin-axner marked this pull request as draft July 8, 2020 15:50

colin-axner changed the title ~~implement channel versioning~~ support restricting nil feature sets for ibc version negotiation Jul 8, 2020

colin-axner added 7 commits July 13, 2020 10:56

revert proto changes

4ad50ab

restructure versioning to go from string to proto

dd1d31e

update versionStr to encodedVersion naming

e34c8dd

fix version test build

64611d6

merge master

1a185e9

fix keeper tests

b00df37

fix various tests

f57fbcc

colin-axner commented Jul 13, 2020

View reviewed changes

x/ibc/03-connection/types/version.go Outdated Show resolved Hide resolved

colin-axner added 4 commits July 13, 2020 16:47

fixes from self review

c21ac65

update docs

0a9ba5e

fix lint

18d02fb

add more code cov

eed690b

colin-axner marked this pull request as ready for review July 13, 2020 15:23

colin-axner requested review from AdityaSripal and fedekunze July 13, 2020 15:23

fedekunze approved these changes Jul 13, 2020

View reviewed changes

rename ToString funcs to Encode/DecodeVersion + GetCompatibleEncodedV…

36c0012

…ersions renaming

AdityaSripal approved these changes Jul 13, 2020

View reviewed changes

colin-axner changed the title ~~Standarize connection versioning + channel versioning docs~~ Standardize connection versioning + channel versioning docs Jul 13, 2020

cwgoes approved these changes Jul 13, 2020

View reviewed changes

colin-axner and others added 2 commits July 14, 2020 10:36

update spec docs

59996b5

Merge branch 'master' into colin/5846-channel-versioning

0b49257

colin-axner added the A:automerge Automatically merge PR once all prerequisites pass. label Jul 14, 2020

mergify bot merged commit 5dfc4a2 into master Jul 14, 2020

mergify bot deleted the colin/5846-channel-versioning branch July 14, 2020 08:43

ethanfrey mentioned this pull request Aug 3, 2020

Provide standard Result<success, error> acknowlegement packet envelope cosmos/ibc#461

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standardize connection versioning + channel versioning docs #6640

Standardize connection versioning + channel versioning docs #6640

colin-axner commented Jul 8, 2020 •

edited

Loading

colin-axner commented Jul 8, 2020

fedekunze commented Jul 8, 2020

colin-axner commented Jul 8, 2020

colin-axner commented Jul 8, 2020

codecov bot commented Jul 8, 2020 •

edited

Loading

colin-axner Jul 8, 2020

colin-axner Jul 8, 2020

colin-axner Jul 8, 2020

colin-axner Jul 8, 2020

colin-axner commented Jul 8, 2020 •

edited

Loading

colin-axner commented Jul 10, 2020

AdityaSripal left a comment

cwgoes left a comment

cwgoes Jul 13, 2020

ethanfrey commented Jul 14, 2020

colin-axner commented Jul 14, 2020

ethanfrey commented Jul 14, 2020

Standardize connection versioning + channel versioning docs #6640

Standardize connection versioning + channel versioning docs #6640

Conversation

colin-axner commented Jul 8, 2020 • edited Loading

Description

colin-axner commented Jul 8, 2020

fedekunze commented Jul 8, 2020

colin-axner commented Jul 8, 2020

colin-axner commented Jul 8, 2020

codecov bot commented Jul 8, 2020 • edited Loading

Codecov Report

colin-axner Jul 8, 2020

Choose a reason for hiding this comment

colin-axner Jul 8, 2020

Choose a reason for hiding this comment

colin-axner Jul 8, 2020

Choose a reason for hiding this comment

colin-axner Jul 8, 2020

Choose a reason for hiding this comment

colin-axner commented Jul 8, 2020 • edited Loading

colin-axner commented Jul 10, 2020

AdityaSripal left a comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

cwgoes Jul 13, 2020

Choose a reason for hiding this comment

ethanfrey commented Jul 14, 2020

colin-axner commented Jul 14, 2020

ethanfrey commented Jul 14, 2020

colin-axner commented Jul 8, 2020 •

edited

Loading

codecov bot commented Jul 8, 2020 •

edited

Loading

colin-axner commented Jul 8, 2020 •

edited

Loading