-
Notifications
You must be signed in to change notification settings - Fork 318
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
CIP-0005 | Add prefixes for output datum hash and script data hash #177
CIP-0005 | Add prefixes for output datum hash and script data hash #177
Conversation
CIP-0005/README.md
Outdated
@@ -78,6 +78,8 @@ We define the following set of common prefixes with their corresponding semantic | |||
| `stake_vkh` | Stake address verification key hash | blake2b\_224 digest of a delegation verification key | | |||
| `stake_shared_vkh` | Shared stake address verification key hash | blake2b\_224 digest of a delegation verification key | | |||
| `vrf_vkh` | VRF verification key hash | blake2b\_256 digest of a VRF verification key | | |||
| `datum` | Output datum hash | blake2b\_256 digest of output datum | | |||
| `redeemer` | Script data hash | blake2b\_256 digest of script data | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what this means. Redeemers aren't hashed, I don't think. Do you mean the hash of an actual script? Then that is definitely not a redeemer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this refers to the script integrity hash, doesn't it? In which case (a) redeemer
is definitely not a good name, and (b) I am not sure we need / want a bech32 prefix for that one. It's very "internal" and not quite relevant to end users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It refers to the script_data_hash
field in the tx_body
(see CDDL). As it's included in the tx body, it gets processed by HW wallets and should be shown to the user.
Perhaps script_data
would be a better prefix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've renamed redeemer
to script_data
-> 769b7c6
CIP-0005/README.md
Outdated
| `addr_shared_vkh` | Shared address verification key hash | blake2b\_224 digest of a payment verification key | | ||
| `stake_vkh` | Stake address verification key hash | blake2b\_224 digest of a delegation verification key | | ||
| `stake_shared_vkh` | Shared stake address verification key hash | blake2b\_224 digest of a delegation verification key | | ||
| `signer_vkh` | Required signer verification key hash | blake2b\_224 digest of a required signer verification key | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussions on IOHK Slack we'd also like to add the signer_vkh
prefix which is to be used to display the required_signers
field of tx_body
.
The existing prefixes are not suitable, because a HW wallet cannot determine whether the key is a payment (1852) key, shared key (1854
) or a stake key so it wouldn't know which prefix to use. Another alternative would be to display the key hashes as hex, but that's also not ideal.
(the diff is showing also changes in the other prefixes but only the alignment of the Contents
column has changed in those lines - GitHub UI doesn't show it too well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you not want to include "required" or "req" somewhere in that prefix? We could have other cases where we need a vhk from a signer, this is a bit ambiguous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, thank you. Changed in f991d4d.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No objections from me.
@gabrielKerekes We'd like to merge this PR but it has conflicts and need to be rebased on top of current master. I would do it myself if I could but I can't push on your branch it seems. If you can either grant rights or, do it yourself 🙏 |
f991d4d
to
2c402a6
Compare
2c402a6
to
7f63055
Compare
@KtorZ I've rebased the branch on top of current master 👍 |
Thanks! |
We're suggesting the additions mainly for HW wallets.
For "Output datum hash" we also considered using
output_data
.For "Script data hash" we also considered using
script_data
orredeemers
(the plural rather than singular).The
datum
andredeemer
prefixes were actually proposed to us by @KtorZ, with the reasoning that this naming was used from the start in regard to the eUTXO model.