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.

Sign up for GitHub

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

Jump to bottom

Gov fix #761

Merged
merged 3 commits into from
Aug 28, 2023
Merged

Gov fix #761

merged 3 commits into from
Aug 28, 2023

Conversation

skyargos
Copy link
Contributor

Closes: #XXX
Related: #XXX

Description

For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]"

@skyargos skyargos marked this pull request as ready for review August 25, 2023 07:37
@skyargos skyargos requested a review from a team as a code owner August 25, 2023 07:37
haozhan9
haozhan9 reviewed Aug 25, 2023
View reviewed changes
x/gov/module.go Outdated
@@ -186,7 +186,7 @@ func (am AppModule) ExportGenesis(ctx sdk.Context, cdc codec.JSONCodec) json.Raw
}

// ConsensusVersion implements AppModule/ConsensusVersion.
func (am AppModule) ConsensusVersion() uint64 { return 3 }
func (am AppModule) ConsensusVersion() uint64 { return 4 }
Copy link
Contributor

Choose a reason for hiding this comment

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

Just adding this would cause error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, no data migration may not need to change consensusVersion

0311xuyang
0311xuyang approved these changes Aug 25, 2023
View reviewed changes
Copy link
Contributor

@0311xuyang 0311xuyang left a comment

Choose a reason for hiding this comment

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

LGTM

kevin-yuhh
kevin-yuhh approved these changes Aug 27, 2023
View reviewed changes
Copy link
Contributor

@kevin-yuhh kevin-yuhh left a comment

Choose a reason for hiding this comment

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

It will be better if text proposal is added to e2e-test

@skyargos skyargos merged commit 52a96fe into gov Aug 28, 2023
15 checks passed
@skyargos skyargos deleted the gov-fix branch August 28, 2023 05:40
skyargos added a commit that referenced this pull request Sep 11, 2023
@skyargos
Gov fix (#761) (#768)
24cc066 
* Fix general proposal of gov module
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0311xuyang 0311xuyang 0311xuyang approved these changes

@kevin-yuhh kevin-yuhh kevin-yuhh approved these changes

@haozhan9 haozhan9 haozhan9 approved these changes

@jackyi2023 jackyi2023 jackyi2023 approved these changes

@zhaozhongni zhaozhongni Awaiting requested review from zhaozhongni

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@skyargos @0311xuyang @kevin-yuhh @haozhan9 @jackyi2023