Unify primitive types for Parity Ethereum and Substrate by sorpaas · Pull Request #86 · paritytech/parity-common

sorpaas · 2018-12-02T15:55:03Z

This puts five primitive types into a crate primitive-types. They are U256, U512, H160, H256 and H512. This allows us to use parity-ethereum crates on substrate, and vice versa.

primitive-types now implements RLP serialization for the types defined. It's expected that later we remove ethereum feature from rlp crate.
Three new helper crate are added -- impl-serde, impl-codec and impl-rlp. This allows code reuse when additional types with similar serialization function need to be defined. It also saves some complications of declaring dependencies.

Robbepop · 2018-12-02T21:25:57Z

LGTM!

Couldn't even find a minor grumble.

dvdplm

Overall this looks good. I think the ergonomics would be improved a bit if we can move more code to uint/fixed-hash and leave only type construction in primitive-types.
I am not convinced of the impls/* name and I think we should consider moving those crates to a subfolder of primitive-types.

Sign in to view

+name = "impl-codec"
+version = "0.1.1"
+authors = ["Parity Technologies <admin@parity.io>"]
+license = "Apache-2.0"


Sign in to view

+name = "impl-rlp"
+version = "0.1.1"
+authors = ["Parity Technologies <admin@parity.io>"]
+license = "Apache-2.0"


Sign in to view

+name = "impl-serde"
+version = "0.1.1"
+authors = ["Parity Technologies <admin@parity.io>"]
+license = "Apache-2.0"


Sign in to view

+
+[dependencies]
+serde = "1.0"
+rustc-hex = "2.0.1"


dvdplm · 2018-12-03T09:35:35Z

+	}
+}
+
+impl From<H160> for H256 {


This should use impl_fixed_hash_conversions!(H256, H160) I think.

We don't have that macro now and I don't think it's a good idea -- converting H160 to H256 is a special operation (that has agreement on Substrate and Parity Ethereum). We cannot apply it directly to any other types (otherwise, we can't answer issues like why is it zero-padded on the front? why not on the end or repeat the bytes?).

I meant this; it does a static assert on the type size and zero-pads in front. I agree this is arbitrary and not something we invented recently, but it is there and it's been used for a long time so I'd argue it's the "standard by convention" in our codebase. As long as we document the behaviour I can live with it.

The reason I'm being difficult here and elsewhere is that I'm taking the POV of a dev that is unfamiliar with the code structure and approaches the code base for the first time: I'm afraid they will grow frustrated with how code is spread out in different crates with a hard-to-grasp logic as to what stuff is found where. If at all possible I'd like logic to be in uint/fixed-hash.

Ah right. Fixed!

dvdplm · 2018-12-03T09:38:56Z

+#[cfg(feature = "impl-rlp")] impl_fixed_hash_rlp!(H160, 20);
+
+#[cfg(feature = "std")]
+impl From<u64> for H160 {


@Robbepop maybe we should have this in uint?

This is problematic since in uint you can also define uint that are smaller than u64 and thus making this conversion failing. However, From impls are guaranteed to never fail. So we would need to make this conversion lossy in order to guarantee that it never fails but I am not sure if this is explicitly wanted behaviour since this may introduce unwanted semantics.

This from impl is fairly short so I think it's okay to just be there.

Hmm I'm thinking -- maybe we should remove those From<u64> conversions? It looks like it's both used in parity-ethereum and substrate, but the issue with it is that it assumes big-endian encoding. To fix those in both codebase is trivial -- we just replace any From<u64> by the explicit function from_low_u64_be.

replace any From<u64> by the explicit function from_low_u64_b

I like this solution. Let's do that. :)

Sign in to view

+	/// Fixed-size uninterpreted hash type with 32 bytes (256 bits) size.
+	pub struct H256(32);
+}
+#[cfg(feature = "impl-serde")] impl_fixed_hash_serde!(H256, 32);


Robbepop · 2018-12-03T10:32:53Z

Sign in to view

+#[cfg(feature = "impl-codec")] impl_fixed_hash_codec!(H256, 32);
+#[cfg(feature = "impl-rlp")] impl_fixed_hash_rlp!(H256, 32);
+
+#[cfg(feature = "std")]


I think this is because of Self::from_low_u64_be which itself requires fixed-hash/byteorder support. As far as I can see this crate tries to simplify crate features so that there is only something like std left which is enabled by default. I can understand this decision, however, one should keep in mind that this makes it impossible to use this API without std.

Fixed. It shouldn't be std.

And see #86 (comment). I think we should remove From<u64> defs.

Sign in to view

+	/// Fixed-size uninterpreted hash type with 64 bytes (512 bits) size.
+	pub struct H512(64);
+}
+#[cfg(feature = "impl-serde")] impl_fixed_hash_serde!(H512, 64);


…into sp-primitive-types-init

sorpaas · 2018-12-21T15:25:12Z

@dvdplm Would you mind to give a final review?

sorpaas added 12 commits December 2, 2018 16:36

Implement all uint features

97c034d

Improve uint defs and finish fixed hash impls

b242323

Add license header to everything

84b1856

Fix new crate license field

41aa107

Add docs

96ee827

Move basic derive to macro

84f56d8

Revert uint convenience improvements so that dep PR works

2eb6b4e

Fix license of impls

5243639

Fix license of primitive-types

e61e1c2

Add U512 in primitive-types

d93f59b

Release primitive-types 0.1.1 so things are not blocked here

765aeb3

Remove unused std feature

52c6847

sorpaas added the A0-pleasereview label Dec 2, 2018

This was referenced Dec 2, 2018

Use primitive-types crate to unify Substrate primitives paritytech/primitives#61

Closed

Use primitive-types crate to unify Parity Ethereum primitives paritytech/substrate#1187

Merged

sorpaas added 2 commits December 3, 2018 00:09

Fix nostd compile for primitive-types

7803bc4

Disable std on impl-serde/codec/rlp crate

7989816

sorpaas force-pushed the sp-primitive-types-init branch from f9c9a23 to 7989816 Compare December 2, 2018 16:20

Missed nostd attr in impl-codec

37ea789

Robbepop self-requested a review December 2, 2018 21:26

Robbepop approved these changes Dec 2, 2018

View reviewed changes

sorpaas added 2 commits December 3, 2018 10:50

Remove rlp default features for impl-rlp

ee3f55d

Add all feature gates from fixed-hash and uint

59d9672

dvdplm suggested changes Dec 3, 2018

View reviewed changes

bkchr reviewed Dec 3, 2018

View reviewed changes

Comment thread impls/codec/src/lib.rs Outdated

sorpaas added 4 commits December 4, 2018 11:33

Update license

c264679

Move impls to primitive-types subfolder

7714a2a

typo: 2.0.1 -> 2

5bbb03f

Expand primitive types docs

9f22691

sorpaas added 10 commits December 4, 2018 11:42

Force std feature in serde and rlp

cfc2a5d

Fix style nit

8a06e41

Fix unnecessary std dep

8d6d787

Remove From<u64> from primitive types

5adf9f1

Use impl_fixed_hash_conversions

dfc65a1

Merge branch 'master' of https://github.com/paritytech/parity-common …

e72a4c8

…into sp-primitive-types-init

Use new construct int style

88efc9b

Remove crunchy and update uint version

42f557d

Bump primitive-types patch version

d632b4c

dvdplm approved these changes Jan 7, 2019

View reviewed changes

sorpaas merged commit 6e04748 into master Jan 7, 2019

sorpaas deleted the sp-primitive-types-init branch January 7, 2019 20:49

ordian mentioned this pull request Mar 27, 2019

Fix impl-rlp for uint in primitive-types #117

Merged

Conversation

sorpaas commented Dec 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Robbepop commented Dec 2, 2018

Uh oh!

dvdplm left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

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!

sorpaas Dec 4, 2018 • 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!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

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!

This comment was marked as resolved.

Uh oh!

Uh oh!

sorpaas commented Dec 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sorpaas commented Dec 2, 2018 •

edited

Loading

sorpaas Dec 4, 2018 •

edited

Loading