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

Remove U64 and U128 #473

Closed
wants to merge 1 commit into from
Closed

Remove U64 and U128 #473

wants to merge 1 commit into from

Conversation

vorot93
Copy link
Contributor

@vorot93 vorot93 commented Dec 7, 2020

Obsoleted by u64 and u128 in Rust standard library.

@ordian
Copy link
Member

ordian commented Dec 7, 2020

My assumption is that these types are used to enforce some serialization format for RPC requests. Maybe this is easy to work around, but maybe there are also some use-cases I'm missing.

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

My assumption is that these types are used to enforce some serialization format for RPC requests. Maybe this is easy to work around, but maybe there are also some use-cases I'm missing.

Yes, these types exist because integer/unformatted data deserialization in ethereum is different because they are prefixed by 0x and won't work with the built-in types (u64/u128) in the Rust standard library without "glue code" (parsing hex strings doesn't work without removing the hex prefix in Rust, special handling of leading zeros and so on)

So, I see no point in removing them otherwise some other library or helpers has to do the same job. I guess it could make sense to use u64 and u128 internally if there is a performance benefit for it.

@sorpaas
Copy link
Member

sorpaas commented Dec 9, 2020

It's right now in Frontier for some borrowed code from OE, but I'm in favor of removing those -- should be easy to change them to the raw type.

@sorpaas
Copy link
Member

sorpaas commented Dec 9, 2020

My assumption is that these types are used to enforce some serialization format for RPC requests. Maybe this is easy to work around, but maybe there are also some use-cases I'm missing.

Yeah but I don't like that design to be honest. It's much better to define a type HexDisplay and use that for RPC return values.

@ordian
Copy link
Member

ordian commented Dec 9, 2020

My assumption is that these types are used to enforce some serialization format for RPC requests. Maybe this is easy to work around, but maybe there are also some use-cases I'm missing.

Yeah but I don't like that design to be honest. It's much better to define a type HexDisplay and use that for RPC return values.

Yeah, I don't mind removing them, but we should provide HexDisplay as a migration path. Not sure if it would fit primitive-types or ethereum-types though.

@ordian
Copy link
Member

ordian commented Dec 25, 2020

Planning to publish a few crates including uint 0.9 after #486 is merged.
As I mentioned I don't mind merging this PR as long as we provide a migration path (HexDisplay or whatever).

@vorot93
Copy link
Contributor Author

vorot93 commented Dec 30, 2020

@ordian My focus has switched from web3 JSONRPC to binary typed RPC protocols (like gRPC), so I have much less need and bandwidth to push this PR over the finish line. I suggest making a new release without it.

@vorot93 vorot93 closed this Feb 4, 2022
@vorot93 vorot93 deleted the remove-u64-u128 branch February 4, 2022 09:50
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.

None yet

4 participants