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

New Byte Type Aliases with Byte Length Indicators #3335

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

Conversation

holgerd77
Copy link
Member

This is on my plate for a very long time, PR adds simple byte aliases for both Uint8Array and PrefixedHexString with byte length indication.

As discussed already quite some time back ago, this implementation has none of the eventual backlashes as stricter solutions (like performance drawbacks on actual length checks everywhere in the code base) and should at the same time already bring a lot of the benefits, mostly being a lot better documented function signatures and length hints. VSC mouseover of the one exemplaric type in Block e.g. now look like this:

Bildschirmfoto 2024-03-25 um 17 10 36

For the PrefixedHex* thing I tested (and "stared" for quite some time 😂) at the different flavors of a possible naming scheme:

  • PrefixedHexString20
  • PrefixedHexStr20
  • PrefixedHex20

I came to the conclusion that the last version should be sufficiently expressive and is at the same time the easiest to grasp, with the need to only digest 3 word parts instead of 4 (which is a lot), and after 3-4 rounds of usage latest it should be very intrinsic that this is about a string (if not clear from the beginning). So this version would be my personal favorite.

I would give this open for a first look and feedback and then continue a bit with it.

We do not need to get to completion here and merge this in a selectively applied state. Then we can just replace types over time, this is otherwise too tedious to accomplish in one go.

Copy link

codecov bot commented Mar 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.87%. Comparing base (4be68d2) to head (3ddb5e0).
Report is 2 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 82.24% <ø> (ø)
blockchain 90.92% <ø> (ø)
client ?
common 94.02% <ø> (ø)
devp2p ?
evm ?
genesis ?
statemanager ?
trie 59.39% <ø> (-0.21%) ⬇️
tx ?
util 81.32% <ø> (+<0.01%) ⬆️
vm ?
wallet ?

Flags with carried forward coverage won't be shown. Click here to find out more.

@acolytec3
Copy link
Contributor

This is lovely. We should definitely do this across the monorepo.

@gabrocheleau
Copy link
Contributor

I like this!

One thing that came up while searching the monorepo, is that we have some exported Bytes32 and other similar types in the client, but in that case they served as aliases for string. It could be helpful to remove that ambiguity (in my view Bytes should be Uint8Array, while PrefixedHex should be strings). Link: https://github.com/ethereumjs/ethereumjs-monorepo/blob/new-byte-type-aliases/packages/client/src/rpc/modules/engine/types.ts#L40

Comment on lines +44 to +46
export type PrefixedHex8 = string
export type PrefixedHex20 = string
export type PrefixedHex32 = string
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be updated to PrefixedHexString rather than string?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do not see the advantage of such alias chaining TBH.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah you're right, but I was thinking in the context of that other PR (where PR was updated to 0x${string}).

@holgerd77
Copy link
Member Author

One thing that came up while searching the monorepo, is that we have some exported Bytes32 and other similar types in the client, but in that case they served as aliases for string. It could be helpful to remove that ambiguity (in my view Bytes should be Uint8Array, while PrefixedHex should be strings). Link: https://github.com/ethereumjs/ethereumjs-monorepo/blob/new-byte-type-aliases/packages/client/src/rpc/modules/engine/types.ts#L40

Yes, that makes definitely sense to align in client, would also agree with the Bytes == Uint8Array stuff.

The whole PR is currently (and maybe permanently block). Context is that the type aliases do not propagate through in the editor (Visual Studio Code), so hovering over the code then shows Uint8Array again instead of e.g. Bytes32 (see chat exchange with Jochem somewhere). This makes the whole thing too brittle and half-useless and we should see if we find something both more stable and robust and still non-breaking (might not be a solution here).

Jochem actually tried the same thing as here already some time ago (didn't know that before) and stumbled upon the very same thing (and therefore drew the same conclusion here).

We'll see. Would be really really nice to have something like this.

Maybe also worth to have a look at other low-level libraries for some inspiration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants