Skip to content
This repository was archived by the owner on Sep 19, 2019. It is now read-only.

Use primitive-types crate to unify Substrate primitives#61

Closed
sorpaas wants to merge 8 commits into
masterfrom
sp-primitive-types
Closed

Use primitive-types crate to unify Substrate primitives#61
sorpaas wants to merge 8 commits into
masterfrom
sp-primitive-types

Conversation

@sorpaas
Copy link
Copy Markdown
Contributor

@sorpaas sorpaas commented Dec 2, 2018

openethereum/parity-ethereum#9994

This uses the new primitive-types crate to allow common types in Parity Ethereum and Substrate to be shared.

  • See Unify primitive types for Parity Ethereum and Substrate parity-common#86 for the PR on primitive-types.
  • In ethereum-types, we define the conversion of uint types to hash. Because it assumes big endian, those are currently specific to Ethereum and if used out of context, it can lead to confusion. For example, parity-codec use little endian for U256 encoding. As a result, I created a new trait BigEndianHash that handles those conversions.
  • ethereum-types-serialize crate is no longer used. So it's removed.

@sorpaas sorpaas force-pushed the sp-primitive-types branch from 5fada08 to 4a24423 Compare December 2, 2018 16:16
fn from(value: &'a $hash) -> Self {
Self::from(value.as_ref() as &[u8])
fn into_uint(&self) -> $uint {
$uint::from(self.as_ref() as &[u8])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not really introduced by this PR but the casting it to &[u8] is redundant because fixed_hash impl AsRef<[u8]>

See, https://github.com/paritytech/parity-common/blob/master/fixed-hash/src/hash.rs#L101-#L106

Copy link
Copy Markdown
Contributor

@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.

The code looks good to me but the code-organization with uint/hash still remains in this repo is hard to understand.

So now we have if I got it right:

  • parity-common/fixed-hash
  • parity-common/uint
  • parity-common/primitives (both uint and hash)
  • this repo (both uint and hash)

Can we move (uint, hash) in this repo to parity-common/primitives?

@sorpaas
Copy link
Copy Markdown
Contributor Author

sorpaas commented Jan 14, 2019

@niklasad1

this repo (both uint and hash)

I think we don't have uint and hash in this repo any more? (Or would you mind to send me a pointer where we still have it?)

@niklasad1
Copy link
Copy Markdown
Contributor

@sorpaas I guess it matter of definition on what is but we still have two files named hash.rs & uint.rs which implement rlp macros and into/from conversions that's why I mean :P

@niklasad1
Copy link
Copy Markdown
Contributor

@sorpaas what to do about this PR?

The end goal should probably be to deprecate this repo after paritytech/parity-common#97 right?

@dvdplm
Copy link
Copy Markdown
Contributor

dvdplm commented May 11, 2019

@sorpaas This can be closed yes?

@sorpaas
Copy link
Copy Markdown
Contributor Author

sorpaas commented May 11, 2019

Yes this can be closed. I think we moved most of the code to parity-common so may be we want to archive this repo.

@sorpaas sorpaas closed this May 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants