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

Updated blocks numbers to use U64 #253

Merged
merged 5 commits into from
Sep 13, 2019
Merged

Updated blocks numbers to use U64 #253

merged 5 commits into from
Sep 13, 2019

Conversation

r3v2d0g
Copy link
Contributor

@r3v2d0g r3v2d0g commented Sep 8, 2019

Hey ✋!

The following methods/types/fields were using U256 to represent a block's number instead of, as everywhere else and decided here, U128:

  • web3::api::Eth::block_number
  • web3::api::Web3::wait_for_confirmations
  • web3::confirm::ConfirmationCheck::Check and all related structs/types
  • web3::types::TransactionReceipt.block_number

Also, maybe web3::types::BlockNumber should use U128 instead of u64?

@tomusdrw
Copy link
Owner

tomusdrw commented Sep 9, 2019

The PR looks good, but can we use U64 instead? The use of U256/U128 is a bit historical (we didn't use the ethereum-types crate in the past, so the types had to be manually implemented).

@r3v2d0g
Copy link
Contributor Author

r3v2d0g commented Sep 9, 2019

@tomusdrw

The PR looks good, but can we use U64 instead? The use of U256/U128 is a bit historical (we didn't use the ethereum-types crate in the past, so the types had to be manually implemented).

Sure 😃. Also, should we also make web3::types::BlockNumber's Number(u64) variant use U64 instead of u64(it would make more sense imho)?

@tomusdrw
Copy link
Owner

tomusdrw commented Sep 9, 2019

Sure smiley. Also, should we also make web3::types::BlockNumber's Number(u64) variant use U64 instead of u64(it would make more sense imho)?

Yes, let's do that.

@r3v2d0g r3v2d0g changed the title Updated blocks numbers to use U128 instead of U256 Updated blocks numbers to use U64 Sep 10, 2019
@r3v2d0g
Copy link
Contributor Author

r3v2d0g commented Sep 10, 2019

@tomusdrw

Yes, let's do that.

There's also types::Trace.block_number but making it use U64 would need us to implement Deserialize on types::Trace because Parity encodes it as a base10 number (instead of a base16 one).

@tomusdrw
Copy link
Owner

@tomusdrw

Yes, let's do that.

There's also types::Trace.block_number but making it use U64 would need us to implement Deserialize on types::Trace because Parity encodes it as a base10 number (instead of a base16 one).

I see. I think that one can be left as-is.

@tomusdrw tomusdrw merged commit c69bf93 into tomusdrw:master Sep 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants