-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
refactor!: migrate to core appmodule.AppModule extension interfaces #13794
Conversation
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.
Left two comments. Wondering why we need to rename interfaces to legacy?
types/module/module.go
Outdated
|
||
// RegisterServices allows a module to register services | ||
// LegacyRegisterServices is the legacy method for modules to register services. | ||
type LegacyRegisterServices interface { |
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.
This is legacy because of the new core 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.
Yes. I have the idea to push the new core API version relatively quickly after this. Or we could do the rename to legacy once the core API version is ready. Don't have a strong preference just thought this was clearer.
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 import will change to use the new api, correct?
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.
Well with extension interfaces imports are actually optional, but yes it might
I've addressed a bunch of review comments and in particular got rid of |
for name, mod := range inputs.Modules { | ||
if basicMod, ok := mod.(module.AppModuleBasic); ok { | ||
app.basicManager[name] = basicMod | ||
basicMod.RegisterInterfaces(inputs.InterfaceRegistry) | ||
basicMod.RegisterLegacyAminoCodec(inputs.LegacyAmino) | ||
} | ||
} |
Check warning
Code scanning / CodeQL
Iteration over map
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 docs lgtm. We backport that to 0.47 right?
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
@@ -81,15 +81,12 @@ type AppInputs struct { | |||
AppConfig *appv1alpha1.Config | |||
Config *runtimev1alpha1.Module | |||
AppBuilder *AppBuilder | |||
Modules map[string]AppModuleWrapper |
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, I think that's a good idea. Totally fine to do in a follow-up too, as this already has approvals. It's not breaking anyways, right?
Yes |
Should we be indicating somehow on the extension interfaces ( |
Also, most of the mock tests for modules no longer work because we are using extension interfaces which the mock app module doesn't have defined. Any thoughts on what to do? I can either:
|
I ended up taking the second approach and created a type |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #13794 +/- ##
==========================================
- Coverage 56.87% 56.44% -0.43%
==========================================
Files 646 648 +2
Lines 55145 56221 +1076
==========================================
+ Hits 31364 31735 +371
- Misses 21226 21924 +698
- Partials 2555 2562 +7
|
|
||
// BeginBlock mocks base method. | ||
func (m *MockAppModuleWithAllExtensions) BeginBlock(arg0 types0.Context, arg1 types1.RequestBeginBlock) { | ||
m.ctrl.T.Helper() |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
// BeginBlock mocks base method. | ||
func (m *MockAppModuleWithAllExtensions) BeginBlock(arg0 types0.Context, arg1 types1.RequestBeginBlock) { | ||
m.ctrl.T.Helper() | ||
m.ctrl.Call(m, "BeginBlock", arg0, arg1) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
|
||
// BeginBlock indicates an expected call of BeginBlock. | ||
func (mr *MockAppModuleWithAllExtensionsMockRecorder) BeginBlock(arg0, arg1 interface{}) *gomock.Call { | ||
mr.mock.ctrl.T.Helper() |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
// BeginBlock indicates an expected call of BeginBlock. | ||
func (mr *MockAppModuleWithAllExtensionsMockRecorder) BeginBlock(arg0, arg1 interface{}) *gomock.Call { | ||
mr.mock.ctrl.T.Helper() | ||
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "BeginBlock", reflect.TypeOf((*MockAppModuleWithAllExtensions)(nil).BeginBlock), arg0, arg1) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
// BeginBlock indicates an expected call of BeginBlock. | ||
func (mr *MockAppModuleWithAllExtensionsMockRecorder) BeginBlock(arg0, arg1 interface{}) *gomock.Call { | ||
mr.mock.ctrl.T.Helper() | ||
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "BeginBlock", reflect.TypeOf((*MockAppModuleWithAllExtensions)(nil).BeginBlock), arg0, arg1) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
|
||
// EndBlock mocks base method. | ||
func (m *MockAppModuleWithAllExtensions) EndBlock(arg0 types0.Context, arg1 types1.RequestEndBlock) []types1.ValidatorUpdate { | ||
m.ctrl.T.Helper() |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
// EndBlock mocks base method. | ||
func (m *MockAppModuleWithAllExtensions) EndBlock(arg0 types0.Context, arg1 types1.RequestEndBlock) []types1.ValidatorUpdate { | ||
m.ctrl.T.Helper() | ||
ret := m.ctrl.Call(m, "EndBlock", arg0, arg1) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
|
||
// EndBlock indicates an expected call of EndBlock. | ||
func (mr *MockAppModuleWithAllExtensionsMockRecorder) EndBlock(arg0, arg1 interface{}) *gomock.Call { | ||
mr.mock.ctrl.T.Helper() |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
// EndBlock indicates an expected call of EndBlock. | ||
func (mr *MockAppModuleWithAllExtensionsMockRecorder) EndBlock(arg0, arg1 interface{}) *gomock.Call { | ||
mr.mock.ctrl.T.Helper() | ||
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "EndBlock", reflect.TypeOf((*MockAppModuleWithAllExtensions)(nil).EndBlock), arg0, arg1) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
// EndBlock indicates an expected call of EndBlock. | ||
func (mr *MockAppModuleWithAllExtensionsMockRecorder) EndBlock(arg0, arg1 interface{}) *gomock.Call { | ||
mr.mock.ctrl.T.Helper() | ||
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "EndBlock", reflect.TypeOf((*MockAppModuleWithAllExtensions)(nil).EndBlock), arg0, arg1) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
…aaronc/appmodule-refactor
Description
This migrates all modules and the module manager to use the
cosmossdk.io/core/appmodule.AppModule
interface. It does this in a mostly backwards compatible way. The main breaking change that is required for modules is to implement the tag methods inappmodule.AppModule
:module.AppModule
andmodule.NewManager
have been deprecated in favor ofappmodule.AppModule
andmodule.NewManagerFromMap
.This refactoring changes the way
runtime
consumes modules usingdepinject
and now modules should provideappmodule.AppModule
.runtime.AppModuleWrapper
andruntime.AppModuleBasicWrapper
have been deleted.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...
!
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