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

refactor: channel handshake version improvements #1283

Merged
merged 23 commits into from
May 10, 2022
Merged

refactor: channel handshake version improvements #1283

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
5b0bc50
refactor: returning version from OnChanOpenInit
seantking Apr 21, 2022
b7088f8
refactor: update tests and add version to proto resp
seantking Apr 21, 2022
6f20ad9
refactor: adding version to msg server resp
seantking Apr 21, 2022
31a2831
refactor: remove unncessary if & update version on Endpoint.Ack
seantking Apr 26, 2022
0178c09
fix: ics29 OnChanOpenInit remake versionMetaData before returning
seantking Apr 26, 2022
8f8ce78
chore: update godoc
seantking Apr 26, 2022
f83b472
Merge branch 'main' into sean/issue#722-channel-handshake-improvements
seantking Apr 26, 2022
2b0cca4
test: adding check for expected version string
seantking Apr 26, 2022
2d7f0b8
test: adding test case for passing empty version string to ics20 onCh…
seantking Apr 26, 2022
534e00c
Update modules/apps/29-fee/ibc_module.go
seantking Apr 26, 2022
8f08760
chore: comment
seantking Apr 26, 2022
7d6246c
Merge branch 'main' into sean/issue#722-channel-handshake-improvements
seantking Apr 26, 2022
a03916d
chore: changelog
seantking Apr 27, 2022
0db8c86
Merge branch 'main' into sean/issue#722-channel-handshake-improvements
seantking Apr 27, 2022
ce656c4
fix: ica now discards auth module version
seantking Apr 27, 2022
092d3fe
chore: update changelog
seantking Apr 27, 2022
d30f887
adding default version for ics29
seantking Apr 29, 2022
1a44c0b
fix: using transfer module directly rather than calling full middlewa…
seantking May 4, 2022
15d7092
fix testing bug
colin-axner May 4, 2022
35f71cf
Merge branch 'main' into sean/issue#722-channel-handshake-improvements
seantking May 4, 2022
0b85196
refactor: test now uses bool for isFeeEnabled rather than direct check
seantking May 4, 2022
0286c64
docs: updating comment and migration docs
seantking May 4, 2022
28d5622
Merge branch 'main' into sean/issue#722-channel-handshake-improvements
seantking May 10, 2022
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
6 changes: 6 additions & 0 deletions docs/ibc/proto-docs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2809,6 +2809,7 @@ MsgChannelOpenInitResponse defines the Msg/ChannelOpenInit response type.
| Field | Type | Label | Description |
| ----- | ---- | ----- | ----------- |
| `channel_id` | [string](#string) | | |
| `version` | [string](#string) | | |



Expand Down Expand Up @@ -2844,6 +2845,11 @@ value will be ignored by core IBC.
MsgChannelOpenTryResponse defines the Msg/ChannelOpenTry response type.


| Field | Type | Label | Description |
| ----- | ---- | ----- | ----------- |
| `version` | [string](#string) | | |





Expand Down
6 changes: 3 additions & 3 deletions modules/apps/27-interchain-accounts/controller/ibc_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,13 @@ func (im IBCModule) OnChanOpenInit(
chanCap *capabilitytypes.Capability,
counterparty channeltypes.Counterparty,
version string,
) error {
) (string, error) {
if !im.keeper.IsControllerEnabled(ctx) {
return types.ErrControllerSubModuleDisabled
return "", types.ErrControllerSubModuleDisabled
}

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

// call underlying app's OnChanOpenInit callback with the appVersion
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
6 changes: 3 additions & 3 deletions modules/apps/27-interchain-accounts/controller/ibc_module_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,8 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenInit = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string,
portID, channelID string, chanCap *capabilitytypes.Capability,
counterparty channeltypes.Counterparty, version string,
) error {
return fmt.Errorf("mock ica auth fails")
) (string, error) {
return "", fmt.Errorf("mock ica auth fails")
}
}, false,
},
Expand Down Expand Up @@ -197,7 +197,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module)
suite.Require().True(ok)

err = cbs.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(),
_, err = cbs.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(),
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, chanCap, channel.Counterparty, channel.GetVersion(),
)

Expand Down
4 changes: 2 additions & 2 deletions modules/apps/27-interchain-accounts/host/ibc_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ func (im IBCModule) OnChanOpenInit(
chanCap *capabilitytypes.Capability,
counterparty channeltypes.Counterparty,
version string,
) error {
return sdkerrors.Wrap(icatypes.ErrInvalidChannelFlow, "channel handshake must be initiated by controller chain")
) (string, error) {
return "", sdkerrors.Wrap(icatypes.ErrInvalidChannelFlow, "channel handshake must be initiated by controller chain")
}

