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: Add MsgSoftwareUpgrade and MsgCancelUpgrade (for new msgs-based gov proposals) #11116

Merged
merged 27 commits into from
Mar 3, 2022

Conversation

blushi
Copy link
Contributor

@blushi blushi commented Feb 4, 2022

Description

Closes: #10489

Adds MsgSoftwareUpgrade to x/upgrade to be used in v1beta2 msgs-based gov proposals.
TODO:

  • add MsgCancelUpgrade
  • spec
  • CLI and CLI tests

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
  • 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)

@blushi blushi changed the title feat: Add MsgSoftwareUpgrade (new msgs-based gov proposals) feat: Add MsgSoftwareUpgrade (for new msgs-based gov proposals) Feb 4, 2022
@blushi blushi changed the title feat: Add MsgSoftwareUpgrade (for new msgs-based gov proposals) feat: Add MsgSoftwareUpgrade (for new msgs-based gov proposals) Feb 4, 2022
@codecov
Copy link

codecov bot commented Feb 4, 2022

Codecov Report

Merging #11116 (f13b5b5) into master (918330b) will decrease coverage by 0.02%.
The diff coverage is 66.66%.

❗ Current head f13b5b5 differs from pull request most recent head 0007831. Consider uploading reports for the commit 0007831 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11116      +/-   ##
==========================================
- Coverage   65.93%   65.90%   -0.03%     
==========================================
  Files         662      664       +2     
  Lines       68527    68548      +21     
==========================================
- Hits        45180    45174       -6     
- Misses      20702    20731      +29     
+ Partials     2645     2643       -2     
Impacted Files Coverage Δ
x/gov/client/cli/tx.go 14.76% <0.00%> (+0.48%) ⬆️
x/upgrade/types/msgs.go 55.55% <55.55%> (ø)
x/upgrade/keeper/msg_server.go 63.63% <63.63%> (ø)
simapp/app.go 83.04% <100.00%> (+0.09%) ⬆️
x/upgrade/keeper/grpc_query.go 76.74% <100.00%> (+1.13%) ⬆️
x/upgrade/keeper/keeper.go 84.67% <100.00%> (+0.06%) ⬆️
x/upgrade/module.go 67.44% <100.00%> (+0.77%) ⬆️
x/upgrade/types/codec.go 100.00% <100.00%> (ø)
x/distribution/simulation/operations.go 80.54% <0.00%> (-9.73%) ⬇️
x/gov/keeper/keeper.go 75.64% <0.00%> (-1.47%) ⬇️
... and 4 more

proto/cosmos/upgrade/v1beta1/tx.proto Outdated Show resolved Hide resolved
proto/cosmos/upgrade/v1beta1/tx.proto Show resolved Hide resolved
x/gov/module.go Outdated
Comment on lines 40 to 41
legacyProposalHandlers []govclient.ProposalHandler // legacy proposal handlers which live in governance cli and rest
proposalHandlers []govclient.ProposalHandler // proposal handlers which live in governance cli and rest
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why we need both?

Copy link
Contributor Author

@blushi blushi Feb 9, 2022

Choose a reason for hiding this comment

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

legacyProposalHandlers CLI handlers will get added as subcommands of NewCmdSubmitLegacyProposal (so we can have simd tx gov submit-legacy-proposal software-upgrade for instance)
same for proposalHandlers and NewCmdSubmitProposal (eg for having simd tx gov submit-proposal software-upgrade)
see NewTxCmd

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the cli would be better if it were simply simd tx upgrade propose and simd tx upgrade cancel

Copy link
Contributor Author

@blushi blushi Feb 18, 2022

Choose a reason for hiding this comment

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

so you're suggesting to have the new commands available directly from tx upgrade command (instead of tx gov submit-proposal) for the new upgrade proposal msgs
but keep the legacy ones using simd tx gov submit-legacy-proposal software-upgrade / cancel-software-upgrade?
I kind of like this new approach, although I'm a bit concerned it might be confusing for users, any thoughts? @cmwaters @AmauryM

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also in favor of the new approach!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that sounds good to me, as long as the docs/changelog are very clear about this

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool! Although gov is the default module in control of upgrades, I wouldn't want to have them so closely coupled. In the future, one application may want a group in control of proposing upgrades

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm on second thought this is a bit more difficult than I thought because the upgrade module doesn't really know who the authority is and whether to submit a proposal or simply execute it

