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 all 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
41 changes: 41 additions & 0 deletions modules/apps/29-fee/fee_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package fee_test

import (
"testing"

"github.com/stretchr/testify/suite"

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

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))
}
178 changes: 178 additions & 0 deletions modules/apps/29-fee/ibc_module.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,178 @@
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"
"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 {
mwVersion, appVersion := channeltypes.SplitChannelVersion(version)
// Since it is valid for fee version to not be specified, the above middleware version may be for a middleware
// lower down in the stack. Thus, if it is not a fee version we pass the entire version string onto the underlying
// application.
// If an invalid fee version was passed, we expect the underlying application to fail on its version negotiation.
if mwVersion == types.Version {
im.keeper.SetFeeEnabled(ctx, portID, channelID)
Comment on lines +40 to +46
Copy link
Member Author

Choose a reason for hiding this comment

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

Note since we no longer require that the fee version is specified, it's possible that the middleware version we get is actually for some middleware lower in the stack. So we must pass the full version on in this case.

Note: For a stack fee -> whitelist -> transfer

the fee middleware cannot differentiate between cases:

  1. wrongfee29-A:whitelist:ics20-1
  2. whitelist:ics20-1

So in both cases, it will assume fee is not enabled and pass the full version to the underlying app.

However in case 1, the underlying app will fail its version negotiation since the wrongfee string is unexpected.

} else {
// middleware version is not the expected version for this midddleware. Pass the full version string along,
// if it not valid version for any other lower middleware, an error will be returned by base application.
appVersion = version
}

// 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 {
mwVersion, appVersion := channeltypes.SplitChannelVersion(version)
cpMwVersion, cpAppVersion := channeltypes.SplitChannelVersion(counterpartyVersion)

// Since it is valid for fee version to not be specified, the above middleware version may be for a middleware
// lower down in the stack. Thus, if it is not a fee version we pass the entire version string onto the underlying
// application.
// If an invalid fee version was passed, we expect the underlying application to fail on its version negotiation.
if mwVersion == types.Version || cpMwVersion == types.Version {
if cpMwVersion != mwVersion {
return sdkerrors.Wrapf(types.ErrInvalidVersion, "fee versions do not match. self version: %s, counterparty version: %s", mwVersion, cpMwVersion)
}

im.keeper.SetFeeEnabled(ctx, portID, channelID)
} else {
// middleware versions are not the expected version for this midddleware. Pass the full version strings along,
// if it not valid version for any other lower middleware, an error will be returned by base application.
appVersion = version
cpAppVersion = counterpartyVersion
}
Comment on lines +70 to +88
Copy link
Member Author

Choose a reason for hiding this comment

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

ditto the above logic, but for the TRY case


// 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 {
// If handshake was initialized with fee enabled it must complete with fee enabled.
// If handshake was initialized with fee disabled it must complete with fee disabled.
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