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 alonzo transactions #71

Closed
wants to merge 13 commits into from

Conversation

davidmisiak
Copy link
Collaborator

@davidmisiak davidmisiak commented Sep 17, 2021

New features:

  • validation of map key canonical ordering (CIP-0021)
  • network_id is optionally serialized in tx body
  • datum_hash may be present in outputs if ORDINARY_TRANSACTION, MULTISIG_TRANSACTION or PLUTUS_TRANSACTION signing mode is used and the output address contains a script hash
  • script_data_hash may be present if ORDINARY_TRANSACTION, MULTISIG_TRANSACTION or PLUTUS_TRANSACTION signing mode is used
  • there is a new PLUTUS_TRANSACTION signing mode, which allows signing transactions with collateral inputs and required signers
  • stake credentials in certificates and withdrawals may be given as key_hash in PLUTUS_TRANSACTION

@davidmisiak davidmisiak changed the title feat(cardano): add support for alonzo feat(cardano): alonzo support Sep 17, 2021
@davidmisiak davidmisiak changed the title feat(cardano): alonzo support feat(cardano): add support for alonzo transactions Sep 17, 2021
Copy link
Collaborator

@gabrielKerekes gabrielKerekes left a comment

Choose a reason for hiding this comment

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

Overall LGTM. I noticed a few minor issues though 😉

I'm not sure about show_warning_tx_output_contains_data_hash - the name and how the hash is displayed. I know it's mentioned in the TODOs, I'm just mentioning it here as another reminder.

common/tests/fixtures/cardano/sign_tx.failed.json Outdated Show resolved Hide resolved
common/tests/fixtures/cardano/sign_tx.failed.json Outdated Show resolved Hide resolved
core/src/apps/cardano/address.py Outdated Show resolved Hide resolved
core/src/apps/cardano/sign_tx.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@gabrielKerekes gabrielKerekes left a comment

Choose a reason for hiding this comment

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

I have reviewed all the source code. I haven't tested it yet nor have I looked at the tests. I will do this tomorrow. So far I haven't found any major issues. Mostly just some refactoring suggestions.

core/src/apps/cardano/helpers/utils.py Outdated Show resolved Hide resolved
core/src/apps/cardano/layout.py Outdated Show resolved Hide resolved
python/src/trezorlib/cardano.py Show resolved Hide resolved
python/src/trezorlib/cardano.py Show resolved Hide resolved
core/src/apps/cardano/sign_tx.py Outdated Show resolved Hide resolved
core/src/apps/cardano/sign_tx.py Outdated Show resolved Hide resolved
core/src/apps/cardano/sign_tx.py Outdated Show resolved Hide resolved
core/src/apps/cardano/sign_tx.py Outdated Show resolved Hide resolved
core/src/apps/cardano/sign_tx.py Outdated Show resolved Hide resolved
core/src/apps/cardano/certificates.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@gabrielKerekes gabrielKerekes left a comment

Choose a reason for hiding this comment

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

Reviewed the tests, tried signing a few transactions and checked their bodies. Seems to work as currently expected 👍

common/tests/fixtures/cardano/sign_tx.plutus.failed.json Outdated Show resolved Hide resolved
common/tests/fixtures/cardano/sign_tx.plutus.failed.json Outdated Show resolved Hide resolved
common/tests/fixtures/cardano/sign_tx.plutus.failed.json Outdated Show resolved Hide resolved
@davidmisiak davidmisiak force-pushed the cardano-alonzo branch 4 times, most recently from a0cf806 to 2c90304 Compare November 11, 2021 17:55
Copy link
Collaborator

@gabrielKerekes gabrielKerekes left a comment

Choose a reason for hiding this comment

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

I think this looks good for now. Please fix my last comments, squash the fixups and then I think we should both go through it and test it once more to verify that all indeed is good.

core/src/apps/cardano/layout.py Outdated Show resolved Hide resolved
core/src/apps/cardano/layout.py Outdated Show resolved Hide resolved
core/src/apps/cardano/layout.py Outdated Show resolved Hide resolved
core/src/apps/cardano/layout.py Outdated Show resolved Hide resolved
core/src/apps/cardano/helpers/utils.py Outdated Show resolved Hide resolved
python/src/trezorlib/cli/cardano.py Show resolved Hide resolved
@gabrielKerekes
Copy link
Collaborator

I just realised that core/src/apps/cardano/README.md has not been updated, specifically the transaction sample. Could you please do that? This (https://gist.github.com/gabrielKerekes/ad6c168b12ebb43b082df5b92d67e276) should be the transaction I used to generate the Transaction body example. And perhaps it's also worth it to briefly mention the new fields and the option of including network id in tx body. And anything else you think is missing 🙈

Copy link
Collaborator

@gabrielKerekes gabrielKerekes left a comment

Choose a reason for hiding this comment

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

Just one nit pick and I would perhaps also add a test to sign_tx.plutus.json with a script payment address output and no datum hash to signify that it indeed should pass.

core/src/apps/cardano/layout.py Outdated Show resolved Hide resolved
@stefanobieler
Copy link

Hello, new to open source development. But Cardano just launched 3 DEXs and there is no hardware support for Alonzo smart contracts yet on the Trezor model T. Been tracking this issue for a while and noticed it's been sitting here. Is there someway i can help contribute in order to merge this feature? It's really really needed. Thanks.

@davidmisiak
Copy link
Collaborator Author

@stefanobieler
Hi, thanks for reaching out and willingness to help! We know that many members of the Cardano community want Plutus support on HW wallets and we’re currently finalising and testing these changes. We hope to create a PR to Trezor soon for them to review and hopefully it will then make it to the March firmware release (if there will be a release in March).

@davidmisiak davidmisiak force-pushed the cardano-alonzo branch 2 times, most recently from b7eac0a to 0d5ff74 Compare February 9, 2022 13:05
@davidmisiak davidmisiak force-pushed the cardano-alonzo branch 3 times, most recently from bfebbbc to ef292c0 Compare March 11, 2022 23:20
@davidmisiak
Copy link
Collaborator Author

Merged in trezor#2114.

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.

3 participants