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(gov): support v1.Proposal in v1beta1.Proposal.Content #14347

Merged
merged 12 commits into from
Dec 22, 2022

Conversation

robert-zaremba
Copy link
Collaborator

@robert-zaremba robert-zaremba commented Dec 16, 2022

Description

Closes: #14334

Strawman solution to handle gov/v1 proposals in gov/v1beta1.Proposal.Content required for existing explorers (mintscan, ping pub, kepler) and wallets.

I tested it locally in Umee and local, with various proposals (including sofware upgrade, custom proposals, params...)


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)

@robert-zaremba robert-zaremba requested a review from a team as a code owner December 16, 2022 17:54
@robert-zaremba
Copy link
Collaborator Author

Adding backports to 0.46 and 0.47, but maybe we will need to make it manually , because the directory names changed.

@robert-zaremba robert-zaremba added backport/0.46.x PR scheduled for inclusion in the v0.46's next stable release backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release labels Dec 16, 2022
x/gov/migrations/v3/convert.go Outdated Show resolved Hide resolved
return legacyProposal, nil
// hack to fill up the content with the first message
legacyProposal.Content, err = codectypes.NewAnyWithValue(msgs[0])

Copy link
Collaborator Author

@robert-zaremba robert-zaremba Dec 16, 2022

Choose a reason for hiding this comment

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

The first idea I had, was to create a simple message:

message BasicContent {
  string title = 1;
  string description = 2 ;
}

and fill it using the Content interface. But, then we will remove additional attributes, which are in fact used by explorers to display the full content (I verified that with ping pub).

Copy link
Member

Choose a reason for hiding this comment

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

I agree, the current way you have is best imo.

@julienrbrt
Copy link
Member

Can we add a NotEmpty / Equal check in tests?

@robert-zaremba
Copy link
Collaborator Author

@julienrbrt what do you mean?

@julienrbrt
Copy link
Member

@julienrbrt what do you mean?

I just mean adding a test in here: https://github.com/cosmos/cosmos-sdk/blob/main/x/gov/migrations/v3/convert_test.go, to check content is properly set.

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

lgtm, can we add a changelog too?

@sonarcloud
Copy link

sonarcloud bot commented Dec 21, 2022

[Cosmos SDK] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@robert-zaremba
Copy link
Collaborator Author

Added an error when amount of messages in v1.Proposal is different than one. The conversion (and the endpoint) will fail in that case.

CHANGELOG.md Outdated Show resolved Hide resolved
x/gov/migrations/v3/convert.go Outdated Show resolved Hide resolved
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
@robert-zaremba robert-zaremba enabled auto-merge (squash) December 22, 2022 22:23
@robert-zaremba robert-zaremba merged commit 330ff5f into main Dec 22, 2022
@robert-zaremba robert-zaremba deleted the robert/gov-v1beta1-proposals branch December 22, 2022 22:44
mergify bot pushed a commit that referenced this pull request Dec 22, 2022
(cherry picked from commit 330ff5f)

# Conflicts:
#	CHANGELOG.md
mergify bot pushed a commit that referenced this pull request Dec 22, 2022
(cherry picked from commit 330ff5f)

# Conflicts:
#	CHANGELOG.md
#	x/gov/migrations/v3/convert.go
#	x/gov/migrations/v3/convert_test.go
@robert-zaremba
Copy link
Collaborator Author

@Mergifyio backport release/v0.47.x

@mergify
Copy link
Contributor

mergify bot commented Dec 22, 2022

backport release/v0.47.x

✅ Backports have been created

@@ -46,6 +48,9 @@ func ConvertToLegacyProposal(proposal v1.Proposal) (v1beta1.Proposal, error) {
if err != nil {
return v1beta1.Proposal{}, err
}
if len(msgs) != 1 {
return v1beta1.Proposal{}, sdkerrors.ErrInvalidType.Wrap("can't convert a gov/v1 Proposal to gov/v1beta1 Proposal when amount of proposal messages is more than one")
}
for _, msg := range msgs {
Copy link
Member

Choose a reason for hiding this comment

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

The for loop isn't needed anymore

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes... too late :/ Is it worth to create another PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release backport/0.46.x PR scheduled for inclusion in the v0.46's next stable release C:x/gov
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gov/v1beta1/proposal doesn't handle correctly new SoftwareUpgradeProposal
3 participants