Add Orchard support to fees & transaction proposals. by nuttycom · Pull Request #1060 · zcash/librustzcash

nuttycom · 2023-12-06T19:02:48Z

Now builds atop #1090

Part of #403.
Closes #1094.
Closes #1097.

codecov · 2023-12-06T21:49:39Z

Codecov Report

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

Comparison is base (0548b3d) 66.45% compared to head (24ebe4c) 66.26%.
Report is 2 commits behind head on main.

Files	Patch %	Lines
zcash_client_backend/src/proto.rs	60.00%	30 Missing ⚠️
...ent_backend/src/data_api/wallet/input_selection.rs	66.66%	23 Missing ⚠️
zcash_client_backend/src/proto/proposal.rs	0.00%	13 Missing ⚠️
zcash_client_backend/src/fees.rs	45.45%	6 Missing ⚠️
zcash_client_backend/src/fees/common.rs	85.00%	6 Missing ⚠️
zcash_client_backend/src/data_api/wallet.rs	54.54%	5 Missing ⚠️
...sh_primitives/src/transaction/components/amount.rs	50.00%	4 Missing ⚠️
zcash_client_backend/src/fees/standard.rs	66.66%	3 Missing ⚠️
zcash_client_backend/src/data_api.rs	0.00%	2 Missing ⚠️
zcash_client_backend/src/wallet.rs	88.88%	2 Missing ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1060      +/-   ##
==========================================
- Coverage   66.45%   66.26%   -0.20%     
==========================================
  Files         113      114       +1     
  Lines       10863    10999     +136     
==========================================
+ Hits         7219     7288      +69     
- Misses       3644     3711      +67

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

str4d

Reviewed as of 4283338.

nuttycom · 2024-01-05T17:04:59Z

+                            Note::Orchard(_) => {
                                // FIXME: Implement this once `Proposal` has been refactored to
                                // include Orchard notes.
                                panic!("Orchard spends are not yet supported");


Added #1096

nuttycom · 2024-01-05T17:05:10Z

                ))
            }
+            ShieldedProtocol::Orchard => {
+                unimplemented!("FIXME: implement Orchard change output creation.")


Added #1095

nuttycom · 2024-01-05T17:06:04Z

+    #[cfg(not(feature = "orchard"))]
+    let orchard_out = NonNegativeAmount::ZERO;
+
+    // FIXME: this is a pretty naive strategy for selecting the pool to which change will be sent.


This could probably just be a TODO, not a FIXME

str4d

Reviewed as of 56609f3.

str4d

Reviewed as of 803e3b4.

This modifies the `compute_balance` method to operate in a bundle-oriented fashion, which simplifies the API and makes it easier to elide Orchard functionality in the case that the `orchard` feature is not enabled.

Co-authored-by: Jack Grigg <jack@electriccoin.co>

nuttycom · 2024-01-06T00:02:06Z

force-pushed to address remaining issues.

nuttycom · 2024-01-06T00:08:08Z

+            (ShieldedProtocol::Sapling, 1, 0)
+        } else {
+            // For all other transactions, send change to Sapling.
+            // FIXME: Change this to Orchard once Orchard outputs are enabled.


Added #1101

This change caused a clippy lint (identical blocks in both conditional cases), but I think that's fine for now if we're going to address 1101 soon.

str4d

utACK 24ebe4c

nuttycom force-pushed the wallet/generalize_proposals branch 4 times, most recently from f2d6315 to e5c086d Compare December 6, 2023 21:29

nuttycom force-pushed the wallet/generalize_proposals branch 3 times, most recently from 78ffa29 to a216989 Compare December 7, 2023 03:54

nuttycom changed the title ~~WIP: Add Orchard support to fees & transaction proposals.~~ Add Orchard support to fees & transaction proposals. Dec 7, 2023

nuttycom marked this pull request as ready for review December 7, 2023 03:55

nuttycom force-pushed the wallet/generalize_proposals branch from a216989 to 4283338 Compare December 7, 2023 15:50

str4d requested changes Dec 8, 2023

View reviewed changes

nuttycom force-pushed the wallet/generalize_proposals branch 2 times, most recently from fddc51c to 013faf6 Compare December 9, 2023 01:48

nuttycom requested a review from str4d December 9, 2023 01:50

nuttycom force-pushed the wallet/generalize_proposals branch from 013faf6 to 48836c4 Compare December 9, 2023 01:57

nuttycom marked this pull request as draft December 11, 2023 22:44

nuttycom force-pushed the wallet/generalize_proposals branch from 48836c4 to 66eeb8f Compare December 12, 2023 19:43

nuttycom force-pushed the wallet/generalize_proposals branch 2 times, most recently from 42de3a2 to 94c7eef Compare December 22, 2023 01:33

nuttycom force-pushed the wallet/generalize_proposals branch 3 times, most recently from 2a21330 to a7e054d Compare January 4, 2024 00:07

nuttycom marked this pull request as ready for review January 4, 2024 00:07

nuttycom commented Jan 4, 2024

View reviewed changes

Comment thread zcash_client_backend/src/data_api.rs Outdated

nuttycom mentioned this pull request Jan 4, 2024

zcash_client_backend: Introduce an "orchard" feature flag. #1080

Merged

nuttycom force-pushed the wallet/generalize_proposals branch 4 times, most recently from d20bbb0 to 4091121 Compare January 5, 2024 16:35

nuttycom force-pushed the wallet/generalize_proposals branch from 4091121 to cd2ee5a Compare January 5, 2024 16:44

str4d added this to the Orchard in Mobile SDKs milestone Jan 5, 2024

nuttycom commented Jan 5, 2024

View reviewed changes

nuttycom force-pushed the wallet/generalize_proposals branch 2 times, most recently from 1384727 to 56609f3 Compare January 5, 2024 17:25

str4d requested changes Jan 5, 2024

View reviewed changes

nuttycom force-pushed the wallet/generalize_proposals branch from 98eb7cd to 803e3b4 Compare January 5, 2024 20:09

str4d requested changes Jan 5, 2024

View reviewed changes

Comment thread zcash_primitives/src/transaction/components/amount.rs

Comment thread zcash_primitives/src/transaction/components/amount.rs

Comment thread zcash_primitives/src/transaction/components/amount.rs Outdated

Comment thread zcash_client_backend/src/data_api/wallet.rs

nuttycom added 2 commits January 5, 2024 16:59

zcash_client_backend: Add Orchard support to change strategies.

56f2ac5

This modifies the `compute_balance` method to operate in a bundle-oriented fashion, which simplifies the API and makes it easier to elide Orchard functionality in the case that the `orchard` feature is not enabled.

zcash_client_backend: Add Orchard support to transaction proposals.

adc7556

nuttycom force-pushed the wallet/generalize_proposals branch from 803e3b4 to 1bd915e Compare January 6, 2024 00:00

Address comments from code review.

24ebe4c

Co-authored-by: Jack Grigg <jack@electriccoin.co>

nuttycom force-pushed the wallet/generalize_proposals branch from 1bd915e to 24ebe4c Compare January 6, 2024 00:00

nuttycom commented Jan 6, 2024

View reviewed changes

nuttycom requested a review from str4d January 6, 2024 03:39

str4d approved these changes Jan 9, 2024

View reviewed changes

nuttycom merged commit c3a630b into zcash:main Jan 9, 2024

nuttycom deleted the wallet/generalize_proposals branch January 9, 2024 16:20

nuttycom mentioned this pull request Jan 9, 2024

Replace Backend.{createToAddress, shieldToAddress} with proposal-based methods zcash/zcash-android-wallet-sdk#1344

Merged

13 tasks

Conversation

nuttycom commented Dec 6, 2023 • edited by str4d Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

str4d left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nuttycom Jan 5, 2024

Choose a reason for hiding this comment

Uh oh!

nuttycom Jan 5, 2024

Choose a reason for hiding this comment

Uh oh!

nuttycom Jan 5, 2024

Choose a reason for hiding this comment

Uh oh!

str4d left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

str4d left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nuttycom commented Jan 6, 2024

Uh oh!

nuttycom Jan 6, 2024

Choose a reason for hiding this comment

Uh oh!

str4d Jan 9, 2024

Choose a reason for hiding this comment

Uh oh!

str4d left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nuttycom commented Dec 6, 2023 •

edited by str4d

Loading

codecov Bot commented Dec 6, 2023 •

edited

Loading