Implement ADR 033 #157
Implement ADR 033 #157
Conversation
…aronc/module-refactor
| } | ||
|
|
||
| rtr.antiReentryMap[moduleName] = true | ||
| defer delete(rtr.antiReentryMap, moduleName) |
There was a problem hiding this comment.
This only works when we won't have async calls in hander (if handler is able to make a async call for other module , then this protection breaks apart.
As an alternative, we never delete from the map. Instead we create a new one once the app will start processing a new transaction.
There was a problem hiding this comment.
Yeah async currently isn't allowed because there is no consensus way to do ordering.
…aronc/module-refactor
Co-authored-by: Robert Zaremba <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #157 +/- ##
==========================================
- Coverage 67.36% 67.23% -0.13%
==========================================
Files 30 30
Lines 1575 1581 +6
==========================================
+ Hits 1061 1063 +2
- Misses 404 406 +2
- Partials 110 112 +2 |
|
Any comments? @robert-zaremba @clevinson ? Does this maybe look good enough to merge? I know there are glaring omissions in terms of docs and unit tests (although most methods are tested by the x/data and x/ecocredit tests). |
There was a problem hiding this comment.
Great work on this @aaronc. I think i've got my head 90% wrapped around all the authorization logic (in Invoker, InvokerFactory, etc.)... but otherwise everything seems clear & makes sense.
Adding a bit more docs/comments would definitely help with some of the readability.
Few small other questions below, but approving as looks good to merge IMO.
| func (mm *Manager) CompleteInitialization() error { | ||
| for typ := range mm.requiredServices { | ||
| if _, found := mm.router.providedServices[typ]; !found { | ||
| return fmt.Errorf("initialization error, service %s was required, but not provided", typ) | ||
| } | ||
|
|
||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The name CompleteInitialization() makes me think that this function performs some action (completing an initialization process). Looking at what it does, I think calling it something like VerifyInitialization() or ValidateInitialization() would be more clear.
There was a problem hiding this comment.
I have in mind this might do some more things later that are more like completion. Maybe we can address later
ref: cosmos/cosmos-sdk#7459 cosmos/cosmos-sdk#7093
also addresses cosmos/cosmos-sdk#7125 and https://github.com/cosmos/cosmos-sdk/issues/8030