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.

Sign up for GitHub

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

Jump to bottom

Derive token policy forging keys according to CIP-1855 #2774

Merged
merged 8 commits into from
Jul 29, 2021
Merged

Derive token policy forging keys according to CIP-1855 #2774

merged 8 commits into from
Jul 29, 2021

Conversation

sevanspowell
Copy link
Contributor

Issue Number

ADP-1044

Overview

  • Add new AddressDerivation module implementing policy key derivation described in CIP-1855 (for minting and burning).
  • Add PolicyK option to the "Depth" enum (for mint/burn policy keys).
  • Add minimal tests to test the key derivation.

@sevanspowell
Resurrect hashVerificationKey
1cb2ff7 
- Resurrect hashVerificationKey (it was deleted by weeder).
@sevanspowell sevanspowell requested a review from rvl July 22, 2021 05:12
@sevanspowell sevanspowell changed the title Feature/adp 1044 derive token policy forging keys cip 1855 Derive token policy forging keys according to CIP-1855 Jul 22, 2021
@sevanspowell sevanspowell mentioned this pull request Jul 22, 2021
Closed
@sevanspowell sevanspowell force-pushed the feature/ADP-1044-derive-token-policy-forging-keys-CIP-1855 branch from 1be28d2 to 119472c Compare July 22, 2021 06:12
@sevanspowell
CIP-1855: Add primitives for MintBurn policy key derivation
726386f 
- Add primitives for deriving policy keys used to mint/burn assets. Implemented
  according to [CIP-1855](https://github.com/cardano-foundation/CIPs/blob/b2e9d02cb9a71ba9e754a432c78197428abf7e4c/CIP-1855/CIP-1855.md).
- Add "PolicyK" option to the "Depth" enum.
- Add simple tests for key derivation.
@sevanspowell sevanspowell force-pushed the feature/ADP-1044-derive-token-policy-forging-keys-CIP-1855 branch from 119472c to 726386f Compare July 22, 2021 06:13
rvl
rvl approved these changes Jul 22, 2021
View reviewed changes
Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

Looks good 👍

lib/core/src/Cardano/Wallet/Primitive/AddressDerivation.hs
| RoleK
| AddressK
| ScriptK
| PolicyK
Copy link
Contributor

Choose a reason for hiding this comment

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

Better update all the comments about Depth, starting with the one just above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmmm... not ideal. Kinda brutalizing the abstraction here, but 'ScriptK did it first and it's the path of least resistance...

Copy link
Contributor Author

@sevanspowell sevanspowell Jul 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Updated doco.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah if it weren't for ScriptK I would have asked you to try and find a better way of representing depth.

lib/core/src/Cardano/Wallet/Primitive/AddressDerivation.hs Outdated
hashVerificationKey
:: WalletKey k
=> KeyRole
-> k d XPub
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
-> k d XPub
-> key depth XPub

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Funnily enough I wanted to do this originally but LSP was giving me errors. Seems to be fine now 👍

Copy link
Contributor Author

@sevanspowell sevanspowell Jul 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Fixed.

lib/core/src/Cardano/Wallet/Primitive/AddressDerivation/MintBurn.hs Outdated
import Cardano.Crypto.Wallet.Types
( DerivationScheme (DerivationScheme2) )
import Cardano.Wallet.Primitive.AddressDerivation
( Depth (PolicyK, PurposeK, RootK)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please just use wildcards for constructor and record imports - then it's less painful to modify the code later.

Suggested change
( Depth (PolicyK, PurposeK, RootK)
( Depth (..)

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 wonder if there's an LSP setting for this. These imports are the way they are because I clicked "LSP: import this for me".

Copy link
Contributor Author

@sevanspowell sevanspowell Jul 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes LSP does the same for me, but it's so excessive ... and if you have LSP it shows where a symbol comes from in the tooltip.

lib/core/src/Cardano/Wallet/Primitive/AddressDerivation/MintBurn.hs
where
policyK = liftRawKey policyPrv
policyPrv = derivePolicyPrivateKey pwd (getRawKey rootPrv) policyIx
vkeyHash = hashVerificationKey CA.Payment (publicKey policyK)
Copy link
Contributor

Choose a reason for hiding this comment

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

Purpose field here doesn't look right ... right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean the CA.Payment value? I stripped out the Role -> KeyRole indirection because I found that confusing when initially using the function. As someone who wasn't familiar with the wallet, Payment | Delegation :: KeyRole made a lot more sense than UtxoExternal | UtxoInternal | MutableAccount :: Role, and I figured since I was the only one using it, I might as well use the simpler option. Also it doesn't feel like there's much need for abstraction here, we're literally reaching in and manipulating bytes. But I'm not strongly opposed to putting the Role -> KeyRole indirection back in.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that's all fine, but KeyHash { role = Payment } ? Actually I suppose Payment is suitable for this.

lib/core/src/Cardano/Wallet/Primitive/AddressDerivation/MintBurn.hs Outdated
-- ^ Index of policy script
-> (key 'PolicyK XPrv, KeyHash)
-- ^ Policy private key
derivePolicyKey pwd rootPrv policyIx = (policyK, vkeyHash)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good - perhaps you can think of a more different name for this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

lib/core/test/unit/Cardano/Wallet/Primitive/AddressDerivation/MintBurnSpec.hs
property prop_keyDerivationFromXPrv
it "Policy public key hash matches private key" $
property prop_keyHashMatchesXPrv

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps one or two golden tests that you have generated manually with cardano-cli?

rvl
rvl approved these changes Jul 23, 2021
View reviewed changes
lib/core/test/unit/Cardano/Wallet/Primitive/AddressDerivation/MintBurnSpec.hs
Comment on lines +118 to +119
key1 :: XPrv
key2 :: XPrv
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
key1 :: XPrv
key2 :: XPrv
key1, key2 :: XPrv

... or just leave out the type annotations.

@sevanspowell
Compare wallet policy key derivation to cardano-addresses
1a74768 
- Add some golden tests comparing wallet policy key derivation to
  cardano-addresses policy key derivation.
@sevanspowell sevanspowell force-pushed the feature/ADP-1044-derive-token-policy-forging-keys-CIP-1855 branch from aaaabb1 to 1a74768 Compare July 27, 2021 04:45
@sevanspowell sevanspowell requested a review from rvl July 29, 2021 02:40
rvl
rvl approved these changes Jul 29, 2021
View reviewed changes
Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

Excellent, thanks @sevanspowell !

lib/core/test/unit/Cardano/Wallet/Primitive/AddressDerivation/MintBurnSpec.hs
-- Hash of child key 1855H/1815H/0H generated by cardano-address
-- CLI from test mnemonic
unit_comparePolicyKeys goldenTestMnemonic (Index 0x80000010)
"acct_xsk1rprds8krzhspy8xhkupk2270hzkexmkdga98zv5ns4x9d9kf89t4qp438a26sn62wsfldthkrvnwe6vnetuehgxyxt4hm46zsw03mknuwef0afcljmultpjjwx8zqm0ftnfkqucwvf5qr4gved6h3gnyeqcsur4p"
Copy link
Contributor

Choose a reason for hiding this comment

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

cardano-address did not produce the correct bech32 prefix for policy keys, but never mind.

@rvl
Copy link
Contributor

rvl commented Jul 29, 2021

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 29, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 0aff501 into master Jul 29, 2021
@iohk-bors iohk-bors bot deleted the feature/ADP-1044-derive-token-policy-forging-keys-CIP-1855 branch July 29, 2021 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rvl rvl rvl approved these changes

Assignees

@sevanspowell sevanspowell

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sevanspowell @rvl