Skip to content
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

zcash_keys: Add decode_extfvk_with_network #1523

Merged
merged 3 commits into from
Aug 30, 2024

Conversation

nuttycom
Copy link
Contributor

No description provided.

@nuttycom nuttycom force-pushed the keys/decode_extfvk_with_network branch from 9adc4ea to 101e0b2 Compare August 30, 2024 20:18
@nuttycom nuttycom force-pushed the keys/decode_extfvk_with_network branch from 101e0b2 to 897018a Compare August 30, 2024 20:22
Copy link

codecov bot commented Aug 30, 2024

Codecov Report

Attention: Patch coverage is 0% with 30 lines in your changes missing coverage. Please review.

Project coverage is 61.06%. Comparing base (59582a7) to head (a732932).
Report is 25 commits behind head on main.

Files with missing lines Patch % Lines
zcash_keys/src/encoding.rs 0.00% 22 Missing ⚠️
zcash_keys/src/keys.rs 0.00% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1523      +/-   ##
==========================================
- Coverage   61.16%   61.06%   -0.10%     
==========================================
  Files         141      141              
  Lines       16649    16675      +26     
==========================================
  Hits        10183    10183              
- Misses       6466     6492      +26     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nuttycom nuttycom force-pushed the keys/decode_extfvk_with_network branch 2 times, most recently from e4dbf11 to 7c05fcb Compare August 30, 2024 21:23
@nuttycom nuttycom force-pushed the keys/decode_extfvk_with_network branch from 7c05fcb to a732932 Compare August 30, 2024 21:30
/// Decodes an [`ExtendedFullViewingKey`] and the [`NetworkType`] that it is intended for use with
/// from a Bech32-encoded string.
#[cfg(feature = "sapling")]
pub fn decode_extfvk_with_network(
Copy link
Contributor

@daira daira Aug 30, 2024

Choose a reason for hiding this comment

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

I know the other functions in this file don't explicitly say in their names that they're for Sapling, but that still seems odd. Non-blocking; I don't think we can do anything about it right now, but we should consider moving them to a zcash_keys::sapling::encoding module if they are never going to be more general.

Comment on lines +276 to +279
let (decoded_hrp, data, variant) = bech32::decode(s)?;
if variant != Variant::Bech32 {
Err(Bech32DecodeError::IncorrectVariant(variant))
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using CheckedHrpstring::new::<Bech32> (although I'm not sure it really comes out any cleaner once error handling has been taken into account).

Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

utACK

@nuttycom nuttycom merged commit 50cdf73 into main Aug 30, 2024
26 of 28 checks passed
@nuttycom nuttycom deleted the keys/decode_extfvk_with_network branch August 30, 2024 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants