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.

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

refactor(core/cardano): decouple address parameters validation #60

Closed
wants to merge 4 commits into from
Closed

refactor(core/cardano): decouple address parameters validation #60

wants to merge 4 commits into from

Conversation

gabrielKerekes
Copy link
Collaborator

Closes #59

@gabrielKerekes gabrielKerekes self-assigned this Apr 14, 2021
janmazak
janmazak approved these changes Apr 15, 2021
View reviewed changes
core/src/apps/cardano/address.py Outdated Show resolved Hide resolved
core/src/apps/cardano/helpers/__init__.py Outdated Show resolved Hide resolved
@gabrielKerekes gabrielKerekes force-pushed the cardano-address-validation-refactor branch from 6789fb4 to f8aa420 Compare April 21, 2021 08:34
janmazak
janmazak approved these changes Apr 21, 2021
View reviewed changes
core/src/apps/cardano/certificates.py Outdated Show resolved Hide resolved
@gabrielKerekes gabrielKerekes force-pushed the cardano-catalyst-voting branch 2 times, most recently from a2e97e9 to 57081a4 Compare April 22, 2021 16:07
@gabrielKerekes gabrielKerekes changed the base branch from cardano-catalyst-voting to master April 26, 2021 10:48
@gabrielKerekes gabrielKerekes force-pushed the cardano-address-validation-refactor branch from b5c1ee5 to b0f1ca4 Compare April 26, 2021 10:49
@gabrielKerekes
Copy link
Collaborator Author

In the end I decided to add the check that staking_key_hash and staking_path aren't present at the same time. 8658d3d

I also added a commit for checking that outputs don't contain both address and address_parameters. We discussed this offline with @refi93 and came to the conlcusion that this check should be there. In order for this to be testable I also updated trezorlib to add both attributes if they are present. b0f1ca4

@gabrielKerekes
Copy link
Collaborator Author

@janmazak please take another look at this.

@gabrielKerekes
Copy link
Collaborator Author

I've also added the commit which reintroduces max tx output size to this PR ac66518. I think it should be better for Trezor guys to have a single PR which contains multiple small fixes than three tiny PRs.

janmazak
janmazak approved these changes Apr 27, 2021
View reviewed changes
Copy link
Collaborator

@janmazak janmazak left a comment

Choose a reason for hiding this comment

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

Only two minor things.

core/src/apps/cardano/address.py Outdated Show resolved Hide resolved
core/src/apps/cardano/sign_tx.py Show resolved Hide resolved
refi93
refi93 requested changes Apr 27, 2021
View reviewed changes
Copy link
Collaborator

@refi93 refi93 left a comment

Choose a reason for hiding this comment

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

just a few improvement ideas and clarifying questions

janmazak
janmazak approved these changes Apr 29, 2021
View reviewed changes
core/src/apps/cardano/address.py Outdated Show resolved Hide resolved
@gabrielKerekes
refactor(core/cardano): decouple address parameters validation
60be4c6
@gabrielKerekes
fix(core/cardano): forbid tx outputs with both address and address pa…
923425e 
…rameters
@gabrielKerekes
fix(core/cardano): reintroduce max tx output size
a006e8a 
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-address-validation-refactor branch from a09056c to a006e8a Compare May 5, 2021 12:50
@gabrielKerekes
Copy link
Collaborator Author

Merged via trezor#1606.

@gabrielKerekes gabrielKerekes deleted the cardano-address-validation-refactor branch July 2, 2021 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janmazak janmazak janmazak approved these changes

@refi93 refi93 refi93 approved these changes

Assignees

@gabrielKerekes gabrielKerekes

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Decouple address parameters validation from address derivation
3 participants
@gabrielKerekes @refi93 @janmazak