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

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

Merged
merged 7 commits into from
Jul 13, 2020

Conversation

petar
Copy link
Contributor

@petar petar commented Jul 9, 2020

…default

@petar petar requested a review from aschmahmann July 9, 2020 22:19
@@ -65,6 +67,7 @@ var keyGenCmd = &cmds.Command{
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