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.

Sign up for GitHub

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

Jump to bottom

Extract zip321 crate from zcash_client_backend #1143

Merged
merged 11 commits into from
Apr 22, 2024
Merged

Extract zip321 crate from zcash_client_backend #1143

merged 11 commits into from
Apr 22, 2024

Conversation

nuttycom
Copy link
Contributor

@nuttycom nuttycom commented Jan 27, 2024 •
edited
Loading

This depends upon #1140 and #1142

Fixes #1158

@nuttycom nuttycom requested a review from str4d January 27, 2024 20:57
@codecov Codecov
Copy link

codecov bot commented Jan 27, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 64.05797% with 124 lines in your changes are missing coverage. Please review.

Project coverage is 63.12%. Comparing base (430212c) to head (b60600a).
Report is 12 commits behind head on main.

❗ Current head b60600a differs from pull request most recent head d2aa6cf. Consider uploading reports for the commit d2aa6cf to get more accurate results

Files Patch % Lines
zcash_client_sqlite/src/lib.rs 21.15% 41 Missing ⚠️
components/zip321/src/lib.rs 78.57% 15 Missing ⚠️
zcash_keys/src/address.rs 34.78% 15 Missing ⚠️
zcash_client_backend/src/data_api/wallet.rs 80.00% 11 Missing ⚠️
...ent_backend/src/data_api/wallet/input_selection.rs 33.33% 10 Missing ⚠️
components/zcash_address/src/lib.rs 85.93% 9 Missing ⚠️
...mponents/zcash_address/src/kind/unified/address.rs 58.82% 7 Missing ⚠️
zcash_client_sqlite/src/wallet.rs 72.00% 7 Missing ⚠️
zcash_client_backend/src/data_api/error.rs 0.00% 5 Missing ⚠️
zcash_client_sqlite/src/error.rs 0.00% 3 Missing ⚠️
... and 1 more
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1143      +/-   ##
==========================================
- Coverage   63.19%   63.12%   -0.07%     
==========================================
  Files         124      124              
  Lines       14507    14716     +209     
==========================================
+ Hits         9168     9290     +122     
- Misses       5339     5426      +87

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

@nuttycom nuttycom mentioned this pull request Feb 2, 2024
Closed
@nuttycom nuttycom force-pushed the crate_zip321 branch 2 times, most recently from f6e75ef to c40d521 Compare February 2, 2024 20:05
@nuttycom nuttycom marked this pull request as draft February 5, 2024 21:45
@nuttycom nuttycom force-pushed the crate_zip321 branch 3 times, most recently from a5ad2e0 to a25f937 Compare February 7, 2024 19:36
@nuttycom nuttycom marked this pull request as ready for review February 7, 2024 19:36
@nuttycom nuttycom mentioned this pull request Feb 19, 2024
Merged
@nuttycom nuttycom added this to the Librustzcash Zashi 1.0 milestone Feb 22, 2024
@nuttycom nuttycom added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-in-progress Status: Work is currently in progress on this item. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 27, 2024
@nuttycom nuttycom removed this from the Librustzcash Zashi 1.0 milestone Mar 26, 2024
@nuttycom nuttycom force-pushed the crate_zip321 branch 6 times, most recently from c590e88 to 9eebca6 Compare April 5, 2024 22:00
@nuttycom nuttycom added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-in-progress Status: Work is currently in progress on this item. labels Apr 6, 2024
daira
daira reviewed Apr 7, 2024
View reviewed changes
components/zip321/LICENSE-MIT Outdated Show resolved Hide resolved
daira
daira reviewed Apr 7, 2024
View reviewed changes
zcash_client_sqlite/src/error.rs

/// An error produced in legacy transparent address derivation
#[cfg(feature = "transparent-inputs")]
HdwalletError(hdwallet::error::Error),

/// An error encountered in decoding a transparent address from its
/// serialized form.
#[cfg(feature = "transparent-inputs")]
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 specific to transparent-inputs? It seems as though it could also occur in principle due to support for transparent outputs, and the fact that it doesn't currently is just an implementation detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is unused if the transparent-inputs feature is not enabled, so it doesn't need to clutter the public API; a user who doesn't enable that feature shouldn't have to write a match for that case.

daira
daira previously approved these changes Apr 7, 2024
View reviewed changes
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.

daira
daira previously approved these changes Apr 18, 2024
View reviewed changes
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.

daira
daira reviewed Apr 18, 2024
View reviewed changes
zcash_keys/src/address.rs Outdated Show resolved Hide resolved
daira
daira previously approved these changes Apr 18, 2024
View reviewed changes
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.

re-utACK with comments.

str4d
str4d requested changes Apr 22, 2024
View reviewed changes
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 aeac544. The dbg! removal is blocking.

components/zcash_address/Cargo.toml Show resolved Hide resolved
components/zcash_address/src/lib.rs Show resolved Hide resolved
components/zip321/CHANGELOG.md Outdated Show resolved Hide resolved
components/zip321/src/lib.rs Outdated Show resolved Hide resolved
components/zip321/src/lib.rs Outdated Show resolved Hide resolved
components/zcash_address/src/kind/unified/address.rs Outdated Show resolved Hide resolved
components/zcash_address/src/lib.rs Outdated Show resolved Hide resolved
zcash_keys/src/address.rs Show resolved Hide resolved
@nuttycom @str4d
Apply suggestions from code review
d2aa6cf 
Co-authored-by: str4d <thestr4d@gmail.com>
@nuttycom nuttycom requested review from str4d and daira April 22, 2024 20:43
str4d
str4d approved these changes Apr 22, 2024
View reviewed changes
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.

utACK d2aa6cf

@str4d str4d merged commit 5c6a6a4 into zcash:main Apr 22, 2024
23 checks passed
@nuttycom nuttycom deleted the crate_zip321 branch May 1, 2024 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@str4d str4d str4d approved these changes

@daira daira Awaiting requested review from daira

Assignees
No one assigned
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use zcash_address instead of protocol-specific address types in ZIP 321 parsing.
3 participants
@nuttycom @daira @str4d