Skip to content

feat(connect): Cardano Conway certificates and tagged sets #10742

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
Merged
merged 3 commits into from
Jun 2, 2024
Merged

feat(connect): Cardano Conway certificates and tagged sets #10742

merged 3 commits into from
Jun 2, 2024

Conversation

jaskp
Copy link
Contributor

@jaskp jaskp commented Jan 19, 2024 •
edited
Loading

Cardano Conway-era certificates and tagged CBOR sets

Update with select Cardano Conway features:

  • Transactions with new stake registration and deregistration certificates and a DRep vote delegation certificate.
  • Flag that tells the firmware to encode all CBOR "sets" with the 258 tag.

Related Issue

trezor/trezor-firmware#3496

Resources

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

@jaskp jaskp marked this pull request as ready for review January 31, 2024 22:20
@mroz22
Copy link
Contributor

mroz22 commented Feb 1, 2024

hello, we will focus this PR once fw is merged

@davidmisiak
Copy link
Contributor

Hi, the corresponding fw PR has been merged. What are the next steps? Should I rebase? (A similar situation will probably follow with message signing fw PR and Connect PR by the way.)

@mroz22
Copy link
Contributor

mroz22 commented May 23, 2024

yes, please rebase everything and we will take a look.

@mroz22
Copy link
Contributor

mroz22 commented May 23, 2024

or maybe a wait a moment until this is merged to avoid conflicts
#10543

@davidmisiak
Copy link
Contributor

I see that the version check (_ensureFeatureIsSupported) in cardanoSignTransaction got completely removed in the PR you linked. What should we do about the check in this PR?

@martykan
Copy link
Member

@mroz22 Maybe we should revert _ensureFeatureIsSupported in this case?

@martykan
Copy link
Member

@davidmisiak So please add _ensureFeatureIsSupported and the related functions back in when rebasing, we can see that it's still useful to keep them.
You can just remove the older feature flags since we bumped the minimum supported version to 2.6.0. and just use it for checking Conway support.

@martykan
Copy link
Member

I re-added the structure for checking feature support in #12555

@davidmisiak davidmisiak force-pushed the feat/cardano-conway-certificates branch from 28c615e to 0d89e41 Compare May 24, 2024 19:34
@davidmisiak
Copy link
Contributor

Rebased, and hopefully everything resolved correctly.

Sadly, I somehow cannot make the integration tests to run - is this something expected or is there something wrong on my side? I'm doing ./docker/docker-connect-test.sh node -i cardanoSignTransaction, there are a few passes and a few fails (FAIL src/types/api/__tests__/bitcoin.ts - Your test suite must contain at least one test.), but the actual integration tests seem not to be executed at all.

@mroz22
Copy link
Contributor

mroz22 commented May 26, 2024

./docker/docker-connect-test.sh node -i cardanoSignTransaction -p methods

but we will happily run them in CI

@davidmisiak
Copy link
Contributor

Any updates please?

mroz22
mroz22 reviewed May 30, 2024
View reviewed changes
docs/packages/connect/methods/cardanoSignTransaction.md
@@ -36,6 +36,7 @@ const result = await TrezorConnect.cardanoSignTransaction(params);
- `derivationType` — _optional_ `CardanoDerivationType` enum. Determines used derivation type. Default is set to ICARUS_TREZOR=2.
- `includeNetworkId` — _optional_ `Boolean`. Determines whether `networkId` should be explicitly serialized into the transaction body. Default is `false`.
- `chunkify` — _optional_ `boolean` determines if recipient address will be displayed in chunks of 4 characters. Default is set to `false`
- `tagCborSets` - _optional_ `boolean` determines if CBOR arrays intended to be sets will be encoded with tag 258. Default is set to `false`
Copy link
Contributor

Choose a reason for hiding this comment

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

could you please add this also to https://github.com/trezor/trezor-suite/blob/develop/packages/connect-explorer/src/pages/methods/cardano/cardanoSignTransaction.mdx#L0-L1 ?

also I wouldn't mind if you checked whether the mdx file isn't missing anything substantial

Copy link
Contributor

Choose a reason for hiding this comment

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

Added, also rebased. I believe nothing is missing from the file now.

@davidmisiak davidmisiak force-pushed the feat/cardano-conway-certificates branch from 0d89e41 to fd92a5c Compare May 31, 2024 09:54
@mroz22 mroz22 merged commit d737d3c into trezor:develop Jun 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mroz22 mroz22 mroz22 approved these changes

@szymonlesisz szymonlesisz Awaiting requested review from szymonlesisz szymonlesisz is a code owner

@martykan martykan Awaiting requested review from martykan martykan is a code owner

+1 more reviewer

@davidmisiak davidmisiak davidmisiak left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
No open projects
Suite Pull Requests
Status: 🗒 Draft
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jaskp @mroz22 @davidmisiak @martykan