-
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
ICS 29: Fee Middleware #276
Changes from 28 commits
18feadf
4b9a832
158a251
c63af4a
e3704c6
70c58af
dd4f8c7
67bd594
405f193
8ebbe18
d419972
4dbc83e
c4dff6c
edd11c6
885fb9a
a737132
53b2f67
e0cc81a
120fd76
0fbc6bb
dbda885
9285133
f3e9f95
26731ce
764df84
d761982
f552fb2
16e452b
b16353e
b618f02
13f77de
6cb4a38
e0161a7
dedbb57
6f19978
ea2984b
323c574
fb243c6
8d226de
2c1ff0b
39ef8d7
1fb4b5a
9895948
acc699d
9b2d96d
06f2730
ff15335
d8b9821
adc66d2
4326c14
ad7827f
7c6076f
179c4f4
c14d2b4
99db143
74afccd
6928af7
6999e10
4fb6d18
b02d193
15fa37b
9350d53
f1ba06f
fcea26d
9c508d2
7991f79
9ece5da
f8b4345
e51e2c9
5f8fc9f
4623772
8d380ba
9137084
d788adf
478db4f
71167c4
4bf859a
b33b0a7
f3ee8de
db88c84
637652d
4a0e00c
ab90f07
3ab2251
e1cca36
e65e881
39d4c56
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
package cli | ||
|
||
import ( | ||
"github.com/cosmos/cosmos-sdk/client" | ||
"github.com/spf13/cobra" | ||
) | ||
|
||
// GetQueryCmd returns the query commands for 29-fee | ||
func GetQueryCmd() *cobra.Command { | ||
queryCmd := &cobra.Command{ | ||
Use: "ibc-fee", | ||
Short: "", // TODO | ||
DisableFlagParsing: true, | ||
SuggestionsMinimumDistance: 2, | ||
} | ||
|
||
queryCmd.AddCommand( | ||
// TODO | ||
) | ||
|
||
return queryCmd | ||
} | ||
|
||
// NewTxCmd returns the transaction commands for 29-fee | ||
func NewTxCmd() *cobra.Command { | ||
txCmd := &cobra.Command{ | ||
Use: "ibc-fee", | ||
Short: "", // TODO | ||
DisableFlagParsing: true, | ||
SuggestionsMinimumDistance: 2, | ||
RunE: client.ValidateCmd, | ||
} | ||
|
||
txCmd.AddCommand( | ||
// TODO | ||
) | ||
|
||
return txCmd | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
package cli | ||
|
||
// TODO |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
package cli | ||
|
||
// TODO |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
package fee_test | ||
|
||
import ( | ||
"testing" | ||
|
||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
"github.com/stretchr/testify/suite" | ||
|
||
"github.com/cosmos/ibc-go/v3/modules/apps/29-fee/types" | ||
transfertypes "github.com/cosmos/ibc-go/v3/modules/apps/transfer/types" | ||
clienttypes "github.com/cosmos/ibc-go/v3/modules/core/02-client/types" | ||
channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" | ||
ibctesting "github.com/cosmos/ibc-go/v3/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)) | ||
} | ||
|
||
func (suite *FeeTestSuite) CreateICS20Packet(coin sdk.Coin) channeltypes.Packet { | ||
|
||
fungibleTokenPacket := transfertypes.NewFungibleTokenPacketData( | ||
coin.Denom, | ||
sdk.NewInt(100).String(), | ||
suite.chainA.SenderAccount.GetAddress().String(), | ||
suite.chainB.SenderAccount.GetAddress().String(), | ||
) | ||
|
||
return channeltypes.NewPacket( | ||
fungibleTokenPacket.GetBytes(), | ||
suite.chainA.SenderAccount.GetSequence(), | ||
suite.path.EndpointA.ChannelConfig.PortID, | ||
suite.path.EndpointA.ChannelID, | ||
suite.path.EndpointB.ChannelConfig.PortID, | ||
suite.path.EndpointB.ChannelID, | ||
clienttypes.NewHeight(0, 100), | ||
0, | ||
) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,253 @@ | ||
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/v3/modules/apps/29-fee/keeper" | ||
"github.com/cosmos/ibc-go/v3/modules/apps/29-fee/types" | ||
channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" | ||
porttypes "github.com/cosmos/ibc-go/v3/modules/core/05-port/types" | ||
"github.com/cosmos/ibc-go/v3/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) | ||
} 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 | ||
// If the channel is not fee enabled the underlying application version will be returned | ||
// If the channel is fee enabled we merge the underlying application version with the ics29 version | ||
func (im IBCModule) OnChanOpenTry( | ||
ctx sdk.Context, | ||
order channeltypes.Order, | ||
connectionHops []string, | ||
portID, | ||
channelID string, | ||
chanCap *capabilitytypes.Capability, | ||
counterparty channeltypes.Counterparty, | ||
counterpartyVersion string, | ||
) (string, error) { | ||
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 cpMwVersion == types.Version { | ||
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. | ||
cpAppVersion = counterpartyVersion | ||
} | ||
|
||
// call underlying app's OnChanOpenTry callback with the app versions | ||
appVersion, err := im.app.OnChanOpenTry(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, cpAppVersion) | ||
if err != nil { | ||
return "", err | ||
} | ||
|
||
if !im.keeper.IsFeeEnabled(ctx, portID, channelID) { | ||
return appVersion, nil | ||
} | ||
|
||
return channeltypes.MergeChannelVersions(types.Version, appVersion), nil | ||
} | ||
|
||
// 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) { | ||
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 { | ||
// delete fee enabled on channel | ||
// and refund any remaining fees escrowed on channel | ||
im.keeper.DeleteFeeEnabled(ctx, portID, channelID) | ||
err := im.keeper.RefundFeesOnChannel(ctx, portID, channelID) | ||
// error should only be non-nil if there is a bug in the code | ||
// that causes module account to have insufficient funds to refund | ||
// all escrowed fees on the channel. | ||
// Disable all channels to allow for coordinated fix to the issue | ||
// and mitigate/reverse damage. | ||
// NOTE: Underlying application's packets will still go through, but | ||
// fee module will be disabled for all channels | ||
if err != nil { | ||
im.keeper.DisableAllChannels(ctx) | ||
} | ||
return im.app.OnChanCloseInit(ctx, portID, channelID) | ||
} | ||
|
||
// OnChanCloseConfirm implements the IBCModule interface | ||
func (im IBCModule) OnChanCloseConfirm( | ||
ctx sdk.Context, | ||
portID, | ||
channelID string, | ||
) error { | ||
// delete fee enabled on channel | ||
// and refund any remaining fees escrowed on channel | ||
im.keeper.DeleteFeeEnabled(ctx, portID, channelID) | ||
err := im.keeper.RefundFeesOnChannel(ctx, portID, channelID) | ||
// error should only be non-nil if there is a bug in the code | ||
// that causes module account to have insufficient funds to refund | ||
// all escrowed fees on the channel. | ||
// Disable all channels to allow for coordinated fix to the issue | ||
// and mitigate/reverse damage. | ||
// NOTE: Underlying application's packets will still go through, but | ||
// fee module will be disabled for all channels | ||
if err != nil { | ||
im.keeper.DisableAllChannels(ctx) | ||
} | ||
return im.app.OnChanCloseConfirm(ctx, portID, channelID) | ||
} | ||
|
||
// OnRecvPacket implements the IBCModule interface. | ||
// If fees are not enabled, this callback will default to the ibc-core packet callback | ||
func (im IBCModule) OnRecvPacket( | ||
ctx sdk.Context, | ||
packet channeltypes.Packet, | ||
relayer sdk.AccAddress, | ||
) exported.Acknowledgement { | ||
if !im.keeper.IsFeeEnabled(ctx, packet.DestinationPort, packet.DestinationChannel) { | ||
return im.app.OnRecvPacket(ctx, packet, relayer) | ||
} | ||
|
||
ack := im.app.OnRecvPacket(ctx, packet, relayer) | ||
|
||
forwardRelayer, found := im.keeper.GetCounterpartyAddress(ctx, relayer.String()) | ||
|
||
// incase of async aknowledgement (ack == nil) store the ForwardRelayer address for use later | ||
if ack == nil && found { | ||
im.keeper.SetForwardRelayerAddress(ctx, channeltypes.NewPacketId(packet.GetSourceChannel(), packet.GetSourcePort(), packet.GetSequence()), forwardRelayer) | ||
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. The source channel and source port are not unique on the destination chain. This has to be set with the destination channel and port, otherwise it will get overridden when two source chains have the same source channel and port. To make things clear, consider two separate functions |
||
} | ||
|
||
return types.NewIncentivizedAcknowledgement(forwardRelayer, ack.Acknowledgement()) | ||
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. If I'm not familiar with how async acknowledgement works. Is there any reference for that? 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. I have been thinking that it may be more practical if the fee middleware can be enabled/disabled for each chain individually without requiring negotiation. Would it be possible to insert that information into the acknowledgement bytes while maintaining backward compatibility? For example, can we add an extra field like |
||
} | ||
|
||
// OnAcknowledgementPacket implements the IBCModule interface | ||
// If fees are not enabled, this callback will default to the ibc-core packet callback | ||
func (im IBCModule) OnAcknowledgementPacket( | ||
ctx sdk.Context, | ||
packet channeltypes.Packet, | ||
acknowledgement []byte, | ||
relayer sdk.AccAddress, | ||
) error { | ||
if !im.keeper.IsFeeEnabled(ctx, packet.SourcePort, packet.SourceChannel) { | ||
return im.app.OnAcknowledgementPacket(ctx, packet, acknowledgement, relayer) | ||
} | ||
|
||
ack := new(types.IncentivizedAcknowledgement) | ||
if err := types.ModuleCdc.UnmarshalJSON(acknowledgement, ack); err != nil { | ||
return sdkerrors.Wrapf(err, "cannot unmarshal ICS-29 incentivized packet acknowledgement: %v", ack) | ||
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. I'm not sure of the exact semantics of I have been thinking that it may be more practical if the fee middleware can be enabled/disabled for each chain individually without requiring negotiation. Looking at all the protocol interaction, the only requirement for both sides to enable fee simultaneously is so the ack bytes containing the forward relayer addresss can be decoded correctly. We should instead re-design this such that in case the decoding of the acknowledgement bytes fail, it would simply refund the escrowed receive fee and forward the raw ack to the inner module. Granted, this would also require |
||
} | ||
|
||
packetId := channeltypes.NewPacketId(packet.SourceChannel, packet.SourcePort, packet.Sequence) | ||
|
||
identifiedPacketFee, found := im.keeper.GetFeeInEscrow(ctx, packetId) | ||
if !found { | ||
// return underlying callback if no fee found for given packetID | ||
return im.app.OnAcknowledgementPacket(ctx, packet, ack.Result, relayer) | ||
} | ||
|
||
im.keeper.DistributePacketFees(ctx, identifiedPacketFee.RefundAddress, ack.ForwardRelayerAddress, relayer, identifiedPacketFee) | ||
|
||
// call underlying callback | ||
return im.app.OnAcknowledgementPacket(ctx, packet, ack.Result, relayer) | ||
} | ||
|
||
// OnTimeoutPacket implements the IBCModule interface | ||
// If fees are not enabled, this callback will default to the ibc-core packet callback | ||
func (im IBCModule) OnTimeoutPacket( | ||
ctx sdk.Context, | ||
packet channeltypes.Packet, | ||
relayer sdk.AccAddress, | ||
) error { | ||
if !im.keeper.IsFeeEnabled(ctx, packet.SourcePort, packet.SourceChannel) { | ||
return im.app.OnTimeoutPacket(ctx, packet, relayer) | ||
} | ||
|
||
packetId := channeltypes.NewPacketId(packet.SourceChannel, packet.SourcePort, packet.Sequence) | ||
|
||
identifiedPacketFee, found := im.keeper.GetFeeInEscrow(ctx, packetId) | ||
if !found { | ||
// return underlying callback if fee not found for given packetID | ||
return im.app.OnTimeoutPacket(ctx, packet, relayer) | ||
} | ||
|
||
im.keeper.DistributePacketFeesOnTimeout(ctx, identifiedPacketFee.RefundAddress, relayer, identifiedPacketFee) | ||
|
||
// call underlying callback | ||
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.
Is it safe to just disable the fee for all channels on one chain, without notifying the counterparty chains?
According to the logic of
OnRecvPacket
andOnAcknowledgementPacket
, whether the fee module is enabled or not depends on the local state of the chain. So what would happen if one chain has fee enabled and the other chain has fee disabled, either viaDisableAllChannels
or some other corner cases? Would it cause the rawAcknowledgement
to be serialized and deserialized in different ways, thereby causing the deserialization to fail?