[Bug]: Feegrant and authz keepers not getting their bank keepers.

[SpicyLemon]

Is there an existing issue for this?

• I have searched the existing issues

What happened?

A bug happened!

A bank keeper was recently added to both the feegrant and authz keepers. In order to not be a breaking change, a SetBankKeeper method was added to both keepers and that method is called in NewAppModule for both modules.

However, the bank keeper is remaining nil in both keepers used by an app.

There are two problems:

1. The receivers for the SetBankKeeper methods are concrete. So it's actually creating a copy of the keeper struct, changing the bank keeper field in the new one, and returning the new one. It doesn't actually update the original keeper.
2. The keepers are being provided to the NewAppModule functions as their struct (not a reference). So any changes made to the keeper inside NewAppModule do not affect the keeper that was provided to that function. The module struct will have a copy of the keeper that has the bank module set in it, but the keeper that's in the app (being provided to other keepers and modules) does not have the bank keeper.

Cosmos SDK Version

0.50.7

How to reproduce?

No response

[julienrbrt]

It should be okay given that the check is only added in the message (and we use appmodule that contains the keeper with the bank keeper, so it is where it matters in the flow).
However, we can indeed provide it with the updated bank keeper in depinject.

We couldn't change NewKeeper to return a reference as it would have been breaking anyway. Users not using depinject already have to use SetBankKeeper manually as well if they wanted to provide it in the app to other modules. However I agree that we can make it better for the keeper provided by depinject 👍

[SpicyLemon]

It should be okay given that the check is only added in the message (and we use appmodule that contains the keeper with the bank keeper, so it is where it matters in the flow). However, we can indeed provide it with the updated bank keeper in depinject.

We couldn't change NewKeeper to return a reference as it would have been breaking anyway. Users not using depinject already have to use SetBankKeeper manually as well if they wanted to provide it in the app to other modules. However I agree that we can make it better for the keeper provided by depinject 👍

I get why it was done that way. I guess people probably just need to know to add calls of .SetBankKeeper to their authz and feegrant keepers if they're using v0.50.6+, specially if those keepers are to be used by other modules. If using depinject, I'm not even sure how they can work around this until #20590 is in a release. It looks like that PR fixes the depinject route, but if not using depinject, people will still need to add the .SetBankKeeper calls even once that PR is in a release.

Semi-relatedly, people not using depinject also need to call app.UpgradeKeeper.SetInitVersionMap(app.ModuleManager.GetVersionMap()) in their app constructor or else the app-state-determinism sim will fail due to missing module-version state entries in the second app. I'm not entirely sure its much of a problem outside that test though.

[tac0turtle]

is this completed and released @julienrbrt

[julienrbrt]

I get why it was done that way. I guess people probably just need to know to add calls of .SetBankKeeper to their authz and feegrant keepers if they're using v0.50.6+, specially if those keepers are to be used by other modules. If using depinject, I'm not even sure how they can work around this until #20590 is in a release. It looks like that PR fixes the depinject route, but if not using depinject, people will still need to add the .SetBankKeeper calls even once that PR is in a release.

True, I've updated the release notes to mention this. For v0.50, our monthly release is next week, but it is already released for v0.47.

Semi-relatedly, people not using depinject also need to call app.UpgradeKeeper.SetInitVersionMap(app.ModuleManager.GetVersionMap()) in their app constructor or else the app-state-determinism sim will fail due to missing module-version state entries in the second app. I'm not entirely sure its much of a problem outside that test though.

Yes that's true. This is due to depinject injecting upgrade before you are able to register non-depinject modules later on runtime: https://github.com/cosmos/cosmos-sdk/blob/v0.50.6/simapp/app_v2.go#L253-L262. It is a problem at chain upgrade, so it is imperative to uncomment it when using depinject with non depinject modules.

is this completed and released @julienrbrt

Completed, v0.50.8 is planned next week. We can close this.