Remove dependency on `primitive_types` for Ethereum types such as Address and TransactionHash by da-kami · Pull Request #2405 · comit-network/comit-rs

da-kami · 2020-04-06T08:33:06Z

H256 (Hash) and H160 (Address) are converted to our own types.
Note that for deserialization we always have to pass the Hash/Address by reference, otherwise we don't reach the right code-path in serde hex. @thomaseizinger already tackled that problem.

Unfortunately, the U256 value in Ethereum is not fixed hex (although it actually is of fixed size, namely [u8, 32]).
This leads to the problem, that we would have to implement our own Serializer and Deserializer because serde-hex does not implement Compact for [u8, 32].

Implementing a custom Serializer and Deserializer is, however, not trivial, because our hex library does not support compacted hex neither.

I started pulling in the serialization and deserialization code from the parity codebase, but it is quite a lot of code to be pulled in, that is rather complicated.
Given, that this ticket was under the premise that we (most likely...) can just use serde-hex or our hex library for encoding/decoding the value, this is getting a rabbit hole. Since I already wasted quite some time on this ticket due to a tricky serde-hex bug I would put the implementation of U256 aside now and keep the dependency on primitive_types for it.

We can decide how/if we want to deal with U256 later. I am not sure we want to own the custom serialization/deserialization code, or if it is not better to use primitive_types::U256.

mergify · 2020-04-06T08:33:41Z

Are you sure the changelog does not need updating?

mergify · 2020-04-06T08:33:41Z

Are you sure the changelog does not need updating?

mergify · 2020-04-06T08:33:41Z

Are you sure the changelog does not need updating?

mergify · 2020-04-06T08:33:41Z

Are you sure the changelog does not need updating?

thomaseizinger

Well done Daniel :)

Good effort for pushing this one through!
I've left some comments about follow-up work that is now finally possible as we are taking control of those types :)

Also, please squash your commits together into one :)

thomaseizinger · 2020-04-06T23:32:22Z

cnd/src/ethereum.rs

+    }
+}
+
+impl From<Address> for Hash {


That looks weird, why do we need to convert between an Address and a Hash?

If we currently do this in production code and removing that is non-trivial, can you please record a follow-up issue?

Recorded as follow up: #2411

The problem is that the Topic used to search in Events just accepts a Hash specifically, but we also use it for searching for Addresses - we should change this to be generic.

Okay interesting, I would put this functionality on the specific topic then and not as a generall conversion between addresses and hashes.

thomaseizinger · 2020-04-06T23:33:11Z

cnd/src/ethereum.rs

+impl Address {
+    pub fn from_slice(src: &[u8]) -> Self {
+        let mut address = Address([0u8; 20]);
+        address.0.copy_from_slice(src);


I think this can panic if the slice is not long enough, i.e. < 20 bytes. Can you please create a follow-up ticket for auditing the usages of this function with the goal of removing it?

follow up: #2412

thomaseizinger · 2020-04-06T23:34:22Z

cnd/src/ethereum.rs

+impl Hash {
+    pub fn from_slice(src: &[u8]) -> Self {
+        let mut h256 = Hash([0u8; 32]);
+        h256.0.copy_from_slice(src);


Same as above, this can panic and we should avoid using this function.

follow up: #2412

cnd/src/ethereum.rs

cnd/src/http_api/problem.rs

thomaseizinger · 2020-04-06T23:36:30Z

cnd/src/network.rs

                match decision {
                    Some(Decision::Accepted) => {
-                        match serde_json::from_value::<rfc003::messages::AcceptResponseBody<AI, BI>>(
-                            response.body().clone(),


Nice, we are avoiding to calls to clone :)

thomaseizinger · 2020-04-06T23:41:55Z

cnd/src/ethereum.rs

+    }
+}
+
+impl Display for Hash {


Eventually, I would like this Display implementation to not print ... but do what the implementation of LowerHex does.
We can then remove the implementation of LowerHex.

This will likely require a few changes to log statements in our code where we currently use {:x} to print the full transaction hash.

Can be done as a follow-up ticket :)

recorded: #2413

thomaseizinger · 2020-04-06T23:44:13Z

cnd/src/ethereum.rs

+    }
+}
+
+impl fmt::LowerHex for Address {


Instead of LowerHex, I would like us to use Display for printing the full address. That is much more natural in log statements.

recorded: #2413

thomaseizinger · 2020-04-06T23:45:19Z

cnd/src/ethereum.rs

 mod tests {
    use super::Log;
-    use crate::ethereum::TransactionReceipt;
+    use crate::ethereum::{Address, Hash, TransactionReceipt};


Nit: If you do use super::* in tests modules, you automatically get all types from the parent module which are very likely the ones you want to test :)

thomaseizinger · 2020-04-06T23:47:40Z

cnd/src/ethereum.rs

 pub struct Block {
    /// Hash of the block
-    pub hash: Option<H256>,
+    pub hash: Option<Hash>,


Can you record a follow-up issue for making this not an Option? It is so annoying 😬

recorded: #2414

luckysori

LGTM.

cnd/src/btsieve/ethereum.rs

cnd/src/ethereum.rs

da-kami · 2020-04-07T04:47:10Z

bors r+

mergify · 2020-04-07T05:06:45Z

bors r+

bors · 2020-04-07T05:06:46Z

Already running a review

bors · 2020-04-07T05:14:53Z

Build succeeded

ci/circleci: debug-build-test

This introduces the ethereum::Address type into our config::File struct. Unfortunately, this opens a can of worms with a bug in the serde-hex library. See [0] for more details. We work around this by removing the dependency on serde-hex from our Ethereum deserialization code and instead hand-roll everything for our usecase. This also has the advantage that we can now again just use our structs directly in the route handlers of warp instead of serde_json::Value. See [1] for more on this. [0]: fspmarshall/serde-hex#8 [1]: #2405

3071: Infer DAI token from Ethereum chain ID r=mergify[bot] a=thomaseizinger We are going to use this to avoid including the token contract address in the orders that we are sending around. This introduces the ethereum::Address type into our config::File struct. Unfortunately, this opens a can of worms with a bug in the serde-hex library. See [0] for more details. We work around this by removing the dependency on serde-hex from our Ethereum deserialization code and instead hand-roll everything for our usecase. This also has the advantage that we can now again just use our structs directly in the route handlers of warp instead of serde_json::Value. See [1] for more on this. [0]: fspmarshall/serde-hex#8 [1]: #2405 Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

da-kami requested review from luckysori, tcharding and thomaseizinger April 6, 2020 08:33

github-actions bot assigned da-kami Apr 6, 2020

da-kami added the no-mergify Stop mergify to merge this automatically label Apr 6, 2020

thomaseizinger approved these changes Apr 6, 2020

View reviewed changes

thomaseizinger changed the title ~~2157 remove primitive ethereum types~~ (Almost) remove dependency on primitive_types for Ethereum types such as Address and TransactionHash Apr 6, 2020

thomaseizinger changed the title ~~(Almost) remove dependency on primitive_types for Ethereum types such as Address and TransactionHash~~ Remove dependency on primitive_types for Ethereum types such as Address and TransactionHash Apr 6, 2020

luckysori approved these changes Apr 7, 2020

View reviewed changes

cnd/src/btsieve/ethereum.rs Outdated Show resolved Hide resolved

tcharding approved these changes Apr 7, 2020

View reviewed changes

cnd/src/ethereum.rs Outdated Show resolved Hide resolved

cnd/src/ethereum.rs Outdated Show resolved Hide resolved

da-kami force-pushed the 2157-remove-primitive-ethereum-types branch 2 times, most recently from f9126c7 to 96b9b39 Compare April 7, 2020 04:25

da-kami added no-mergify Stop mergify to merge this automatically and removed no-mergify Stop mergify to merge this automatically labels Apr 7, 2020

Replace H256 and H160 with own Hash and Address

3177357

da-kami force-pushed the 2157-remove-primitive-ethereum-types branch from 96b9b39 to 3177357 Compare April 7, 2020 04:35

da-kami removed the no-mergify Stop mergify to merge this automatically label Apr 7, 2020

bors bot merged commit 2c265c2 into dev Apr 7, 2020

mergify bot deleted the 2157-remove-primitive-ethereum-types branch April 7, 2020 05:15

thomaseizinger mentioned this pull request Aug 20, 2020

Infer DAI token from Ethereum chain ID #3071

Merged

Conversation

da-kami commented Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergify bot commented Apr 6, 2020

Uh oh!

mergify bot commented Apr 6, 2020

Uh oh!

mergify bot commented Apr 6, 2020

Uh oh!

mergify bot commented Apr 6, 2020

Uh oh!

thomaseizinger left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luckysori left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

da-kami commented Apr 7, 2020

Uh oh!

mergify bot commented Apr 7, 2020

Uh oh!

bors bot commented Apr 7, 2020

Uh oh!

bors bot commented Apr 7, 2020

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

da-kami commented Apr 6, 2020 •

edited

Loading

thomaseizinger left a comment •

edited

Loading