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_address: Add support for ZIP 316, Revision 1 #1135

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

nuttycom
Copy link
Contributor

@nuttycom nuttycom commented Jan 25, 2024

Closes #1153.

@nuttycom nuttycom marked this pull request as draft January 25, 2024 23:50
Copy link

codecov bot commented Jan 26, 2024

Codecov Report

Attention: Patch coverage is 56.56716% with 291 lines in your changes missing coverage. Please review.

Project coverage is 53.10%. Comparing base (3a678bb) to head (38cb946).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
zcash_keys/src/keys.rs 64.86% 91 Missing ⚠️
devtools/src/bin/inspect/keys/view.rs 0.00% 60 Missing ⚠️
components/zcash_address/src/kind/unified.rs 55.46% 53 Missing ⚠️
zcash_keys/src/address.rs 53.60% 45 Missing ⚠️
...mponents/zcash_address/src/kind/unified/address.rs 85.29% 10 Missing ⚠️
...ent_backend/src/data_api/wallet/input_selection.rs 14.28% 6 Missing ⚠️
..._sqlite/src/wallet/init/migrations/ufvk_support.rs 0.00% 5 Missing ⚠️
components/zcash_address/src/kind/unified/fvk.rs 77.77% 4 Missing ⚠️
components/zcash_address/src/kind/unified/ivk.rs 77.77% 4 Missing ⚠️
devtools/src/bin/inspect/address.rs 0.00% 4 Missing ⚠️
... and 4 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1135      +/-   ##
==========================================
- Coverage   53.23%   53.10%   -0.14%     
==========================================
  Files         168      168              
  Lines       20945    21276     +331     
==========================================
+ Hits        11150    11298     +148     
- Misses       9795     9978     +183     

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

@nuttycom nuttycom force-pushed the zcash_address/expiry branch from 87bcae5 to 3314d2a Compare January 26, 2024 00:39
@nuttycom nuttycom force-pushed the zcash_address/expiry branch from 3314d2a to e78a5ec Compare February 15, 2024 23:46
@nuttycom nuttycom added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 16, 2024
@nuttycom nuttycom marked this pull request as ready for review February 16, 2024 00:19
@nuttycom nuttycom force-pushed the zcash_address/expiry branch from e78a5ec to 776ec12 Compare February 16, 2024 04:32
@nuttycom nuttycom marked this pull request as draft February 16, 2024 04:32
@nuttycom
Copy link
Contributor Author

Returning to draft status to update the parsing of the HRP for ZIP 316, Revision 1

@nuttycom nuttycom changed the title zcash_address: Add handling for Unified Metadata Items zcash_address: Add support for ZIP 316, Revision 1 Feb 16, 2024
@nuttycom nuttycom marked this pull request as ready for review February 16, 2024 22:04
@nuttycom nuttycom force-pushed the zcash_address/expiry branch from 93225df to 2833744 Compare February 16, 2024 22:16
zcash_keys/src/address.rs Outdated Show resolved Hide resolved
zcash_keys/src/address.rs Outdated Show resolved Hide resolved
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.

Some changes needed — see the "(blocking)" comments.

@nuttycom nuttycom force-pushed the zcash_address/expiry branch from 3c90d56 to 7852a31 Compare December 10, 2024 00:39
Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

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

Reviewed 7852a31.

components/zcash_address/CHANGELOG.md Outdated Show resolved Hide resolved
components/zcash_address/src/kind/unified.rs Show resolved Hide resolved
components/zcash_address/src/kind/unified.rs Outdated Show resolved Hide resolved
components/zcash_address/src/kind/unified.rs Outdated Show resolved Hide resolved
zcash_keys/CHANGELOG.md Outdated Show resolved Hide resolved
zcash_keys/src/keys.rs Outdated Show resolved Hide resolved
zcash_keys/src/keys.rs Show resolved Hide resolved
zcash_keys/src/keys.rs Show resolved Hide resolved
assert_eq!(
Address::parse_internal(Address::MAINNET, &encoded[..]),
Address::parse_internal(Address::MAINNET_R0, &encoded[..]),
Copy link
Contributor

Choose a reason for hiding this comment

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

(Just picking this test as an example)

I'm a little unhappy about the lack of Revision 1 tests. I think we'll be fine for now (particularly if my earlier suggestion to generate arbitrary revisions for proptests is accepted), but please open an issue for expanding testing to cover revision 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened #1647

Copy link
Contributor

@daira daira Dec 13, 2024

Choose a reason for hiding this comment

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

I actually think it's essential to have tests covering the differences between R0 and R1 (for example, tests that a single transparent Receiver is allowed for R1 but not for R0).

@nuttycom
Copy link
Contributor Author

force-pushed to address comments from code review.

@nuttycom nuttycom requested a review from str4d December 10, 2024 23:36
daira
daira previously approved these changes Dec 13, 2024
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 with comments. (Note that I was tired by the end of the review so it might not be as thorough as usual.)

@nuttycom nuttycom force-pushed the zcash_address/expiry branch 3 times, most recently from 3a04ed2 to c7b0e01 Compare December 15, 2024 16:41
@nuttycom
Copy link
Contributor Author

force-pushed to rebase on main & fix conflicts with no_std changes.

Then force-pushed to take into account review suggestions.

@nuttycom nuttycom force-pushed the zcash_address/expiry branch from 154b002 to 16b78e1 Compare December 15, 2024 21:52
@nuttycom nuttycom force-pushed the zcash_address/expiry branch from 16b78e1 to 022685a Compare December 20, 2024 17:47
@str4d str4d mentioned this pull request Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-zip-impl Category: An implementation of a ZIP, or a request to do so S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

zcash_address: Support Zip 316, Revision 1 (MUST-understand metadata)
3 participants