zcash / librustzcash

Rust-language assets for Zcash
Other
333 stars 249 forks source link

zcash_primitives: Refactor `Transaction` to permit omitting protocol-specific bundles #1388

Open str4d opened 4 months ago

str4d commented 4 months ago

Draft because there are still some things that need doing:

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 76.19048% with 60 lines in your changes missing coverage. Please review.

Project coverage is 63.23%. Comparing base (5bd911f) to head (16988be).

Files Patch % Lines
zcash_primitives/src/transaction/mod.rs 62.71% 22 Missing :warning:
zcash_primitives/src/transaction/components/tze.rs 0.00% 18 Missing :warning:
zcash_primitives/src/transaction/sighash_v4.rs 89.23% 7 Missing :warning:
zcash_primitives/src/transaction/txid.rs 76.92% 6 Missing :warning:
zcash_primitives/src/transaction/components.rs 50.00% 4 Missing :warning:
...sh_primitives/src/transaction/components/sprout.rs 94.73% 1 Missing :warning:
...imitives/src/transaction/components/transparent.rs 93.75% 1 Missing :warning:
zcash_primitives/src/transaction/sighash_v5.rs 88.88% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1388 +/- ## ========================================== + Coverage 63.20% 63.23% +0.02% ========================================== Files 128 129 +1 Lines 14869 14948 +79 ========================================== + Hits 9398 9452 +54 - Misses 5471 5496 +25 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

daira commented 4 months ago

I haven't done anything with ZFuture, and am not quite sure what to do there.

Can we just delete TZE support? I don't care about them and I very much doubt they are ever going to be deployed. We should ask on #libraries in the R&D Discord obviously. Deleting them should be easy given that they're feature flagged.

str4d commented 4 months ago

I was a little bit alarmed about the name UnauthorizedTransactionDataWithSaplingProofs (i.e. why so unorthogonal?), but then I saw the motivation and it's fine.

My hope is that this type goes away once I land the refactor that follows after this one (to restructure the zcash_primitives transaction builder to match Sapling and Orchard, by separating the building of the unauthorized bundle and its authorization). At that point, this type will be a well-defined intermediate state (as an explicit method output), instead of an ill-defined internal detail of Builder::build that requires type-pinning to help the compiler.

str4d commented 4 months ago

In e3a29b86cd473324b3a8bd480e6640b6fac03d84 I've added Yet Another Intermediate Trait Bundles, which I think solves the "conditional compilation of bundles" that is necessary for merging code that is not currently part of consensus (as well as making the enabling of these bundles in consensus less of a breaking change to downstream code). If this approach looks good, I'll rebase the PR to merge the Bundles changes back into the earlier commits (simplifying them).

str4d commented 4 months ago

Force-pushed to merge the commit introducing the Bundles trait back into the rest of the commits, simplifying them.

str4d commented 4 months ago

Force-pushed to address some initial review comments from a pairing with @nuttycom.

str4d commented 4 months ago

Force-pushed to move Sprout onto the Bundles trait. It has no authorization traits because we only support already-authorized Sprout bundles (due to not supporting Sprout in transaction building).

str4d commented 4 months ago

Force-pushed to move TZEs onto the Bundles trait, as a proof-of-concept for other conditionally-compiled transaction format changes that we want to merge into the codebase before inclusion in consensus (e.g. ZSAs).

str4d commented 4 months ago

Force-pushed to adjust how the new traits work in order to eliminate the bounds on the protocol-crate-specific authorization traits.

str4d commented 4 months ago

Force-pushed with some further decoupling adjustments and trait renames. It also removes the NoTransparent etc. changes (and the TransactionWith refactor) as not currently necessary, because the dependency removal will be done by altering the Transparent<A> etc. types (in a way that is technically a breaking change, but in a way that should only be observable as purely additive in our API).

str4d commented 3 months ago

Rebased on main to fix a small merge conflict.