Cardano shelley update 1/3 #1096

gabrielKerekes · 2020-07-06T12:40:29Z

Cardano Shelley update issue: #948

As promised (#1048) we are splitting the Cardano Shelley update into (at least) 3 PRs. This is the first of them.

This PR contains minimal changes with which Trezor can work on the new Shelley fork. It enables sending funds from one Byron address to another. New addresses are not yet added. Neither are certificates and withdrawals.

What's changed:

Previous transactions (input sources) no longer need to be requested/received since Fee is included directly in the transaction.
TTL parameter has been added to transaction - this is the slot number until which the transaction can be submited.
Transaction format has changed a bit.
The biggest changes are in the sign_tx.py file. The logic around fee calculation has been removed and it has also been refactored quite a lot - e.g. Transaction class has been removed since it now mostly contained only functions anyways.
Support for CBOR NULL had to be added to support empty transaction metadata.
Protocol magic is included in Byron addresses on testnets - this has always been a part of the spec, but it hasn't been needed so far. We now also verify that outputs are from the network which the protocol magic represents.

What can still change in the following days (regarding this PR):

~~We still don't have specs/information about witnesses for Byron inputs from IOHK. This will probably require changes to the way the witnesses are built for Byron inputs. There's also a todo there.~~ This has already been added.
~~Protocol magic in Byron addresses on Testnets - this functionality is drafted, but also needs to be confirmed by IOHK.~~ This has already been added.
Maybe some other minor changes.

Here's a gist with a sample transaction input file for trezorctl (although tests also provide some samples).
Here's a gist with a description of the Shelley transaction format (including certificates and withdrawals).
Here's the official CDDL spec for Shelley.

This hasn't been tested on the latest testnet yet, since Byron addresses are not yet fully supported. Support should be added in the following days. When this happens, we will test it and let you know of the status (Some changes may still need to be made regarding this).

prusnak · 2020-07-13T12:54:28Z

For the record: IOHK wants to do a hard-fork on the 29th of July.

MichalPetro · 2020-07-13T12:58:32Z

@matejcik @tsusanka guys are you going to review this? 2 more PRs will be coming in the following week as we divided the functionalities in 3 PRs.

tsusanka · 2020-07-13T14:34:46Z

@MichalPetro we will look into this ASAP, tomorrow most likely.

Regarding the PR schedule: we would like to "freeze" (branch of code into a separate branch and forward firmwares to QA) the upcoming release on July 24th. That gives us 10 working days to include the necessary changes.

Although I believe this was mentioned somewhere please confirm that to support sending funds we need to include this PR and one more you are about to open. Please open this PR asap so we can review it and include it till July 24th. Apologies for the sudden rush, I haven't realized the hard-fork date is so close.

As to the third PR (which will introduce staking support, correct?) we need to discuss this internally in the Product team whether we want such change in firmware. In another words, it depends on the number of needed changes. We usually support only a subset of functionalities because it is not sustainable to have advanced functionalities for each and every altcoin. I am basically trying to manage expectations here - while we definitely want to include PR 1 and 2 I can not guarantee the same for PR number 3. Feel free to file a draft PR again to decide this before you fine-tune it.

MichalPetro · 2020-07-14T11:50:40Z

@tsusanka we will add the 2nd PR today or tomorrow. Also, we probably need to do the small fix in the 1st (this) PR because some specs changed, we are currently assessing the impact.

The 3rd PR will be smaller than the others. We will be not introducing staking for stake pools, just staking delegation to the stake pools.

tsusanka · 2020-07-14T11:55:20Z

Great, thank you.

The 3rd PR will be smaller than the others. We will be not introducing staking for stake pools, just staking delegation to the stake pools.

Ok ok. But it is something we might include in the next release if things go south, correct? It is not that essential as the other two.

MichalPetro · 2020-07-14T13:01:24Z

Yes definitely the first 2 PRs are critical. Lets make sure those are included. Lets see what we can do about the 3rd.

gabrielKerekes · 2020-07-14T14:12:16Z

I've updated the code with:

correct mainnet protocol magic
correct transaction witnesses
correct way of including protocol magic in Byron on addresses on Testnet
some type hinting

Hopefully nothing else will change. I've kept the fixup commits (which is failing the checks), so it's easy to see what changed. I can rebase them or squash them or whatever if you wish.

matejcik

does your core code typecheck? if yes, please add src/apps/cardano to core/Makefile mypy target

core/src/apps/cardano/protocol_magics.py

matejcik · 2020-07-15T10:23:33Z

I've kept the fixup commits (which is failing the checks), so it's easy to see what changed. I can rebase them or squash them or whatever if you wish.

Thanks.
Our review flow is as follows:

once you request reviews, only add fixup commits (you are doing that already, so 👍)
respond to individual review comments that this is done, ideally with a link to commit
the reviewer marks comments resolved
when you get all approvals, autosquash the fixup commits and force-push

matejcik · 2020-07-15T10:24:33Z

please regenerate UI test fixtures, by running the test suite with --ui=record

common/protob/messages-cardano.proto

core/src/apps/cardano/address.py

core/src/apps/cardano/sign_tx.py

tsusanka · 2020-07-15T15:35:01Z

please regenerate UI test fixtures, by running the test suite with --ui=record

Or simply use pipenv run make -C core test_emu_ui_record, see https://docs.trezor.io/trezor-firmware/tests/ui-tests.html#updating-fixtures-recording.

gabrielKerekes · 2020-07-15T18:56:00Z

I've updated the code with the suggestions. I've also updated the fixtures json here.

gabrielKerekes · 2020-07-16T05:39:28Z

I had to update transaction signing due to Byron witnesses spec update. My commit. I am however unable to test this at the moment since they also have to release their version of cardano-node.

I suggest we get the code in order and I will test everything once more when it's ready. I expect this to be on monday.

core/src/apps/cardano/layout/__init__.py

core/src/apps/cardano/address.py

matejcik · 2020-07-16T12:48:12Z

ah, I think I said this wrong

once you request reviews, only add fixup commits (you are doing that already, so +1)

You can add new commits. The "only fixup" thing is intended as "no rebasing", so modifications to existing commits should be fixups. But if the changes don't cleanly match some existing commits, a new commit is a better variant.
(you can also fully revert commits via git revert, but then you need to make sure during autosquash that the commit/revert pairs are removed)

gabrielKerekes · 2020-07-16T12:51:20Z

Yes, that makes sense. I wasn't sure how to go about that so I kept on "fixing-up". I'll know better next time.

matejcik · 2020-07-16T12:51:47Z

yeah, sorry about that 🙄

matejcik

LGTM now.

what is the status of the second PR?
it should be based on top of this one, obviously. We will do something like merge this into a separate branch, and you'll be able to send a PR against that branch.

gabrielKerekes · 2020-07-16T13:49:52Z

The second PR needs to be rebased onto this one - I've partially done this already, but I'll probably have to do it again. Then I'll clean it up, test it (also against the testnet) and it should be ready for review - hopefully until tomorrow afternoon.

And yes, we can probably do something like that. Should I squash and force push the fixups?

matejcik · 2020-07-16T13:51:03Z

And yes, we can probably do something like that. Should I squash and force push the fixups?

not yet, please wait for approval from @tsusanka

tsusanka

LGTM, one nit-pick!

tsusanka · 2020-07-17T07:00:22Z

common/protob/messages-cardano.proto

 * @next Failure
 */
 message CardanoSignTx {
    repeated CardanoTxInputType inputs = 1;     // inputs to be used in transaction
    repeated CardanoTxOutputType outputs = 2;   // outputs to be used in transaction
-    optional uint32 transactions_count = 3;     // transactions count


Please leave this commented out, so we know we should not use 3 anymore.

tsusanka · 2020-07-17T08:09:59Z

Thanks, great job! Please squash the commits now and force push. Then change this PR to be against the cardano-shelley branch I've just created.

gabrielKerekes · 2020-07-17T08:35:06Z

Awesome! Should I squash all the comits into one, or just the fixups?

matejcik · 2020-07-17T08:36:26Z

up to you honestly. ISTM that via the fixup confusion the original commits don't make sense separately anyway, so feel free to squash everything together.
alternately you can rename some commits and keep some semblance of history

Update protobuf - Previous transactions don't need to be sent anymore, because fee is included in the transaction now. Thus transactions_count can be removed from CardanoSignTx message and the CardanoTxAck and CardanoTxRequest messages can be removed altogether. - CardanoTxInputType.type is unused so remove it Add NULL (None type) serialisation to CBOR - Transaction metada must either have a valid structure or CBOR NULL must be used (if metadata is empty) - it can't be simply left out. Add protocol_magics file - Just to have a nicer way of representing protocol magics Update transaction signing - Previous transactions no longer need to be requested - Output building is simplified, since fee doesn't need to be calculated - Remove transaction class since it is no longer needed (only functions remained) - Reorder functions so it reads top to bottom Add protocol magic to byron address on testnet - This has always been a part of the spec, but it hasn't been implemented before, because it wasn't really needed. Update trezorlib Update tests - Transaction messages are no longer required - Expected values are different since tx format changed - Common values in test cases have been extracted Remove unused file - Progress was used when receiving previous transactions Add CRC check to output address validation

gabrielKerekes · 2020-07-17T10:42:52Z

I've opened the second PR -> #1112

gabrielKerekes requested review from matejcik and tsusanka as code owners July 6, 2020 12:40

MichalPetro mentioned this pull request Jul 6, 2020

Add Cardano Shelley support #948

Closed

gabrielKerekes mentioned this pull request Jul 7, 2020

Cardano Shelley Update 1/3 trezor/connect#623

Closed

prusnak added this to the 2020-08 milestone Jul 13, 2020

tsusanka mentioned this pull request Jul 14, 2020

Reuse derived Shamir seed and split off test fixtures #1107

Closed

7 tasks

matejcik reviewed Jul 15, 2020

View reviewed changes

core/src/apps/cardano/protocol_magics.py Outdated Show resolved Hide resolved

matejcik requested changes Jul 15, 2020

View reviewed changes

matejcik reviewed Jul 16, 2020

View reviewed changes

core/src/apps/cardano/layout/__init__.py Outdated Show resolved Hide resolved

matejcik reviewed Jul 16, 2020

View reviewed changes

core/src/apps/cardano/address.py Outdated Show resolved Hide resolved

tsusanka linked an issue Jul 16, 2020 that may be closed by this pull request

Add Cardano Shelley support #948

Closed

matejcik approved these changes Jul 16, 2020

View reviewed changes

tsusanka approved these changes Jul 17, 2020

View reviewed changes

gabrielKerekes force-pushed the cardano-shelley-update-pr1 branch from ea4a4b3 to 12ecbb5 Compare July 17, 2020 09:30

gabrielKerekes changed the base branch from master to cardano-shelley July 17, 2020 09:30

matejcik merged commit 49a017a into trezor:cardano-shelley Jul 17, 2020

gabrielKerekes mentioned this pull request Jul 17, 2020

Cardano shelley update 2/3 #1112

Merged

gabrielKerekes deleted the cardano-shelley-update-pr1 branch July 2, 2021 11:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cardano shelley update 1/3 #1096

Cardano shelley update 1/3 #1096

gabrielKerekes commented Jul 6, 2020 •

edited

Loading

prusnak commented Jul 13, 2020

MichalPetro commented Jul 13, 2020

tsusanka commented Jul 13, 2020 •

edited

Loading

MichalPetro commented Jul 14, 2020

tsusanka commented Jul 14, 2020

MichalPetro commented Jul 14, 2020

gabrielKerekes commented Jul 14, 2020

matejcik left a comment

matejcik commented Jul 15, 2020

matejcik commented Jul 15, 2020

tsusanka commented Jul 15, 2020

gabrielKerekes commented Jul 15, 2020

gabrielKerekes commented Jul 16, 2020

matejcik commented Jul 16, 2020

gabrielKerekes commented Jul 16, 2020

matejcik commented Jul 16, 2020

matejcik left a comment

gabrielKerekes commented Jul 16, 2020

matejcik commented Jul 16, 2020

tsusanka left a comment

tsusanka Jul 17, 2020

tsusanka commented Jul 17, 2020 •

edited

Loading

gabrielKerekes commented Jul 17, 2020

matejcik commented Jul 17, 2020

gabrielKerekes commented Jul 17, 2020

Cardano shelley update 1/3 #1096

Cardano shelley update 1/3 #1096

Conversation

gabrielKerekes commented Jul 6, 2020 • edited Loading

prusnak commented Jul 13, 2020

MichalPetro commented Jul 13, 2020

tsusanka commented Jul 13, 2020 • edited Loading

MichalPetro commented Jul 14, 2020

tsusanka commented Jul 14, 2020

MichalPetro commented Jul 14, 2020

gabrielKerekes commented Jul 14, 2020

matejcik left a comment

Choose a reason for hiding this comment

matejcik commented Jul 15, 2020

matejcik commented Jul 15, 2020

tsusanka commented Jul 15, 2020

gabrielKerekes commented Jul 15, 2020

gabrielKerekes commented Jul 16, 2020

matejcik commented Jul 16, 2020

gabrielKerekes commented Jul 16, 2020

matejcik commented Jul 16, 2020

matejcik left a comment

Choose a reason for hiding this comment

gabrielKerekes commented Jul 16, 2020

matejcik commented Jul 16, 2020

tsusanka left a comment

Choose a reason for hiding this comment

tsusanka Jul 17, 2020

Choose a reason for hiding this comment

tsusanka commented Jul 17, 2020 • edited Loading

gabrielKerekes commented Jul 17, 2020

matejcik commented Jul 17, 2020

gabrielKerekes commented Jul 17, 2020

gabrielKerekes commented Jul 6, 2020 •

edited

Loading

tsusanka commented Jul 13, 2020 •

edited

Loading

tsusanka commented Jul 17, 2020 •

edited

Loading