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.

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

feat(cardano): add support for plutus transactions #2114

Merged
merged 13 commits into from
Mar 18, 2022

Conversation

davidmisiak
Copy link
Contributor

@davidmisiak davidmisiak commented Feb 9, 2022

This PR adds support for Alonzo-era transactions. CDDL

New features:

  • A new signing mode (PLUTUS_TRANSACTION) that is to be used with transactions involving Plutus script evaluation. Because of how Plutus scripts work, this requires all tx items to be shown to the user to verify (including tx inputs, since they are no longer interchangeable). New tx items 13: collateral_inputs and 14: required_signers are only allowed in this signing mode.
  • New tx item 11: script_data_hash and an output item datum_hash which are used when attaching datums to a tx (when sending funds to a script address). Allowed in all signing modes except POOL_REGISTRATION_AS_OWNER.
  • Stake credentials in certificates and withdrawals may be given as key_hash in PLUTUS_TRANSACTION.
  • 15: network_id may be serialized into tx body.
  • Validation of map key canonical ordering (CIP-0021).

It turns out that the implemented UX may be suboptimal in some cases (e.g. if the tx uses a Plutus script to mint a token and adds it to a "collector" UTXO which holds all the user's token, the user has to click through all the tokens, since all outputs must be shown, including the device-owned outputs). These cases shouldn’t be too common and can also be avoided by storing/moving the tokens to a different address. At the moment, we are contemplating possible UX improvements (without compromising security) and will eventually propose them as a new PR.

The number of added lines seems daunting, but most of it are tests.

We will update the changelog and UI fixtures after the PR is otherwise approved.

We made corresponding changes in Connect as well, but I suggest opening a PR when this one is approved by you.

@davidmisiak davidmisiak requested a review from matejcik as a code owner February 9, 2022 13:25
@davidmisiak
Copy link
Contributor Author

Regarding the Plutus UX issue mentioned in the PR description, we have added two incremental improvements:

  • In ac64b22, we added tx hash displaying to Plutus txs, so that experienced users with a trusted device to compute the tx hash can compare the tx hash displayed on screen with the computed one (instead of e.g. manually verifying a large number of assets).
  • In 2d628a6, we allowed outputs with addresses given as address parameters in Plutus txs. This way, user still has to click through all tokens in the output, but at least they are clearly notified that the output is owned by their device and the tokens are not being sent anywhere.

If you have any suggestions how to further improve the Plutus UX, we would very much appreciate them.

Also, @matejcik is there still a release planned in March please? And if so, is there any chance this could make it into the March release?

@matejcik
Copy link
Contributor

There will not be a release in March.
We don't know yet if there is going to be a release in April, I will let you know once we are more sure.

@mmahut mmahut mentioned this pull request Feb 21, 2022
Copy link
Contributor

@matejcik matejcik left a comment

Choose a reason for hiding this comment

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

LGTM overall. Some small issues in comments.

core/src/apps/cardano/helpers/credential.py Outdated Show resolved Hide resolved
core/src/apps/cardano/helpers/utils.py Outdated Show resolved Hide resolved
core/src/apps/common/cbor.py Outdated Show resolved Hide resolved
core/src/apps/cardano/sign_tx.py Outdated Show resolved Hide resolved
@matejcik
Copy link
Contributor

matejcik commented Mar 3, 2022

General comment -- not a prerequisite for this PR, but work to be done as the next step in Cardano evolution.

The sign_tx file is getting too big and is in a dire need of refactoring.

An outline of what seems reasonable:

Create a Signer class that will store the context, keychain, the initial CardanoSignTx message, tx body builder, approver, etc., all the things that are effectively global but everything is passing them around all the time. Most functions in sign_tx will be methods of this class.

ISTM you could subclass Signer for the different signing modes, so e.g., OrdinarySigner will not accept collateral inputs; PlutusSigner will override per-input function to show confirmation, etc. Look for inspiration in Bitcoin / BitcoinLike class structure.

sign_tx should not import so many things by name. It's blowing up the module dict. Instead of importing pretty much layout.*, we should just import layout and call the functions as layout.something, dtto for a lot of other things.

We should get rid of the static exception objects in helpers and inline the wire.DataError("something") instead of using the constants; the constants are saving very little.

In general the identifiers are extremely long, we should shorten at least local variable names -- but also things like cborize_initial_pool_registration_certificate_fields are NOT great.


please let me know if you will be working on any of this, otherwise I'll probably start with the refactor myself.

@davidmisiak
Copy link
Contributor Author

@matejcik Thanks for the review.

please let me know if you will be working on any of this, otherwise I'll probably start with the refactor myself.

We could do the refactoring but only as part of a larger workload. Currently we are discussing with IOHK the next steps regarding HW wallets but it’s possible no further features/updates will be needed in the following months. So if it is OK for you to wait until the next workload, then I suppose we could do the refactoring.

@matejcik
Copy link
Contributor

matejcik commented Mar 9, 2022

FYI, we are now leaning towards an April relese. still not confirmed but a little more likely than not.

Copy link
Contributor

@matejcik matejcik left a comment

Choose a reason for hiding this comment

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

LGTM on code side, please rebase, squash fixups, add UI tests and changelogs, etc.

@matejcik
Copy link
Contributor

So if it is OK for you to wait until the next workload, then I suppose we could do the refactoring.

Thanks for the info, we will see what happens. Depending on the workload on our side, I might start with the refactor sooner, or wait for your next workload.

@davidmisiak davidmisiak force-pushed the cardano-alonzo branch 2 times, most recently from bfebbbc to ef292c0 Compare March 11, 2022 23:20
@davidmisiak
Copy link
Contributor Author

Squashed and rebased; changelog and UI fixtures updated in ef292c0.

@matejcik
Copy link
Contributor

the UI test pipeline is failing -- it seems that you removed or renamed two tests but didn't remove the fixtures entries?
https://gitlab.com/satoshilabs/trezor/trezor-firmware/-/jobs/2220360680

feel free to force-push the fix directly

@davidmisiak
Copy link
Contributor Author

I'm sorry for the inconvenience, it should be fixed now.

@matejcik matejcik merged commit 915781b into trezor:master Mar 18, 2022
@matejcik
Copy link
Contributor

and done.
thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants