Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

ethkey and ethstore use hash structures from bigint #1851

Merged
merged 15 commits into from
Aug 15, 2016
Merged

ethkey and ethstore use hash structures from bigint #1851

merged 15 commits into from
Aug 15, 2016

Conversation

debris
Copy link
Collaborator

@debris debris commented Aug 5, 2016 •
edited
Loading

in next pr, I will remove duplicated signing logic (it's in ethkey and util/crypto)

@debris debris added the A0-pleasereview 🤓 Pull request needs code review. label Aug 5, 2016
@coveralls
Copy link

coveralls commented Aug 5, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.03%) to 86.733% when pulling 17a559f on core_cleanup into 725d320 on master.

@rphmeier
Copy link
Contributor

rphmeier commented Aug 5, 2016

needs a merge

NikVolf
NikVolf reviewed Aug 9, 2016
View reviewed changes
util/src/lib.rs
@@ -156,59 +156,5 @@ pub use log::*;
pub use kvdb::*;
pub use timer::*;

#[cfg(test)]
Copy link
Contributor

@NikVolf NikVolf Aug 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

how the code below related to the pr topic?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ahh... you are right. this test was just moved to different file `util/bigint/src/uint.rs', cause it fits there better

@coveralls
Copy link

coveralls commented Aug 10, 2016 •
edited
Loading

Coverage Status

Coverage decreased (-3.6%) to 82.707% when pulling f1e1dcd on core_cleanup into 3b6bc97 on master.

@tomusdrw tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 10, 2016
gavofyork
gavofyork reviewed Aug 10, 2016
View reviewed changes
json/src/hash.rs
@@ -19,7 +19,7 @@
use std::str::FromStr;
use serde::{Deserialize, Deserializer, Error};
use serde::de::Visitor;
use util::hash::{H64 as Hash64, Address as Hash160, H256 as Hash256, H2048 as Hash2048};
use util::hash::{H64 as Hash64, H160 as Hash160, H256 as Hash256, H2048 as Hash2048};
Copy link
Contributor

Choose a reason for hiding this comment

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

prefer to stick to Address over H160 as long as it's being used for an address.

@gavofyork gavofyork added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Aug 10, 2016
@gavofyork
Copy link
Contributor

gavofyork commented Aug 10, 2016 •
edited
Loading

unless we're actually using a 160-bit datum for something generic, then it should be named Address. same goes for H520 and Signature and H256 and Secret.

@debris
Copy link
Collaborator Author

debris commented Aug 12, 2016

unless we're actually using a 160-bit datum for something generic, then it should be named Address. same goes for H520 and Signature and H256 and Secret.

I agree. I just renamed usages of Address to H160 in bigint library and every place where it's not using address is not specific (eg. serialization).

@debris debris mentioned this pull request Aug 12, 2016
Merged
@coveralls
Copy link

coveralls commented Aug 12, 2016 •
edited
Loading

Coverage Status

Coverage decreased (-2.9%) to 83.034% when pulling c675fc9 on core_cleanup into f5a8c73 on master.

@debris debris added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Aug 15, 2016
@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 15, 2016
@gavofyork gavofyork merged commit c39761c into master Aug 15, 2016
@gavofyork gavofyork deleted the core_cleanup branch August 15, 2016 13:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@debris @coveralls @rphmeier @gavofyork @NikVolf @tomusdrw