Preparatory refactoring for zcash/librustzcash#1673 #1675

nuttycom · 2024-12-30T16:49:12Z

This is the preparatory work pulled out from #1673

nuttycom · 2024-12-30T16:51:18Z

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

This could be better handled by taking a golden testing approach: branching from the commit where this test was added, create a wallet database file containing the buggy state, and store it in the repository; then, add an ordinary test that begins from that buggy database file and ensure that migrating to the current state results in the bug having been fixed.

nuttycom · 2024-12-30T16:54:01Z

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

This could be better handled by taking a golden testing approach: branching from the commit where this test was added, create a wallet database file containing the buggy state, and store it in the repository; then, add an ordinary test that begins from that buggy database file and ensure that migrating to the current state results in the bug having been fixed.

zcash_keys/src/keys.rs

daira

utACK with comments.

zcash_client_sqlite/src/wallet.rs

zcash_client_backend/src/data_api/wallet.rs

zcash_client_backend/src/wallet.rs

… `NetworkConstants` trait.

…he `transparent-inputs` feature flag. This variant should not have been a part of the public API unless `tranpsarent-inputs` was enabled, as it's necessary for the wallet to be able to spend a transparent input in order for a ZIP 320 transaction to be properly constructed and authorized. In addition, this simplifies the `Recipient` API by removing its type parameters in favor of concrete types, made possible by using a separate type for the build process.

…resses`

…gration. This test is not specific to the migration; it's a more general test of ephemeral address rotation behavior and should evolve with the evolution of address rotation and gap limit handling, not be tied to the behavior of methods at the time that this migration was created.

This permits `UnifiedAddressRequest` values an additional dimension of flexibility, permitting generation of unified addresses having receivers for all recever types for which a key item exists and a diversifier index is valid.

This removes a `fix_bad_change_flagging` migration test duplicated from `zcash_client_backend::data_api::testing::pool::shiled_transparent`. It is impractical to maintain backwards compatibility to earlier database states for the entire test harness, which is more or less what retaining this test would require, and the desired outcome is already demonstrated by the `shield_transparent` test; demonstrating the fix directly is unnecessary.

str4d

utACK e6b45f7

str4d · 2024-12-30T21:28:05Z

zcash_keys/src/keys.rs

+            (Require, Omit) => None,
+            (Require, Require) => Some(Require),
+            (Require, Allow) => Some(Require),
+            (Allow, Require) => Some(Require),
+            (Allow, Allow) => Some(Allow),
+            (Allow, Omit) => Some(Omit),
+            (Omit, Require) => None,
+            (Omit, Allow) => Some(Omit),
+            (Omit, Omit) => Some(Omit),


Slightly more concise:

Suggested change

(Require, Omit) => None,

(Require, Require) => Some(Require),

(Require, Allow) => Some(Require),

(Allow, Require) => Some(Require),

(Allow, Allow) => Some(Allow),

(Allow, Omit) => Some(Omit),

(Omit, Require) => None,

(Omit, Allow) => Some(Omit),

(Omit, Omit) => Some(Omit),

(Require, Omit) | (Omit, Require) => None,

(Require, _) | (_, Require) => Some(Require),

(Omit, _) | (_, Omit) => Some(Omit),

(Allow, Allow) => Some(Allow),

codecov · 2024-12-30T23:39:42Z

Codecov Report

Attention: Patch coverage is 47.32510% with 128 lines in your changes missing coverage. Please review.

Project coverage is 52.22%. Comparing base (6ead8f8) to head (e6b45f7).
Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
zcash_client_sqlite/src/wallet.rs	53.75%	37 Missing ⚠️
zcash_client_backend/src/data_api/wallet.rs	50.00%	35 Missing ⚠️
zcash_keys/src/keys.rs	53.57%	26 Missing ⚠️
components/zcash_protocol/src/consensus.rs	0.00%	21 Missing ⚠️
zcash_keys/src/address.rs	0.00%	6 Missing ⚠️
...lite/src/wallet/init/migrations/addresses_table.rs	50.00%	1 Missing ⚠️
...llet/init/migrations/ensure_orchard_ua_receiver.rs	0.00%	1 Missing ⚠️
..._sqlite/src/wallet/init/migrations/ufvk_support.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1675      +/-   ##
==========================================
- Coverage   52.30%   52.22%   -0.09%     
==========================================
  Files         179      179              
  Lines       21343    21353      +10     
==========================================
- Hits        11164    11152      -12     
- Misses      10179    10201      +22

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

daira · 2024-12-31T07:13:16Z

zcash_client_backend/src/data_api/wallet.rs

+                output_pool,
+            },
+            #[cfg(feature = "transparent-inputs")]
+            BuildRecipient::EphemeralTransparent { .. } => unreachable!(),


Why is this unreachable?

daira · 2024-12-31T07:15:00Z

zcash_client_backend/src/data_api/wallet.rs

+                ephemeral_address,
+                outpoint,
+            },
+            BuildRecipient::InternalAccount { .. } => unreachable!(),


Why is this unreachable?

daira · 2024-12-31T07:25:37Z

zcash_keys/src/keys.rs

    /// this UFVK.
    ///
-    /// Returns `None` if the specified index does not produce a valid diversifier.
+    /// Returns an error if the this key does not produce a valid receiver for a required receiver


Nit: "if the this"

daira

Post-hoc utACK with questions.

nuttycom commented Dec 30, 2024

View reviewed changes

nuttycom force-pushed the feature/transparent_gap_limit_handling-prep branch from 3e51560 to 7a46303 Compare December 30, 2024 16:53

nuttycom commented Dec 30, 2024

View reviewed changes

daira reviewed Dec 30, 2024

View reviewed changes

zcash_keys/src/keys.rs Outdated Show resolved Hide resolved

daira reviewed Dec 30, 2024

View reviewed changes

zcash_keys/src/keys.rs Outdated Show resolved Hide resolved

daira reviewed Dec 30, 2024

View reviewed changes

zcash_keys/src/keys.rs Outdated Show resolved Hide resolved

daira reviewed Dec 30, 2024

View reviewed changes

zcash_keys/src/keys.rs Outdated Show resolved Hide resolved

daira previously approved these changes Dec 30, 2024

View reviewed changes

nuttycom dismissed daira’s stale review via 4a08196 December 30, 2024 18:16

nuttycom force-pushed the feature/transparent_gap_limit_handling-prep branch 4 times, most recently from 03d4d2e to 6ba9b48 Compare December 30, 2024 19:39

str4d reviewed Dec 30, 2024

View reviewed changes

zcash_client_sqlite/src/wallet.rs Outdated Show resolved Hide resolved

nuttycom force-pushed the feature/transparent_gap_limit_handling-prep branch from 6ba9b48 to cccbf01 Compare December 30, 2024 20:57

str4d reviewed Dec 30, 2024

View reviewed changes

zcash_client_backend/src/data_api/wallet.rs Outdated Show resolved Hide resolved

str4d reviewed Dec 30, 2024

View reviewed changes

zcash_client_backend/src/data_api/wallet.rs Outdated Show resolved Hide resolved

str4d reviewed Dec 30, 2024

View reviewed changes

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

nuttycom force-pushed the feature/transparent_gap_limit_handling-prep branch 4 times, most recently from 990cd7e to c2dbd20 Compare December 30, 2024 22:10

nuttycom added 8 commits December 30, 2024 15:11

zcash_protocol: Add unified address/fvk/ivk HRPs retrieval methods to…

5d7a185

… `NetworkConstants` trait.

zcash_client_sqlite: factor out commonalities of account retrieval

da98f9f

Move-only: group account retrieval methods together.

736bfd5

zcash_client_backend: Improve type safety of `get_known_ephemeral_add…

