-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
x/upgrade: Refactor CLI to use protobuf query #6623
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6623 +/- ##
==========================================
+ Coverage 57.50% 57.52% +0.02%
==========================================
Files 500 500
Lines 30024 30013 -11
==========================================
+ Hits 17264 17266 +2
+ Misses 11515 11503 -12
+ Partials 1245 1244 -1 |
x/upgrade/client/cli/query.go
Outdated
// GetPlanCmd returns the query upgrade plan command | ||
func GetPlanCmd(storeName string, cdc *codec.Codec) *cobra.Command { | ||
// GetCurrentPlanCmd returns the query upgrade plan command | ||
func GetCurrentPlanCmd(clientCtx client.Context) *cobra.Command { |
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.
note: just a rename to follow same convention everywhere in x/upgrade: *CurrentPlan*
and *AppliedPlan*
} | ||
|
||
// always output json as Header is unreable in toml ([]byte is a long list of numbers) | ||
bz, err = cdc.MarshalJSONIndent(headers.BlockMetas[0], "", " ") | ||
bz, err := clientCtx.Codec.MarshalJSONIndent(headers.BlockMetas[0], "", " ") |
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.
didn't find BlockMetas in tm's proto files, so not using protobuf here
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.
Hi @amaurymartiny, thanks for contributing! Since we're modifying the commands and some tests here, we may as well update x/upgrade
to follow in-process integration tests (#6423). Otherwise, some of the work you have here would get wiped out in a few days.
Is this something you think you can tackle? x/bank
has already been upgraded so you can follow that as an easy example.
sounds good, I'll do that in this PR |
@alexanderbez currently there are no integration tests (AFAIK) for the upgrade module. I think it would be great if we did but this is actually pretty involved and would likely only make sense by building actual binaries (not the in-process test suite) and using |
@aaronc regardless of what is tested, it still needs to be refactored. Namely,
|
@alexanderbez I'd love a pointer on how to move this PR forward.
Had a look in x/bank, and it uses cosmos-sdk/x/bank/client/cli/query.go Line 25 in b4a027c
The tests I'm refactoring here are keeper unit tests, I believe they are not relevant to #6423. |
Indeed, my mistake, it's not relevant. But the changes required I laid out in #6623 (comment). As you can tell from x/bank, it no longer takes a |
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.
LGTM @amaurymartiny. Minor comments
|
||
return cmd | ||
} | ||
|
||
// NewCmdSubmitUpgradeProposal implements a command handler for submitting a software upgrade proposal transaction. | ||
func NewCmdSubmitUpgradeProposal(clientCtx client.Context) *cobra.Command { |
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.
This still needs to be updated: see #6639 for reference
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.
I actually went down this path, but it requires a bigger refactor:
- x/gov has a
CLIHandlerFn
that takes client.Context as arg - x/distribution, x/params & x/upgrade use this handler. But PRs are still pending for using Protobuf: x/params: query gRPC service #6585 x/distribution: gRPC query service #6600, didn't want to interfere too much
what I propose:
- leave this PR scoped to x/upgrade, as initially intended
- I'll take care of one follow-up PR to refactor all x/{distribution,params,upgrade} at once once those PRs are merged
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.
LGTM! Left two minor comments.
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.
ACK 🎉
Description
ref: #5921
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes