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

Implement the NumericString type accross the monorepo #3659

Open
gabrocheleau opened this issue Sep 11, 2024 · 6 comments
Open

Implement the NumericString type accross the monorepo #3659

gabrocheleau opened this issue Sep 11, 2024 · 6 comments

Comments

@gabrocheleau
Copy link
Contributor

We have recently added a NumericString type ( ${number}). I have only implemented it in a single location, but there are most likely other opportunities to implement it accross the monorepo.

@MukulKolpe
Copy link

Hey @gabrocheleau, can I work on this issue?

@holgerd77
Copy link
Member

holgerd77 commented Sep 12, 2024

Hi Mukul,
I guess I can answer this, since we have discussed this issue yesterday in our team call and so I also know that Gabriel does not necessarily plan to work on this himself, so you can take on this if you want! 🙂

So, to give you a bit more context: the issue is about this NumericString type, newly introduced to our @ethereumjs/util library. The task is a bit similar to when we introduced the PrefixedHexString type (some lines below) a couple of months earlier, in #3348.

The issue here is labeled as "good first issue", but I am not totally sure on this TBH or so at least its not totally trivial to find all the places where this is applicable, so this needs some solid knowledge of the code base/libraries or some good detective capabilities. 😂

If you want to take on maybe start with a first PR with only a handful of changes, than we can review and give feedback and we can see if you are on a good track! 🙂

Also to note: these changes will be breaking (not backwards compatible) on a TypeScript level, this is ok though since we are currently preparing for breaking releases with the work we are doing on our master branch.

@gabrocheleau
Copy link
Contributor Author

Hey @gabrocheleau, can I work on this issue?

Hey @MukulKolpe ! Nice that you want to take this on :)

As Holger mentioned, we are in the process of refining types accross de monorepo, and we've done so notably with the PrefixedHexString type. We recently added the NumericString type (https://github.com/ethereumjs/ethereumjs-monorepo/blob/master/packages/util/src/types.ts#L26) and implemented it in a few places (most notably here: https://github.com/ethereumjs/ethereumjs-monorepo/blob/master/packages/block/src/from-beacon-payload.ts#L35) but we have not taken the time to look through the monorepo for other opportunities to implement it.

How I would recommend approaching this is going through the types.ts files of every package, and look for places where a string type might actually refer to a NumericString. You can deduct this from looking at how the type is used (for e.g. if it imports test data, you can look at the data). It might very well be that this is a relativley rare occurence, but we'll only know once we do that work! :)

Thanks again for tackling this!

@MukulKolpe
Copy link

Thank you, @gabrocheleau, @holgerd77, for sharing the context. I appreciate your insights and will let you know if I encounter any issues.

@holgerd77
Copy link
Member

Hi @MukulKolpe, are you still planning to take on this issue or should we distribute/take on within the team?

@MukulKolpe
Copy link

Hi @holgerd77, sorry for the delay. I'll have the PR ready shortly.

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

No branches or pull requests

3 participants