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

refactor(core/ethereum): improve API of the rlp module #1704

Merged
merged 1 commit into from
Jul 19, 2021

Conversation

matejcik
Copy link
Contributor

  • improve length calculation so that we don't need to actually encode to get the length
  • instead of returning bytes, directly write into a provided Writer

the test vectors in test_trezor.crypto.rlp.py, and the Ethereum device tests, show that the new implementation produces results identical to the old one

@matejcik matejcik requested a review from mmilata July 12, 2021 12:19
@matejcik matejcik force-pushed the matejcik/rlp-refactor branch from fb126d5 to c126cc0 Compare July 12, 2021 13:49
if x < 0:
raise ValueError # only unsigned ints are supported
for exp in range(64):
if x < 0xFF ** exp:
Copy link
Member

@prusnak prusnak Jul 12, 2021

Choose a reason for hiding this comment

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

This seems weird. Maybe you meant:

Suggested change
if x < 0xFF ** exp:
if x < 0x100 ** exp:

Also this function returns 0 for x=0. Not sure if that is the intention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also this function returns 0 for x=0. Not sure if that is the intention.

This is intentional, yes

But good point about the equality test. I'll need to add an edge value to the testcases first as it's apparently not covered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@mmilata mmilata left a comment

Choose a reason for hiding this comment

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

One question, otherwise looks better and more efficient than the original version.

from typing import Union
from trezor.utils import Writer

RLPItem = Union["RLPItem", bytes, int]
Copy link
Member

Choose a reason for hiding this comment

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

What does the recursion do here?

Also, should there be bytearray since it's checked against in the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I meant to run this though mypy and forgot. Good catch, this is supposed to be something like Union[list["RLPItem"], bytes, int]

(also I suspect that mypy won't actually allow a recursive type)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@matejcik matejcik force-pushed the matejcik/rlp-refactor branch from 4a009c8 to 57e8f6c Compare July 16, 2021 12:24
@matejcik matejcik merged commit cf15dce into master Jul 19, 2021
@matejcik matejcik deleted the matejcik/rlp-refactor branch July 19, 2021 11:59
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