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

R4R: Add Stake Genesis Intra-Tx Counter #1724

Merged
merged 7 commits into from
Jul 18, 2018
Merged

R4R: Add Stake Genesis Intra-Tx Counter #1724

merged 7 commits into from
Jul 18, 2018

Conversation

rigelrozanski
Copy link
Contributor

@rigelrozanski rigelrozanski commented Jul 18, 2018 •
edited
Loading

Closes #1666

  • Also added revoked to human-readable validator, this is why it's so important to have all the functions close to the type definition
  • n/a Updated all relevant documentation (docs/)
  • Updated all relevant code comments
  • Wrote tests
  • Updated CHANGELOG.md
  • n/a Updated cmd/gaia and examples/

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

ValarDragon
ValarDragon approved these changes Jul 18, 2018
View reviewed changes
Copy link
Contributor

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

LGTM, good catch

@rigelrozanski rigelrozanski changed the title R4R: add revoked to human-readable validator WIP: Stake Genesis Bug Jul 18, 2018
@rigelrozanski rigelrozanski changed the title WIP: Stake Genesis Bug R4R: Add Stake Genesis Intra-Tx Counter Jul 18, 2018
@codecov
Copy link

codecov bot commented Jul 18, 2018 •
edited
Loading

Codecov Report

Merging #1724 into develop will increase coverage by 0.01%.
The diff coverage is 90.9%.

@@             Coverage Diff             @@
##           develop    #1724      +/-   ##
===========================================
+ Coverage    62.35%   62.36%   +0.01%     
===========================================
  Files          120      120              
  Lines         7119     7121       +2     
===========================================
+ Hits          4439     4441       +2     
  Misses        2429     2429              
  Partials       251      251

This was referenced Jul 18, 2018
Open
Closed
@rigelrozanski rigelrozanski added the T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). label Jul 18, 2018
@rigelrozanski
Copy link
Contributor Author

i * think * this consistutes a breaking change - although it shouldn't break the testnet

@ValarDragon
Copy link
Contributor

I think you're right, adding the intra-tx counter would be breaking.

Should we merge this into a branch thats not develop? (I'm a bit confused about where we decided we're merging breaking / non-breaking stuff)

@alexanderbez
Copy link
Contributor

I don't understand why breaking changes cannot go into develop?

@rigelrozanski
Copy link
Contributor Author

@ValarDragon yeah breaking changes should be able to go to develop non-breaking patches can go to develop and master - if we want to create a new patch release we can do it directly from master

@ValarDragon
Copy link
Contributor

ohh, thanks for the clarification!

cwgoes
cwgoes approved these changes Jul 18, 2018
View reviewed changes
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

utACK

Maybe we should sanity-check non-duplicate keys in the power store.

@cwgoes cwgoes merged commit cfe7802 into develop Jul 18, 2018
@cwgoes cwgoes deleted the rigel/stake-cleanup branch July 18, 2018 20:09
@rigelrozanski
Copy link
Contributor Author

agreed on the sanity check

@cwgoes cwgoes mentioned this pull request Jul 19, 2018
Merged
6 tasks
ebuchman
ebuchman reviewed Jul 19, 2018
View reviewed changes
x/stake/genesis.go
@@ -29,6 +29,8 @@ func InitGenesis(ctx sdk.Context, keeper Keeper, data types.GenesisState) error

// Manually set indexes for the first time
keeper.SetValidatorByPubKeyIndex(ctx, validator)

validator.BondIntraTxCounter = int16(i) // set the intra-tx counter to the order the validators are presented
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we set this value right away so its consistent ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Only the power store key uses this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ebuchman as in set it in the genesis file as opposed to in this ordering? Yeah could be done, might be better to keep it more explicit. But then again. who get's to decide the order! I almost think for that organizational reason it should be set where it is currently.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's still effectively set by the genesis file though, since we iterate through the validator array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I guess it's alphabetical - honestly I don't think this is a real concern it's an extreme edge case, we just need something in there for key collision

jaekwon pushed a commit that referenced this pull request Jul 26, 2018
@rigelrozanski @jaekwon
Merge PR #1724: R4R: Add Stake Genesis Intra-Tx Counter
46abe9f 
* add revoked to human-readable validator
* changelog
* added failing test
* add intra-tx counter to the genesis validators
* changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cwgoes cwgoes cwgoes approved these changes

@ebuchman ebuchman ebuchman left review comments

@ValarDragon ValarDragon ValarDragon approved these changes

Assignees
No one assigned
Labels
C:x/staking T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Genesis Validators bonding status showing Unbond after starting a new chain
5 participants
@rigelrozanski @ValarDragon @alexanderbez @ebuchman @cwgoes