-
Notifications
You must be signed in to change notification settings - Fork 624
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
Changes from 23 commits
7b51ebd
150211d
ae319ea
b8f2a7a
fb8522b
1f62964
4711774
dac74bc
d190a19
2f73fd4
f49f0b4
4a73249
d622916
094fef9
763159a
f60897b
a167c49
bc90365
b9028a2
d37a7f7
3deee69
131e95a
40783bf
24d2a20
6b45015
56d80fa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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)) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,176 @@ | ||
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 { | ||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 the fee middleware cannot differentiate between cases:
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
cpAppVersion := counterpartyVersion | ||
if im.keeper.IsFeeEnabled(ctx, portID, channelID) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No because in that case the entire counterparty version Though it's a good thing to test. I added a test case to prove it. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
} |
There was a problem hiding this comment.
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