Add pub option for public keys #87

paweljakubas · 2020-10-30T17:46:59Z

For credentials from public key we need to have public keys without chain code.
cardano-foundation/cardano-wallet#2253

This PR adds a way to get public key (without chain code) if --pub is sneaked. Otherwise extended public key is returned.

paweljakubas · 2020-10-30T17:47:53Z

README.md

+$ cat recovery-phrase.prv | cardano-address key from-recovery-phrase Shelley \
+| cardano-address key child 1852H/1815H/0H/2/0 \
+| cardano-address key public --pub
+xpub16apaenn9ut6s40lcw3l8v68xawlrlq20z2966uzcx8jmv2q9uy7qh83kg9


I would like here pub rather than xpub - but do not know at this moment how to elegantly do it 🤔

or more precisely I would like addr_vk if we are dealing with payment/delegating address and stake_vk if we are dealing with stake address (reward account). If not possible we will need a way to have --bech32WithHrp prefix

Perhaps use pub/xpub only in cardano-address, and show how to use the bech32 util to change HRP.

What do you think of flipping this around and, have this being the default behavior, but require an explicit --with-chain-code flag to return the public key with chain code.

We should avoid returning the chain code unless the user knows what he/she is doing with it. Changing the default, we would also need to change a bit the error message when failing to parse bytes into a public key + chain code (so that it hints users in the right direction, that a 32-byte chain code is missing).

I like that idea of requiring the user to specify what kind of key they need. Such as:

(--with-chain-code | --without-chain-code ).

cardano-address key xpub / cardano-address key public

( --extended-key | --key-only )

KtorZ · 2020-11-02T08:26:30Z

README.md

-$ cardano-address recovery-phrase generate --size 15 > recovery-phrase.prv
-$ cat recovery-phrase.prv | cardano-address key from-recovery-phrase Shelley
-xprv1fzu4e8cecxshgzzxzh7557sd8tffqreeq2je7fgsm7f02mq849vdupw7qwgxc3qawyqev0l8ew0f4fkp8hvr8mskz4hz6e6ejzjlevcskcl6lqpr07u7552fsfgteztuclse7luh4cp493zdhkrjdss0250cdw8n
+$ cat recovery-phrase.txt | cardano-address key from-recovery-phrase Shelley


Why this change ? I think it was clearer before with both instructions visible in the same excerpt.

@KtorZ fair point. I was thinking here in the following way. We use prv, pub, xpub, pub for keys...and for me mnemonic sequence is just text.... If that does not persuade you I will revert this in coming commit. @rvl do you have an opinion on that?

Yes and yes

It would be better if the recovery-phrase generate command were added back to provide context.

.txt is a good extension to describe the content of the file.

KtorZ · 2020-11-02T08:28:49Z

README.md

+$ cat recovery-phrase.prv | cardano-address key from-recovery-phrase Shelley \
+| cardano-address key child 1852H/1815H/0H/2/0 \
+| cardano-address key public --pub
+xpub16apaenn9ut6s40lcw3l8v68xawlrlq20z2966uzcx8jmv2q9uy7qh83kg9


What do you think of flipping this around and, have this being the default behavior, but require an explicit --with-chain-code flag to return the public key with chain code.

We should avoid returning the chain code unless the user knows what he/she is doing with it. Changing the default, we would also need to change a bit the error message when failing to parse bytes into a public key + chain code (so that it hints users in the right direction, that a 32-byte chain code is missing).

rvl

Looks good so far - do you think pub or xpub should be the "default"? Or should there be separate subcommands for pub and xpub?

rvl · 2020-11-02T08:10:47Z

command-line/lib/Options/Applicative/Public.hs

+    ( Parser, flag, long )
+
+
+data PublicType = WithChainCode | WithoutChainCode


Does a single type and parse function need its own module?

I somewhat just complied with the pattern we have for other things. So although I am slightly in favour with your opinion on this this would be outlier and may surprise anyone NEW looking at the code....

rvl · 2020-11-02T08:13:07Z

command-line/lib/Command/Key/Public.hs

+            , "Extended public key is default. In order to get public key"
+            , ", ei., without chain code use '--pub'."


Do you know which use case is more common?
I am thinking that public key only might be a better default. Not sure though.
Another alternative could be to have completely separate subcommands -- e.g. public and xpub, or something like that. Then you could tell users to run xpub if they need to derive child keys.

More than which one is most common, I'd go for the "safest" from a privacy standpoint here. Since sharing your chain code allows other to create new (public) child keys from your public key, you may inadvertently gives access to an entire part of your sub-tree by sharing your extended public key.

Plus, cardano-cli and other cardano-tools mostly deals with non-extended ed25519 keys only.

introduced --with-chain-code and --without-chain-code here c2e3c78

rvl · 2020-11-02T08:25:32Z

README.md

+$ cat recovery-phrase.prv | cardano-address key from-recovery-phrase Shelley \
+| cardano-address key child 1852H/1815H/0H/2/0 \
+| cardano-address key public --pub
+xpub16apaenn9ut6s40lcw3l8v68xawlrlq20z2966uzcx8jmv2q9uy7qh83kg9


Perhaps use pub/xpub only in cardano-address, and show how to use the bech32 util to change HRP.

rvl · 2020-11-02T08:45:33Z

command-line/lib/Command/Key/Public.hs

            ])
  where
    parser = Public
        <$> encodingOpt [humanReadablePart|xpub|]
+        <*> publicOpt


Sometthing like this will let you change the HRP...

Suggested change

<*> publicOpt

parser = do

pub <- publicOpt

Public <$> encodingOpt (if pub == WithChainCode then [humanReadablePart|xpub|] else [humanReadablePart|pub|])

<*> pure pub

(clean it up though)

thanks for suggestion! in the end solved this like that 4ad1a31

if you use monadic rather than applicative then the ??? placeholder is not needed.

…n PublicType

rvl

A couple style issues, but otherwise good.

rvl · 2020-11-04T12:05:15Z

README.md

-$ cardano-address recovery-phrase generate --size 15 > recovery-phrase.prv
-$ cat recovery-phrase.prv | cardano-address key from-recovery-phrase Shelley
-xprv1fzu4e8cecxshgzzxzh7557sd8tffqreeq2je7fgsm7f02mq849vdupw7qwgxc3qawyqev0l8ew0f4fkp8hvr8mskz4hz6e6ejzjlevcskcl6lqpr07u7552fsfgteztuclse7luh4cp493zdhkrjdss0250cdw8n
+$ cat recovery-phrase.txt | cardano-address key from-recovery-phrase Shelley


Yes and yes

It would be better if the recovery-phrase generate command were added back to provide context.

.txt is a good extension to describe the content of the file.

rvl · 2020-11-04T12:09:35Z

command-line/lib/Command/Key/Public.hs

            ])
  where
    parser = Public
        <$> encodingOpt [humanReadablePart|xpub|]
+        <*> publicOpt


if you use monadic rather than applicative then the ??? placeholder is not needed.

rvl · 2020-11-04T12:12:35Z

command-line/lib/Command/Key/Public.hs

    xprv <- hGetXPrv stdin
    let xpub = toXPub xprv
-    hPutBytes stdout (xpubToBytes xpub) encoding
+    (bytes, encoding') <- case chainCode of


This line doesn't need to be an IO action. It could be a plain let or where clause.

I would be inclined to set up hrp in the CLI Parser, and use a function.

let toBytes = case chainCode of WithChainCode -> xpubToBytes WithoutChainCode -> xpubPublicKey hPutBytes stdout (toBytes xpub) encoding

rvl · 2020-11-04T12:12:48Z

command-line/lib/Options/Applicative/Public.hs

+    ( Parser, flag, long )
+
+
+data PublicType = WithChainCode | WithoutChainCode


rvl · 2020-11-04T12:15:32Z

command-line/lib/Command/Key/Public.hs

    { encoding :: Encoding
+    , chainCode :: PublicType


Parser will look better if the order of constructor fields are swapped.

2253: ADP-494 - Add POST endpoint for script address r=paweljakubas a=paweljakubas # Issue Number https://jira.iohk.io/browse/ADP-494 # Overview - [x] I have updated swagger with script as string - [x] I have added script as json - [x] I have extended Api - [x] I have extended core unit tests - [x] I have added cardano-addresses based impl - [x] I have added illustrative integration tests # Comments - Uses IntersectMBO/cardano-addresses#87 Co-authored-by: Pawel Jakubas <[email protected]> Co-authored-by: Rodney Lorrimar <[email protected]> Co-authored-by: IOHK <[email protected]>

paweljakubas added 3 commits October 30, 2020 17:32

add public applicative parser

9ec8f19

embrace publicOpt parser in public cmd

d07059e

update README.md

6844368

paweljakubas requested review from rvl and KtorZ October 30, 2020 17:47

paweljakubas self-assigned this Oct 30, 2020

paweljakubas commented Oct 30, 2020

View reviewed changes

paweljakubas mentioned this pull request Nov 1, 2020

ADP-494 - Add POST endpoint for script address cardano-foundation/cardano-wallet#2253

Merged

6 tasks

KtorZ reviewed Nov 2, 2020

View reviewed changes

rvl reviewed Nov 2, 2020

View reviewed changes

paweljakubas added 2 commits November 4, 2020 11:44

introduce explicit with/without-chain-code in public

c2e3c78

revise all footers, descriptions, README.md also make hrp dependent o…

4ad1a31

…n PublicType

paweljakubas requested review from rvl and KtorZ November 4, 2020 11:50

rvl approved these changes Nov 4, 2020

View reviewed changes

KtorZ approved these changes Nov 4, 2020

View reviewed changes

improve on README.md

dfe9e0c

paweljakubas merged commit eaa946c into master Nov 4, 2020

paweljakubas deleted the paweljakubas/add-pub-option-for-public-keys branch November 4, 2020 17:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pub option for public keys #87

Add pub option for public keys #87

paweljakubas commented Oct 30, 2020

paweljakubas Oct 30, 2020

paweljakubas Oct 30, 2020

rvl Nov 2, 2020

KtorZ Nov 2, 2020

rvl Nov 2, 2020

KtorZ Nov 2, 2020

paweljakubas Nov 4, 2020

rvl Nov 4, 2020

KtorZ Nov 2, 2020

rvl left a comment

rvl Nov 2, 2020

paweljakubas Nov 4, 2020

rvl Nov 4, 2020

rvl Nov 2, 2020

KtorZ Nov 2, 2020

paweljakubas Nov 4, 2020

rvl Nov 2, 2020

rvl Nov 2, 2020

paweljakubas Nov 4, 2020

rvl Nov 4, 2020

rvl left a comment

rvl Nov 4, 2020

rvl Nov 4, 2020

rvl Nov 4, 2020

rvl Nov 4, 2020

rvl Nov 4, 2020

		( Parser, flag, long )


		data PublicType = WithChainCode \| WithoutChainCode

		, "Extended public key is default. In order to get public key"
		, ", ei., without chain code use '--pub'."

-        <*> publicOpt
+  parser = do
+    pub <- publicOpt
+    Public <$> encodingOpt (if pub == WithChainCode then [humanReadablePart|xpub|] else [humanReadablePart|pub|])
+        <*> pure pub

Add pub option for public keys #87

Add pub option for public keys #87

Conversation

paweljakubas commented Oct 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment