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: revert and adjust some prefixedHexTypes #3382

Merged
merged 1 commit into from
Apr 28, 2024

Conversation

gabrocheleau
Copy link
Contributor

This PR reverts some updates that had been made to string in #3348 and #3357, where string types were updated to PrefixedHexString when that was relevant, but this can lead to breaking changes in certain contexts. I've reverted what seemed like the most obvious common usages (e.g. block or common types that can be used externally). Instead of actually reverting to string, I updated the types to PrefixedHexString | string so that we can easily eventually remove string fully. I've also added TODO's above each of these instances so we can quickly retieve these instances and do that eventually.

Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

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

LGTM

@acolytec3 acolytec3 merged commit e418c17 into master Apr 28, 2024
33 of 34 checks passed
@holgerd77 holgerd77 deleted the monorepo/revert-some-prefixed-hex-types branch April 29, 2024 15:46
@holgerd77
Copy link
Member

Cool 🎉, can you also add a note to #3216 ?

td?: BigIntLike
blockNumber?: BigIntLike | string
timestamp?: BigIntLike | string
td?: BigIntLike | string
Copy link
Member

Choose a reason for hiding this comment

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

Oh. This seems wrong I guess?

@holgerd77
Copy link
Member

I could instantly spot too missed instances, hexToBytes() and intToHex() which are still missed, so I think we need to do a closer search here what is still missing.

(or are these not breaking for some reasons?)

@gabrocheleau
Copy link
Contributor Author

I could instantly spot too missed instances, hexToBytes() and intToHex() which are still missed, so I think we need to do a closer search here what is still missing.

(or are these not breaking for some reasons?)

Circling back to this.

I don't think that methods that output PrefixedHexString are a problem (since PrefixedHexstring can be assigned to string, it shouldn't break anything). However taking PrefixedHexString as input is definitely breaking. I can revert hexToBytes. We'll have the best of both worlds in a way, as we have already updated all the monorepo. We can perhaps consider then re-switching to PrefixedHexString only in a future release.

@gabrocheleau
Copy link
Contributor Author

Addressed in #3427 @holgerd77

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