-
Notifications
You must be signed in to change notification settings - Fork 21
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 metadata support #57
Conversation
DavidTranDucVL
commented
Mar 12, 2021
•
edited
Loading
edited
- Transactions created with cardano-cli in mary-era create metaData in format unsupported by current trezor connect version.
- Adds metadata support for both ledger and trezor.
{ | ||
"type": "TxUnsignedShelley", | ||
"description": "", | ||
"cborHex": "82a5008182582008f7b94f631b931ee48bb9c9447f9cc7c366a76ad04b66033cfd3f4ba95740500001818258390080f9e2c88e6c817008f3a812ed889b4a4da8e0bd103f86e7335422aa122a946b9ad3d2ddf029d3a828f0468aece76895f15c9efbd69b42771a3b97e3d0021a0002e630031a014369d7075820c0523b09bef39412ffd6e9eeb4c4171673821bbe5c06c7e522ecffca8b856eeda1016763617264616e6f" |
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.
Are the full metadata really appended to the unsigned tx body by cardano-cli? I thought not but I hope I'm wrong :)
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.
Yes, metadata are part of tx.
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.
Other than the comments LGTM as long as its tested.
@@ -314,6 +316,13 @@ const TrezorCryptoProvider: () => Promise<CryptoProvider> = async () => { | |||
validityIntervalStart && validityIntervalStart.toString() | |||
) | |||
|
|||
const prepareMetaDataHex = (metaData: Buffer | null): string | null => { |
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.
Is there a trezor type we could define as return type of this function? Maybe not but if so it would be nice.
Also we could have also the prepareTtl
and prepareValidityIntervalStart
return some Trezor/Ledger
prefixed type so all the prepare
function return either Ledger
or Trezor
type, if those types change we can always update the corresponding type-file and the ide would tell us what needs to be fixed. But I'd suggest addressing it in separate PR.
@@ -293,6 +293,10 @@ export const LedgerCryptoProvider: () => Promise<CryptoProvider> = async () => { | |||
validityIntervalStart && validityIntervalStart.toString() | |||
) | |||
|
|||
const prepareMetaDataHashHex = (metaDataHash: Buffer | null): string | null => ( |
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.
Also in another PR, I would create _Metadata
type, also _Ttl
and _ValidityIntervalStart
types.
a8f8b33
to
82c6a9b
Compare