29d8f10

…resses`

zcash_keys: Add ReceiverRequirement enum.

e4dac19

This permits `UnifiedAddressRequest` values an additional dimension of flexibility, permitting generation of unified addresses having receivers for all recever types for which a key item exists and a diversifier index is valid.

zcash_keys: Add Address::to_transparent_address

c278405

nuttycom force-pushed the feature/transparent_gap_limit_handling-prep branch from c2dbd20 to e6b45f7 Compare December 30, 2024 22:15

str4d approved these changes Dec 30, 2024

View reviewed changes

str4d merged commit 5651d80 into zcash:main Dec 30, 2024
30 checks passed

daira reviewed Dec 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preparatory refactoring for zcash/librustzcash#1673 #1675

Preparatory refactoring for zcash/librustzcash#1673 #1675

nuttycom commented Dec 30, 2024 •

edited

Loading

nuttycom Dec 30, 2024

nuttycom Dec 30, 2024

daira left a comment

str4d left a comment

str4d Dec 30, 2024

codecov bot commented Dec 30, 2024

daira Dec 31, 2024

daira Dec 31, 2024

daira Dec 31, 2024

daira left a comment

Preparatory refactoring for zcash/librustzcash#1673 #1675

Preparatory refactoring for zcash/librustzcash#1673 #1675

Conversation

nuttycom commented Dec 30, 2024 • edited Loading

nuttycom Dec 30, 2024

Choose a reason for hiding this comment

nuttycom Dec 30, 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

str4d Dec 30, 2024

Choose a reason for hiding this comment

codecov bot commented Dec 30, 2024

Codecov Report

daira Dec 31, 2024

Choose a reason for hiding this comment

daira Dec 31, 2024

Choose a reason for hiding this comment

daira Dec 31, 2024

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

nuttycom commented Dec 30, 2024 •

edited

Loading