Replace H256 with the one in rust-numext #15
Conversation
src/h256.rs
Outdated
| /// Represent 256 bits | ||
| #[derive(Eq, PartialEq, Debug, Default, Hash, Clone, Copy)] | ||
| pub struct H256([u8; 32]); | ||
| pub type H256 = numext_fixed_hash::H256; |
There was a problem hiding this comment.
This shall be a private type declaration, or at least crate-level public. We don't want to confuse the users of this library with yet another H256 definition.
src/tests/tree.rs
Outdated
|
|
||
| type SMT = SparseMerkleTree<Blake2bHasher, H256, DefaultStore<H256>>; | ||
|
|
||
| #[derive(Eq, PartialEq, Debug, Default, Hash, Clone)] | ||
| pub struct H256OrdTest { |
There was a problem hiding this comment.
Can we reuse H256Ord structure instead of defining a new one?
src/tree.rs
Outdated
| @@ -51,6 +44,23 @@ pub struct SparseMerkleTree<H, V, S> { | |||
| phantom: PhantomData<(H, V)>, | |||
| } | |||
|
|
|||
| #[derive(Eq, PartialEq, Debug, Default, Hash, Clone)] | |||
| struct H256OrdTree { | |||
There was a problem hiding this comment.
Same thing as the above, can we define only one H256Ord, possibly in h256.rs?
This comment has been minimized.
This comment has been minimized.
…merge H256Ord to avoid code duplication.
…fusion; merge H256Ord to avoid code duplication." This reverts commit dff1e56.
src/merkle_proof.rs
Outdated
| pub fn compute_root<H: Hasher + Default>(&self, leaves: Vec<(H256, H256)>) -> Result<H256> { | ||
| let mut leaves_ord: Vec<(H256Ord, H256)> = leaves | ||
| .iter() | ||
| .take(leaves.len()) |
There was a problem hiding this comment.
Can we just do an into_iter here?
| @@ -18,6 +18,7 @@ blake2b = ["blake2b-rs"] | |||
| [dependencies] | |||
| cfg-if = "0.1" | |||
| blake2b-rs = { version = "0.1", optional = true } | |||
| numext-fixed-hash = "0.1.6" | |||
There was a problem hiding this comment.
numext-fixed-hash breaks the no-std feature. We should introduce it as optional, if user choose feature=std then enable the numext_fixed_hash::H256, otherwise we use the H256([u8; 32]).
There was a problem hiding this comment.
I think we had agreed earlier, that no_std feature here does not make any sense, it's best we remove it and we should only leverage C based verifier in smart contract? Has anything changed since then?
There was a problem hiding this comment.
If we decided to remove no_std support, we should actually remove the std feature from features section.
src/error.rs
Outdated
| @@ -1,4 +1,5 @@ | |||
| use crate::{string, H256}; | |||
| extern crate std; | |||
There was a problem hiding this comment.
I don't think we need to mark std as external crate
|
Line 62 in 08c7122 no_std
|
lib.rs about no_std and error.rs about extern std
I replaced the native H256 with the one in Rust-numext。