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: allow the mock module to be used multiple times as base ibc application in middleware stack #892

Merged
merged 9 commits into from
Feb 10, 2022
Merged

refactor: allow the mock module to be used multiple times as base ibc application in middleware stack #892

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
70e6a3a
refactor: allow the mock module to be used multiple times as base ibc…
colin-axner Feb 9, 2022
ba3d43d
add changelog entry
colin-axner Feb 9, 2022
174057e
update testing docs
colin-axner Feb 9, 2022
a78bc3f
chore: add and use 'NewMockIBCApp'
colin-axner Feb 9, 2022
aeb3d61
Merge branch 'main' into colin/ibcmock-improvements
colin-axner Feb 9, 2022
f330f34
Update CHANGELOG.md
colin-axner Feb 9, 2022
0b3d732
fix merge conflicts
colin-axner Feb 10, 2022
5c38780
fix merge conflicts
colin-axner Feb 10, 2022
662b5e7
Update CHANGELOG.md
colin-axner Feb 10, 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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (modules/core/02-client) [\#536](https://github.com/cosmos/ibc-go/pull/536) `GetSelfConsensusState` return type changed from bool to error.
* (channel) [\#644](https://github.com/cosmos/ibc-go/pull/644) Removes `CounterpartyHops` function from the ChannelKeeper.
* (testing) [\#776](https://github.com/cosmos/ibc-go/pull/776) Adding helper fn to generate capability name for testing callbacks
* (testing) [\#892](https://github.com/cosmos/ibc-go/pull/892) IBC Mock modules store the scoped keeper and portID within the IBCMockApp. They also maintain reference to the AppModule to update the AppModule's list of IBC applications it references. Allows for the mock module to be reused as a base application in middleware stacks.
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
* (channel) [\#882](https://github.com/cosmos/ibc-go/pull/882) The `WriteAcknowledgement` API now takes `exported.Acknowledgement` instead of a byte array

### State Machine Breaking
Expand Down
1 change: 1 addition & 0 deletions testing/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,7 @@ The mock module may also be leveraged to act as a base application in the instan

The mock IBC module contains a `MockIBCApp`. This struct contains a function field for every IBC App Module callback.
Each of these functions can be individually set to mock expected behaviour of a base application.
The portID and scoped keeper for the `MockIBCApp` should be set within `MockIBCApp` before calling `NewIBCModule`.

For example, if one wanted to test that the base application cannot affect the outcome of the `OnChanOpenTry` callback, the mock module base application callback could be updated as such:
```go
Expand Down
12 changes: 12 additions & 0 deletions testing/mock/ibc_app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package mock

import (
sdk "github.com/cosmos/cosmos-sdk/types"
capabilitykeeper "github.com/cosmos/cosmos-sdk/x/capability/keeper"
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"

channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
Expand All @@ -10,6 +11,9 @@ import (

// MockIBCApp contains IBC application module callbacks as defined in 05-port.
type MockIBCApp struct {
PortID string
ScopedKeeper capabilitykeeper.ScopedKeeper

OnChanOpenInit func(
ctx sdk.Context,
order channeltypes.Order,
Expand Down Expand Up @@ -81,3 +85,11 @@ type MockIBCApp struct {
relayer sdk.AccAddress,
) error
}

// NewMockIBCApp returns a MockIBCApp. An empty PortID indicates the mock app doesn't bind/claim ports.
func NewMockIBCApp(portID string, scopedKeeper capabilitykeeper.ScopedKeeper) *MockIBCApp {
return &MockIBCApp{
PortID: portID,
ScopedKeeper: scopedKeeper,
}
}
22 changes: 11 additions & 11 deletions testing/mock/ibc_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"strconv"

sdk "github.com/cosmos/cosmos-sdk/types"
capabilitykeeper "github.com/cosmos/cosmos-sdk/x/capability/keeper"
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"

channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
Expand All @@ -16,15 +15,16 @@ import (

// IBCModule implements the ICS26 callbacks for testing/mock.
type IBCModule struct {
IBCApp *MockIBCApp // base application of an IBC middleware stack
scopedKeeper capabilitykeeper.ScopedKeeper
appModule *AppModule
IBCApp *MockIBCApp // base application of an IBC middleware stack
}

// NewIBCModule creates a new IBCModule given the underlying mock IBC application and scopedKeeper.
func NewIBCModule(app *MockIBCApp, scopedKeeper capabilitykeeper.ScopedKeeper) IBCModule {
func NewIBCModule(appModule *AppModule, app *MockIBCApp) IBCModule {
appModule.ibcApps = append(appModule.ibcApps, app)
return IBCModule{
IBCApp: app,
scopedKeeper: scopedKeeper,
appModule: appModule,
IBCApp: app,
}
}

Expand All @@ -39,7 +39,7 @@ func (im IBCModule) OnChanOpenInit(
}

// Claim channel capability passed back by IBC module
if err := im.scopedKeeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil {
if err := im.IBCApp.ScopedKeeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil {
return err
}

Expand All @@ -56,7 +56,7 @@ func (im IBCModule) OnChanOpenTry(
}

// Claim channel capability passed back by IBC module
if err := im.scopedKeeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil {
if err := im.IBCApp.ScopedKeeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil {
return "", err
}

Expand Down Expand Up @@ -107,7 +107,7 @@ func (im IBCModule) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, re

// set state by claiming capability to check if revert happens return
capName := GetMockRecvCanaryCapabilityName(packet)
if _, err := im.scopedKeeper.NewCapability(ctx, capName); err != nil {
if _, err := im.IBCApp.ScopedKeeper.NewCapability(ctx, capName); err != nil {
// application callback called twice on same packet sequence
// must never occur
panic(err)
Expand All @@ -129,7 +129,7 @@ func (im IBCModule) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes
}

capName := GetMockAckCanaryCapabilityName(packet)
if _, err := im.scopedKeeper.NewCapability(ctx, capName); err != nil {
if _, err := im.IBCApp.ScopedKeeper.NewCapability(ctx, capName); err != nil {
// application callback called twice on same packet sequence
// must never occur
panic(err)
Expand All @@ -145,7 +145,7 @@ func (im IBCModule) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Packet,
}

capName := GetMockTimeoutCanaryCapabilityName(packet)
if _, err := im.scopedKeeper.NewCapability(ctx, capName); err != nil {
if _, err := im.IBCApp.ScopedKeeper.NewCapability(ctx, capName); err != nil {
// application callback called twice on same packet sequence
// must never occur
panic(err)
Expand Down
20 changes: 11 additions & 9 deletions testing/mock/mock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/module"
capabilitykeeper "github.com/cosmos/cosmos-sdk/x/capability/keeper"
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
"github.com/gorilla/mux"
"github.com/grpc-ecosystem/grpc-gateway/runtime"
Expand Down Expand Up @@ -89,15 +88,14 @@ func (AppModuleBasic) GetQueryCmd() *cobra.Command {
// AppModule represents the AppModule for the mock module.
type AppModule struct {
AppModuleBasic
scopedKeeper capabilitykeeper.ScopedKeeper
portKeeper PortKeeper
ibcApps []*MockIBCApp
Copy link
Member

Choose a reason for hiding this comment

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

So this is so that one AppModule can be shared between multiple MockIBCApps?
I find it slightly confusing how IBCModule contains a reference to its MockIBCApp but also a reference to AppModule which contains references to all other MockIBCApps.

Is there a way it could be structured so that you could call NewAppModule per mock application? I guess different args would need to be supplied if that was done and maybe complicate things further. Perhaps I'm overthinking things a bit, I'm just thinking out loud here really for what may be the easiest to follow option.

Copy link
Contributor Author

@colin-axner colin-axner Feb 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Is there a way it could be structured so that you could call NewAppModule per mock application?

No this was the purpose of splitting out the IBC callbacks. SDK doesn't allow you to register the same AppModule twice. The alternative approaches:

  • developers pass in all IBC apps to NewAppModule
  • global variable

I felt the dev UX improvements were worth the under the hood magic (hence why those variables are private). I'm happy to change the API if we feel it is better for the ibc apps to be passed in at NewAppModule instantiation

Copy link
Member

Choose a reason for hiding this comment

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

No this was the purpose of splitting out the IBC callbacks

Ah yes, that slipped my mind from before! Got a little lost in the weeds thinking of alternatives. Makes sense along with the context from your other comment too, thanks a lot for the thorough explanation! I agree, since this is kind of hidden away anyway, its totally worth it for the testability of middleware..etc

portKeeper PortKeeper
}

// NewAppModule returns a mock AppModule instance.
func NewAppModule(sk capabilitykeeper.ScopedKeeper, pk PortKeeper) AppModule {
func NewAppModule(pk PortKeeper) AppModule {
return AppModule{
scopedKeeper: sk,
portKeeper: pk,
portKeeper: pk,
}
}

Expand All @@ -124,9 +122,13 @@ func (am AppModule) RegisterServices(module.Configurator) {}

// InitGenesis implements the AppModule interface.
func (am AppModule) InitGenesis(ctx sdk.Context, cdc codec.JSONCodec, data json.RawMessage) []abci.ValidatorUpdate {
// bind mock port ID
cap := am.portKeeper.BindPort(ctx, ModuleName)
am.scopedKeeper.ClaimCapability(ctx, cap, host.PortPath(ModuleName))
for _, ibcApp := range am.ibcApps {
if ibcApp.PortID != "" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

some apps like ica auth modules don't bind/claim ports

// bind mock portID
cap := am.portKeeper.BindPort(ctx, ibcApp.PortID)
ibcApp.ScopedKeeper.ClaimCapability(ctx, cap, host.PortPath(ModuleName))
}
}

return []abci.ValidatorUpdate{}
}
Expand Down
6 changes: 3 additions & 3 deletions testing/simapp/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,8 +350,8 @@ func NewSimApp(

// NOTE: the IBC mock keeper and application module is used only for testing core IBC. Do
// not replicate if you do not need to test core IBC or light clients.
mockModule := ibcmock.NewAppModule(scopedIBCMockKeeper, &app.IBCKeeper.PortKeeper)
mockIBCModule := ibcmock.NewIBCModule(&ibcmock.MockIBCApp{}, scopedIBCMockKeeper)
mockModule := ibcmock.NewAppModule(&app.IBCKeeper.PortKeeper)
mockIBCModule := ibcmock.NewIBCModule(&mockModule, ibcmock.NewMockIBCApp(ibcmock.ModuleName, scopedIBCMockKeeper))

app.ICAControllerKeeper = icacontrollerkeeper.NewKeeper(
appCodec, keys[icacontrollertypes.StoreKey], app.GetSubspace(icacontrollertypes.SubModuleName),
Expand All @@ -369,7 +369,7 @@ func NewSimApp(
icaModule := ica.NewAppModule(&app.ICAControllerKeeper, &app.ICAHostKeeper)

// initialize ICA module with mock module as the authentication module on the controller side
icaAuthModule := ibcmock.NewIBCModule(&ibcmock.MockIBCApp{}, scopedICAMockKeeper)
icaAuthModule := ibcmock.NewIBCModule(&mockModule, ibcmock.NewMockIBCApp("", scopedICAMockKeeper))
app.ICAAuthModule = icaAuthModule

icaControllerIBCModule := icacontroller.NewIBCModule(app.ICAControllerKeeper, icaAuthModule)
Expand Down