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

Cardano small updates #1606

Merged
merged 5 commits into from
Jun 8, 2021
Merged

Conversation

gabrielKerekes
Copy link
Contributor

This PR contains small refactorings and updates for the Cardano app.

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.

very nice
one point about typing annotations, otherwise LGTM

core/src/apps/cardano/address.py Outdated Show resolved Hide resolved
@matejcik
Copy link
Contributor

matejcik commented Jun 4, 2021

also please add a changelog per https://docs.trezor.io/trezor-firmware/misc/changelog.html

@matejcik
Copy link
Contributor

matejcik commented Jun 4, 2021

it seems some of your address test vectors are wrong now: https://gitlab.com/satoshilabs/trezor/trezor-firmware/-/jobs/1318118622

@gabrielKerekes
Copy link
Contributor Author

also please add a changelog per https://docs.trezor.io/trezor-firmware/misc/changelog.html

Changelogs added - 0010bbc. I am not sure if two entries are desirable or just one would be enough similar to Cardano: refactor and improve address validation and reintroduce maximum transaction output size. Or perhaps there should be one entry for each point in the PR description.

it seems some of your address test vectors are wrong now: https://gitlab.com/satoshilabs/trezor/trezor-firmware/-/jobs/1318118622

Indeed. Luckily nothing got broken, it really was just an invalid test vector. Fixed in 7c573c9.

@matejcik
Copy link
Contributor

matejcik commented Jun 8, 2021

I am not sure if two entries are desirable

these entries seem appropriate 👍

in the meantime we changed import paths so you got some conflicts, please squash the fixups and rebase on top of master

@matejcik
Copy link
Contributor

matejcik commented Jun 8, 2021

changed import paths

for messages, instead of from trezor.messages.Foo import Foo it's just from trezor.messages import Foo
for enums, instead of from trezor.messages import FooEnum it's from trezor.enums import FooEnum, and this FooEnum name is now valid for typechecking, so no more EnumTypeFooEnum

Output size is currently limited to 4000 bytes at protocol level. Given the maximum transaction size Trezor can handle (~9kB), we also want to enforce this size limit here so that when the limit is raised at protocol level again, Trezor would still not be able to produce larger outputs than it could reliably spend. Once Cardano-transaction signing is refactored to be completely streamed and maximum supported transaction size is thus raised, this limit can be lifted.
@gabrielKerekes gabrielKerekes force-pushed the cardano-small-updates branch from 0010bbc to 38844ee Compare June 8, 2021 12:54
@gabrielKerekes
Copy link
Contributor Author

please squash the fixups and rebase on top of master

Done.

@matejcik matejcik merged commit ae831ab into trezor:master Jun 8, 2021
@matejcik
Copy link
Contributor

matejcik commented Jun 8, 2021

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