blushi and others added 4 commits February 9, 2022 15:23
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
@blushi blushi changed the title feat: Add MsgSoftwareUpgrade (for new msgs-based gov proposals) feat: Add MsgSoftwareUpgrade and MsgCancelUpgrade (for new msgs-based gov proposals) Feb 10, 2022
@amaury1093
Copy link
Contributor

I pushed a commit for MsgCancelUpgrade msg service implementation and proto files. The last thing needed is CLI for MsgCancelUpgrade and CLI tests.

@cmwaters
Copy link
Contributor

We agreed at the call yesterday that I would pick this up

@cmwaters
Copy link
Contributor

cmwaters commented Feb 16, 2022

We shouldn't have it built-in that the governance module account is the authority rather application developers should probably specify an initial address in the constructor as the authority. Some chains may want to create a sub account that is the authority.

@amaury1093
Copy link
Contributor

Yeah, this was a question I asked on Discord, wasn't 100% sure.

I think we can put authority in the keeper config object, which can be overwritten by app developers. It can default to the gov module address.

@github-actions github-actions bot added the C:orm label Feb 21, 2022
@cmwaters
Copy link
Contributor

So I'm not entirely convinced by the cli commands. My current thinking is that if you want to submit an upgrade msg you just use simd tx gov submit-proposal my-upgrade.json or if the authority is a group then simd tx group submit-proposal my-upgrade.json. We could have the cli for the upgrade module generate the msg in json if we want (i.e. simd tx gov submit-proposal $(simd tx upgrade [insert some argumenst])` but I'm not sure if that's necessary.

@blushi
Copy link
Contributor Author

blushi commented Feb 22, 2022

So I'm not entirely convinced by the cli commands. My current thinking is that if you want to submit an upgrade msg you just use simd tx gov submit-proposal my-upgrade.json or if the authority is a group then simd tx group submit-proposal my-upgrade.json. We could have the cli for the upgrade module generate the msg in json if we want (i.e. simd tx gov submit-proposal $(simd tx upgrade [insert some argumenst])` but I'm not sure if that's necessary.

But then that means you would basically need to execute 2 commands, which might not be considered as a UX improvement... Although from a dev standpoint, that seems more consistent with the new gov json-based CLI approach for submitting proposal with arbitrary msgs. Let's maybe discuss during our sync later today?

@cmwaters cmwaters marked this pull request as ready for review February 25, 2022 18:34
@cmwaters
Copy link
Contributor

FYI: I've made the authority configurable by the application (simd uses the gov module account and I'd expect most other apps to do the same). You can now also query what the authority for the upgrade module is.

In our call, we also agreed to not include any new cli commands as people could no construct the message in json and submit it via <app> tx gov submit-proposals [path/to/proposal.json]

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

LGTM, except the fact that the Msg server should check against the keeper authority (not hardcode the gov address)

x/upgrade/types/codec.go Show resolved Hide resolved
x/upgrade/keeper/keeper.go Outdated Show resolved Hide resolved
x/upgrade/keeper/msg_server.go Outdated Show resolved Hide resolved
x/upgrade/types/expected_keepers.go Outdated Show resolved Hide resolved
x/gov/client/cli/tx.go Outdated Show resolved Hide resolved
x/upgrade/keeper/keeper.go Outdated Show resolved Hide resolved
x/upgrade/keeper/msg_server.go Outdated Show resolved Hide resolved
@cmwaters cmwaters requested a review from amaury1093 March 3, 2022 11:13
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

LGTM!

x/gov/client/cli/tx.go Outdated Show resolved Hide resolved
x/upgrade/types/codec.go Show resolved Hide resolved
x/upgrade/client/cli/parse.go Outdated Show resolved Hide resolved
x/upgrade/client/cli/tx.go Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@cmwaters cmwaters added the A:automerge Automatically merge PR once all prerequisites pass. label Mar 3, 2022
@amaury1093
Copy link
Contributor

@cmwaters We need 2 approvals for automerge. Since Marie authored, you can just approve I guess

@cmwaters cmwaters merged commit 75d9d0d into master Mar 3, 2022
@cmwaters cmwaters deleted the marie/10489-gov-upgrade-msg branch March 3, 2022 15:45
@amaury1093 amaury1093 mentioned this pull request May 20, 2022
72 tasks
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:CLI C:orm C:x/gov C:x/upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gov: replace Content and Proposal handlers in other modules with messages
4 participants