feat(bank)!: Move the bank module SendEnabled info into state (from Params). #11977
Conversation
…or getting, setting, and deleting them.
…cluded." This reverts commit 8b7acf8.
…roto." This reverts commit 0bd904c.
…oto." This reverts commit 33d4652.
…endEnabledCoins. Store creation has some overhead.
…ple TODO notes. Create a way to iterate over the SendEnabled entries and get all of them.
…ed entries similar to when they were params.
…unction significantlly simpler.
# Conflicts: # api/cosmos/bank/v1beta1/bank.pulsar.go # api/cosmos/bank/v1beta1/genesis.pulsar.go
…o longer returns {} when there aren't any SendEnabled entries and the default is false. SendEnabled is back to outputting a yaml format.
… set SendEnabled.
…or MsgSetSendEnabled.
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
…tion too. Co-authored-by: Aleksandr Bezobchuk <[email protected]>
…entiate between a missing entry and one that's using the default value.
dwedul-figure
dismissed
alexanderbez’s stale review
All concerns have been addressed.
|
Hey @dwedul-figure, thanks for pushing this over the finish line! However, I think this PR was merged just a bit prematurely. The core changes in the PR look great, but there's still a few things that need to be polished up prior to being merged into In general, I'd like to see an approval from at least one core SDK team member prior to merging substantial units of work. Would you be open to addressing the last few remaining items? |
I guess I didn't realize that @robert-zaremba wasn't part of the core sdk team. The review I dismissed as "All concerns have been addressed." was about the version string to use in comments. But that discussion looked like it was over, settled on using What other changes do you feel were needed? |
|
The proto comments in this PR follow the ADR 44. |
|
There's still usage of default params which should not exist, also there is some general structure cleanup we should perform. |
I'm asking for specifics here. At this point, I feel the best way forward is for you to create an issue and I'll open a new PR just for those changes. And I'm feeling a bit defensive here, so I'd like to point out that neither of those concerns were brought up in the four weeks that this PR was open. Continually "And Then"ing a PR is disrespectful of everyone's time. |
| func (m Migrator) Migrate3to4(ctx sdk.Context) error { | ||
| oldParams := m.keeper.GetParams(ctx) | ||
| m.keeper.SetAllSendEnabled(ctx, oldParams.GetSendEnabled()) | ||
| m.keeper.SetParams(ctx, banktypes.NewParams(oldParams.DefaultSendEnabled)) |
There was a problem hiding this comment.
Why are we calling this line?
There was a problem hiding this comment.
The Params.DefaultSendEnabled field is not being deprecated. It still exists and is functional.
This line clears out all of the Params.SendEnabled entries since they're all now stored directly in the store. This makes it significantly easier (cheaper) to look up the Params.DefaultSendEnabled value when needed.
There was a problem hiding this comment.
It is deprecated -- that's the point of this feature. Deprecate the DefaultSendEnabled and use a more strongly structured list. In fact, the Proto schema states it's deprecated.
There was a problem hiding this comment.
No. The SendEnabled entries are deprecated. The DefaultSendEnabled is not. DefaultSendEnabled is just a single boolean that is used when there is no SendEnabled entry for a specific denom. The Params.SendEnabled field was a list of denoms and whether sending is enabled for that specific denom. Not all denoms have to have entries, and not all entries are different from the default.
The point of this feature is to move the SendEnabled list out of the Params so that when we need to know whether or not sending is allowed for a denom, we can look up just that single denom. With all of those SendEnabled flags in Params, in order to check a single denom, we have to read all of them in, then loop over the list looking for the denom in question. But in both cases, if there's no entry for a denom, there needs to be a default to use. There's already functionality for defining and controlling that default, and it's Params.DefaultSendEnabled. And since that doesn't have the growth problem that the SendEnabled list does, there's no reason to remove it from Params.
As a reference, the Provenance Blockchain testnet has over 2000 denoms which has rendered the bank module nearly unusable.
| if len(params.SendEnabled) > 0 { | ||
| k.SetAllSendEnabled(ctx, params.SendEnabled) | ||
| } | ||
| p := types.NewParams(params.DefaultSendEnabled) |
There was a problem hiding this comment.
Why are we modifying default/legacy params?
There was a problem hiding this comment.
The Params.DefaultSendEnabled field is not deprecated.
| } | ||
| store := ctx.KVStore(k.storeKey) | ||
| haveDefault := false | ||
| var defaultVal bool |
There was a problem hiding this comment.
This is an incorrect implementation. Again, we should not be referencing or using the legacy params at all.
There was a problem hiding this comment.
Here again, the Params.DefaultSendEnabled value is not depreciated and is still to be used. It is the fallback for cases when there is no SendEnabled entry for a denom. That functionality hasn't changed. The extra code in here just makes it so that the bank module's Params only need to be looked up once during this function call (rather than once per coin).
|
|
||
| // IsSendEnabledDenom returns the current SendEnabled status of the provided denom. | ||
| func (k BaseSendKeeper) IsSendEnabledDenom(ctx sdk.Context, denom string) bool { | ||
| return k.getSendEnabledOrDefault(ctx.KVStore(k.storeKey), denom, func() bool { return k.GetParams(ctx).DefaultSendEnabled }) |
There was a problem hiding this comment.
This needs to be undone -- again, legacy or default params should not be used.
There was a problem hiding this comment.
The Params.DefaultSendEnabled field is not deprecated. Only the Params.SendEnabled field.
There still needs to be a default defined, and currently, all chains have it defined in the bank module's Params.DefaultSendEnabled. And it works just fine being there, so there's no reason to change that.
|
I understand @dwedul-figure, but keep in mind our team is small and we have tons of going on. A PR like this is relatively substantial and it takes time to thoroughly review. I've add some additional comments. |
I'm sorry. I'm frustrated and didn't mean to take it out on you. I will show more restraint in the future. I'm frustrated because I feel like my PRs don't get much attention, and end up being open for significantly longer than what I'm used to. Also having an open PR requires a not insignificant amount of my time and energy on a regular basis, which is why I really like to get them completed as quickly as possible. I'm also sorry I dismissed your review and merged the PR. I will have more patience next time, and I won't do it again. |
|
This may fall on me as well for not including it in the teams sprint. the past two weeks were focused on the sprint board, which can be followed here: https://github.com/orgs/cosmos/projects/26/views/22. If your pr is sitting around for a while, please feel free to ping a member of the team to try to get your review on the sprint board. This will lead to faster responses and reviews |
|
We will keep this PR and have follow up PRs with minor tweaks and adjustments. Thanks for the valuable contribution @dwedul-figure :) |
…arams). (cosmos#11977) * go mod tidy everything. * Add some third_party proto files that are imported but not included. * [11859]: Add a new key for the SendEnabled flags and keeper methods for getting, setting, and deleting them. * [11859]: Remove the send_enabled field from the bank Params proto. * Revert "Add some third_party proto files that are imported but not included." This reverts commit 8b7acf8. * [11859]: Regenerate the bank params stuff from the changed proto. * [11859]: Add a send_enabled field to the bank genesis proto. * Revert "[11859]: Remove the send_enabled field from the bank Params proto." This reverts commit 0bd904c. * Revert "[11859]: Regenerate the bank params stuff from the changed proto." This reverts commit 33d4652. * [11859]: Deprecate the bank Params send_enabled field. * [11859]: Regenerate the bank go code from the updated protos. * [11859]: Reduce the number of times the store is recreated during IsSendEnabledCoins. Store creation has some overhead. * [11859]: Add the SendEnabled stuff to the genesis methods. Make a couple TODO notes. Create a way to iterate over the SendEnabled entries and get all of them. * [11859]: Update the bank sim genesis stuff to create random SendEnabled entries similar to when they were params. * Remove some of the bank params methods that are no longer meaningful. * Add a comment about why we're calling a mutation method in a Validate function. * [11859]: Add some more TODO notes and make the SendEnabled.String() function significantlly simpler. * [11859]: Get rid of the SendEnabledParams type. * Fix up a few comments. * [11859]: Update the bank keeper test due to recent changes. * [11859]: Tweak the bank Params and SendEnabled String funcs. Params no longer returns {} when there aren't any SendEnabled entries and the default is false. SendEnabled is back to outputting a yaml format. * [11859]: Fix the params tests and add some new ones to it and key_test. * [11859]: Create a 1-store method for updating several SendEnabled entries at once. * [11859]: Create a migration for both the module and genesis state. * [11859]: Create a new MsgSetSendEnabled for governanance proposals to set SendEnabled. * [11859]: Add SetAllSendEnabled to the SendKeeper interface. * [11859]: Add an authority to the bank keeper and create the handler for MsgSetSendEnabled. * [11859]: Add an rpc endpoint for querying SendEnabled. * [11859]: Implement the SendEnabled query. * [11859]: Add a function for decoding a --page-key base64 value so that pagination can work as expected. * [11859]: Implement a CLI command for querying SendEnabled. * [11859]: Move the v047 store migration stuff into Migrate3to4 directly to prevent a circular dependency between 047 and the keeper. Not using the keeper for that would be a significant pain in the butt. * [11869]: Implement the Msg interface for MsgSetSendEnabled. * [11859]: Fix some unit tests that I broke along the way. * [11859]: Reorg the funcs added to the SendKeeper interface. * [11859]: Fix the return values of a couple of the MsgSetSendEnabled LegacyMsg funcs. * [11859]: Tweak MigrateSendEnabled to add stuff to the existing slice (if there's anything to add). And then use that in the MigrateGenState function. * [11859]: Don't set the Pagination field when looking up specific entries. * [11859]: Put validateSendEnabledParams back to the way it was to allow reading the old Params without error. * [11859]: Write up a bunch of unit tests. * [11859]: Update the MsgSetSendEnabled.ValidateBasic() function with some extra failure points. Write up some tests. * Update a test I fixed then broke. * [11859]: Have the run-tests make target exit with a non-zero status if any of the tests fail. * [11859]: Add changelog entries. * [11859]: Add a missing func comment. * [11859]: Only do a couple assertions if the elements exist to do so. * [11859]: Add some more missing function comments. * [11859]: Update the bank spec documentation. * [11859]: Change name of WithPageKeyDecoded to FlagSetWithPageKeyDecoded, have it return an error and make MustFlagSetWithPageKeyDecoded for the one-liner. * [11859]: Update the documentation on the SendEnabled query. * [11859]: Add final newline to query.proto. * [11859]: Remove the SetSendEnabled msg and endpoint. * [11859]: Use nil instead of an empty slice of SendEnabled for defaults and where called for. * [11859]: Update SetParams to migrate entries too. * [11859]: Remove the spec doc info about the MsgSetSendEnabled that's part of another PR. * [11859]: Update the changelog. * Revert "[11859]: Update the changelog." This reverts commit 85052b8. * [11859]: Rename the QuerySendEnabled message to QuerySendEnabledRequest to match the other messages in that proto. * [11859]: Remove the authority field that is only needed for governance stuff (in the other PR). * [11859]: Add a version to the deprecation message. * [11859]: Update the comment on the now-deprecated SendEnabled params proto field to reference 0.46 instead of 0.47. * Add some spacing to GetCmdQuerySendEnabled -> RunE. * [11859]: Create banktypes.GenesisState.GetAllSendEnabled() to house the combination logic of the SendEnabled field and Params.SendEnabled. Have MigrateSendEnabled() use that. Remove some calls to MigrateSendEnabled and use GetAllSendEnabled in those cases. * [11859]: Update Bank's ConsensusVersion to 4. * [11859]: Add 'Since' comments to the new proto stuff. * [11859]: Fix a unit test that broke because it assumed the bank module's version was 3. * [11859]: Remove an empty line. Co-authored-by: Aleksandr Bezobchuk <[email protected]> * [11859]: Remove movement of SendEnabled from the `ExportGenesis` function too. Co-authored-by: Aleksandr Bezobchuk <[email protected]> * [11859]: Add a function for getting an entry so that users can differentiate between a missing entry and one that's using the default value. Co-authored-by: Aleksandr Bezobchuk <[email protected]>
* feat(bank)!: Move the bank module SendEnabled info into state (from Params). (cosmos#11977) * go mod tidy everything. * Add some third_party proto files that are imported but not included. * [11859]: Add a new key for the SendEnabled flags and keeper methods for getting, setting, and deleting them. * [11859]: Remove the send_enabled field from the bank Params proto. * Revert "Add some third_party proto files that are imported but not included." This reverts commit 8b7acf8. * [11859]: Regenerate the bank params stuff from the changed proto. * [11859]: Add a send_enabled field to the bank genesis proto. * Revert "[11859]: Remove the send_enabled field from the bank Params proto." This reverts commit 0bd904c. * Revert "[11859]: Regenerate the bank params stuff from the changed proto." This reverts commit 33d4652. * [11859]: Deprecate the bank Params send_enabled field. * [11859]: Regenerate the bank go code from the updated protos. * [11859]: Reduce the number of times the store is recreated during IsSendEnabledCoins. Store creation has some overhead. * [11859]: Add the SendEnabled stuff to the genesis methods. Make a couple TODO notes. Create a way to iterate over the SendEnabled entries and get all of them. * [11859]: Update the bank sim genesis stuff to create random SendEnabled entries similar to when they were params. * Remove some of the bank params methods that are no longer meaningful. * Add a comment about why we're calling a mutation method in a Validate function. * [11859]: Add some more TODO notes and make the SendEnabled.String() function significantlly simpler. * [11859]: Get rid of the SendEnabledParams type. * Fix up a few comments. * [11859]: Update the bank keeper test due to recent changes. * [11859]: Tweak the bank Params and SendEnabled String funcs. Params no longer returns {} when there aren't any SendEnabled entries and the default is false. SendEnabled is back to outputting a yaml format. * [11859]: Fix the params tests and add some new ones to it and key_test. * [11859]: Create a 1-store method for updating several SendEnabled entries at once. * [11859]: Create a migration for both the module and genesis state. * [11859]: Create a new MsgSetSendEnabled for governanance proposals to set SendEnabled. * [11859]: Add SetAllSendEnabled to the SendKeeper interface. * [11859]: Add an authority to the bank keeper and create the handler for MsgSetSendEnabled. * [11859]: Add an rpc endpoint for querying SendEnabled. * [11859]: Implement the SendEnabled query. * [11859]: Add a function for decoding a --page-key base64 value so that pagination can work as expected. * [11859]: Implement a CLI command for querying SendEnabled. * [11859]: Move the v047 store migration stuff into Migrate3to4 directly to prevent a circular dependency between 047 and the keeper. Not using the keeper for that would be a significant pain in the butt. * [11869]: Implement the Msg interface for MsgSetSendEnabled. * [11859]: Fix some unit tests that I broke along the way. * [11859]: Reorg the funcs added to the SendKeeper interface. * [11859]: Fix the return values of a couple of the MsgSetSendEnabled LegacyMsg funcs. * [11859]: Tweak MigrateSendEnabled to add stuff to the existing slice (if there's anything to add). And then use that in the MigrateGenState function. * [11859]: Don't set the Pagination field when looking up specific entries. * [11859]: Put validateSendEnabledParams back to the way it was to allow reading the old Params without error. * [11859]: Write up a bunch of unit tests. * [11859]: Update the MsgSetSendEnabled.ValidateBasic() function with some extra failure points. Write up some tests. * Update a test I fixed then broke. * [11859]: Have the run-tests make target exit with a non-zero status if any of the tests fail. * [11859]: Add changelog entries. * [11859]: Add a missing func comment. * [11859]: Only do a couple assertions if the elements exist to do so. * [11859]: Add some more missing function comments. * [11859]: Update the bank spec documentation. * [11859]: Change name of WithPageKeyDecoded to FlagSetWithPageKeyDecoded, have it return an error and make MustFlagSetWithPageKeyDecoded for the one-liner. * [11859]: Update the documentation on the SendEnabled query. * [11859]: Add final newline to query.proto. * [11859]: Remove the SetSendEnabled msg and endpoint. * [11859]: Use nil instead of an empty slice of SendEnabled for defaults and where called for. * [11859]: Update SetParams to migrate entries too. * [11859]: Remove the spec doc info about the MsgSetSendEnabled that's part of another PR. * [11859]: Update the changelog. * Revert "[11859]: Update the changelog." This reverts commit 85052b8. * [11859]: Rename the QuerySendEnabled message to QuerySendEnabledRequest to match the other messages in that proto. * [11859]: Remove the authority field that is only needed for governance stuff (in the other PR). * [11859]: Add a version to the deprecation message. * [11859]: Update the comment on the now-deprecated SendEnabled params proto field to reference 0.46 instead of 0.47. * Add some spacing to GetCmdQuerySendEnabled -> RunE. * [11859]: Create banktypes.GenesisState.GetAllSendEnabled() to house the combination logic of the SendEnabled field and Params.SendEnabled. Have MigrateSendEnabled() use that. Remove some calls to MigrateSendEnabled and use GetAllSendEnabled in those cases. * [11859]: Update Bank's ConsensusVersion to 4. * [11859]: Add 'Since' comments to the new proto stuff. * [11859]: Fix a unit test that broke because it assumed the bank module's version was 3. * [11859]: Remove an empty line. Co-authored-by: Aleksandr Bezobchuk <[email protected]> * [11859]: Remove movement of SendEnabled from the `ExportGenesis` function too. Co-authored-by: Aleksandr Bezobchuk <[email protected]> * [11859]: Add a function for getting an entry so that users can differentiate between a missing entry and one that's using the default value. Co-authored-by: Aleksandr Bezobchuk <[email protected]> * Fix a unit test that was failing to compile. * Update the changelog. * Fix the bank module's migration order since it's different for us than the main sdk. * Remove some stuff that isn't actually needed (probably added by mistake with the initial merge). Co-authored-by: Daniel Wedul <[email protected]> Co-authored-by: Aleksandr Bezobchuk <[email protected]>
…arams). (cosmos#11977) * go mod tidy everything. * Add some third_party proto files that are imported but not included. * [11859]: Add a new key for the SendEnabled flags and keeper methods for getting, setting, and deleting them. * [11859]: Remove the send_enabled field from the bank Params proto. * Revert "Add some third_party proto files that are imported but not included." This reverts commit 8b7acf8. * [11859]: Regenerate the bank params stuff from the changed proto. * [11859]: Add a send_enabled field to the bank genesis proto. * Revert "[11859]: Remove the send_enabled field from the bank Params proto." This reverts commit 0bd904c. * Revert "[11859]: Regenerate the bank params stuff from the changed proto." This reverts commit 33d4652. * [11859]: Deprecate the bank Params send_enabled field. * [11859]: Regenerate the bank go code from the updated protos. * [11859]: Reduce the number of times the store is recreated during IsSendEnabledCoins. Store creation has some overhead. * [11859]: Add the SendEnabled stuff to the genesis methods. Make a couple TODO notes. Create a way to iterate over the SendEnabled entries and get all of them. * [11859]: Update the bank sim genesis stuff to create random SendEnabled entries similar to when they were params. * Remove some of the bank params methods that are no longer meaningful. * Add a comment about why we're calling a mutation method in a Validate function. * [11859]: Add some more TODO notes and make the SendEnabled.String() function significantlly simpler. * [11859]: Get rid of the SendEnabledParams type. * Fix up a few comments. * [11859]: Update the bank keeper test due to recent changes. * [11859]: Tweak the bank Params and SendEnabled String funcs. Params no longer returns {} when there aren't any SendEnabled entries and the default is false. SendEnabled is back to outputting a yaml format. * [11859]: Fix the params tests and add some new ones to it and key_test. * [11859]: Create a 1-store method for updating several SendEnabled entries at once. * [11859]: Create a migration for both the module and genesis state. * [11859]: Create a new MsgSetSendEnabled for governanance proposals to set SendEnabled. * [11859]: Add SetAllSendEnabled to the SendKeeper interface. * [11859]: Add an authority to the bank keeper and create the handler for MsgSetSendEnabled. * [11859]: Add an rpc endpoint for querying SendEnabled. * [11859]: Implement the SendEnabled query. * [11859]: Add a function for decoding a --page-key base64 value so that pagination can work as expected. * [11859]: Implement a CLI command for querying SendEnabled. * [11859]: Move the v047 store migration stuff into Migrate3to4 directly to prevent a circular dependency between 047 and the keeper. Not using the keeper for that would be a significant pain in the butt. * [11869]: Implement the Msg interface for MsgSetSendEnabled. * [11859]: Fix some unit tests that I broke along the way. * [11859]: Reorg the funcs added to the SendKeeper interface. * [11859]: Fix the return values of a couple of the MsgSetSendEnabled LegacyMsg funcs. * [11859]: Tweak MigrateSendEnabled to add stuff to the existing slice (if there's anything to add). And then use that in the MigrateGenState function. * [11859]: Don't set the Pagination field when looking up specific entries. * [11859]: Put validateSendEnabledParams back to the way it was to allow reading the old Params without error. * [11859]: Write up a bunch of unit tests. * [11859]: Update the MsgSetSendEnabled.ValidateBasic() function with some extra failure points. Write up some tests. * Update a test I fixed then broke. * [11859]: Have the run-tests make target exit with a non-zero status if any of the tests fail. * [11859]: Add changelog entries. * [11859]: Add a missing func comment. * [11859]: Only do a couple assertions if the elements exist to do so. * [11859]: Add some more missing function comments. * [11859]: Update the bank spec documentation. * [11859]: Change name of WithPageKeyDecoded to FlagSetWithPageKeyDecoded, have it return an error and make MustFlagSetWithPageKeyDecoded for the one-liner. * [11859]: Update the documentation on the SendEnabled query. * [11859]: Add final newline to query.proto. * [11859]: Remove the SetSendEnabled msg and endpoint. * [11859]: Use nil instead of an empty slice of SendEnabled for defaults and where called for. * [11859]: Update SetParams to migrate entries too. * [11859]: Remove the spec doc info about the MsgSetSendEnabled that's part of another PR. * [11859]: Update the changelog. * Revert "[11859]: Update the changelog." This reverts commit 85052b8. * [11859]: Rename the QuerySendEnabled message to QuerySendEnabledRequest to match the other messages in that proto. * [11859]: Remove the authority field that is only needed for governance stuff (in the other PR). * [11859]: Add a version to the deprecation message. * [11859]: Update the comment on the now-deprecated SendEnabled params proto field to reference 0.46 instead of 0.47. * Add some spacing to GetCmdQuerySendEnabled -> RunE. * [11859]: Create banktypes.GenesisState.GetAllSendEnabled() to house the combination logic of the SendEnabled field and Params.SendEnabled. Have MigrateSendEnabled() use that. Remove some calls to MigrateSendEnabled and use GetAllSendEnabled in those cases. * [11859]: Update Bank's ConsensusVersion to 4. * [11859]: Add 'Since' comments to the new proto stuff. * [11859]: Fix a unit test that broke because it assumed the bank module's version was 3. * [11859]: Remove an empty line. Co-authored-by: Aleksandr Bezobchuk <[email protected]> * [11859]: Remove movement of SendEnabled from the `ExportGenesis` function too. Co-authored-by: Aleksandr Bezobchuk <[email protected]> * [11859]: Add a function for getting an entry so that users can differentiate between a missing entry and one that's using the default value. Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Description
Addresses most of: #11859
Params.SendEnabledfield.SendEnabledinformation directly in the state store.Params.SendEnabledinto the state store.SendEnabledfield directly in the GenesisState struct.genutil.Creates a newThis has been split out to another PR: feat(bank): Create message for Setting SendEnabled settings #11981MsgSetSendEnabledmessage for use with the governance module, allowing the setting of any number of denoms.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.mdReviewers 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