R4R [Staging PR 1/3]: module generalization #4033
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #4033 +/- ##
===========================================
- Coverage 60.16% 59.38% -0.79%
===========================================
Files 212 220 +8
Lines 15187 15284 +97
===========================================
- Hits 9137 9076 -61
- Misses 5421 5581 +160
+ Partials 629 627 -2 |
types/module.go
Outdated
| // routes | ||
| Route() | ||
| NewHandler() | ||
| QuerierRoute() |
There was a problem hiding this comment.
As a side note, can we make queriers more like the handler? Cause right now there is that whole path[0] thing...
There was a problem hiding this comment.
I don't know what you mean... handlers and queriers follow a very similar pattern... open up a new issue? probably should be tackled outside of this PR
|
thanks for the comments @jack - yeah there were a few missing returns in the module type as I hadn't attempted to roll with it yet |
|
There is a fun breaking test as a part of the command to run is |
| type queryRouter struct { | ||
| routes map[string]sdk.Querier | ||
| } | ||
|
|
||
| var _ sdk.QueryRouter = NewQueryRouter() |
There was a problem hiding this comment.
Does the QueryRouter implementation also want to live in types too?
There was a problem hiding this comment.
yes, for the reason mentioned in my previous comment
| type router struct { | ||
| routes map[string]sdk.Handler | ||
| } | ||
|
|
||
| var _ sdk.Router = NewRouter() |
There was a problem hiding this comment.
Does the sdk.Router implementation want to live in types too?
There was a problem hiding this comment.
Correct it's required to live there in order to not cause circular dependancies because the ModuleManager wants to use the the router interface
| @@ -231,7 +231,7 @@ func TestCoinMultiSendGenerateOnly(t *testing.T) { | |||
| var stdTx auth.StdTx | |||
| require.Nil(t, cdc.UnmarshalJSON([]byte(body), &stdTx)) | |||
| require.Equal(t, len(stdTx.Msgs), 1) | |||
| require.Equal(t, stdTx.GetMsgs()[0].Route(), "bank") | |||
| require.Equal(t, stdTx.GetMsgs()[0].Route(), bank.RouterKey) | |||
There was a problem hiding this comment.
Doesn't the AppModule interface have a Name()? Should we be using that instead?
There was a problem hiding this comment.
Yeah I can just kinda change everything to be using the module name, I've currently left things moreless how their structured which is define other constants to the name and then pass those these ways... I'm not sure that really makes sense - probably everything should just be switched to the module name aye?
| @@ -202,6 +189,18 @@ func GaiaAppGenState(cdc *codec.Codec, genDoc tmtypes.GenesisDoc, appGenTxs []js | |||
| return genesisState, nil | |||
| } | |||
|
|
|||
| // GaiaAppGenState but with JSON | |||
| func GaiaAppGenStateJSON(cdc *codec.Codec, genDoc tmtypes.GenesisDoc, appGenTxs []json.RawMessage) ( | |||
There was a problem hiding this comment.
| func GaiaAppGenStateJSON(cdc *codec.Codec, genDoc tmtypes.GenesisDoc, appGenTxs []json.RawMessage) ( | |
| func GaiaAppGenStateToJSON(cdc *codec.Codec, genDoc tmtypes.GenesisDoc, appGenTxs []json.RawMessage) ( |
There was a problem hiding this comment.
Not sure this change makes sense because this function doesn't take genesisState as an input. (this is just a moved function)
|
Started a (partial) review; I will complete later, but feel free to continue and make progress 👍 |
|
|
||
| // NewModuleManager creates a new ModuleManager object | ||
| func NewModuleManager(modules ...AppModule) *ModuleManager { | ||
|
|
There was a problem hiding this comment.
Maybe the module manager wants to store the []string of the order in which the modules were passed in and save it as a default for some of the SetOrder* functions?
There was a problem hiding this comment.
Hmm good idea! - this will save on if statement's latter on
types/staking.go
Outdated
| @@ -160,3 +160,62 @@ type StakingHooks interface { | |||
| AfterDelegationModified(ctx Context, delAddr AccAddress, valAddr ValAddress) | |||
| BeforeValidatorSlashed(ctx Context, valAddr ValAddress, fraction Dec) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Are these hooks generally useful for other staking implementations? Or is this something that could/should live in x/staking?
There was a problem hiding this comment.
hmm yeah good point this belongs in x/staking
| ) | ||
|
|
||
| // name of this module | ||
| const ModuleName = "auth" |
There was a problem hiding this comment.
Why not const Name = "auth" -> auth.Name?
There was a problem hiding this comment.
You're going to need to elabourate
There was a problem hiding this comment.
I think @jackzampolin is taking this from the auth module user perspective as in he'd like to reference the module's name simply as Name instead of ModuleName. @jackzampolin correct me if I am wrong
There was a problem hiding this comment.
Oh yeah that makes sense
| func (AppModule) RegisterInvariants(_ sdk.InvariantRouter) {} | ||
|
|
||
| // module message route name | ||
| func (AppModule) Route() string { return "" } |
There was a problem hiding this comment.
Isn't this supposed to be (or something similar):
func (AppModule) Route() string { return TransactionRoute }There was a problem hiding this comment.
There are no routes, but this function still need to exist to fulfill the AppModule interface
| func (AppModule) Route() string { return "" } | ||
|
|
||
| // module handler | ||
| func (AppModule) NewHandler() sdk.Handler { return nil } |
There was a problem hiding this comment.
This is just not implemented right?
There was a problem hiding this comment.
see above comment
| } | ||
|
|
||
| // NewAppModule creates a new AppModule object | ||
| func NewAppModule(keeper Keeper, accountKeeper auth.AccountKeeper) AppModule { |
There was a problem hiding this comment.
can we pass keys in instead and create the keepers in the modules?
There was a problem hiding this comment.
Due to the interdependence of the keepers on each other that would make things much uglier at the app.go level. I think the high level wiring of the keepers together should stay as is
rigelrozanski
changed the title
rigelrozanski
changed the title
Most downstream PR
Upstream Staging #4128
closes #3006
REF affects #3976
Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (
docs/)Added a relevant changelog entry:
sdkch add [section] [stanza] [message]rereviewed
Files changedin the github PR explorerFor Admin Use: