-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
feat: start app wiring with runtime and x/params modules #11924
Conversation
Looks like the graphviz dep in container breaks the arm build which sounds like a deal breaker. I should probably replace it with something lighter weight like https://github.com/emicklei/dot. Will open an issue |
@@ -151,6 +159,8 @@ require ( | |||
) | |||
|
|||
replace ( | |||
cosmossdk.io/api => ./api |
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.
After this PR we can tag and remove this
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.
How often should we tag a new version then? (as this has not yet been done)
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.
As often as we need to, but with alpha tags only for now
<3 |
We need to refactor this because of the reversion of middleware. I think I will need to tackle x/auth in this PR as well because we need a |
runtime/builder.go
Outdated
for _, option := range a.app.baseAppOptions { | ||
baseAppOptions = append(baseAppOptions, option) | ||
} | ||
bApp := baseapp.NewBaseApp(a.app.config.AppName, logger, db, baseAppOptions...) |
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.
The NewBaseApp
constructor now requires a TxDecoder
param. Before it was using a TxHandler
setter so I didn't need to do this in the constructor.
There are two options:
- do
x/auth
in this PR as well which will provide theTxEncoder
- create a
SetTxEncoder
setter onBaseApp
and allow this to benil
in the constructor
Thoughts
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.
I'm in favor of constructor injection wherever possible, so therefore expanding the scope of this PR to include the x/auth changes seems OK to me.
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.
But we don't want auth to be required in order for app wiring to work. Therefore I think I'll add the SetTxEncoder
method
This has been updated. Would appreciate at least one more look from someone before merging |
return k, runtime.WrapAppModule(m), baseappOpt | ||
} | ||
|
||
func provideSubSpace(key container.ModuleKey, k keeper.Keeper) types.Subspace { |
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.
I don't see this output being used anywhere... are there future plans for it or have I missed something?
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.
In pretty much every follow up PR it will be used
👀 Trying to understand the context for the test failures-
|
- go mod tidy
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.
LGTM.
2 CI failures which seem OK:
../../../../pkg/mod/golang.org/x/[email protected]/constraints/constraints.go:41:2: syntax error: unexpected ~, expecting method or interface name
../../../../pkg/mod/golang.org/x/[email protected]/constraints/constraints.go:49:10: syntax error: unexpected |, expecting semicolon or newline or }
note: module requires Go 1.18
gobencher seems not compatible with go 1.18.
and in the liveness-test (make localnet-start
)
Step 8/20 : RUN go mod download
---> Running in cdba6f9ada8f
go: cosmossdk.io/[email protected] (replaced by ./api): reading api/go.mod: open /work/api/go.mod: no such file or directory
I think will be resolved when a release of core
is made and the replace
directives in ./go.mod
can be removed.
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.
✅ late approval. Seems like different pieces are finally getting together.
func initParamsKeeper(appCodec codec.BinaryCodec, legacyAmino *codec.LegacyAmino, key, tkey storetypes.StoreKey) paramskeeper.Keeper { | ||
paramsKeeper := paramskeeper.NewKeeper(appCodec, legacyAmino, key, tkey) | ||
|
||
func initParamsKeeper(paramsKeeper paramskeeper.Keeper) { |
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.
out of curiosity: How would this function look in the final app-wired simapp?
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.
it would be gone
|
||
// NOTE: The genutils module must occur after staking so that pools are | ||
// properly initialized with tokens from genesis accounts. | ||
// NOTE: The genutils module must also occur after auth so that it can access the params from auth. | ||
// NOTE: Capability module must occur first so that it can initialize any capabilities | ||
// so that other modules that want to create or claim capabilities afterwards in InitChain | ||
// can do so safely. | ||
app.mm.SetOrderInitGenesis( | ||
app.ModuleManager.SetOrderInitGenesis( |
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.
Any reason you kept this here, instead of putting it into the runtime module config?
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.
because there is some custom stuff happening with x/upgrade and module versions. once we get to x/upgrade i think we can switch this
// RegisterTxService implements the Application.RegisterTxService method. | ||
func (a *App) RegisterTxService(clientCtx client.Context) { | ||
authtx.RegisterTxService(a.GRPCQueryRouter(), clientCtx, a.Simulate, a.interfaceRegistry) | ||
} | ||
|
||
// RegisterTendermintService implements the Application.RegisterTendermintService method. | ||
func (a *App) RegisterTendermintService(clientCtx client.Context) { |
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.
these 2 seem weird in the runtime. Can they be modules?
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.
yes, they should be. at least they shouldn't be standalone methods on server.Application
. maybe runtime should register the tendermint service, but authtx should either go in auth or we should have a standalone tx
or auth/tx
module. should we add an issue as part of the epic to fix this?
## Description This was causing sim failures like: ``` exporting genesis... 2022-05-25T10:36:38-05:00 INFO asserting crisis invariants inv=0/12 module=x/crisis name=gov/module-account 2022-05-25T10:36:38-05:00 INFO asserting crisis invariants inv=1/12 module=x/crisis name=bank/nonnegative-outstanding 2022-05-25T10:36:38-05:00 INFO asserting crisis invariants inv=2/12 module=x/crisis name=bank/total-supply 2022-05-25T10:36:38-05:00 INFO asserting crisis invariants inv=3/12 module=x/crisis name=group/Group-TotalWeight 2022-05-25T10:36:38-05:00 INFO asserting crisis invariants inv=4/12 module=x/crisis name=distribution/nonnegative-outstanding 2022-05-25T10:36:38-05:00 INFO asserting crisis invariants inv=5/12 module=x/crisis name=distribution/can-withdraw 2022-05-25T10:36:38-05:00 INFO asserting crisis invariants inv=6/12 module=x/crisis name=distribution/reference-count 2022-05-25T10:36:38-05:00 INFO asserting crisis invariants inv=7/12 module=x/crisis name=distribution/module-account 2022-05-25T10:36:38-05:00 INFO asserting crisis invariants inv=8/12 module=x/crisis name=staking/module-accounts 2022-05-25T10:36:38-05:00 INFO asserting crisis invariants inv=9/12 module=x/crisis name=staking/nonnegative-power 2022-05-25T10:36:38-05:00 INFO asserting crisis invariants inv=10/12 module=x/crisis name=staking/positive-delegation 2022-05-25T10:36:38-05:00 INFO asserting crisis invariants inv=11/12 module=x/crisis name=staking/delegator-shares 2022-05-25T10:36:39-05:00 INFO asserted all invariants duration=1194.381198 height=50 module=x/crisis importing genesis... --- FAIL: TestAppSimulationAfterImport (20.98s) panic: validator set is empty after InitGenesis, please ensure at least one validator is initialized with a delegation greater than or equal to the DefaultPowerReduction ({824635701280}) [recovered] panic: validator set is empty after InitGenesis, please ensure at least one validator is initialized with a delegation greater than or equal to the DefaultPowerReduction ({824635701280}) goroutine 76 [running]: testing.tRunner.func1.2({0x1b88020, 0xc0035d70b0}) /home/mkoco/sdk/go1.18.2/src/testing/testing.go:1389 +0x24e testing.tRunner.func1() /home/mkoco/sdk/go1.18.2/src/testing/testing.go:1392 +0x39f panic({0x1b88020, 0xc0035d70b0}) /home/mkoco/sdk/go1.18.2/src/runtime/panic.go:838 +0x207 github.com/cosmos/cosmos-sdk/types/module.(*Manager).InitGenesis(_, {{0x28b5990, 0xc0001b2000}, {0x28c2da0, 0xc002494600}, {{0x0, 0x0}, {0x0, 0x0}, 0x0, ...}, ...}, ...) /home/mkoco/dev/regen/cosmos-sdk/types/module/module.go:329 +0x4fd github.com/cosmos/cosmos-sdk/runtime.(*App).InitChainer(_, {{0x28b5990, 0xc0001b2000}, {0x28c2da0, 0xc002494600}, {{0x0, 0x0}, {0x0, 0x0}, 0x0, ...}, ...}, ...) /home/mkoco/dev/regen/cosmos-sdk/runtime/app.go:121 +0x12e github.com/cosmos/cosmos-sdk/simapp.(*SimApp).InitChainer(_, {{0x28b5990, 0xc0001b2000}, {0x28c2da0, 0xc002494600}, {{0x0, 0x0}, {0x0, 0x0}, 0x0, ...}, ...}, ...) /home/mkoco/dev/regen/cosmos-sdk/simapp/app.go:504 +0x178 github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).InitChain(0xc000fb1340, {{0x0, 0x0, 0x0}, {0x0, 0x0}, 0x0, {0x0, 0x0, 0x0}, ...}) /home/mkoco/dev/regen/cosmos-sdk/baseapp/abci.go:69 +0x3f5 github.com/cosmos/cosmos-sdk/simapp.TestAppSimulationAfterImport(0xc000abf520) /home/mkoco/dev/regen/cosmos-sdk/simapp/sim_test.go:269 +0xd67 testing.tRunner(0xc000abf520, 0x2590ac0) /home/mkoco/sdk/go1.18.2/src/testing/testing.go:1439 +0x102 created by testing.(*T).Run /home/mkoco/sdk/go1.18.2/src/testing/testing.go:1486 +0x35f FAIL github.com/cosmos/cosmos-sdk/simapp 21.018s FAIL ``` ### Root cause Prior to merge of #11924, SimApp calls [module/NewManager](https://github.com/cosmos/cosmos-sdk/blob/2b549b807c13f66dca68965206163ca400d0dbe0/simapp/app.go#L344), which implicitly sets sane default values of [OrderExportGenesis](https://github.com/cosmos/cosmos-sdk/blob/2b549b807c13f66dca68965206163ca400d0dbe0/types/module/module.go#L239) from the modules passed as parameters. Since this usage was deprecated in favor of [RegisterModules](https://github.com/cosmos/cosmos-sdk/blob/823d2a7f28f10a360b543748a6e90e01bb5d3c23/simapp/app.go#L354) in the app wiring work, `OrderExportGenesis` was now `nil`, resulting in empty `genesisData` in [ExportGenesis](https://github.com/cosmos/cosmos-sdk/blob/823d2a7f28f10a360b543748a6e90e01bb5d3c23/types/module/module.go#L339) ### Fix Explicitly set `OrderExportGenesis` with same module order as `OrderInitGenesis` from `SimApp`. --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)
* feat: start app wiring with runtime and x/params modules * WIP * WIP * docs * docs, cleanup * fixing tests * rollback unrelated changes * fix * test fixes * simplification, tests * fix tests * docs * go mod tidy * update module path * codegen * address middleware removal * update container alpha 4 * Fix cosmossdk.io/api dependency conflict - go mod tidy Co-authored-by: Matt Kocubinski <[email protected]>
## Description This was causing sim failures like: ``` exporting genesis... 2022-05-25T10:36:38-05:00 INFO asserting crisis invariants inv=0/12 module=x/crisis name=gov/module-account 2022-05-25T10:36:38-05:00 INFO asserting crisis invariants inv=1/12 module=x/crisis name=bank/nonnegative-outstanding 2022-05-25T10:36:38-05:00 INFO asserting crisis invariants inv=2/12 module=x/crisis name=bank/total-supply 2022-05-25T10:36:38-05:00 INFO asserting crisis invariants inv=3/12 module=x/crisis name=group/Group-TotalWeight 2022-05-25T10:36:38-05:00 INFO asserting crisis invariants inv=4/12 module=x/crisis name=distribution/nonnegative-outstanding 2022-05-25T10:36:38-05:00 INFO asserting crisis invariants inv=5/12 module=x/crisis name=distribution/can-withdraw 2022-05-25T10:36:38-05:00 INFO asserting crisis invariants inv=6/12 module=x/crisis name=distribution/reference-count 2022-05-25T10:36:38-05:00 INFO asserting crisis invariants inv=7/12 module=x/crisis name=distribution/module-account 2022-05-25T10:36:38-05:00 INFO asserting crisis invariants inv=8/12 module=x/crisis name=staking/module-accounts 2022-05-25T10:36:38-05:00 INFO asserting crisis invariants inv=9/12 module=x/crisis name=staking/nonnegative-power 2022-05-25T10:36:38-05:00 INFO asserting crisis invariants inv=10/12 module=x/crisis name=staking/positive-delegation 2022-05-25T10:36:38-05:00 INFO asserting crisis invariants inv=11/12 module=x/crisis name=staking/delegator-shares 2022-05-25T10:36:39-05:00 INFO asserted all invariants duration=1194.381198 height=50 module=x/crisis importing genesis... --- FAIL: TestAppSimulationAfterImport (20.98s) panic: validator set is empty after InitGenesis, please ensure at least one validator is initialized with a delegation greater than or equal to the DefaultPowerReduction ({824635701280}) [recovered] panic: validator set is empty after InitGenesis, please ensure at least one validator is initialized with a delegation greater than or equal to the DefaultPowerReduction ({824635701280}) goroutine 76 [running]: testing.tRunner.func1.2({0x1b88020, 0xc0035d70b0}) /home/mkoco/sdk/go1.18.2/src/testing/testing.go:1389 +0x24e testing.tRunner.func1() /home/mkoco/sdk/go1.18.2/src/testing/testing.go:1392 +0x39f panic({0x1b88020, 0xc0035d70b0}) /home/mkoco/sdk/go1.18.2/src/runtime/panic.go:838 +0x207 github.com/cosmos/cosmos-sdk/types/module.(*Manager).InitGenesis(_, {{0x28b5990, 0xc0001b2000}, {0x28c2da0, 0xc002494600}, {{0x0, 0x0}, {0x0, 0x0}, 0x0, ...}, ...}, ...) /home/mkoco/dev/regen/cosmos-sdk/types/module/module.go:329 +0x4fd github.com/cosmos/cosmos-sdk/runtime.(*App).InitChainer(_, {{0x28b5990, 0xc0001b2000}, {0x28c2da0, 0xc002494600}, {{0x0, 0x0}, {0x0, 0x0}, 0x0, ...}, ...}, ...) /home/mkoco/dev/regen/cosmos-sdk/runtime/app.go:121 +0x12e github.com/cosmos/cosmos-sdk/simapp.(*SimApp).InitChainer(_, {{0x28b5990, 0xc0001b2000}, {0x28c2da0, 0xc002494600}, {{0x0, 0x0}, {0x0, 0x0}, 0x0, ...}, ...}, ...) /home/mkoco/dev/regen/cosmos-sdk/simapp/app.go:504 +0x178 github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).InitChain(0xc000fb1340, {{0x0, 0x0, 0x0}, {0x0, 0x0}, 0x0, {0x0, 0x0, 0x0}, ...}) /home/mkoco/dev/regen/cosmos-sdk/baseapp/abci.go:69 +0x3f5 github.com/cosmos/cosmos-sdk/simapp.TestAppSimulationAfterImport(0xc000abf520) /home/mkoco/dev/regen/cosmos-sdk/simapp/sim_test.go:269 +0xd67 testing.tRunner(0xc000abf520, 0x2590ac0) /home/mkoco/sdk/go1.18.2/src/testing/testing.go:1439 +0x102 created by testing.(*T).Run /home/mkoco/sdk/go1.18.2/src/testing/testing.go:1486 +0x35f FAIL github.com/cosmos/cosmos-sdk/simapp 21.018s FAIL ``` ### Root cause Prior to merge of cosmos#11924, SimApp calls [module/NewManager](https://github.com/cosmos/cosmos-sdk/blob/2b549b807c13f66dca68965206163ca400d0dbe0/simapp/app.go#L344), which implicitly sets sane default values of [OrderExportGenesis](https://github.com/cosmos/cosmos-sdk/blob/2b549b807c13f66dca68965206163ca400d0dbe0/types/module/module.go#L239) from the modules passed as parameters. Since this usage was deprecated in favor of [RegisterModules](https://github.com/cosmos/cosmos-sdk/blob/823d2a7f28f10a360b543748a6e90e01bb5d3c23/simapp/app.go#L354) in the app wiring work, `OrderExportGenesis` was now `nil`, resulting in empty `genesisData` in [ExportGenesis](https://github.com/cosmos/cosmos-sdk/blob/823d2a7f28f10a360b543748a6e90e01bb5d3c23/types/module/module.go#L339) ### Fix Explicitly set `OrderExportGenesis` with same module order as `OrderInitGenesis` from `SimApp`. --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)
Description
Ref: #11899
This PR initializes app wiring in the most basic way I could think of (more basic than #11900).
It adds:
runtime
module which allows for a hybrid app config/old app.go setupx/params
moduleruntime
does some of the stuffapp.go
used to do (begin and end blockers),app.go
still does init chain and registers services (for now)x/params
keeper is injected by the containerAuthor Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change