feat(cardano): conway staking certificates #86

jaskp · 2023-11-06T14:59:49Z

Update with select Cardano Conway features:

Transactions with new stake registration and deregistration certificates and a DRep vote delegation certificate (does not include any new key derivations nor voting procedures)
Explicit tagging of CBOR arrays as sets with tag 258
Increase of URL length limit from 64 to 128 bytes

https://github.com/IntersectMBO/cardano-ledger/blob/master/eras/conway/impl/cddl-files/conway.cddl
https://github.com/cardano-foundation/CIPs/tree/master/CIP-0005
https://github.com/cardano-foundation/CIPs/tree/master/CIP-1694
https://sancho.network/tools-resources/faq/#4-certificate-and-transaction-field-witnesses-and-deposits

davidmisiak

Apart from the comments, LGTM!

common/protob/messages-cardano.proto

core/src/apps/cardano/certificates.py

core/src/apps/cardano/layout.py

common/tests/fixtures/cardano/sign_tx.json

python/src/trezorlib/cardano.py

jaskp · 2023-11-10T17:23:51Z

Handled comments except for the naming which I believe hasn't been decided on yet

davidmisiak

For now LGTM! Let's add the third certificate.

core/src/apps/cardano/certificates.py

common/protob/messages-cardano.proto

python/src/trezorlib/cardano.py

core/src/apps/cardano/certificates.py

core/src/apps/cardano/layout.py

davidmisiak · 2023-11-24T22:42:37Z

core/src/apps/cardano/layout.py

+def _format_drep(drep: messages.CardanoDRep) -> tuple[str, str]:
+    if drep.type == CardanoDRepType.KEY_HASH:
+        assert drep.key_hash is not None  # validate_drep
+        return (
+            "Delegating to key hash:",
+            bech32.encode(bech32.HRP_KEY_HASH, drep.key_hash),
+        )
+    elif drep.type == CardanoDRepType.SCRIPT_HASH:
+        assert drep.script_hash is not None  # validate_drep
+        return (
+            "Delegating to script:",
+            bech32.encode(bech32.HRP_SCRIPT_HASH, drep.script_hash),
+        )


@janmazak just to make sure - the drep key hash / script hash should be displayed with the usual addr_vkh and script bech32 prefixes, right?

The source code of this demo app suggests the prefix should be drep

const dRepIDBech32 = dRepID.to_bech32('drep');

But I wasn't able to find anything about it elsewhere

https://github.com/cardano-foundation/CIPs/blob/master/CIP-0005/README.md?plain=1#L118

https://github.com/cardano-foundation/CIPs/blob/master/CIP-0005/README.md?plain=1#L118

@Ryun1 thanks, that explains key-hash drep IDs, is it also specified somewhere whether a script-hash drep ID should also have this prefix, or should it just be "script"?

davidmisiak

LGTM, I will do one final pass when we finalize the remaining features :)

davidmisiak

Apart from the comments, LGTM.

Please also run make pystyle.

common/protob/messages-cardano.proto

tests/device_tests/cardano/test_sign_tx.py

core/src/apps/common/cbor.py

core/src/apps/cardano/sign_tx/signer.py

common/tests/fixtures/cardano/sign_tx.json

janmazak · 2024-01-10T10:46:28Z

core/src/apps/cardano/certificates.py

+            certificate.key_hash,
+            ProcessError("Invalid certificate"),
+        )
+
    account_path_checker.add_certificate(certificate)


 def _validate_structure(certificate: messages.CardanoTxCertificate) -> None:
    pool = certificate.pool  # local_cache_attribute
    pool_parameters = certificate.pool_parameters  # local_cache_attribute


I don't understand these comments --- should the newly added items get some comments too?

I don't understand them either. I gather they're used for some metaprogramming to reduce code size but I'm not sure when to use them. But it's a trivial fix so I expect to find out in a review from trezor

janmazak · 2024-01-10T12:51:35Z

core/src/apps/common/cbor.py

+
+# See https://www.iana.org/assignments/cbor-tags/cbor-tags.xhtml
+_CBOR_RAW_TAG = const(0x18)  # Tag 24
+_CBOR_SET_TAG = const(0x102)  # Tag 258


What about tag 30? It is used in pool registration for unit_interval, so I'd perhaps expect it to appear in this list too?

For whatever reason, all the cbor constants are private in this file. But yeah tag 30 is used in pool registration. Tag 24 is also used somewhere but, again, hardcoded instead of using the constant that already exists here

janmazak · 2024-01-10T14:47:49Z

What about increased max URL / DNS name length? Not included in this PR yet?

github-actions · 2024-01-14T22:00:34Z

legacy UI changes	device test(screens) main(screens)

github-actions · 2024-01-14T22:00:36Z

core UI changes	device test	click test	persistence test
T2T1 Model T	test(screens) main(screens)	test(screens) main(screens)	test(screens) main(screens)
T2B1 Safe 3	3280	test(screens) main(screens)	2724
T3T1	test(screens) main(screens)	test(screens) main(screens)	test(screens) main(screens)
All	main(screens)

jaskp · 2024-01-14T22:16:42Z

What about increased max URL / DNS name length? Not included in this PR yet?

Right, length update added to this pr

jaskp added the WIP Work In Progress label Nov 6, 2023

jaskp requested a review from davidmisiak November 6, 2023 14:59

jaskp self-assigned this Nov 6, 2023

jaskp changed the title ~~feat(cardano): conway staking reg certificate~~ feat(cardano): conway certificates Nov 6, 2023

jaskp force-pushed the feat/cardano/conway/stake-certs branch from 2f2adc9 to bfad4c7 Compare November 7, 2023 13:43

jaskp changed the title ~~feat(cardano): conway certificates~~ feat(cardano): conway staking certificates Nov 8, 2023

jaskp marked this pull request as ready for review November 8, 2023 21:05

davidmisiak reviewed Nov 9, 2023

View reviewed changes

jaskp force-pushed the feat/cardano/conway/stake-certs branch 2 times, most recently from cfa8c24 to c20cefc Compare November 10, 2023 17:22

jaskp removed the WIP Work In Progress label Nov 10, 2023

jaskp requested a review from davidmisiak November 10, 2023 17:23

davidmisiak reviewed Nov 12, 2023

View reviewed changes

jaskp force-pushed the feat/cardano/conway/stake-certs branch 2 times, most recently from 98ed741 to e6c17c4 Compare November 20, 2023 17:22

jaskp requested a review from davidmisiak November 20, 2023 17:26

davidmisiak requested changes Nov 22, 2023

View reviewed changes

jaskp requested a review from davidmisiak November 23, 2023 23:59

davidmisiak reviewed Nov 24, 2023

View reviewed changes

jaskp force-pushed the feat/cardano/conway/stake-certs branch from a6ac340 to 8e84fc5 Compare December 8, 2023 15:53

jaskp requested a review from davidmisiak December 11, 2023 11:54

davidmisiak requested changes Dec 11, 2023

View reviewed changes

davidmisiak approved these changes Jan 3, 2024

View reviewed changes

janmazak approved these changes Jan 10, 2024

View reviewed changes

jaskp requested review from janmazak and davidmisiak January 13, 2024 22:58

jaskp force-pushed the feat/cardano/conway/stake-certs branch from fdbfa20 to 8cd00fd Compare January 14, 2024 22:00

jaskp force-pushed the feat/cardano/conway/stake-certs branch from 8cd00fd to 6a8b99c Compare January 14, 2024 22:03

davidmisiak approved these changes Jan 15, 2024

View reviewed changes

jaskp force-pushed the feat/cardano/conway/stake-certs branch 2 times, most recently from 9846b99 to d8922c3 Compare January 18, 2024 16:50

jaskp added 4 commits May 16, 2024 23:17

feat(cardano): Add Conway certificates

0f1ae95

feat(cardano): Tag sets

0c0be49

chore(cardano): Update URL max length

97f8490

chore: Changelog

661e6a3

davidmisiak force-pushed the feat/cardano/conway/stake-certs branch from d8922c3 to 7cd8dad Compare May 16, 2024 22:08

chore(cardano): update UI fixtures

ad9b6e7

davidmisiak force-pushed the feat/cardano/conway/stake-certs branch from 9ca2c8d to ad9b6e7 Compare May 22, 2024 14:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cardano): conway staking certificates #86

feat(cardano): conway staking certificates #86

jaskp commented Nov 6, 2023 •

edited

Loading

davidmisiak left a comment

jaskp commented Nov 10, 2023

davidmisiak left a comment

davidmisiak Nov 24, 2023

jaskp Dec 7, 2023

Ryun1 Dec 7, 2023

jaskp Dec 8, 2023

davidmisiak left a comment

davidmisiak left a comment

janmazak Jan 10, 2024

jaskp Jan 13, 2024

janmazak Jan 10, 2024

jaskp Jan 13, 2024

janmazak commented Jan 10, 2024

github-actions bot commented Jan 14, 2024 •

edited

Loading

github-actions bot commented Jan 14, 2024 •

edited

Loading

jaskp commented Jan 14, 2024

feat(cardano): conway staking certificates #86

Are you sure you want to change the base?

feat(cardano): conway staking certificates #86

Conversation

jaskp commented Nov 6, 2023 • edited Loading

davidmisiak left a comment

Choose a reason for hiding this comment

jaskp commented Nov 10, 2023

davidmisiak 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

davidmisiak left a comment

Choose a reason for hiding this comment

davidmisiak 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

janmazak commented Jan 10, 2024

github-actions bot commented Jan 14, 2024 • edited Loading

github-actions bot commented Jan 14, 2024 • edited Loading

jaskp commented Jan 14, 2024

jaskp commented Nov 6, 2023 •

edited

Loading

github-actions bot commented Jan 14, 2024 •

edited

Loading

github-actions bot commented Jan 14, 2024 •

edited

Loading