-
Notifications
You must be signed in to change notification settings - Fork 170
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: checkpointing/add create-bls-key cli #162
Conversation
Can you add an example of how this command is called to the PR description? |
|
||
func createBlsKeyCmd() *cobra.Command { | ||
cmd := &cobra.Command{ | ||
Use: "create-bls-key [account-address]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use: "create-bls-key [account-address]", | |
Use: "create-bls-key [--account-address <bbn-account-address>]", |
[]
brackets mean this flag is optional, and the example should contain --
to show it's a flag, otherwise I would think the way to call this is create-bls-key
or crate-bls-key bbn1f5tnl46mk4dfp4nx3n2vnrvyw2h2ydz6ykhk3r
.
If it's not an optional parameter than maybe it should be an arg
instead of a flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right. It should an arg instead of a flag. Thanks!
babylond create-bls-key --account-address bbn1f5tnl46mk4dfp4nx3n2vnrvyw2h2ydz6ykhk3r --home ./ | ||
`, | ||
RunE: func(cmd *cobra.Command, _ []string) error { | ||
homeDir, _ := cmd.Flags().GetString(flags.FlagHome) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error is ignored here, but not for the account address. Does that mean this is optional, but the other isn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe look at https://stackoverflow.com/questions/38105859/make-a-cobra-command-flag-required to mark them as required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other one is an arg instead of a flag. FlagHome
is optional. There's a default value for that.
cmd/babylond/cmd/create_bls_key.go
Outdated
send BLS signatures for checkpointing. | ||
|
||
BLS keys are stored along with other validator keys in priv_validator_key.json, | ||
which should exist before running the command. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can mention which command creates those files.
} | ||
pv := privval.LoadWrappedFilePV(keyPath, statePath) | ||
wrappedPV := privval.NewWrappedFilePV(pv.GetValPrivKey(), bls12381.GenPrivKey(), keyPath, statePath) | ||
wrappedPV.SetAccAddress(addr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it automatically saves itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, when we set the account address, we should save the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good apart from some possible enhancements to make sure the user actually passes some values.
…ting/create-bls-cli
Thanks @aakoshh for your comments. I changed the account address into an arg instead of a flag. I also added an example in the description of the PR. Please see the updates. Thanks! |
Hi @aakoshh, just a kind reminder that this PR might need another round of review. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks for the changes!
* Initial version of doc for btc scripts
This PR added a new command
create-bls-key
to create a pair of BLS keys for the validator. This command should be run afterpriv_validator_key.json
is generated viababylond init
orbabylond testnet
.An example of running this command is as follows.
After the command is successfully executed, the content of the
priv_validator_key.json
should be like the following:This command is a prerequisite of adding a validator with BLS keys.