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

moved hash.rs to bigint library #1827

Merged
merged 20 commits into from Aug 9, 2016
Merged

moved hash.rs to bigint library #1827

merged 20 commits into from Aug 9, 2016

Conversation

debris
Copy link
Collaborator

@debris debris commented Aug 3, 2016

in next pr, I will make ethkey/ethstore use bigint library

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

coveralls commented Aug 3, 2016

Coverage Status

Coverage decreased (-0.2%) to 86.929% when pulling e8c451a on move_hash into 40a304b on master.

@@ -648,7 +648,7 @@ impl BlockChainClient for Client {
timestamp: view.timestamp(),
difficulty: view.difficulty(),
last_hashes: last_hashes,
gas_used: U256::zero(),
gas_used: Default::default(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

TBH I prefer more explicit approach in such cases (U256::default(), U256::from(0)), for me it just means less cognitive load.

Look at transaction_hash below especialy, It's quite difficult to understand what type is returned (it gets even more tricky when there is some more .maping on options/results/vectors done).

But it's just an opinion :) I'm good with the changes 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

yes i think U256::zero() is much more preferred even
we deal with numbers here, not with some objects/classes

@coveralls
Copy link

coveralls commented Aug 4, 2016

Coverage Status

Coverage decreased (-0.1%) to 86.488% when pulling 4051524 on move_hash into 7093651 on master.

fn shift_bloomed() {
use sha3::Hashable;
//use sha3::Hashable;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happened here? Ignored and commented out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I should have mentioned that in PR description. It's commented out and will be moved out of hash.rs. These tests use keccak function and I didn't want to bring sha3 as a dependency here.

@tomusdrw tomusdrw added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 4, 2016
@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 4, 2016
@debris
Copy link
Collaborator Author

debris commented Aug 4, 2016

blooms methods are moved to separate trait - Bloomable

tests are no longer commented

@coveralls
Copy link

coveralls commented Aug 4, 2016

Coverage Status

Coverage increased (+0.2%) to 86.607% when pulling a1867a7 on move_hash into ab079fd on master.

@coveralls
Copy link

coveralls commented Aug 5, 2016

Coverage Status

Coverage decreased (-0.1%) to 86.566% when pulling df29fcf on move_hash into b208331 on master.

@coveralls
Copy link

coveralls commented Aug 5, 2016

Coverage Status

Coverage decreased (-0.01%) to 86.67% when pulling df29fcf on move_hash into b208331 on master.

@coveralls
Copy link

coveralls commented Aug 5, 2016

Coverage Status

Coverage increased (+0.05%) to 86.75% when pulling 74c9ecb on move_hash into 725d320 on master.

@rphmeier rphmeier added A8-looksgood 🦄 Pull request is reviewed well. A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A0-pleasereview 🤓 Pull request needs code review. A8-looksgood 🦄 Pull request is reviewed well. labels Aug 5, 2016
@rphmeier
Copy link
Contributor

rphmeier commented Aug 5, 2016

needs a merge -- the signature_to_rsv stuff is a little less pretty than before. maybe the newtype pattern would help there.

@coveralls
Copy link

coveralls commented Aug 5, 2016

Coverage Status

Coverage decreased (-0.03%) to 86.333% when pulling 88c5f55 on move_hash into e72fc53 on master.

@@ -1150,7 +1150,7 @@ fn key_save_load() {
#[test]
fn host_client_url() {
let mut config = NetworkConfiguration::new();
let key = h256_from_hex("6f7b0d801bc7b5ce7bbd930b84fd0369b3eb25d09be58d64ba811091046f3aa2");
let key = "6f7b0d801bc7b5ce7bbd930b84fd0369b3eb25d09be58d64ba811091046f3aa2".into();
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this a simple .into() whereas the address stuff requires a .parse().unwrap()?

@gavofyork gavofyork added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. labels Aug 6, 2016
@gavofyork
Copy link
Contributor

looks good (particularly the .into() refactoring) except for the errant .parse().unwrap()s.

@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 8, 2016
@coveralls
Copy link

coveralls commented Aug 8, 2016

Coverage Status

Coverage decreased (-0.007%) to 86.349% when pulling 8269887 on move_hash into 4f32a9c on master.

@coveralls
Copy link

coveralls commented Aug 9, 2016

Coverage Status

Coverage decreased (-0.1%) to 86.159% when pulling 974d537 on move_hash into ab8b763 on master.

@keorn keorn added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 9, 2016
@debris debris merged commit 3b6bc97 into master Aug 9, 2016
@debris debris deleted the move_hash branch August 9, 2016 12:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants