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: Support Vesting Accounts in add-genesis-account #3549

Merged
merged 6 commits into from
Feb 8, 2019
Merged

R4R: Support Vesting Accounts in add-genesis-account #3549

merged 6 commits into from
Feb 8, 2019

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Feb 7, 2019 •
edited
Loading

closes: #3484

  • 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.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added entries in PENDING.md with issue #

  • rereviewed Files changed in the github PR explorer

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)

alessio
alessio reviewed Feb 7, 2019
View reviewed changes
cmd/gaia/init/genesis_accts.go Outdated
func AddGenesisAccountCmd(ctx *server.Context, cdc *codec.Codec) *cobra.Command {
cmd := &cobra.Command{
Use: "add-genesis-account [address_or_key_name] [coin][,[coin]]",
Short: "Add genesis account to genesis.json",
Short: "Add genesis account to a genesis file",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd second this new wording if were to support an extra optional flag/argument to provide the genesis.json; needless to say that it should default to gaiad default genesis file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A bit lost here, are you asking me to revert?

Copy link
Member

Choose a reason for hiding this comment

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

I think what @alessio is saying is that there is no way for this command to specify what the particular file path to genesis is, so to a genesis file is maybe overselling.

@codecov
Copy link

codecov bot commented Feb 7, 2019

Codecov Report

Merging #3549 into develop will increase coverage by 2.47%.
The diff coverage is 44.11%.

@@             Coverage Diff             @@
##           develop    #3549      +/-   ##
===========================================
+ Coverage     57.1%   59.57%   +2.47%     
===========================================
  Files          179      131      -48     
  Lines        14168     9710    -4458     
===========================================
- Hits          8090     5785    -2305     
+ Misses        5597     3585    -2012     
+ Partials       481      340     -141

@codecov
Copy link

codecov bot commented Feb 7, 2019 •
edited
Loading

Codecov Report

Merging #3549 into develop will increase coverage by 2.48%.
The diff coverage is 46.87%.

@@             Coverage Diff             @@
##           develop    #3549      +/-   ##
===========================================
+ Coverage     57.1%   59.59%   +2.48%     
===========================================
  Files          179      131      -48     
  Lines        14168     9708    -4460     
===========================================
- Hits          8090     5785    -2305     
+ Misses        5597     3583    -2014     
+ Partials       481      340     -141

jackzampolin
jackzampolin reviewed Feb 7, 2019
View reviewed changes
cmd/gaia/cli_test/test_helpers.go
sdk.NewCoin(denom, staking.TokensFromTendermintPower(150)),
}

vestingCoins = sdk.Coins{
Copy link
Member

Choose a reason for hiding this comment

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

++

jackzampolin
jackzampolin reviewed Feb 7, 2019
View reviewed changes
cmd/gaia/cli_test/test_helpers.go
@@ -120,6 +129,12 @@ func InitFixtures(t *testing.T) (f *Fixtures) {

// Start an account with tokens
f.AddGenesisAccount(f.KeyAddress(keyFoo), startCoins)
f.AddGenesisAccount(
Copy link
Member

Choose a reason for hiding this comment

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

The integration test we should have had. Ty

@alexanderbez alexanderbez changed the title WIP: Support Vesting Accounts in add-genesis-account R4R: Support Vesting Accounts in add-genesis-account Feb 7, 2019
@jackzampolin
Copy link
Member

Great add here @alexanderbez

jackzampolin
jackzampolin approved these changes Feb 8, 2019
View reviewed changes
Copy link
Member

@jackzampolin jackzampolin left a comment

Choose a reason for hiding this comment

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

LGTM once tests are passing 👍

cmd/gaia/init/genesis_accts.go Outdated Show resolved Hide resolved
@jackzampolin
Copy link
Member

Still failing tests here :/

@alexanderbez
Copy link
Contributor Author

CI failed due to an unrelated/non-deterministic test: #3559

@cwgoes cwgoes merged commit d759bef into develop Feb 8, 2019
@cwgoes cwgoes deleted the bez/3484-gen-vacc-cmd-support branch February 8, 2019 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackzampolin jackzampolin jackzampolin approved these changes

@alessio alessio alessio approved these changes

@cwgoes cwgoes Awaiting requested review from cwgoes

@ebuchman ebuchman Awaiting requested review from ebuchman

@rigelrozanski rigelrozanski Awaiting requested review from rigelrozanski

Assignees
No one assigned
Labels
C:CLI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

gaiad add-genesis-account to support vesting
4 participants
@alexanderbez @jackzampolin @alessio @cwgoes