Skip to content

add flag to "ipfs key gen" to output keys in b36/CIDv1, also make it …#7531

Merged
aschmahmann merged 7 commits intomasterfrom
petar/keyflag
Jul 13, 2020
Merged

add flag to "ipfs key gen" to output keys in b36/CIDv1, also make it …#7531
aschmahmann merged 7 commits intomasterfrom
petar/keyflag

Conversation

@petar
Copy link
Contributor

@petar petar commented Jul 9, 2020

…default

@petar petar requested a review from aschmahmann July 9, 2020 22:19
Options: []cmds.Option{
cmds.StringOption(keyStoreTypeOptionName, "t", "type of the key to create: rsa, ed25519").WithDefault("rsa"),
cmds.IntOption(keyStoreSizeOptionName, "s", "size of the key to generate"),
cmds.BoolOption(cidOptionName, "c", "return a base-36 CIDv1 encoding of the key").WithDefault(true),
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that we're planning on using b36 as the default the documentation and style of this flag doesn't really make much sense. i.e. what does it mean if "return a base-36 CIDv1 encoding of the key" is false, how is it encoded?

We could do the opposite sort of flag (e.g. "--b58Mh"), but that leaves us slightly exposed in the event we ever want to change the default going forward (hopefully not for a long time 🤞). WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think, there should be explicit flags for every supported variant, because it sounds like the default may flip-flop. So how about a flag with mutually-exclusive variants: --format b58mh | b36cid?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that seems reasonable. If someone wants anything else they can pass the b36cid into the ipfs cid function. I'm not sure whether the cid option should be called b36cid or just cid, with a comment saying it's currently b36, both seem fine so your call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like b58mh and b36cid, because they have the same schema.

Copy link
Contributor

@aschmahmann aschmahmann left a comment

Choose a reason for hiding this comment

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

Looks great!

@aschmahmann aschmahmann merged commit b3e5ffc into master Jul 13, 2020
@aschmahmann aschmahmann mentioned this pull request Sep 22, 2020
72 tasks
@hacdias hacdias deleted the petar/keyflag branch May 9, 2023 10:59
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.

2 participants