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

refactor: cleanup middleware stacks #1248

Merged
merged 21 commits into from
Apr 28, 2022
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
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
71 changes: 47 additions & 24 deletions modules/apps/27-interchain-accounts/controller/ibc_module.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,30 +10,34 @@ import (
icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/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"
ibcexported "github.com/cosmos/ibc-go/v3/modules/core/exported"
)

// IBCModule implements the ICS26 interface for interchain accounts controller chains
type IBCModule struct {
keeper keeper.Keeper
var _ porttypes.Middleware = &IBCMiddleware{}

// IBCMiddlware implements the ICS26 callbacks for the fee middleware given the
Copy link
Contributor

Choose a reason for hiding this comment

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

...callbacks for the fee middleware given...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The next line continues the comment :)

// IBCMiddlware implements the ICS26 callbacks for the fee middleware given the
// ICA controller keeper and the underlying application.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// IBCMiddlware implements the ICS26 callbacks for the fee middleware given the
// IBCMiddleware implements the ICS26 callbacks for the fee middleware given the

// ICA controller keeper and the underlying application.
type IBCMiddleware struct {
app porttypes.IBCModule
keeper keeper.Keeper
}

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

// OnChanOpenInit implements the IBCModule interface
// OnChanOpenInit implements the IBCMiddleware interface
//
// Interchain Accounts is implemented to act as middleware for connected authentication modules on
// the controller side. The connected modules may not change the controller side portID or
// version. They will be allowed to perform custom logic without changing
// the parameters stored within a channel struct.
func (im IBCModule) OnChanOpenInit(
func (im IBCMiddleware) OnChanOpenInit(
ctx sdk.Context,
order channeltypes.Order,
connectionHops []string,
Expand All @@ -56,8 +60,8 @@ func (im IBCModule) OnChanOpenInit(
chanCap, counterparty, version)
}

// OnChanOpenTry implements the IBCModule interface
func (im IBCModule) OnChanOpenTry(
// OnChanOpenTry implements the IBCMiddleware interface
func (im IBCMiddleware) OnChanOpenTry(
ctx sdk.Context,
order channeltypes.Order,
connectionHops []string,
Expand All @@ -70,13 +74,13 @@ func (im IBCModule) OnChanOpenTry(
return "", sdkerrors.Wrap(icatypes.ErrInvalidChannelFlow, "channel handshake must be initiated by controller chain")
}

// OnChanOpenAck implements the IBCModule interface
// OnChanOpenAck implements the IBCMiddleware interface
//
// Interchain Accounts is implemented to act as middleware for connected authentication modules on
// the controller side. The connected modules may not change the portID or
// version. They will be allowed to perform custom logic without changing
// the parameters stored within a channel struct.
func (im IBCModule) OnChanOpenAck(
func (im IBCMiddleware) OnChanOpenAck(
ctx sdk.Context,
portID,
channelID string,
Expand All @@ -95,17 +99,17 @@ func (im IBCModule) OnChanOpenAck(
return im.app.OnChanOpenAck(ctx, portID, channelID, counterpartyChannelID, counterpartyVersion)
}

// OnChanOpenAck implements the IBCModule interface
func (im IBCModule) OnChanOpenConfirm(
// OnChanOpenAck implements the IBCMiddleware interface
func (im IBCMiddleware) OnChanOpenConfirm(
ctx sdk.Context,
portID,
channelID string,
) error {
return sdkerrors.Wrap(icatypes.ErrInvalidChannelFlow, "channel handshake must be initiated by controller chain")
}

// OnChanCloseInit implements the IBCModule interface
func (im IBCModule) OnChanCloseInit(
// OnChanCloseInit implements the IBCMiddleware interface
func (im IBCMiddleware) OnChanCloseInit(
ctx sdk.Context,
portID,
channelID string,
Expand All @@ -114,26 +118,26 @@ func (im IBCModule) OnChanCloseInit(
return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "user cannot close channel")
}

// OnChanCloseConfirm implements the IBCModule interface
func (im IBCModule) OnChanCloseConfirm(
// OnChanCloseConfirm implements the IBCMiddleware interface
func (im IBCMiddleware) OnChanCloseConfirm(
ctx sdk.Context,
portID,
channelID string,
) error {
return im.keeper.OnChanCloseConfirm(ctx, portID, channelID)
}

// OnRecvPacket implements the IBCModule interface
func (im IBCModule) OnRecvPacket(
// OnRecvPacket implements the IBCMiddleware interface
func (im IBCMiddleware) OnRecvPacket(
ctx sdk.Context,
packet channeltypes.Packet,
_ sdk.AccAddress,
) ibcexported.Acknowledgement {
return channeltypes.NewErrorAcknowledgement("cannot receive packet on controller chain")
}

// OnAcknowledgementPacket implements the IBCModule interface
func (im IBCModule) OnAcknowledgementPacket(
// OnAcknowledgementPacket implements the IBCMiddleware interface
func (im IBCMiddleware) OnAcknowledgementPacket(
ctx sdk.Context,
packet channeltypes.Packet,
acknowledgement []byte,
Expand All @@ -147,8 +151,8 @@ func (im IBCModule) OnAcknowledgementPacket(
return im.app.OnAcknowledgementPacket(ctx, packet, acknowledgement, relayer)
}

// OnTimeoutPacket implements the IBCModule interface
func (im IBCModule) OnTimeoutPacket(
// OnTimeoutPacket implements the IBCMiddleware interface
func (im IBCMiddleware) OnTimeoutPacket(
ctx sdk.Context,
packet channeltypes.Packet,
relayer sdk.AccAddress,
Expand All @@ -163,3 +167,22 @@ func (im IBCModule) OnTimeoutPacket(

return im.app.OnTimeoutPacket(ctx, packet, relayer)
}

// SendPacket implements the ICS4 Wrapper interface
func (im IBCMiddleware) SendPacket(
ctx sdk.Context,
chanCap *capabilitytypes.Capability,
packet exported.PacketI,
) error {
panic("SendPacket not supported for ICA controller module")
Copy link
Member

Choose a reason for hiding this comment

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

ICA auth SendPacket should call this function and then this function will pass the call one layer up.

Copy link
Contributor

@colin-axner colin-axner Apr 21, 2022

Choose a reason for hiding this comment

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

Why would this occur? ICA auth calls SendTx. ICA auth shouldn't send non ICA packets?

Copy link
Member

Choose a reason for hiding this comment

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

ICA auth is being wrapped by ICA correct? In that case we will want the packet flow to be:

ICA-Auth -SendPacket-> ICA -SendPacket-> IBC

The ICA middleware in this case will do nothing but forward the packet one layer up. This is to keep consistency and to make sure ICA-auth packets are not missed by additional middleware that is wrapping the stack

Copy link
Contributor

@colin-axner colin-axner Apr 25, 2022

Choose a reason for hiding this comment

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

ICA auth is being wrapped by ICA correct?

Only partially (not all callbacks/interfaces should be implemented). SendPacket usage is replaced by SendTx

ICA controller constructs the packet and thus calls SendPacket. ICA Auth shouldn't call SendPacket. It could bypass SendTx by calling SendPacket directly, if we panic, we disable this functionality. I don't understand the purpose of SendTx if we do allow bypassing. The spec doesn't indicate the appropriate functionality

}

// WriteAcknowledgement implements the ICS4 Wrapper interface
func (im IBCMiddleware) WriteAcknowledgement(
ctx sdk.Context,
chanCap *capabilitytypes.Capability,
packet exported.PacketI,
ack exported.Acknowledgement,
) error {
panic("WriteAcknowledgement not supported for ICA controller module")
}
2 changes: 0 additions & 2 deletions modules/apps/27-interchain-accounts/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (
abci "github.com/tendermint/tendermint/abci/types"

"github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/client/cli"
"github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/controller"
controllerkeeper "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/controller/keeper"
controllertypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/controller/types"
"github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/host"
Expand All @@ -31,7 +30,6 @@ var (
_ module.AppModule = AppModule{}
_ module.AppModuleBasic = AppModuleBasic{}

_ porttypes.IBCModule = controller.IBCModule{}
_ porttypes.IBCModule = host.IBCModule{}
)

Expand Down
72 changes: 47 additions & 25 deletions modules/apps/29-fee/ibc_module.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,22 +12,25 @@ import (
"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
var _ porttypes.Middleware = &IBCMiddleware{}

// IBCMiddlware implements the ICS26 callbacks for the fee middleware given the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// IBCMiddlware implements the ICS26 callbacks for the fee middleware given the
// IBCMiddleware implements the ICS26 callbacks for the fee middleware given the

// fee keeper and the underlying application.
type IBCMiddleware struct {
app porttypes.IBCModule
keeper keeper.Keeper
}

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

// OnChanOpenInit implements the IBCModule interface
func (im IBCModule) OnChanOpenInit(
// OnChanOpenInit implements the IBCMiddleware interface
func (im IBCMiddleware) OnChanOpenInit(
ctx sdk.Context,
order channeltypes.Order,
connectionHops []string,
Expand Down Expand Up @@ -57,10 +60,10 @@ func (im IBCModule) OnChanOpenInit(
chanCap, counterparty, versionMetadata.AppVersion)
}

// OnChanOpenTry implements the IBCModule interface
// OnChanOpenTry implements the IBCMiddleware 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(
func (im IBCMiddleware) OnChanOpenTry(
ctx sdk.Context,
order channeltypes.Order,
connectionHops []string,
Expand Down Expand Up @@ -100,8 +103,8 @@ func (im IBCModule) OnChanOpenTry(
return string(versionBytes), nil
}

// OnChanOpenAck implements the IBCModule interface
func (im IBCModule) OnChanOpenAck(
// OnChanOpenAck implements the IBCMiddleware interface
func (im IBCMiddleware) OnChanOpenAck(
ctx sdk.Context,
portID,
channelID string,
Expand All @@ -128,8 +131,8 @@ func (im IBCModule) OnChanOpenAck(
return im.app.OnChanOpenAck(ctx, portID, channelID, counterpartyChannelID, counterpartyVersion)
}

// OnChanOpenConfirm implements the IBCModule interface
func (im IBCModule) OnChanOpenConfirm(
// OnChanOpenConfirm implements the IBCMiddleware interface
func (im IBCMiddleware) OnChanOpenConfirm(
ctx sdk.Context,
portID,
channelID string,
Expand All @@ -138,8 +141,8 @@ func (im IBCModule) OnChanOpenConfirm(
return im.app.OnChanOpenConfirm(ctx, portID, channelID)
}

// OnChanCloseInit implements the IBCModule interface
func (im IBCModule) OnChanCloseInit(
// OnChanCloseInit implements the IBCMiddleware interface
func (im IBCMiddleware) OnChanCloseInit(
ctx sdk.Context,
portID,
channelID string,
Expand All @@ -155,8 +158,8 @@ func (im IBCModule) OnChanCloseInit(
return nil
}

// OnChanCloseConfirm implements the IBCModule interface
func (im IBCModule) OnChanCloseConfirm(
// OnChanCloseConfirm implements the IBCMiddleware interface
func (im IBCMiddleware) OnChanCloseConfirm(
ctx sdk.Context,
portID,
channelID string,
Expand All @@ -172,9 +175,9 @@ func (im IBCModule) OnChanCloseConfirm(
return nil
}

// OnRecvPacket implements the IBCModule interface.
// OnRecvPacket implements the IBCMiddleware interface.
// If fees are not enabled, this callback will default to the ibc-core packet callback
func (im IBCModule) OnRecvPacket(
func (im IBCMiddleware) OnRecvPacket(
ctx sdk.Context,
packet channeltypes.Packet,
relayer sdk.AccAddress,
Expand All @@ -197,9 +200,9 @@ func (im IBCModule) OnRecvPacket(
return types.NewIncentivizedAcknowledgement(forwardRelayer, ack.Acknowledgement(), ack.Success())
}

// OnAcknowledgementPacket implements the IBCModule interface
// OnAcknowledgementPacket implements the IBCMiddleware interface
// If fees are not enabled, this callback will default to the ibc-core packet callback
func (im IBCModule) OnAcknowledgementPacket(
func (im IBCMiddleware) OnAcknowledgementPacket(
ctx sdk.Context,
packet channeltypes.Packet,
acknowledgement []byte,
Expand Down Expand Up @@ -239,9 +242,9 @@ func (im IBCModule) OnAcknowledgementPacket(
return im.app.OnAcknowledgementPacket(ctx, packet, ack.Result, relayer)
}

// OnTimeoutPacket implements the IBCModule interface
// OnTimeoutPacket implements the IBCMiddleware interface
// If fees are not enabled, this callback will default to the ibc-core packet callback
func (im IBCModule) OnTimeoutPacket(
func (im IBCMiddleware) OnTimeoutPacket(
ctx sdk.Context,
packet channeltypes.Packet,
relayer sdk.AccAddress,
Expand All @@ -267,3 +270,22 @@ func (im IBCModule) OnTimeoutPacket(
// call underlying callback
return im.app.OnTimeoutPacket(ctx, packet, relayer)
}

// SendPacket implements the ICS4 Wrapper interface
func (im IBCMiddleware) SendPacket(
ctx sdk.Context,
chanCap *capabilitytypes.Capability,
packet exported.PacketI,
) error {
return im.keeper.SendPacket(ctx, chanCap, packet)
}

// WriteAcknowledgement implements the ICS4 Wrapper interface
func (im IBCMiddleware) WriteAcknowledgement(
ctx sdk.Context,
chanCap *capabilitytypes.Capability,
packet exported.PacketI,
ack exported.Acknowledgement,
) error {
return im.keeper.WriteAcknowledgement(ctx, chanCap, packet, ack)
}
2 changes: 0 additions & 2 deletions modules/apps/29-fee/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,10 @@ import (
"github.com/cosmos/ibc-go/v3/modules/apps/29-fee/client/cli"
"github.com/cosmos/ibc-go/v3/modules/apps/29-fee/keeper"
"github.com/cosmos/ibc-go/v3/modules/apps/29-fee/types"
porttypes "github.com/cosmos/ibc-go/v3/modules/core/05-port/types"
)

var (
_ module.AppModule = AppModule{}
_ porttypes.IBCModule = IBCModule{}
_ module.AppModuleBasic = AppModuleBasic{}
)

Expand Down
Loading