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

Jump to bottom

feat(app)!: add ICA Host module with enabled false #1441

Merged
merged 10 commits into from
Sep 1, 2022
Merged

feat(app)!: add ICA Host module with enabled false #1441

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0f4429b
feat: add interchain accounts
Aug 30, 2022
3d6bd95
chore: remove custom handler
Aug 30, 2022
78f986f
chore: add changelog entry
Aug 30, 2022
39362a7
fix: add interchain accounts module to module account perms
Aug 30, 2022
1c21e1e
Merge branch 'main' into ty/1328-ica_module
Aug 30, 2022
0391896
fix: temporary hack to fix simulations
Aug 31, 2022
7a5cb02
feat(app): force ICA host and controller to be disabled
Aug 31, 2022
bf6f818
chore: update todo with issue #
Aug 31, 2022
128fe83
Merge branch 'main' into ty/1328-ica_module
ryanchristo Aug 31, 2022
615943e
Merge branch 'main' into ty/1328-ica_module
ryanchristo Sep 1, 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 @@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
#### Added

- [#1340](https://github.com/regen-network/regen-ledger/pull/1340) Add Cosmos SDK group module to app configuration
- [#1441](https://github.com/regen-network/regen-ledger/pull/1441) Add interchain accounts module

#### Changed

Expand Down
54 changes: 43 additions & 11 deletions app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ import (
govclient "github.com/cosmos/cosmos-sdk/x/gov/client"
paramsclient "github.com/cosmos/cosmos-sdk/x/params/client"
upgradeclient "github.com/cosmos/cosmos-sdk/x/upgrade/client"
ica "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts"
icahost "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/host"
icahosttypes "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/host/types"
icatypes "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/types"

"github.com/gorilla/mux"
"github.com/rakyll/statik/fs"
"github.com/spf13/cast"
Expand Down Expand Up @@ -87,7 +92,7 @@ import (
"github.com/cosmos/cosmos-sdk/x/upgrade"
upgradekeeper "github.com/cosmos/cosmos-sdk/x/upgrade/keeper"
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"

icahostkeeper "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/host/keeper"
"github.com/cosmos/ibc-go/v5/modules/apps/transfer"
ibctransferkeeper "github.com/cosmos/ibc-go/v5/modules/apps/transfer/keeper"
ibctransfertypes "github.com/cosmos/ibc-go/v5/modules/apps/transfer/types"
Expand Down Expand Up @@ -151,13 +156,15 @@ var (
upgradeclient.LegacyProposalHandler, upgradeclient.LegacyCancelProposalHandler,
},
),
ica.AppModuleBasic{},
)

// module account permissions
maccPerms = func() map[string][]string {
perms := map[string][]string{
authtypes.FeeCollectorName: nil,
distrtypes.ModuleName: nil,
icatypes.ModuleName: nil,
minttypes.ModuleName: {authtypes.Minter},
stakingtypes.BondedPoolName: {authtypes.Burner, authtypes.Staking},
stakingtypes.NotBondedPoolName: {authtypes.Burner, authtypes.Staking},
Expand All @@ -177,7 +184,7 @@ func init() {
sdk.DefaultPowerReduction = sdk.NewIntFromBigInt(new(big.Int).Exp(big.NewInt(10), big.NewInt(2), nil))
}

// Extended ABCI application
// RegenApp extends an ABCI application.
type RegenApp struct {
*baseapp.BaseApp
cdc *codec.LegacyAmino
Expand All @@ -203,6 +210,7 @@ type RegenApp struct {
UpgradeKeeper upgradekeeper.Keeper
ParamsKeeper paramskeeper.Keeper
IBCKeeper *ibckeeper.Keeper // IBC Keeper must be a pointer in the app, so we can SetRouter on it correctly
ICAHostKeeper *icahostkeeper.Keeper
EvidenceKeeper evidencekeeper.Keeper
TransferKeeper ibctransferkeeper.Keeper
FeeGrantKeeper feegrantkeeper.Keeper
Expand All @@ -212,7 +220,7 @@ type RegenApp struct {
// make scoped keepers public for test purposes
ScopedIBCKeeper capabilitykeeper.ScopedKeeper
ScopedTransferKeeper capabilitykeeper.ScopedKeeper
ScopedIBCMockKeeper capabilitykeeper.ScopedKeeper
ScopedICAHostKeeper capabilitykeeper.ScopedKeeper

// the module manager
ModuleManager *module.Manager
Expand Down Expand Up @@ -248,6 +256,7 @@ func NewRegenApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest
authzkeeper.StoreKey,
ibchost.StoreKey, ibctransfertypes.StoreKey, group.StoreKey,
ecocredit.ModuleName, data.ModuleName,
icahosttypes.StoreKey,
)

tkeys := sdk.NewTransientStoreKeys(paramstypes.TStoreKey)
Expand All @@ -271,8 +280,9 @@ func NewRegenApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest

// add capability keeper and ScopeToModule for ibc module
app.CapabilityKeeper = capabilitykeeper.NewKeeper(appCodec, keys[capabilitytypes.StoreKey], memKeys[capabilitytypes.MemStoreKey])
scopedIBCKeeper := app.CapabilityKeeper.ScopeToModule(ibchost.ModuleName)
scopedTransferKeeper := app.CapabilityKeeper.ScopeToModule(ibctransfertypes.ModuleName)
app.ScopedIBCKeeper = app.CapabilityKeeper.ScopeToModule(ibchost.ModuleName)
app.ScopedTransferKeeper = app.CapabilityKeeper.ScopeToModule(ibctransfertypes.ModuleName)
app.ScopedICAHostKeeper = app.CapabilityKeeper.ScopeToModule(icahosttypes.SubModuleName)

// Applications that wish to enforce statically created ScopedKeepers should call `Seal` after creating
// their scoped modules in `NewApp` with `ScopeToModule`
Expand Down Expand Up @@ -317,7 +327,7 @@ func NewRegenApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest

// Create IBC Keeper
app.IBCKeeper = ibckeeper.NewKeeper(
appCodec, keys[ibchost.StoreKey], app.GetSubspace(ibchost.ModuleName), app.StakingKeeper, app.UpgradeKeeper, scopedIBCKeeper,
appCodec, keys[ibchost.StoreKey], app.GetSubspace(ibchost.ModuleName), app.StakingKeeper, app.UpgradeKeeper, app.ScopedIBCKeeper,
)

// register the proposal types
Expand All @@ -333,14 +343,30 @@ func NewRegenApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest
appCodec, keys[ibctransfertypes.StoreKey], app.GetSubspace(ibctransfertypes.ModuleName),
app.IBCKeeper.ChannelKeeper,
app.IBCKeeper.ChannelKeeper, &app.IBCKeeper.PortKeeper,
app.AccountKeeper, app.BankKeeper, scopedTransferKeeper,
app.AccountKeeper, app.BankKeeper, app.ScopedTransferKeeper,
)
transferModule := transfer.NewAppModule(app.TransferKeeper)
transferIBCModule := transfer.NewIBCModule(app.TransferKeeper)

icaHostKeeper := icahostkeeper.NewKeeper(
appCodec,
app.keys[icahosttypes.StoreKey],
app.GetSubspace(icahosttypes.SubModuleName),
app.IBCKeeper.ChannelKeeper,
&app.IBCKeeper.PortKeeper,
app.AccountKeeper,
app.ScopedICAHostKeeper,
app.MsgServiceRouter(),
)
app.ICAHostKeeper = &icaHostKeeper

icaHostIBCModule := icahost.NewIBCModule(*app.ICAHostKeeper)

// Create static IBC router, add transfer route, then set and seal it
ibcRouter := porttypes.NewRouter()
ibcRouter.AddRoute(ibctransfertypes.ModuleName, transferIBCModule)
ibcRouter.
AddRoute(icahosttypes.SubModuleName, icaHostIBCModule).
AddRoute(ibctransfertypes.ModuleName, transferIBCModule)
app.IBCKeeper.SetRouter(ibcRouter)

// create evidence keeper with router
Expand Down Expand Up @@ -406,6 +432,11 @@ func NewRegenApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest
ecocreditMod,
dataMod,
groupmodule.NewAppModule(appCodec, app.GroupKeeper, app.AccountKeeper, app.BankKeeper, app.interfaceRegistry),
// TODO: decide if we want to be a controller chain. For now, we only setup the host.
// this means actions can be executed on Regen Ledger from another chain.
// however, until we add the controller, Regen Ledger accounts will not be able to execute messages
// on other chains.
Copy link
Member

Choose a reason for hiding this comment

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

We should wire up the controlled and make sure it is disabled. This way it could be enabled via governance:

https://github.com/cosmos/ibc-go/blob/main/modules/apps/27-interchain-accounts/controller/types/params.go#L9-L12

Copy link
Member

Choose a reason for hiding this comment

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

Currently the command to query the controller is available but it produces the following error:

Error: rpc error: code = Unknown desc = value method github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/controller/keeper.Keeper.Params called using nil *Keeper pointer: panic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah the controller is a lot more tricky than the host. it needs middleware, and in the demo i linked in discord, it appears the thing chain devs should be doing is creating their own x/intertx module (the one in the demo says its experimental and not to use it in prod) ? but it also looks like it can be wired up using a new IBCFee module.

the docs are also a bit confusing for chain devs, as the integration guide mentions an ICAAuth module that can be used for the controller's middleware, but that module doesn't appear to exist ??

we may just want to start w/ host for now and wait for them to update the docs to see what chaindevs should do for integrating controllers 🤷🏻

Copy link
Member

Choose a reason for hiding this comment

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

Can we make this an issue rather than a todo? No need for the todo here but an issue to track would be great.

Copy link
Member

Choose a reason for hiding this comment

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

We can throw it in the backlog. Probably not something we need for v5.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

issue: #1453

commented in code: bf6f818

ica.NewAppModule(nil, app.ICAHostKeeper),
)

// During begin block slashing happens after distr.BeginBlocker so that
Expand Down Expand Up @@ -437,6 +468,7 @@ func NewRegenApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest
// ibc modules
ibchost.ModuleName,
ibctransfertypes.ModuleName,
icatypes.ModuleName,
)
app.ModuleManager.SetOrderEndBlockers(
crisistypes.ModuleName,
Expand All @@ -462,6 +494,7 @@ func NewRegenApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest
// ibc modules
ibchost.ModuleName,
ibctransfertypes.ModuleName,
icatypes.ModuleName,
)
// NOTE: The genutils module must occur after staking so that pools are
// properly initialized with tokens from genesis accounts.
Expand Down Expand Up @@ -492,6 +525,7 @@ func NewRegenApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest
// ibc modules
ibctransfertypes.ModuleName,
ibchost.ModuleName,
icatypes.ModuleName,
)
app.configurator = module.NewConfigurator(app.appCodec, app.MsgServiceRouter(), app.GRPCQueryRouter())
app.ModuleManager.RegisterServices(app.configurator)
Expand Down Expand Up @@ -544,9 +578,6 @@ func NewRegenApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest
}
}

app.ScopedIBCKeeper = scopedIBCKeeper
app.ScopedTransferKeeper = scopedTransferKeeper

return app
}

Expand Down Expand Up @@ -712,6 +743,7 @@ func initParamsKeeper(appCodec codec.BinaryCodec, legacyAmino *codec.LegacyAmino
paramsKeeper.Subspace(ibctransfertypes.ModuleName)
paramsKeeper.Subspace(ibchost.ModuleName)
paramsKeeper.Subspace(ecocredit.DefaultParamspace)
paramsKeeper.Subspace(icahosttypes.SubModuleName)

return paramsKeeper
}
3 changes: 3 additions & 0 deletions app/simulation/app_after_import_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ func TestAppAfterImport(t *testing.T) {
)
require.Equal(t, regen.AppName, app.Name())

// TODO: remove after https://github.com/cosmos/ibc-go/issues/2151 is resolved
removeICAFromSimulation(app)

// run randomized simulation
stopEarly, simParams, simErr := simulateFromSeed(t, app, config)

Expand Down
3 changes: 3 additions & 0 deletions app/simulation/app_import_export_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ func TestAppImportExport(t *testing.T) {
)
require.Equal(t, regen.AppName, app.Name())

// TODO: remove after https://github.com/cosmos/ibc-go/issues/2151 is resolved
removeICAFromSimulation(app)

// run randomized simulation
_, simParams, simErr := simulateFromSeed(t, app, config)

Expand Down
19 changes: 19 additions & 0 deletions app/simulation/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
storetypes "github.com/cosmos/cosmos-sdk/store/types"
simtypes "github.com/cosmos/cosmos-sdk/types/simulation"
"github.com/cosmos/cosmos-sdk/x/simulation"
ica "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts"

regen "github.com/regen-network/regen-ledger/v4/app"
)
Expand Down Expand Up @@ -50,3 +51,21 @@ func simulateFromSeed(t *testing.T, app *regen.RegenApp, config simtypes.Config)
app.AppCodec(),
)
}

// removeICAFromSimulation is a utility function that removes from genesis exporting due to a panic bug.
//
// TODO: remove after https://github.com/cosmos/ibc-go/issues/2151 is resolved
func removeICAFromSimulation(app *regen.RegenApp) {
remove := func(target string, mods []string) []string {
for i, mod := range mods {
if mod == target {
return append(mods[:i], mods[i+1:]...)
}
}
return mods
}

icaModName := ica.AppModule{}.Name()

app.ModuleManager.OrderExportGenesis = remove(icaModName, app.ModuleManager.OrderExportGenesis)
}
Comment on lines +55 to +71
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately ICA module does not implement AppModuleSimulation so the module does initialize with any state during simulations. This in turn causes ExportGenesis calls on the regen.App to panic due to a bug described here cosmos/ibc-go#2151.

This is a temporary workaround until the above issue is resolved on the IBC-go repo, or a more preferable solution is found.

Copy link
Member

Choose a reason for hiding this comment

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

Work around for now works for me.

2 changes: 2 additions & 0 deletions app/stable_appconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"github.com/cosmos/cosmos-sdk/x/auth/ante"
"github.com/cosmos/cosmos-sdk/x/group"
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
icahosttypes "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/host/types"

"github.com/regen-network/regen-ledger/x/data"
"github.com/regen-network/regen-ledger/x/ecocredit"
Expand Down Expand Up @@ -36,6 +37,7 @@ func (app *RegenApp) registerUpgradeHandlers() {
storeUpgrades := storetypes.StoreUpgrades{
Added: []string{
group.ModuleName,
icahosttypes.StoreKey,
Copy link
Member

Choose a reason for hiding this comment

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

We should probably have the host disabled upon upgrade and require a governance proposal to enable it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you mean to edit the upgrade handler to directly set the host params to false? I don't think we can remove the key in the commented line there

in any case, the host submod will be disabled either way, however disabling it manually in the upgrade handler will at least make it's params queryable post-upgrade. wdyt, edit the handler or leave as is?

Copy link
Member

Choose a reason for hiding this comment

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

I just left the comment here because it was the only line changed. I do mean setting the host param to false.

Copy link
Member

Choose a reason for hiding this comment

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

Edit the handler. It will be queryable either way as long as the module is wired up. Currently the upgrade will set the host to enabled by default. We should require a governance proposal to enable it before adding messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah i missed the default genesis setting it to true, i'll set the params in the upgrade handler 👍🏻

},
}

Expand Down