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

client/keys: print out pubkeys in JSON format #808

Closed
wants to merge 1 commit into from
Closed

client/keys: print out pubkeys in JSON format #808

wants to merge 1 commit into from

Conversation

alessio
Copy link
Contributor

@alessio alessio commented Apr 6, 2018

No description provided.

Add flag to the show command to print out
public keys in JSON format.

See tendermint/go-crypto#79 for more information.
@alessio alessio requested a review from ebuchman as a code owner April 6, 2018 14:16
@codecov
Copy link

codecov bot commented Apr 6, 2018

Codecov Report

Merging #808 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #808   +/-   ##
========================================
  Coverage    65.26%   65.26%           
========================================
  Files           65       65           
  Lines         3524     3524           
========================================
  Hits          2300     2300           
  Misses        1041     1041           
  Partials       183      183

@@ -30,16 +39,20 @@ func getKey(name string) (keys.Info, error) {
// CMD

func runShowCmd(cmd *cobra.Command, args []string) error {
if len(args) != 1 || len(args[0]) == 0 {
return errors.New("You must provide a name for the key")
info, err := getKey(args[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

We should still check the length of args and print a helpful error if it isn't equal to 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See github.com/spf13/cobra.ExactArgs()

)

var showKeysCmd = &cobra.Command{
Use: "show <name>",
Short: "Show key info for the given name",
Long: `Return public details of one local key.`,
RunE: runShowCmd,
Args: cobra.ExactArgs(1),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cwgoes nope, this just does it

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, got it.

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.

LGTM

if err != nil {
return err
}
fmt.Println(string(out))
Copy link
Contributor

@rigelrozanski rigelrozanski Apr 10, 2018

Choose a reason for hiding this comment

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

I don't like the idea of moving out of the current process train of printing everything through the printInfo function (right below this comment)...
Also don't like introducing another key. Already you can get the pubkey in json by using gaiacli keys show <yourkey> --output json - If you want to also see the pubkey with "text" output (aka --output text aka the default) then we should add that right about here:

fmt.Printf("%s%s%s\n", info.Name, sep, addr)

maybe with a table header so it would look like:

name          address                pubkey 
alessio        197609375...       0x1241412.... 

just need to make sure we'd get it right for keys list as well as keys show but meh - I'm okay with just using --output json myself

Copy link
Contributor Author

@alessio alessio Apr 13, 2018

Choose a reason for hiding this comment

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

Hi!

Apologies for the late reply.

Already you can get the pubkey in json by using gaiacli keys show --output json

True, but the following prints the private key out too:

alessio@bizet:~/.../github.com/tendermint/clearchain$ clearchainctl keys show --output json ch_admin_1
{"name":"ch_admin_1","pubkey":[1,"CEB016A0495DEE269D655F12DE14941963214B3BEE034C3DDFDBCECB2627D3C7"],"privkey.armor":"-----BEGIN TENDERMINT PRIVATE KEY-----\nkdf: bcrypt\nsalt: CB69DDE1B83E179E7DED2C9A7FDE1BA8\n\n3Q2YAte6PArOEnMt9YR5CNmAyk7H6cFvZLuibwxoIlf+B7whJ8Xj4TZmBDo/JfEE\nlcdUZTafvGvHp2vxZ7PVg0ria+sJIPqQlgaNI1iGXqZ/kRYHSuRPIKwniRwrk6LE\nXFE5c8DN3Ceu\n=v8dc\n-----END TENDERMINT PRIVATE KEY-----"}

Furthermore, the public key JSON format seems a bit fuzzy: [1,"CEB016A0495DEE269D655F12DE14941963214B3BEE034C3DDFDBCECB2627D3C7"]

If you want to also see the pubkey with "text" output (aka --output text aka the default) then we should add that right about here: [snip]

That's fair, I'd be happy with that too! I just need to allow users to exchange public keys in a portable format, be it a string or a JSON thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

So it sounds like we just want to improve the JSON output format, which I could totally get behind - maybe don't print the private key by default too

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay yeah the validator json key is not a HEX string which stinks

@rigelrozanski
Copy link
Contributor

Closed in favour of #886

@alessio alessio deleted the show-pubkey-json branch April 24, 2018 03:03
mmsqe added a commit to mmsqe/cosmos-sdk that referenced this pull request Sep 29, 2024
…osmos#808)

* fix(x/tx): don't shadow Amino marshalling error messages (cosmos#19955)

* use directly instead of cast

---------

Co-authored-by: Antonio Pitasi <antonio@pitasi.dev>
dudong2 pushed a commit to b-harvest/cosmos-sdk that referenced this pull request Oct 30, 2024
…osmos#808)

* fix(x/tx): don't shadow Amino marshalling error messages (cosmos#19955)

* use directly instead of cast

---------

Co-authored-by: Antonio Pitasi <antonio@pitasi.dev>
dudong2 added a commit to b-harvest/cosmos-sdk that referenced this pull request Nov 6, 2024
* feat(x/tx): support bytes field as signer (cosmos#21850)

* Problem: multisig account failed on threshold encode after send tx   (cosmos#808)

* fix(x/tx): don't shadow Amino marshalling error messages (cosmos#19955)

* use directly instead of cast

---------

Co-authored-by: Antonio Pitasi <antonio@pitasi.dev>

* feat: Fix datarace

---------

Co-authored-by: mmsqe <mavis@crypto.com>
Co-authored-by: Antonio Pitasi <antonio@pitasi.dev>
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.

3 participants