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 upgrade proposal plan validation to CLI #10379

Merged
merged 47 commits into from
Nov 12, 2021

Conversation

dwedul-figure
Copy link
Collaborator

@dwedul-figure dwedul-figure commented Oct 15, 2021

Description

Closes: #10286

When submitting a software upgrade proposal (e.g. $DAEMON tx gov submit-proposal software-upgrade)

  • Validate the plan info by default.
  • Add flag --no-validate to allow skipping that validation.
  • Add flag --daemon-name to designate the executable name (needed for validation).
  • The daemon name comes first from the --daemon-name flag. If that's not provided, it looks for a DAEMON_NAME environment variable (to match what's used by Cosmovisor). If that's not set, the name of the currently running executable is used.

Things that are validated:

  • The plan info cannot be empty or blank.
  • If the plan info is a url:
    • It must have a checksum query parameter.
    • It must return properly formatted plan info JSON.
    • The checksum is correct.
  • If the plan info is not a url:
    • It must be propery formatted plan info JSON.
  • There is at least one entry in the binaries field.
  • The keys of the binaries field are either "any" or in the format of "os/arch".
  • All URLs contain a checksum query parameter.
  • Each URL contains a usable response.
  • The checksum is correct for each URL.

Note: With this change, either a valid --upgrade-info will need to be provided, or else --no-validate must be provided. If no --upgrade-info is given, a validation error is returned.


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)

…ate flag and validate the plan info by default.
…ier for reuse by cosmovisor. Also, add a --daemon-name flag to the software-upgrade. The deamon name is needed in order to properly validate the plan info. It defaults to the name of the running executable, but since that might not be right, the flag allows defining it.
…NAME env var as the default if it's set, then there isn't a need for a special getter for that flag's value.
… file. It's in the cosmovisor go.mod file, but similar functionality is needed in the x/upgrade module now.
…, and split out the downloading functions into a new downloader.go file. Change PlanInfo.ValidateBasic to ValidateFull that does both the validation pieces on the binaries map.
@dwedul-figure dwedul-figure changed the title Dwedul/10286 upgrade prop validation feat: Add upgrade proposal plan validation to CLI Oct 15, 2021
@codecov
Copy link

codecov bot commented Oct 15, 2021

Codecov Report

Merging #10379 (71f1ba1) into master (878e3e8) will decrease coverage by 0.82%.
The diff coverage is 86.36%.

❗ Current head 71f1ba1 differs from pull request most recent head 382a137. Consider uploading reports for the commit 382a137 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10379      +/-   ##
==========================================
- Coverage   65.11%   64.29%   -0.83%     
==========================================
  Files         604      583      -21     
  Lines       57862    55338    -2524     
==========================================
- Hits        37678    35579    -2099     
+ Misses      18090    17735     -355     
+ Partials     2094     2024      -70     
Impacted Files Coverage Δ
x/upgrade/types/planinfo/downloader.go 81.70% <81.70%> (ø)
x/upgrade/types/planinfo/plan_info.go 94.00% <94.00%> (ø)
x/nft/keeper/class.go 73.33% <0.00%> (-6.67%) ⬇️
x/mint/module.go 52.54% <0.00%> (-3.02%) ⬇️
x/auth/tx/sigs.go 77.57% <0.00%> (-2.81%) ⬇️
store/cachekv/store.go 77.63% <0.00%> (-1.32%) ⬇️
x/auth/tx/builder.go 78.60% <0.00%> (-0.88%) ⬇️
simapp/app.go 81.26% <0.00%> (-0.06%) ⬇️
types/errors/errors.go 83.17% <0.00%> (ø)
x/mint/types/genesis.go 0.00% <0.00%> (ø)
... and 32 more

@likhita-809
Copy link
Contributor

likhita-809 commented Oct 29, 2021

lgtm, but it'd be great if someone more familiar with this stuff approves first @blushi @alexanderbez

