fixed recursive queries losing context #213
Conversation
Codecov Report
@@ Coverage Diff @@
## master #213 +/- ##
=======================================
Coverage 72.02% 72.02%
=======================================
Files 27 27
Lines 2624 2624
=======================================
Hits 1890 1890
Misses 621 621
Partials 113 113
Continue to review full report at Codecov.
|
| @@ -82,7 +82,7 @@ func NewKeeper(cdc *codec.Codec, storeKey sdk.StoreKey, paramSpace params.Subspa | |||
| authZPolicy: DefaultAuthorizationPolicy{}, | |||
| paramSpace: paramSpace, | |||
| } | |||
| keeper.queryPlugins = DefaultQueryPlugins(bankKeeper, stakingKeeper, keeper).Merge(customPlugins) | |||
| keeper.queryPlugins = DefaultQueryPlugins(bankKeeper, stakingKeeper, &keeper).Merge(customPlugins) | |||
There was a problem hiding this comment.
Nice to use a pointer/reference here. I support it.
Ah... now I get it. We copy the keeper into the query plugins before we set keeper.queryPlugins 🤯 🤦
Now I understand how this ends up as nil and all the issues.
Thanks for hunting it down, I was super busy today with other org stuff.
There was a problem hiding this comment.
Something that may or may not be a bug, similarly to this, is that all the methods on the keeper take it by value. I think you should review the codebase for this sort of thing, because there may be many more places where we copy things that should be passed by reference.
There was a problem hiding this comment.
This is all over the cosmos sdk. I remember a large debate with the core devs when someone wanted to make the keepers pointers. There was big backlash on the grounds of immuntabilitt or such. Anyway, I agree with you, I prefer pointer methods
This PR fixes an issue where performing recursive Smart Queries would fail on the second recursion, because
keeper.queryPluginswould be uninitialized. The problem was thatDefaultQueryPluginsandWasmQuerierwere getting partially initialized copies of the keeper object, and would try to use the partly initialized copy instead of the concrete object.In our testing in SecretNetwork, this fix allowed us to recurse queries multiple times (x >= 5) :)
CC: @assafmo