Skip to content
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

feat(bank): Create message for Setting SendEnabled settings #11981

Merged
merged 165 commits into from
Sep 9, 2022

Conversation

dwedul-figure
Copy link
Collaborator

@dwedul-figure dwedul-figure commented May 17, 2022

Description

Closes: #11859

This PR is related to #11977. It creates and implements MsgSetSendEnabled, a message for use as a governance proposal to update the SendEnabled information.

Note that this PR is currently set to merge into dwedul/11859-send-disabled-change instead of main. This PR is dependent on functionality added by that branch. Once that PR has merged into main, I'll update this to also merge into main.


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...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change N/A
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

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...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

…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.
# 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.
@dwedul-figure
Copy link
Collaborator Author

@dwedul-figure are you waiting on a review from the sdk team here?

Yup. Sorry, I had let it get a bit behind, but it's all good now.

There's just the question of the SetSendEnabled endpoint vs UpdateParams. As things are right now (in this branch), the SetSendEnabled can do everything that UpdateParams does. That suggests we might want to get rid of the UpdateParams endpoint. However, UpdateParams is a standard for modules that have params, so it might be worth keeping it around (for familiarity). Along that line, if a param were ever added to the bank module that isn't associated with the send-enabled stuff, we'd want an UpdateParams endpoint for sure.

My personal gut feeling is to have both; the overlapping responsibilities are small enough that it shouldn't be much of an extra burden. But I also don't mind getting rid of UpdateParams. Another alternative is to remove the default stuff from SetSendEnabled, but I'm not a fan of that option. I feel like there will be times when it is desirable to set some flags and change the default, all at the same time and make them part of the same governance proposal.

Just let me know which direction I should go on that.

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

# Conflicts:
#	CHANGELOG.md
#	x/bank/spec/03_messages.md
#	x/bank/spec/README.md
…mos/cosmos-sdk into dwedul/11859-send-enabled-w-gov-prop

# Conflicts:
#	CHANGELOG.md
…mos-sdk/simapp) in x/bank/app_test.go and update other go mod stuff associated with that.
@tac0turtle tac0turtle merged commit 6a6e6c7 into main Sep 9, 2022
@tac0turtle tac0turtle deleted the dwedul/11859-send-enabled-w-gov-prop branch September 9, 2022 00:21
Wryhder pushed a commit to Wryhder/cosmos-sdk that referenced this pull request Oct 26, 2022
…1981)

* 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]: Upddate the SetSendEnabled endpoint to allow deleting entries and update the default value.

* [11859]: Regen code from protos to include recent changes.

* [11859]: Implement the functionality newly added to MsgSetSendEnabled.

* [11859]: Remove the SetSendEnabled msg and endpoint.

* [11859]: Add new fields to NewMsgSetSendEnabled.

* [11859]: Tweak the incorrect authority error a little to make it clearer.

* [11859]: Add some unit tests on handling of the MsgSetSendEnabled message.

* [11859]: Change the use_default field to use_default_for to make it less ambiguous.

* [11859]: Tweak a test to check that DeleteSendEnabled works with multiple denoms.

* [11859]: Add a test about a bad denom in the UseDefaultFor list.

* [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 bank spec docs again since that last merge undid it.

* [11859]: Update the changelog.

* Revert "[11859]: Update the changelog."

This reverts commit 85052b8.

* [11859]: Update the changelog.

* [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).

* Revert "[11859]: Remove the authority field that is only needed for governance stuff (in the other PR)."

This reverts commit 93f4148.

* [11859]: Add a version to the deprecation message.

* [11859]: Add unit test on MsgSetSendEnabled with the correct authority, but not signed by that address.

* [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.

* Add the authority to the bankInputs so it can be provided to NewBaseKeeper. I've no clue how that will get set though.

* [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 <alexanderbez@users.noreply.github.com>

* [11859]: Remove movement of SendEnabled from the `ExportGenesis` function too.

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

* [11859]: Hard-code the bank authority value with a TODO to allow injection of that value.

* [11859]: Inject the authority string for the bank module provider.

* [11859]: inject the bank authority value for module unit tests that need it.

* [11859]: Add a unit test for MsgSendEnabled.ValidateBasic that has an empty authority.

* [11859]: Don't require a MsgSetSendEnabled.Authority value in ValidateBasic. Maybe that field is supposed to be filed in by the governance module before passing on the message.

* [11859]: Update the capability testutil app_config to include the new bank authority value.

* [11859]: Undo the banktypes.Authority type thing.

* [11859]: Fix bad merge piece.

* Fix a couple unit tests that broke with the merge.

* [11859]: Remove the Route and Type LegacyMsg functions from the new MsgSetSendEnabled.

* [11859]: Add 'Since: cosmos-sdk 0.47' to the new proto stuff.

* [11859]: Get rid fo the BaseKeeper.GetAuthority since it was added to the SendKeeper.

* [11859]: emit an event when processing a SetSendEnabled.

* [11859]: Update MsgSetSendEnabled.ValidateBasic to return wrapped sdk errors.

* Add a link in the bank spec readme to the MsgUpdateParams.

* rename govtypesv1 import to govv1 since that's what's used elsewhere.

* [11859]: Update changelog link to point at the PR instead of the issue number.

* [11859]: Add some extra test cases for GetAuthority.

* Move changelog entry to unreleased.

* [11859]: Change a call to moduletestutil.MakeTestEncodingConfig (from simapp.MakeTestEncodingConfig).

* [11859]: Add SetSendEnabled to the MockBankKeeper.

* [11859]: make mocks.

* [11859]: Fix the bank app tests (had compilation issues due to merge changes).

* [11859]: Remove the set_default_send_enabled and default_send_enabled fields from MsgSetSendEnabled.

* [11859]: Remove any uses/references to the removed set_default_send_enabled and default_send_enabled fields.

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
Co-authored-by: Daniel Wedul <github@wedul.com>
Co-authored-by: Marko <marbar3778@yahoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Bank Module Send Disabled Token Configuration
8 participants