Factor out protocol constants and value types into a `zcash_protocol` crate. #1142

nuttycom · 2024-01-27T19:35:37Z

This is best reviewed commit-by-commit.

Closes #1157.

codecov · 2024-01-27T19:55:08Z

Codecov Report

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

Project coverage is 63.97%. Comparing base (0c12f31) to head (376db46).

Files	Patch %	Lines
components/zcash_protocol/src/value.rs	65.21%	48 Missing ⚠️
zcash_keys/src/keys.rs	24.24%	25 Missing ⚠️
components/zcash_protocol/src/consensus.rs	75.40%	15 Missing ⚠️
zcash_client_sqlite/src/wallet/init.rs	0.00%	7 Missing ⚠️
...src/wallet/init/migrations/receiving_key_scopes.rs	45.45%	6 Missing ⚠️
components/zcash_protocol/src/lib.rs	0.00%	4 Missing ⚠️
zcash_client_sqlite/src/error.rs	0.00%	4 Missing ⚠️
zcash_keys/src/address.rs	66.66%	4 Missing ⚠️
components/zcash_address/src/encoding.rs	90.90%	2 Missing ⚠️
zcash_client_backend/src/fees/sapling.rs	0.00%	2 Missing ⚠️
... and 3 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1142      +/-   ##
==========================================
- Coverage   64.33%   63.97%   -0.36%     
==========================================
  Files         115      115              
  Lines       11872    11895      +23     
==========================================
- Hits         7638     7610      -28     
- Misses       4234     4285      +51

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

zcash_client_backend/CHANGELOG.md

zcash_client_backend/src/zip321.rs

zcash_client_backend/CHANGELOG.md

zcash_client_backend/src/zip321.rs

Co-authored-by: Daira-Emma Hopwood <daira@jacaranda.org>

daira

Flushing comments.

zcash_client_backend/src/zip321.rs

daira · 2024-03-06T03:18:33Z

zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs

+                        "Unable to parse raw transaction: {:?}",
+                        e
+                    ))
+                })?;
                let output = tx
                    .sapling_bundle()
                    .and_then(|b| b.shielded_outputs().get(output_index))


Should the panic just below also be a WalletMigrationError::CorruptedData return?

Nit: we sometimes use "string literal".to_owned(), and sometimes "string literal".to_string(). They do the same thing, but let's be consistent and use to_owned().

We are so totally inconsistent about this that fixing it should be done outside of this PR; I find 70 different occurrences of .to_string all across the codebase.

daira · 2024-03-06T03:28:08Z

zcash_client_sqlite/src/wallet/init/migrations/ufvk_support.rs

@@ -111,7 +109,7 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
                            "Address field value decoded to a transparent address; should have been Sapling or unified.".to_string()));


Suggested change

"Address field value decoded to a transparent address; should have been Sapling or unified.".to_string()));

"Address field value decoded to a transparent address; should have been Sapling or unified.".to_owned()));

These two are semantically identical.

daira · 2024-03-06T03:33:10Z

zcash_keys/src/keys.rs

-        self.to_unified_full_viewing_key().default_address(request)
+        self.to_unified_full_viewing_key()
+            .default_address(request)
+            .unwrap()


This should probably be .expect(...).

daira

utACK modulo comments.

str4d

Re-utACK 376db46

daira

Post-hoc re-ACK 376db46

nuttycom requested a review from str4d January 27, 2024 19:35

nuttycom force-pushed the crate_zcash_protocol branch from bbf1dc8 to f356fa3 Compare January 27, 2024 19:36

nuttycom force-pushed the crate_zcash_protocol branch from f356fa3 to 91a65e3 Compare January 27, 2024 19:59

nuttycom mentioned this pull request Jan 27, 2024

Extract zip321 crate from zcash_client_backend #1143

Merged

nuttycom force-pushed the crate_zcash_protocol branch 2 times, most recently from ccfd94a to f9a900e Compare February 2, 2024 18:12

nuttycom mentioned this pull request Feb 2, 2024

Extract "component" crates into a separate repository #768

Open

nuttycom force-pushed the crate_zcash_protocol branch 3 times, most recently from 3abc70e to 1264182 Compare February 2, 2024 21:42

nuttycom marked this pull request as draft February 3, 2024 00:28

nuttycom force-pushed the crate_zcash_protocol branch 5 times, most recently from bd33550 to 234522f Compare February 5, 2024 20:00

nuttycom marked this pull request as ready for review February 5, 2024 20:05

nuttycom force-pushed the crate_zcash_protocol branch 5 times, most recently from e387ae2 to 01fd657 Compare February 7, 2024 19:05

nuttycom mentioned this pull request Feb 7, 2024

zcash_primitives: Factor out the memo module into the zcash_memos crate #1140

Closed

nuttycom force-pushed the crate_zcash_protocol branch from 01fd657 to 23bc3fe Compare February 8, 2024 04:41

nuttycom added this to the Librustzcash Zashi 1.0 milestone Feb 22, 2024

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 force-pushed the crate_zcash_protocol branch from 23bc3fe to b43bb65 Compare March 1, 2024 23:45

daira reviewed Mar 6, 2024

View reviewed changes

zcash_client_backend/CHANGELOG.md Outdated Show resolved Hide resolved

daira reviewed Mar 6, 2024

View reviewed changes

zcash_client_backend/src/zip321.rs Show resolved Hide resolved

daira reviewed Mar 6, 2024

View reviewed changes

zcash_client_backend/src/zip321.rs Outdated Show resolved Hide resolved

daira reviewed Mar 6, 2024

View reviewed changes

zcash_client_backend/src/zip321.rs Outdated Show resolved Hide resolved

daira reviewed Mar 6, 2024

View reviewed changes

zcash_client_backend/src/zip321.rs Outdated Show resolved Hide resolved

nuttycom dismissed str4d’s stale review via f2f8af6 March 6, 2024 02:44

nuttycom force-pushed the crate_zcash_protocol branch from f2f8af6 to c4fe5f2 Compare March 6, 2024 02:51

nuttycom requested review from daira and str4d March 6, 2024 02:51

daira reviewed Mar 6, 2024

View reviewed changes

zcash_client_backend/CHANGELOG.md Outdated Show resolved Hide resolved

daira reviewed Mar 6, 2024

View reviewed changes

zcash_client_backend/src/zip321.rs Outdated Show resolved Hide resolved

Apply suggestions from code review

eaabc0f

Co-authored-by: Daira-Emma Hopwood <daira@jacaranda.org>

nuttycom force-pushed the crate_zcash_protocol branch from c4fe5f2 to eaabc0f Compare March 6, 2024 03:03

daira reviewed Mar 6, 2024

View reviewed changes

zcash_client_backend/src/zip321.rs Outdated Show resolved Hide resolved

zcash_client_backend/src/zip321.rs Show resolved Hide resolved

daira reviewed Mar 6, 2024

View reviewed changes

daira previously approved these changes Mar 6, 2024

View reviewed changes

nuttycom added 2 commits March 5, 2024 21:48

Add failing test for incorrect zip321 amount parsing.

6b4942f

Fix incorrect zip321 amount parsing.

376db46

nuttycom dismissed daira’s stale review via 376db46 March 6, 2024 04:50

nuttycom requested a review from daira March 6, 2024 04:53

str4d approved these changes Mar 6, 2024

View reviewed changes

str4d merged commit 16e3d1b into zcash:main Mar 6, 2024
23 of 24 checks passed

nuttycom deleted the crate_zcash_protocol branch March 6, 2024 14:48

This was referenced Mar 6, 2024

zcash_address 0.3.2: ZIP 320 TEX address support #1226

Merged

zcash_address: Add support for ZIP 316, Revision 1 #1135

Open

daira reviewed Mar 6, 2024

View reviewed changes

This was referenced Apr 1, 2024

Make Zcash memo types usable without dependencies on the sapling-crypto or orchard crates. #1156

Closed

Make Zcash monetary types usable without a dependency on the sapling-crypto or orchard crates. #1155

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Factor out protocol constants and value types into a `zcash_protocol` crate. #1142

Factor out protocol constants and value types into a `zcash_protocol` crate. #1142

nuttycom commented Jan 27, 2024 •

edited by str4d

codecov bot commented Jan 27, 2024 •

edited

daira left a comment

daira Mar 6, 2024

daira Mar 6, 2024

nuttycom Mar 6, 2024

daira Mar 6, 2024

nuttycom Mar 6, 2024

daira Mar 6, 2024

daira left a comment

str4d left a comment

daira left a comment •

edited

		@@ -111,7 +109,7 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
		"Address field value decoded to a transparent address; should have been Sapling or unified.".to_string()));

Factor out protocol constants and value types into a zcash_protocol crate. #1142

Factor out protocol constants and value types into a zcash_protocol crate. #1142

Conversation

nuttycom commented Jan 27, 2024 • edited by str4d

codecov bot commented Jan 27, 2024 • edited

Codecov Report

daira left a comment

Choose a reason for hiding this comment

daira Mar 6, 2024

Choose a reason for hiding this comment

daira Mar 6, 2024

Choose a reason for hiding this comment

nuttycom Mar 6, 2024

Choose a reason for hiding this comment

daira Mar 6, 2024

Choose a reason for hiding this comment

nuttycom Mar 6, 2024

Choose a reason for hiding this comment

daira Mar 6, 2024

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

daira left a comment • edited

Choose a reason for hiding this comment

Factor out protocol constants and value types into a `zcash_protocol` crate. #1142

Factor out protocol constants and value types into a `zcash_protocol` crate. #1142

nuttycom commented Jan 27, 2024 •

edited by str4d

codecov bot commented Jan 27, 2024 •

edited

daira left a comment •

edited