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.

Sign up for GitHub

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

refactor: cleanup middleware stacks #1248

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
227f47b
refactor: UX improvements for middleware
seantking Apr 12, 2022
725936f
fix: move fee keeper
seantking Apr 12, 2022
700ad6a
fix: remove unncessary wrapping
seantking Apr 12, 2022
5852bdf
refactor: ica module stack setup
seantking Apr 13, 2022
94f3e23
fix: define icaControllerStack as type porttypes.IBCModule
colin-axner Apr 13, 2022
e0e405e
add back controller routing for port
colin-axner Apr 13, 2022
fd2360f
refactor: remove unncessary SendPacket
seantking Apr 14, 2022
2793d73
refactor: panic instead of return nil
seantking Apr 14, 2022
b419f8f
refactor: changing structure to init all keepers, then stacks & routing
seantking Apr 14, 2022
4a60ff6
docs: add comments
seantking Apr 14, 2022
2aa1f8c
Merge branch 'main' into sean/issue#773-cleanup-middleware-stacks
seantking Apr 14, 2022
31c3f93
nit: comment
seantking Apr 14, 2022
40fbdcc
chore: rename files
seantking Apr 19, 2022
68f4843
chore: comment + nit
seantking Apr 19, 2022
6624b73
refactor: stacks
seantking Apr 19, 2022
611c5db
Update modules/apps/27-interchain-accounts/controller/ibc_middleware.go
seantking Apr 26, 2022
5898bfc
nits: nits
seantking Apr 27, 2022
80816aa
chore: comment
seantking Apr 27, 2022
112d1c1
Merge branch 'main' into sean/issue#773-cleanup-middleware-stacks
seantking Apr 27, 2022
ac937cb
chore: chore
seantking Apr 28, 2022
80de572
Merge branch 'main' into sean/issue#773-cleanup-middleware-stacks
seantking Apr 28, 2022
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
76 changes: 50 additions & 26 deletions ...erchain-accounts/controller/ibc_module.go → ...ain-accounts/controller/ibc_middleware.go
View file
Open in desktop
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"
Copy link
Member

Choose a reason for hiding this comment

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

This is imported twice

Copy link
Contributor

Choose a reason for hiding this comment

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

we should add a linter for this

Copy link
Contributor

Choose a reason for hiding this comment

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

duplicate import is still there

)

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

// 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{
keeper: k,
// IBCMiddleware creates a new IBCMiddleware given the associated keeper and underlying application
func NewIBCMiddleware(app porttypes.IBCModule, k keeper.Keeper) IBCMiddleware {
return IBCMiddleware{
app: app,
keeper: k,
}
}

// 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 @@ -164,7 +168,27 @@ func (im IBCModule) OnTimeoutPacket(
return im.app.OnTimeoutPacket(ctx, packet, relayer)
}

// SendPacket implements the ICS4 Wrapper interface
// The ICA controller module builds the outgoing ICA packet and calls the core IBC SendPacket
func (im IBCMiddleware) SendPacket(
Copy link
Member

Choose a reason for hiding this comment

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

Ok I understand now why you've done it this way.

I think it should be documented in this function's go-doc that the packet is going to get constructed directly by ICA controller and sent to IBC rather than being created by ICA-auth.

Reading this file, made me think SendPacket wasn't happening at all when really it is happening through the controller's SendTx function

ctx sdk.Context,
chanCap *capabilitytypes.Capability,
packet exported.PacketI,
) error {
panic("SendPacket not supported for ICA-auth module. Please use SendTx")
}

// WriteAcknowledgement implements the ICS4 Wrapper interface
func (im IBCMiddleware) WriteAcknowledgement(
ctx sdk.Context,
chanCap *capabilitytypes.Capability,
packet ibcexported.PacketI,
ack ibcexported.Acknowledgement,
) error {
panic("WriteAcknowledgement not supported for ICA controller module")
}

// GetAppVersion returns the interchain accounts metadata.
func (im IBCModule) GetAppVersion(ctx sdk.Context, portID, channelID string) (string, bool) {
func (im IBCMiddleware) GetAppVersion(ctx sdk.Context, portID, channelID string) (string, bool) {
return im.keeper.GetAppVersion(ctx, portID, channelID)
}
2 changes: 1 addition & 1 deletion ...in-accounts/controller/ibc_module_test.go → ...ccounts/controller/ibc_middleware_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -717,7 +717,7 @@ func (suite *InterchainAccountsTestSuite) TestGetAppVersion() {
cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module)
suite.Require().True(ok)

controllerModule := cbs.(icacontroller.IBCModule)
controllerModule := cbs.(icacontroller.IBCMiddleware)

appVersion, found := controllerModule.GetAppVersion(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
suite.Require().True(found)
Expand Down
2 changes: 0 additions & 2 deletions modules/apps/27-interchain-accounts/module.go
View file
Open in desktop
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
74 changes: 48 additions & 26 deletions modules/apps/29-fee/ibc_module.go → modules/apps/29-fee/ibc_middleware.go
View file
Open in desktop
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{}

// 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,
// NewIBCMiddleware 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 @@ -268,7 +271,26 @@ 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 {
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)
}

// GetAppVersion returns the application version of the underlying application
func (im IBCModule) GetAppVersion(ctx sdk.Context, portID, channelID string) (string, bool) {
func (im IBCMiddleware) GetAppVersion(ctx sdk.Context, portID, channelID string) (string, bool) {
return im.keeper.GetAppVersion(ctx, portID, channelID)
}
2 changes: 1 addition & 1 deletion modules/apps/29-fee/ibc_module_test.go → modules/apps/29-fee/ibc_middleware_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -896,7 +896,7 @@ func (suite *FeeTestSuite) TestGetAppVersion() {
cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module)
suite.Require().True(ok)

feeModule := cbs.(fee.IBCModule)
feeModule := cbs.(fee.IBCMiddleware)

appVersion, found := feeModule.GetAppVersion(suite.chainA.GetContext(), portID, channelID)

Expand Down
2 changes: 0 additions & 2 deletions modules/apps/29-fee/module.go
View file
Open in desktop
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