// OnChanOpenTry implements the IBCModule interface
Expand Down
10 changes: 8 additions & 2 deletions modules/apps/29-fee/ibc_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func (im IBCModule) OnChanOpenInit(
chanCap *capabilitytypes.Capability,
counterparty channeltypes.Counterparty,
version string,
) error {
) (string, error) {
var versionMetadata types.Metadata
if err := types.ModuleCdc.UnmarshalJSON([]byte(version), &versionMetadata); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

This will not enable fee by default. If provided string is empty then, get default version from underlying app and wrap with fee version.
If underlying app returns error with empty version then return error as well

Copy link
Contributor

@colin-axner colin-axner Apr 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Do we want to enable fees by default? Otherwise there's no way to only use the default version of the base application. I'd think middleware applications must be explicitly activated or, the versionMetadata should be specified with an empty fee version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this was my assumption also. What is the benefit of enabling the fee middleware by default with an empty version passed in?

Copy link
Member

Choose a reason for hiding this comment

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

I do think fees should be enabled by default. My preference is that relayers get the default version of the stack if they don't explicitly specify otherwise. Some middleware will want to be activated by default and some may want to be explicitly activated. For adoption purposes, I think it would be good to make incentivization the default rather than having to explicitly enable it. Because as I think the number of applications scale, most relayers will only be passing in empty strings and use non-empty strings only in very special cases (otherwise they would need to have context on every single port they relay for).

So a relayer that always defaults to what the application prefers will be opening incentivized channels.

Of course, if the relayer wants to explicitly disable fees then they can by passing in a version without the feeVersion. But we should design for a future where relayers are rarely passing in specific strings

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me. In favor of @AdityaSripal's approach. It should just be well documented that ics29 is an exceptional middleware application that we believe should be enabled by default, but that other applications (who might be copy/pasting our code) should give good consideration to whether their application should be enabled by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the explanation @AdityaSripal. Makes sense to me!

// Since it is valid for fee version to not be specified, the above middleware version may be for a middleware
Expand All @@ -46,8 +46,14 @@ func (im IBCModule) OnChanOpenInit(
chanCap, counterparty, version)
}

if versionMetadata.FeeVersion == "" {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
// call underlying app's OnChanOpenInit callback with the appVersion
return im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID,
chanCap, counterparty, versionMetadata.AppVersion)
}

if versionMetadata.FeeVersion != types.Version {
return sdkerrors.Wrapf(types.ErrInvalidVersion, "expected %s, got %s", types.Version, versionMetadata.FeeVersion)
return "", sdkerrors.Wrapf(types.ErrInvalidVersion, "expected %s, got %s", types.Version, versionMetadata.FeeVersion)
}

im.keeper.SetFeeEnabled(ctx, portID, channelID)
Expand Down
8 changes: 4 additions & 4 deletions modules/apps/29-fee/ibc_module_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,11 @@ func (suite *FeeTestSuite) TestOnChanOpenInit() {
suite.chainA.GetSimApp().FeeMockModule.IBCApp.OnChanOpenInit = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string,
portID, channelID string, chanCap *capabilitytypes.Capability,
counterparty channeltypes.Counterparty, version string,
) error {
) (string, error) {
if version != ibcmock.Version {
return fmt.Errorf("incorrect mock version")
return "", fmt.Errorf("incorrect mock version")
}
return nil
return ibcmock.Version, nil
}

suite.path.EndpointA.ChannelID = ibctesting.FirstChannelID
Expand All @@ -95,7 +95,7 @@ func (suite *FeeTestSuite) TestOnChanOpenInit() {
cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module)
suite.Require().True(ok)

err = cbs.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(),
_, err = cbs.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(),
suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, chanCap, counterparty, channel.Version)

if tc.expPass {
Expand Down
14 changes: 9 additions & 5 deletions modules/apps/transfer/ibc_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,21 +70,25 @@ func (im IBCModule) OnChanOpenInit(
chanCap *capabilitytypes.Capability,
counterparty channeltypes.Counterparty,
version string,
) error {
) (string, error) {
if err := ValidateTransferChannelParams(ctx, im.keeper, order, portID, channelID); err != nil {
return err
return "", err
}

if version == "" {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
version = types.Version
}

if version != types.Version {
return sdkerrors.Wrapf(types.ErrInvalidVersion, "got %s, expected %s", version, types.Version)
return "", sdkerrors.Wrapf(types.ErrInvalidVersion, "got %s, expected %s", version, types.Version)
}

// Claim channel capability passed back by IBC module
if err := im.keeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil {
return err
return "", err
}

return nil
return version, nil
}

// OnChanOpenTry implements the IBCModule interface.
Expand Down
2 changes: 1 addition & 1 deletion modules/apps/transfer/ibc_module_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func (suite *TransferTestSuite) TestOnChanOpenInit() {

tc.malleate() // explicitly change fields in channel and testChannel

err = cbs.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(),
_, err = cbs.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(),
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, chanCap, counterparty, channel.GetVersion(),
)

Expand Down
Loading