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.

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

feat: Add handshake logic to ics29 #307

Merged
merged 26 commits into from
Sep 27, 2021
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
7b51ebd
do handshake logic, create test file
AdityaSripal Aug 3, 2021
150211d
do cap logic and fix build
AdityaSripal Aug 5, 2021
ae319ea
open handshake implementation and tests
AdityaSripal Sep 2, 2021
b8f2a7a
remove prints
AdityaSripal Sep 2, 2021
fb8522b
Merge branch 'ics29-fee-middleware' into aditya/handshake
AdityaSripal Sep 2, 2021
1f62964
Update modules/apps/29-fee/module.go
AdityaSripal Sep 3, 2021
4711774
debugging progress
AdityaSripal Sep 10, 2021
dac74bc
fee enabled flag
AdityaSripal Sep 16, 2021
d190a19
cleanup handshake logic
AdityaSripal Sep 20, 2021
2f73fd4
fix merge
AdityaSripal Sep 20, 2021
f49f0b4
fix tests
AdityaSripal Sep 20, 2021
4a73249
much cleaner simapp
AdityaSripal Sep 20, 2021
d622916
split module.go file
AdityaSripal Sep 20, 2021
094fef9
cleanup and docs
AdityaSripal Sep 20, 2021
763159a
assert IBC interfaces are fulfilled in middleware
AdityaSripal Sep 20, 2021
f60897b
Update modules/apps/transfer/module.go
AdityaSripal Sep 20, 2021
a167c49
fix merge
AdityaSripal Sep 20, 2021
bc90365
Apply suggestions from code review
AdityaSripal Sep 23, 2021
b9028a2
fix unnecessary crossing hello logic
AdityaSripal Sep 24, 2021
d37a7f7
fix version negotiation bugs and improve tests
AdityaSripal Sep 24, 2021
3deee69
Merge branch 'aditya/handshake' of github.com:cosmos/ibc-go into adit…
AdityaSripal Sep 24, 2021
131e95a
cleanup tests
AdityaSripal Sep 24, 2021
40783bf
Apply suggestions from code review
AdityaSripal Sep 24, 2021
24d2a20
Apply suggestions from code review
AdityaSripal Sep 27, 2021
6b45015
address rest of colin comments
AdityaSripal Sep 27, 2021
56d80fa
Merge branch 'aditya/handshake' of github.com:cosmos/ibc-go into adit…
AdityaSripal Sep 27, 2021
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
40 changes: 40 additions & 0 deletions modules/apps/29-fee/fee_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package fee_test

import (
"testing"

"github.com/cosmos/ibc-go/modules/apps/29-fee/types"
transfertypes "github.com/cosmos/ibc-go/modules/apps/transfer/types"
channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types"
ibctesting "github.com/cosmos/ibc-go/testing"
"github.com/stretchr/testify/suite"
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
)

type FeeTestSuite struct {
suite.Suite

coordinator *ibctesting.Coordinator

chainA *ibctesting.TestChain
chainB *ibctesting.TestChain

path *ibctesting.Path
}

func (suite *FeeTestSuite) SetupTest() {
suite.coordinator = ibctesting.NewCoordinator(suite.T(), 2)
suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(0))
suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(1))

path := ibctesting.NewPath(suite.chainA, suite.chainB)
feeTransferVersion := channeltypes.MergeChannelVersions(types.Version, transfertypes.Version)
path.EndpointA.ChannelConfig.Version = feeTransferVersion
path.EndpointB.ChannelConfig.Version = feeTransferVersion
path.EndpointA.ChannelConfig.PortID = transfertypes.PortID
path.EndpointB.ChannelConfig.PortID = transfertypes.PortID
suite.path = path
}

func TestIBCFeeTestSuite(t *testing.T) {
suite.Run(t, new(FeeTestSuite))
}
167 changes: 167 additions & 0 deletions modules/apps/29-fee/ibc_module.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
package fee

import (
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
"github.com/cosmos/ibc-go/modules/apps/29-fee/keeper"
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
"github.com/cosmos/ibc-go/modules/apps/29-fee/types"
channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types"
porttypes "github.com/cosmos/ibc-go/modules/core/05-port/types"
ibcexported "github.com/cosmos/ibc-go/modules/core/exported"
)

// IBCModule implements the ICS26 callbacks for the fee middleware given the fee keeper and the underlying application.
type IBCModule struct {
Copy link
Member Author

Choose a reason for hiding this comment

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

Separated IBCModule struct from AppModule

AppModule should only be instantiated once and will hook up to msg server, genesis, etc

IBCModule may be instantiated any number of times with different underlying applications

keeper keeper.Keeper
app porttypes.IBCModule
}

// NewIBCModule creates a new IBCModule given the keeper and underlying application
func NewIBCModule(k keeper.Keeper, app porttypes.IBCModule) IBCModule {
return IBCModule{
keeper: k,
app: app,
}
}

// OnChanOpenInit implements the IBCModule interface
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 {
feeVersion, appVersion := channeltypes.SplitChannelVersion(version)
if feeVersion != "" {
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
if feeVersion != types.Version {
return sdkerrors.Wrapf(types.ErrInvalidVersion, "expected: %s, got: %s", types.Version, feeVersion)
}

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

// OnChanOpenTry implements the IBCModule interface
func (im IBCModule) OnChanOpenTry(
ctx sdk.Context,
order channeltypes.Order,
connectionHops []string,
portID,
channelID string,
chanCap *capabilitytypes.Capability,
counterparty channeltypes.Counterparty,
version,
counterpartyVersion string,
) error {
feeVersion, appVersion := channeltypes.SplitChannelVersion(version)
cpFeeVersion, cpAppVersion := channeltypes.SplitChannelVersion(counterpartyVersion)

if feeVersion != "" || cpFeeVersion != "" {
if feeVersion != types.Version {
return sdkerrors.Wrapf(types.ErrInvalidVersion, "expected: %s, got: %s", types.Version, feeVersion)
}
if cpFeeVersion != feeVersion {
return sdkerrors.Wrapf(types.ErrInvalidVersion, "expected counterparty version: %s, got: %s", types.Version, cpFeeVersion)
}
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved

im.keeper.SetFeeEnabled(ctx, portID, channelID)
} else if im.keeper.IsFeeEnabled(ctx, portID, channelID) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm pretty sure crossing hello's are not possible on INIT with auto-generated IDs. So I think I can remove this case. @colin-axner

Copy link
Contributor

Choose a reason for hiding this comment

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

Crossing hello's are still possible. I'm not sure I understand why they wouldn't be? The counterparty channel identifiers are not specified until OpenTry

so in this case:
chainA init channel-1 for fees
chainB init channel-2 for fees (or even without fee version)

chainA attempts try for pairing to channel-2 with a non fee version

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out this is irrelevant. We don't have to check here because core IBC checks that in case of crossing hellos,

the channel params in current TRY are exact same as the params in previous INIT: https://github.com/cosmos/ibc-go/blob/main/modules/core/04-channel/keeper/handshake.go#L131

which is exactly the behaviour we want out of core IBC, so we don't have to handle that case in the application.

I fixed the incorrect godoc in this PR.

// return error if a previous ChanInit set fee enabled but subsequent OpenTry does not have fee enabled
return sdkerrors.Wrapf(types.ErrInvalidVersion, "previous INIT call (crossing hellos) had fee version set but OpenTry call does not set fee version: %s", version)
}
// call underlying app's OnChanOpenTry callback with the app versions
return im.app.OnChanOpenTry(ctx, order, connectionHops, portID, channelID,
chanCap, counterparty, appVersion, cpAppVersion)
}

// OnChanOpenAck implements the IBCModule interface
func (im IBCModule) OnChanOpenAck(
ctx sdk.Context,
portID,
channelID string,
counterpartyVersion string,
) error {
cpAppVersion := counterpartyVersion
if im.keeper.IsFeeEnabled(ctx, portID, channelID) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It is possible the version changed. If INIT started with non fee version and Ack returns with fee version, this would perform incorrect behavior

Since OpenTry chooses the version, my preference is still to set fee enabled on try/ack. Let the relayer decide how the channels will be used. I don't see the benefit of deciding on INIT. From my point of view, it reduces the code complexity

Copy link
Member Author

Choose a reason for hiding this comment

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

No because in that case the entire counterparty version fee29-1:ics20-1 would be passed down to transfer which would error.

Though it's a good thing to test. I added a test case to prove it.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't want future relayers to override the initial decisions of past relayers.

Future relayers should only complete the handshake as initial relayers intended it

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. Can you add a inline comment to the code?

var cpFeeVersion string
cpFeeVersion, cpAppVersion = channeltypes.SplitChannelVersion(counterpartyVersion)

if cpFeeVersion != types.Version {
return sdkerrors.Wrapf(types.ErrInvalidVersion, "expected counterparty version: %s, got: %s", types.Version, cpFeeVersion)
}
}
// call underlying app's OnChanOpenAck callback with the counterparty app version.
return im.app.OnChanOpenAck(ctx, portID, channelID, cpAppVersion)
}

// OnChanOpenConfirm implements the IBCModule interface
func (im IBCModule) OnChanOpenConfirm(
ctx sdk.Context,
portID,
channelID string,
) error {
// call underlying app's OnChanOpenConfirm callback.
return im.app.OnChanOpenConfirm(ctx, portID, channelID)
}

// OnChanCloseInit implements the IBCModule interface
func (im IBCModule) OnChanCloseInit(
ctx sdk.Context,
portID,
channelID string,
) error {
// TODO: Unescrow all remaining funds for unprocessed packets
im.keeper.DeleteFeeEnabled(ctx, portID, channelID)
return im.app.OnChanCloseInit(ctx, portID, channelID)
}

// OnChanCloseConfirm implements the IBCModule interface
func (im IBCModule) OnChanCloseConfirm(
ctx sdk.Context,
portID,
channelID string,
) error {
// TODO: Unescrow all remaining funds for unprocessed packets
im.keeper.DeleteFeeEnabled(ctx, portID, channelID)
return im.app.OnChanCloseConfirm(ctx, portID, channelID)
}

// OnRecvPacket implements the IBCModule interface.
func (im IBCModule) OnRecvPacket(
ctx sdk.Context,
packet channeltypes.Packet,
relayer sdk.AccAddress,
) ibcexported.Acknowledgement {
// TODO: Implement fee specific logic if fee is enabled for the given channel
return im.app.OnRecvPacket(ctx, packet, relayer)
}

// OnAcknowledgementPacket implements the IBCModule interface
func (im IBCModule) OnAcknowledgementPacket(
ctx sdk.Context,
packet channeltypes.Packet,
acknowledgement []byte,
relayer sdk.AccAddress,
) error {
// TODO: Implement fee specific logic if fee is enabled for the given channel
return im.app.OnAcknowledgementPacket(ctx, packet, acknowledgement, relayer)
}

// OnTimeoutPacket implements the IBCModule interface
func (im IBCModule) OnTimeoutPacket(
ctx sdk.Context,
packet channeltypes.Packet,
relayer sdk.AccAddress,
) error {
// TODO: Implement fee specific logic if fee is enabled for the given channel
return im.app.OnTimeoutPacket(ctx, packet, relayer)
}
Loading