@robert-zaremba robert-zaremba self-assigned this Oct 29, 2021
Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

Looks good. We did an online review and created a follow up issue: #10464

Pre-approving - let's improve the docs and few things related to the comments.

x/upgrade/client/cli/tx.go Outdated Show resolved Hide resolved
x/upgrade/types/planinfo/downloader.go Outdated Show resolved Hide resolved
x/upgrade/types/planinfo/downloader.go Outdated Show resolved Hide resolved
x/upgrade/types/planinfo/downloader.go Outdated Show resolved Hide resolved
x/upgrade/types/planinfo/downloader.go Outdated Show resolved Hide resolved
x/upgrade/types/planinfo/downloader.go Outdated Show resolved Hide resolved
)

// PlanInfo is the special structure that the Plan.Info string can be (as json).
type PlanInfo struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Idea: how about having a package /x/upgrade/plan and renaming this package to Info?

In any case, let's move away from types.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I made this change.

I moved x/upgrade/types/planinfo/ stuff to x/upgrades/plan/, renamed plan_info.go to just info.go, and PlanInfo to just Info.

I'm not quite sure how I feel about it, though.

  1. Info is such a generic word that I feel it'll be easy to get confused about it.
  2. The Plan struct is still defined by the protos and placed in x/upgrades/types/upgrade.pb.go with extra functionality added in x/upgrades/types/plan.go. Having this x/upgrades/plan/ directory without the Plan stuff seems confusing. Additionally, it would now feel weird to put the Plan stuff into the x/upgrades/plan/ directory because it'll be referred to as plan.Plan.

@robert-zaremba What do you think about leaving it named the way it was, and in the types/planinfo/ directory until a refactor breaks out all the types stuff at the same time?

x/upgrade/types/planinfo/plan_info.go Outdated Show resolved Hide resolved
…e that's more accurate to what the url must be.
…d make the error messages a little more generic.
…e flag descriptions since the others don't have it and the default is added to the end of that string making it weird with punctuation.
@robert-zaremba robert-zaremba added the A:automerge Automatically merge PR once all prerequisites pass. label Nov 12, 2021
@mergify mergify bot merged commit 181ba0e into master Nov 12, 2021
@mergify mergify bot deleted the dwedul/10286-upgrade-prop-validation branch November 12, 2021 17:44
blewater pushed a commit to e-money/cosmos-sdk that referenced this pull request Dec 8, 2021
<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

Closes: cosmos#10286

When submitting a software upgrade proposal (e.g. `$DAEMON tx gov submit-proposal software-upgrade`)
* Validate the plan info by default.
* Add flag `--no-validate` to allow skipping that validation.
* Add flag `--daemon-name` to designate the executable name (needed for validation).
* The daemon name comes first from the `--daemon-name` flag. If that's not provided, it looks for a `DAEMON_NAME` environment variable (to match what's used by Cosmovisor). If that's not set, the name of the currently running executable is used.

Things that are validated:
* The plan info cannot be empty or blank.
* If the plan info is a url:
  * It must have a `checksum` query parameter.
  * It must return properly formatted plan info JSON.
  * The `checksum` is correct.
* If the plan info is not a url:
  * It must be propery formatted plan info JSON.
* There is at least one entry in the `binaries` field.
* The keys of the `binaries` field are either "any" or in the format of "os/arch".
* All URLs contain a `checksum` query parameter.
* Each URL contains a usable response.
* The `checksum` is correct for each URL.

Note: With this change, either a valid `--upgrade-info` will need to be provided, or else `--no-validate` must be provided. If no `--upgrade-info` is given, a validation error is returned.

---

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

- [x] 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~~ _N/A_
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [x] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [x] added a changelog entry to `CHANGELOG.md`
- [x] 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
- [x] 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)
@amaury1093 amaury1093 mentioned this pull request May 20, 2022
72 tasks
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:x/upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When creating a new sofware-upgrade Proposal add a flag to validate Info content.
5 participants