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 7 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: 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
20 changes: 15 additions & 5 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 @@ -47,14 +47,24 @@ func (im IBCModule) OnChanOpenInit(
}

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

appVersion, err := im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, versionMetadata.AppVersion)
if err != nil {
return "", err
}

versionMetadata.AppVersion = appVersion
versionBytes, err := types.ModuleCdc.MarshalJSON(&versionMetadata)
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return "", err
}

im.keeper.SetFeeEnabled(ctx, portID, channelID)

// call underlying app's OnChanOpenInit callback with the appVersion
return im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID,
chanCap, counterparty, versionMetadata.AppVersion)
return string(versionBytes), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was the call to underlying app's OnChanOpenInit removed here?

Copy link
Member

Choose a reason for hiding this comment

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

Its moved to above on line 53 so that the returned application version (for example: ics20-1) can be inserted into the AppVersion field of types.Metadata, and re-marshalled to a JSON encoded version string. Otherwise we would return directly from the app callback (writing ics20-1 as the channel version into state), we would expect ics29 to be enabled but subsequent handshake callbacks on would fail as the channel version in state would not be the metadata object:

{
    "AppVersion": "ics20-1",
    "FeeVersion": "ics29-1",
}

Does that make sense?

}

// OnChanOpenTry implements the IBCModule interface
Expand Down Expand Up @@ -113,7 +123,7 @@ func (im IBCModule) OnChanOpenAck(
if im.keeper.IsFeeEnabled(ctx, portID, channelID) {
var versionMetadata types.Metadata
if err := types.ModuleCdc.UnmarshalJSON([]byte(counterpartyVersion), &versionMetadata); err != nil {
return sdkerrors.Wrap(types.ErrInvalidVersion, "failed to unmarshal ICS29 counterparty version metadata")
return sdkerrors.Wrapf(types.ErrInvalidVersion, "failed to unmarshal ICS29 counterparty version metadata: %s", counterpartyVersion)
seantking marked this conversation as resolved.
Show resolved Hide resolved
}

if versionMetadata.FeeVersion != types.Version {
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
1 change: 0 additions & 1 deletion modules/apps/29-fee/transfer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,5 +68,4 @@ func (suite *FeeTestSuite) TestFeeTransfer() {
fee.AckFee.Add(fee.TimeoutFee...), // ack fee paid, timeout fee refunded
sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), ibctesting.TestCoin.Denom)).Sub(originalChainASenderAccountBalance),
)

}
15 changes: 10 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 @@ -3,6 +3,7 @@ package transfer
import (
"fmt"
"math"
"strings"

sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
Expand Down Expand Up @@ -70,21 +71,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 strings.TrimSpace(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.

I can add a test case for this

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't all of the applications/middleware have a similar code block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

responded below

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
16 changes: 11 additions & 5 deletions modules/core/05-port/types/module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,16 @@ import (
// IBCModule defines an interface that implements all the callbacks
// that modules must define as specified in ICS-26
type IBCModule interface {
// OnChanOpenInit will verify that the relayer-chosen parameters are
// valid and perform any custom INIT logic.It may return an error if
// the chosen parameters are invalid in which case the handshake is aborted.
// OnChanOpenInit should return an error if the provided version is invalid.
// OnChanOpenInit will verify that the relayer-chosen parameters
// are valid and perform any custom INIT logic.
// It may return an error if the chosen parameters are invalid
// in which case the handshake is aborted.
// If the provided version string is non-empty, OnChanOpenInit should return
// the version string if valid or an error if the provided version is invalid.
Comment on lines +18 to +19
Copy link
Member

Choose a reason for hiding this comment

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

Do we require that Init returns the exact same version if version is non-empty? I don't believe that is the case. Applications may want to modify the version string even if it is non-empty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took this directly from the spec: https://github.com/cosmos/ibc/pull/629/files#diff-508b3e7784a300436fbeb6940be22f31c74e958270a36bb851d06a8782ad7e7aR50

Should we update both? Agree with your assessment.

Copy link
Member

Choose a reason for hiding this comment

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

Yes I agree we should change the spec as well

// If the version string is empty, OnChanOpenInit is expected to
// return a default version string representing the version(s) it supports.
// If there is no default version string for the application,
// it should return an error if provided version is empty string.
OnChanOpenInit(
ctx sdk.Context,
order channeltypes.Order,
Expand All @@ -24,7 +30,7 @@ type IBCModule interface {
channelCap *capabilitytypes.Capability,
counterparty channeltypes.Counterparty,
version string,
) error
) (string, error)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

// OnChanOpenTry will verify the relayer-chosen parameters along with the
// counterparty-chosen version string and perform custom TRY logic.
Expand Down
7 changes: 4 additions & 3 deletions modules/core/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,16 +185,17 @@ func (k Keeper) ChannelOpenInit(goCtx context.Context, msg *channeltypes.MsgChan
}

// Perform application logic callback
if err = cbs.OnChanOpenInit(ctx, msg.Channel.Ordering, msg.Channel.ConnectionHops, msg.PortId, channelID, cap, msg.Channel.Counterparty, msg.Channel.Version); err != nil {
version, err := cbs.OnChanOpenInit(ctx, msg.Channel.Ordering, msg.Channel.ConnectionHops, msg.PortId, channelID, cap, msg.Channel.Counterparty, msg.Channel.Version)
if err != nil {
return nil, sdkerrors.Wrap(err, "channel open init callback failed")
}

// Write channel into state
k.ChannelKeeper.WriteOpenInitChannel(ctx, msg.PortId, channelID, msg.Channel.Ordering, msg.Channel.ConnectionHops, msg.Channel.Counterparty, msg.Channel.Version)
k.ChannelKeeper.WriteOpenInitChannel(ctx, msg.PortId, channelID, msg.Channel.Ordering, msg.Channel.ConnectionHops, msg.Channel.Counterparty, 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.

Changing this causes the fee tests to break. Looking into it 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed: 0178c09


return &channeltypes.MsgChannelOpenInitResponse{
ChannelId: channelID,
Version: msg.Channel.Version,
Version: version,
}, nil
}

Expand Down
4 changes: 4 additions & 0 deletions testing/endpoint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,10 @@ func (endpoint *Endpoint) ChanOpenInit() error {
endpoint.ChannelID, err = ParseChannelIDFromEvents(res.GetEvents())
require.NoError(endpoint.Chain.T, err)

// update version to selected app version
// NOTE: this update must be performed after SendMsgs()
endpoint.ChannelConfig.Version = endpoint.GetChannel().Version

return nil
}

Expand Down
2 changes: 1 addition & 1 deletion testing/mock/ibc_app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ type MockIBCApp struct {
channelCap *capabilitytypes.Capability,
counterparty channeltypes.Counterparty,
version string,
) error
) (string, error)

OnChanOpenTry func(
ctx sdk.Context,
Expand Down
7 changes: 3 additions & 4 deletions testing/mock/ibc_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,18 +32,17 @@ func NewIBCModule(appModule *AppModule, app *MockIBCApp) IBCModule {
func (im IBCModule) OnChanOpenInit(
ctx sdk.Context, order channeltypes.Order, connectionHops []string, portID string,
channelID string, chanCap *capabilitytypes.Capability, counterparty channeltypes.Counterparty, version string,
) error {
) (string, error) {
if im.IBCApp.OnChanOpenInit != nil {
return im.IBCApp.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version)

}

// Claim channel capability passed back by IBC module
if err := im.IBCApp.ScopedKeeper.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