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

dev: change type of branch node subnodes #1066

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

enitrat
Copy link

@enitrat enitrat commented Dec 20, 2024

What was wrong?

The type for the BranchNode's subnodes was List[rlp.Extended]. While this is not inherently wrong, a more suitable type would be a tuple with 16 rlp.Extended elements, each representing a BranchNode of the trie, as there's no intent to mutate the subnodes after instantiation.

Related to Issue #

How was it fixed?

Changed to Annotated[Tuple[rlp.Extended, ...], 16]. Also open to just repeating 16 times the rlp.Extended argument, but using an annotation sounds more convenient rather than repeating. The annotation conveys at type hint that the tuple is of size 16.

Cute Animal Picture

Our beloved moodeng friend
Put a link to a cute animal picture inside the parenthesis-->

@enitrat enitrat force-pushed the dev/change-type-branch-nodes branch from bcef93c to 2a3b61a Compare December 30, 2024 09:25
@SamWilsn
Copy link
Collaborator

Does the annotation get enforced? If not, I'd prefer the tuple with 16 elements, probably as a proper type or type alias.

@enitrat
Copy link
Author

enitrat commented Dec 30, 2024

The Annotation is not enforced. It's only a "hint" for the reader that it expects 16 elements. We could create a proper type that

Perhaps something like:

BranchSubnodes = Tuple[rlp.Extended, rlp.Extended, rlp.Extended, rlp.Extended,
                      rlp.Extended, rlp.Extended, rlp.Extended, rlp.Extended,
                      rlp.Extended, rlp.Extended, rlp.Extended, rlp.Extended,
                      rlp.Extended, rlp.Extended, rlp.Extended, rlp.Extended]


@slotted_freezable
@dataclass
class BranchNode:
    """Branch node in the Merkle Trie"""

    subnodes: BranchSubnodes
    value: rlp.Extended

@SamWilsn
Copy link
Collaborator

Kinda ugly, but I don't see a shorter way of making the type annotation meaningful to mypy. Let's go with the sixteen rlp.Extended type alias 🤣

@enitrat
Copy link
Author

enitrat commented Dec 30, 2024

done :)

@enitrat
Copy link
Author

enitrat commented Jan 4, 2025

Hi @SamWilsn, any chance you can help me understand why CI is failing ?

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