-
Notifications
You must be signed in to change notification settings - Fork 94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[r2r] Add checksum verification of Zcash params files #1549
Conversation
…ion_of_Zcash_params
…ion_of_Zcash_params # Conflicts: # mm2src/coins/z_coin/z_coin_errors.rs
…ion_of_Zcash_params
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! It's the first review iteration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@ozkanonur Could you take a look too, please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work, just notes :)
mm2src/coins/z_coin.rs
Outdated
/// open file and calculate its sha256 digest as lowercase hex string | ||
fn sha256_digest(path: &PathBuf) -> Result<String, ZCoinBuildError> { | ||
let input = File::open(path)?; | ||
let mut reader = BufReader::new(input); | ||
|
||
let digest = { | ||
let mut hasher = Sha256::new(); | ||
let mut buffer = [0; 1024]; | ||
loop { | ||
let count = reader.read(&mut buffer)?; | ||
if count == 0 { | ||
break; | ||
} | ||
hasher.update(&buffer[..count]); | ||
} | ||
format!("{:x}", hasher.finalize()) | ||
}; | ||
Ok(digest) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks like a general purpose function that could be placed in common module for reusability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
mm2src/coins/z_coin.rs
Outdated
let (spend_path, output_path) = if params_dir.exists() { | ||
( | ||
params_dir.join(SAPLING_SPEND_NAME), | ||
params_dir.join(SAPLING_OUTPUT_NAME), | ||
) | ||
} else { | ||
return Err(ZCoinBuildError::ZCashParamsNotFound); | ||
}; | ||
if !(spend_path.exists() && output_path.exists()) { | ||
return Err(ZCoinBuildError::ZCashParamsNotFound); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, the following code looks more readable because it includes less blocks.
if !params_dir.exists() {
return Err(ZCoinBuildError::ZCashParamsNotFound);
}
let spend_path = params_dir.join(SAPLING_SPEND_NAME);
let output_path = params_dir.join(SAPLING_OUTPUT_NAME);
if !(spend_path.exists() && output_path.exists()) {
return Err(ZCoinBuildError::ZCashParamsNotFound);
}
Just sharing my thought, maybe it's just me :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, it looks better.
Done
…ion_of_Zcash_params
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2nd review iteraton
mm2src/common/common.rs
Outdated
@@ -117,6 +117,8 @@ pub mod wio; | |||
|
|||
#[cfg(target_arch = "wasm32")] pub use wasm::*; | |||
|
|||
#[cfg(target_arch = "wasm32")] use std::path::PathBuf; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be imported without any condition. Atm it's imported twice for native and wasm mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just had this error and recomendation to import std::path::PathBuf
. Sorry, what do you mean by condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#[cfg(target_arch = "wasm32")]
this is compile-time check which provides conditional compilation.
We have PathBuf
imported as
#[cfg(target_arch = "wasm32")] use std::path::PathBuf;
and
cfg_native!{ // checks if target isn't wasm
use std::path::PathBuf;
}
Because we are using it for both targets, we can import it as it is without any compile-time condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, got it, thanks! It is imported without condition now.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me :)
…ion_of_Zcash_params
related to ARRR integration. #927