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

Use ethereum-types library for uints and hashes #76

Merged
merged 6 commits into from
Feb 7, 2018
Merged

Conversation

tomusdrw
Copy link
Owner

@tomusdrw tomusdrw commented Nov 9, 2017

Closes #69
Closes #84

@tomusdrw tomusdrw requested a review from debris November 10, 2017 12:09
@debris
Copy link
Collaborator

debris commented Nov 21, 2017

isn't it better to go with ethereum-types instead?

@tomusdrw
Copy link
Owner Author

Yes, I'd prefer to go with uint and fixed-hash libraries directly, although they are not yet on crates io.

@hswick
Copy link
Contributor

hswick commented Nov 23, 2017

Hmmm would it be possible to FFI with a C/C++ library like this https://sourceforge.net/projects/ttmath/?

@debris
Copy link
Collaborator

debris commented Nov 24, 2017

@hswick to be honest I see no point in using tmath. rust-web3 library does little to none math operations and bringing C++ library as a dependency is just a bit of a hassle

@hswick
Copy link
Contributor

hswick commented Nov 24, 2017

@debris I agree the C++ lib doesn't belong in this repo, however at some point the lack of crates problem needs to be solved. So wrapping an existing C++ seems like the path of least resistance.

@fspmarshall
Copy link
Contributor

I'm hesitant about the idea of using bigint or its derivatives by default. Given how foundational the web3 crate is, I think it's goal should be to represent the jsonrpc APIs as transparently as possible (within reason). Many consumers are either going to explicitly require canonical byte representations (e.g. hashers), or be indifferent to receiving a math enabled type (e.g. comparison ops). The bigint crate is serializing and deserializing via intermediate arrays and vectors respectively anyhow. I think ensuring that conversion to math enabled types is simple and easy would be more in line with rust's 'only pay for what your use' philosophy.

Issue #16 proposes breaking the transports out into a separate crate. Perhaps the types module could become a reexport of a separate crate with minimal dependencies. This would allow the authors of crates like ethereum-types to implement web3 specific conversions without adding unnecessary dependencies, while also allowing web3's consumers to maximize those sweet sweet zero-cost abstractions :)

@debris
Copy link
Collaborator

debris commented Feb 1, 2018

I think ensuring that conversion to math enabled types is simple and easy would be more in line with rust's 'only pay for what your use' philosophy.

I agree, but this is a bit of a hassle for us to maintain. Not long ago, we had 4 or 5 different implementations of uint and hash being used in parity. Converting between them was just super annoying. We've decided to 'sacrifice' modularity to simplify development.

Issue #16 proposes breaking the transports out into a separate crate. Perhaps the types module could become a reexport of a separate crate with minimal dependencies. This would allow the authors of crates like ethereum-types to implement web3 specific conversions without adding unnecessary dependencies, while also allowing web3's consumers to maximize those sweet sweet zero-cost abstractions :)

ethereum-types and web3 authors are the same people ;) And these 'ethereum' and 'web3' conversions are the same thing :)

@fspmarshall
Copy link
Contributor

I agree, but this is a bit of a hassle for us to maintain. Not long ago, we had 4 or 5 different implementations of uint and hash being used in parity. Converting between them was just super annoying. We've decided to 'sacrifice' modularity to simplify development.

Good point. I can see how that would have serious diminishing returns.

ethereum-types and web3 authors are the same people ;) And these 'ethereum' and 'web3' conversions are the same thing :)

Haha, fair enough. In defense of my sanity, the comment about unnecessary conversions was based on tracing minimum steps from hex to canonical byte repr (from point of view of caller) in current master branches:

@tomusdrw
Copy link
Owner Author

tomusdrw commented Feb 2, 2018

My 2cents. The goal of web3 is to provide simple to use ethereum integration, so most ethereum-specific operations should be possible without using any 3rd party libraries, as such I think that stuff like:

  • Incrementing the nonce
  • Calculating the gas price (from estimated gas price)
  • Calculating the gas
    should be possible and requires U256 arithmetics

Otherwise people may choose any jsonrpc-client (probably better than web3) since there is not that much we add on top of it.

Also web3 is extensible, so it should be easy to create your own API that deserializes directly to [u8; 32] if you prefer. That should cover all cases where extreme performance is required.
I'm also open to simplify that even further (maybe exposing a method like: let x: [u8; 32] = web3.raw_request("eth_getBalance", Address::from(5)).wait().unwrap())

Feel free to also contribute to ethereum-types to get rid of unnecessary allocations :)

@tomusdrw
Copy link
Owner Author

tomusdrw commented Feb 2, 2018

@fspmarshall
Copy link
Contributor

@tomusdrw well played :)

@tomusdrw tomusdrw changed the title Use bigint library for types. Use ethereum-types library for uints and hashes Feb 6, 2018
Copy link
Collaborator

@debris debris left a comment

Choose a reason for hiding this comment

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

looks good. Please just bump ethabi to 5.1, cause version 4.0 is using an old ethereum-types crate (1.0)

Also, by bumping you'll be able to remove a lot of unnecessary conversions - from/into

@@ -147,13 +147,13 @@ impl Tokenizable for H256 {
impl Tokenizable for Address {
fn from_token(token: Token) -> Result<Self, Error> {
match token {
Token::Address(data) => Ok(types::H160(data)),
Token::Address(data) => Ok(data.into()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

in ethabi 5.1 data is already an Address, there is no need to call .into(), docs

Cargo.toml Outdated
@@ -11,17 +11,18 @@ authors = ["Tomasz Drwięga <tomasz@ethcore.io>"]

[dependencies]
arrayvec = "0.3"
error-chain = "0.11.0"
ethabi = "4.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

please bump ethabi to 5.1

@debris
Copy link
Collaborator

debris commented Feb 6, 2018

and let's merge it tomorrow! :)

@debris
Copy link
Collaborator

debris commented Feb 7, 2018

there are still some conflicts ;)

@debris debris merged commit 48dfad7 into master Feb 7, 2018
@debris debris deleted the td-bigint branch February 7, 2018 13:18
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.

4 participants