zcash_client_backend: add Orchard spends and outputs to transaction construction #1105
Conversation
nuttycom
force-pushed
the
wallet/orchard_spends_and_outputs
branch
6 times, most recently
from
2229f31 to
a3cc2df
Compare
nuttycom
force-pushed
the
wallet/orchard_spends_and_outputs
branch
from
a3cc2df to
80585bb
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1105 +/- ##
==========================================
- Coverage 64.98% 64.44% -0.54%
==========================================
Files 115 115
Lines 11437 11639 +202
==========================================
+ Hits 7432 7501 +69
- Misses 4005 4138 +133 ☔ View full report in Codecov by Sentry. |
nuttycom
force-pushed
the
wallet/orchard_spends_and_outputs
branch
from
80585bb to
12b6909
Compare
nuttycom
force-pushed
the
wallet/orchard_spends_and_outputs
branch
2 times, most recently
from
ed7de98 to
97ee67a
Compare
nuttycom
force-pushed
the
wallet/orchard_spends_and_outputs
branch
3 times, most recently
from
bedf9fc to
c198c9f
Compare
nuttycom
force-pushed
the
wallet/orchard_spends_and_outputs
branch
from
f67158a to
fcbfdaa
Compare
In the event that the pool to which change should be sent cannot automatically be determined based upon the inputs and outputs of a transaction, it is up to the caller to specify where change should be sent.
nuttycom
force-pushed
the
wallet/orchard_spends_and_outputs
branch
from
fcbfdaa to
4e3d99f
Compare
|
Accrues toward #403 |
|
|
||
| /// The type of the backing [`ShardStore`] for the Orchard note commitment tree. | ||
| #[cfg(feature = "orchard")] | ||
| type OrchardShardStore<'a>: ShardStore< |
There was a problem hiding this comment.
Hmm, we can't use the default trick to make the feature flag additive here, because the bound directly involves orchard crate types.
In a pairing with @nuttycom we came to the conclusion that the WalletRead, WalletWrite, and WalletCommitmentTrees are close enough to the application layer that it is less likely that people will run into "spooky feature flag enabling at a distance". However, the other things in zcash_client_backend that also interact with the feature flags (e.g. batch scanning) can result in this, and we have concrete evidence of people wanting to depend on zcash_client_backend solely to build on those things and not the app-layer traits.
So what we will develop towards is splitting out the non-app-layer things into separate crates (which are the boundaries on which feature flags operate, thus providing insulation against the problem), and focus zcash_client_backend solely on "just below app" use cases.
If we can also get rid of the associated types here (which @nuttycom said he would like to do), then we can also go back to providing a default implementation of these feature-flagged methods, fixing the problem.
| for<'a> F: FnMut( | ||
| &'a mut ShardTree< | ||
| Self::OrchardShardStore<'a>, | ||
| { ORCHARD_SHARD_HEIGHT * 2 }, |
There was a problem hiding this comment.
Nit: I think I'd prefer a cast from the orchard crate type, but this is fine given that everything here is divisible by 2.
| for<'a> F: FnMut( | ||
| &'a mut ShardTree< | ||
| Self::OrchardShardStore<'a>, | ||
| { ORCHARD_SHARD_HEIGHT * 2 }, |
| let selectable_pools = &[ShieldedProtocol::Sapling]; | ||
| #[cfg(zcash_unstable = "orchard")] | ||
| #[cfg(feature = "orchard")] |
There was a problem hiding this comment.
Hmm: we now don't attempt to select from the Orchard pool if orchard is disabled. What was the previous behaviour that made this change necessary, given that the wallet_db method is publicly reachable and thus other people could also run into it?
There was a problem hiding this comment.
The objective here is to make the zcash_unstable only be a gate for the 'orchard' feature, which is then the ultimate gate (that will remain in place permanently) for Orchard functionality. Removing the other uses of the config flag means that we have less variation in compilation to account for.
nuttycom
force-pushed
the
wallet/orchard_spends_and_outputs
branch
2 times, most recently
from
1a9bdb7 to
81694e1
Compare
nuttycom
force-pushed
the
wallet/orchard_spends_and_outputs
branch
from
81694e1 to
050a124
Compare
Co-authored-by: str4d <thestr4d@gmail.com>
nuttycom
force-pushed
the
wallet/orchard_spends_and_outputs
branch
from
5303cb0 to
184286c
Compare
| match output_pool { | ||
| #[cfg(not(feature = "orchard"))] | ||
| PoolType::Shielded(ShieldedProtocol::Orchard) => { | ||
| return Err(Error::ProposalNotSupported); |
There was a problem hiding this comment.
The documentation for this error variant is now out of date.
str4d
removed
the
S-waiting-on-review
Builds upon #1187