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

Additional review of prop 29 and migration testing #559

Merged
merged 5 commits into from Jan 25, 2021
Merged

Conversation

zmanian
Copy link
Member

@zmanian zmanian commented Jan 22, 2021

The current prop29 implementation causes errors when migrating the state due two issues.

  1. The JSON parsing of the recovery messages is failing on main due to handling of escape priorities.

  2. There were also correctness issues in the verification of Ethereum signatures both in terms of correctness. This is also resolved.

This can be tested by downloading the Nov 1 hub snapshot and running.

cosmos/gaia/build/gaiad migrate ~/3924406.cosmoshub-3.json

https://storage.googleapis.com/stargate-genesis/3924406.cosmoshub-3.json


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]" (coding standards)

@zmanian zmanian marked this pull request as ready for review January 22, 2021 17:02
Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

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

Good stuff

@codecov
Copy link

codecov bot commented Jan 22, 2021

Codecov Report

Merging #559 (cb0e824) into main (4f94504) will increase coverage by 0.98%.
The diff coverage is 27.69%.

@@            Coverage Diff             @@
##             main     #559      +/-   ##
==========================================
+ Coverage   11.58%   12.57%   +0.98%     
==========================================
  Files           8        8              
  Lines         673      684      +11     
==========================================
+ Hits           78       86       +8     
+ Misses        586      580       -6     
- Partials        9       18       +9     

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

ACK, see comment

}

// add the balances to the bank genesis
bankGenesis.Balances = append(bankGenesis.Balances, fundRecovery.GetRemainingBalances()...)
Copy link
Collaborator

Choose a reason for hiding this comment

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

My only concern here is that a balance uniqueness is not validated on the bank genesis validation

@zmanian zmanian merged commit 5ec6b5f into main Jan 25, 2021
@zmanian zmanian deleted the zaki-prop29-issues branch January 25, 2021 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants