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

Post sapling crypto extraction cleanup #1068

Merged
merged 3 commits into from
Dec 12, 2023
Merged

Post sapling crypto extraction cleanup #1068

merged 3 commits into from
Dec 12, 2023

Conversation

str4d
Copy link
Contributor

@str4d str4d commented Dec 12, 2023 •
edited
Loading

@str4d str4d mentioned this pull request Dec 12, 2023
Merged
@str4d str4d force-pushed the post-sapling-crypto-extraction-cleanup branch from 9928fe1 to 341637c Compare December 12, 2023 19:00
@codecov Codecov
Copy link

codecov bot commented Dec 12, 2023 •
edited
Loading

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (dc80948) 66.17% compared to head (35ea2ff) 66.19%.
Report is 1 commits behind head on main.

Files Patch % Lines
zcash_client_sqlite/src/wallet.rs 66.66% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1068      +/-   ##
==========================================
+ Coverage   66.17%   66.19%   +0.01%     
==========================================
  Files         112      112              
  Lines       10795    10797       +2     
==========================================
+ Hits         7144     7147       +3     
+ Misses       3651     3650       -1

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

nuttycom
nuttycom previously approved these changes Dec 12, 2023
View reviewed changes
Copy link
Contributor

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

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

utACK

zcash_primitives/CHANGELOG.md
@@ -220,45 +91,33 @@ and this library adheres to Rust's notion of

### Removed
- `zcash_primitives::constants`:
- All `const` values (moved to `zcash_primitives::sapling::constants`).
- All `const` values (moved to `sapling_crypto::constants`).
Copy link
Contributor

Choose a reason for hiding this comment

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

I find the use of sapling, sapling_crypto, and sapling-crypto to all mean the same thing a bit confusing.

Copy link
Contributor Author

@str4d str4d Dec 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

sapling-crypto and sapling_crypto are the same thing in Rust; from a module import perspective, they are both sapling_crypto. And thus to a downstream user of this crate, they will be importing from use sapling_crypto::foo unless they rename the dependency. We do rename the dependency in our workspace here to sapling for parity with orchard.

@str4d str4d force-pushed the post-sapling-crypto-extraction-cleanup branch from 341637c to 35ea2ff Compare December 12, 2023 19:50
nuttycom
nuttycom approved these changes Dec 12, 2023
View reviewed changes
Copy link
Contributor

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

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

utACK

@str4d str4d merged commit 926c5dc into main Dec 12, 2023
29 of 31 checks passed
@str4d str4d deleted the post-sapling-crypto-extraction-cleanup branch December 12, 2023 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nuttycom nuttycom nuttycom approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@str4d @nuttycom