-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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: add array of messages to a proposal #9810
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @cmwaters for this first smaller PR.
Some gov/keeper tests need to be fixed, which should be minor adjustments.
Regarding the migration tests failing, the options proposed by @aaronc in #9810 (comment) should get around it. I would tend to prefer bumping the proto version which seems cleaner and more aligned with our plan to bump everything to v1 for 0.44 (#9446 (comment)). If we go with the latter option, then this means some additional work related to migrations (something similar to what @AmauryM had done as part of #9492)
Visit https://dashboard.github.orijtech.com?pr=9810&repo=cosmos%2Fcosmos-sdk to see benchmark details. |
When you say "bump everything to v1", does this just means to change:
to
|
Yes as part of a new folder But before that, let's make sure this is what we wanna do here, thoughts @aaronc @AmauryM? |
I'm not part of the WG, so I didn't follow how confident you guys are on this API, but I put some thoughts here: #9810 (comment)
We actually didn't finalize the decision on how to handle proto version bumps in #9613. Legacy package is a solution, but aaron also proposed for 1 node to handle 2 versions at the same time. |
Right, thanks for pointing that out! @cmwaters FYI This is something that is on the agenda for today's architecture call: https://hackmd.io/vh9ck9qvSiaM1ivb50NWMg |
Ok. I will wait out until a decision is made on this 👍 |
Hey, did you guys reach a decision on what path to take regarding breaking the proto structs? |
I added an agenda item to our internal Regen meeting today to discuss if we're bumping all proto pkgs to v1 for 0.44 or not. I'd like to add that if we're bumping to v1, then there's some groundwork to prepare (either the legacy migrations that Marie mentioned #9810 (comment), or for the SDK to support multiple proto versions at the same time). This groundwork should be ideally done separately, on a So to unblock your work for this PR, maybe let's temporarily keep |
govAcc, err := s.cfg.AccountRetriever.GetAccount(val.ClientCtx, authtypes.NewModuleAddress(types.ModuleName)) | ||
s.Require().NoError(err) | ||
|
||
proposal := []sdk.Msg{types.NewMsgVote(govAcc.GetAddress(), 1, types.OptionYes)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
am i reading it correctly that the proposal msg that is getting voted on is itself a vote proposal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes lol. This is just a dummy proposal used in testing but essentially the governance account would vote on a proposal.
## Description Ref: #9810 Bumps gov proto files to v1beta2 and creates new `Proposal` type --- ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)
putting a short merge date on this. In two days we will merge this PR with any open questions landing in a follow up PRs |
I'm all for trying to move this quickly. But we also need to make sure simulations aren't broken |
didn't see the failing tests. @cmwaters once we fix these we will be able to merge. |
How do I go about debugging the sims test? |
I'm maybe not the best resource here. @alexanderbez @AmauryM does either of you have advice? |
For additional context, I've run this failing simulation:
and get:
|
## Description Ref: cosmos#9810 Bumps gov proto files to v1beta2 and creates new `Proposal` type --- ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)
Should we close this PR? |
Ref: #9810 Moves all legacy gov code to `v1beta1`. This preserves all existing behavior (i.e. everything still uses v1beta1). It's merely moving things around to get everything in the right place logistically (hence the large diff still) --- ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)
Ref: cosmos#9810 Moves all legacy gov code to `v1beta1`. This preserves all existing behavior (i.e. everything still uses v1beta1). It's merely moving things around to get everything in the right place logistically (hence the large diff still) --- ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)
Description
Closes: #9438
This PR allows for an array of arbitrary sdk messages to be added to a proposal. It introduces a new proposal,
Proposal2
which has a field messages that accepts an array ofsdk.Msg{}
instead of using theContent
interface. Both proposal types:Proposal
andProposal2
co-exist with the former being deprecated in a later release. The introduction of a new proposal type touches a lot of the gov module (down to introducing a new key type to store the proposals). Although this approach allows for Clients to gradually adopt the new format it does introduce the problem that querying will need to be for both types.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.md
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...
!
in the type prefix if API or client breaking change