-
Notifications
You must be signed in to change notification settings - Fork 772
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
…om/ethereumjs/ethereumjs-monorepo into monorepo/type-fixes-0xprefixed-hex
There was a problem hiding this 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:
-
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? -
I also wonder if we would want to downgrade here again and re-type at least prominent API calls (most obvious one:
hexToBytes
) fromPrefixedHexString
tostring
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! 👍 🙂
This PR addresses misc. type issues that made their way into the monorepo after #3348