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

monorepo: PrefixedHexString related type fixes #3357

Merged
merged 18 commits into from
Apr 22, 2024

Conversation

gabrocheleau
Copy link
Contributor

This PR addresses misc. type issues that made their way into the monorepo after #3348

Copy link

codecov bot commented Apr 16, 2024

Codecov Report

Attention: Patch coverage is 85.41667% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 81.48%. Comparing base (4be68d2) to head (6278715).
Report is 12 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block ?
blockchain ?
client 84.74% <84.78%> (-0.18%) ⬇️
common ?
devp2p ?
evm ?
genesis ?
statemanager ?
trie ?
tx ?
util ?
vm 61.34% <100.00%> (-0.22%) ⬇️
wallet ?

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

@gabrocheleau gabrocheleau marked this pull request as ready for review April 22, 2024 05:38
Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Have updated the PR, and tests are passing.

Nice, this looks super great! 🤩

A general note: I DO think that we will likely introduce some update problems for upstream libraries/users with this. This is somewhat breaking on the type level, both if input and output values of API calls are tightened.

I will nevertheless merge this in now, so much work and really worth the additional security guarantees coming with it. This is likely nevertheless at its limits of what we can/should do along non-breaking releases.

Two thoughts here:

  1. I wonder if we want to make this an "official" policy (so: write down somewhere prominently (likely: root monorepo docs) in the docs + announce in Twitter, that we will be doing minor release versions which might be breaking on the TypeScript level and if people want to be safe there they should use ~ version ranges. Would this make sense?

  2. I also wonder if we would want to downgrade here again and re-type at least prominent API calls (most obvious one: hexToBytes) from PrefixedHexString to string and add this instead to a task to Breaking/Deprecation Tracking Issue #3216. We now with this already have all the pre-conditions in place, so still 95% of the way done, and with this we would ease this a bit for our users.

I think I am somewhat in favor to do.

Nevertheless: great stuff! 👍 🙂

@holgerd77 holgerd77 merged commit f3cbb2e into master Apr 22, 2024
36 checks passed
@holgerd77 holgerd77 deleted the monorepo/type-fixes-0xprefixed-hex branch April 22, 2024 08:36
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.

2 participants