Skip to content

Add Orchard to transaction builder#863

Merged
str4d merged 5 commits intozcash:mainfrom
nuttycom:add_orchard_to_transaction_builder
Jun 23, 2023
Merged

Add Orchard to transaction builder#863
str4d merged 5 commits intozcash:mainfrom
nuttycom:add_orchard_to_transaction_builder

Conversation

@nuttycom
Copy link
Copy Markdown
Collaborator

@nuttycom nuttycom commented Jun 23, 2023

Subsumes #583

…:builder::Error`

Co-authored-by: Jack Grigg <jack@electriccoin.co>
@nuttycom nuttycom force-pushed the add_orchard_to_transaction_builder branch from 1e2b652 to aa9015d Compare June 23, 2023 20:07
Co-authored-by: Jack Grigg <jack@electriccoin.co>
Co-authored-by: Kris Nuttycombe <kris@nutty.land>
@nuttycom nuttycom force-pushed the add_orchard_to_transaction_builder branch from aa9015d to 4fbdd64 Compare June 23, 2023 20:10
Copy link
Copy Markdown
Collaborator Author

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 4fbdd64

@nuttycom nuttycom requested a review from str4d June 23, 2023 20:16
Copy link
Copy Markdown
Contributor

@AloeareV AloeareV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, no more transaction::components::orchard::builder mod, with all that MaybeOrchard stuff... It means that a builder now has the orchard-related methods even if it's not provided with an orchard anchor, which is now a runtime error when you try to call them with no anchor...but on the other hand, it's makes the implementation so much cleaner.

It's a tradeoff, but one that I'm fine with! The other change I've noticed is assert_matches, which just makes more sense than Eqable errors

@nuttycom
Copy link
Copy Markdown
Collaborator Author

Ah, no more transaction::components::orchard::builder mod, with all that MaybeOrchard stuff... It means that a builder now has the orchard-related methods even if it's not provided with an orchard anchor, which is now a runtime error when you try to call them with no anchor...but on the other hand, it's makes the implementation so much cleaner.

Not only that, there was already a runtime error that had to be raised if NU5 wasn't active, and so statically making those methods unavailable strictly increased the space of types that implementations had to deal with without reducing the number of possible runtime errors at all. So overall, the MaybeOrchard stuff wasn't paying its way.

@str4d str4d changed the title Add orchard to transaction builder Add Orchard to transaction builder Jun 23, 2023
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 23, 2023

Codecov Report

Patch coverage: 38.15% and project coverage change: -0.22 ⚠️

Comparison is base (06a7849) 69.97% compared to head (1b4017e) 69.76%.

❗ Current head 1b4017e differs from pull request most recent head 7fe02f0. Consider uploading reports for the commit 7fe02f0 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #863      +/-   ##
==========================================
- Coverage   69.97%   69.76%   -0.22%     
==========================================
  Files         124      124              
  Lines       11551    11618      +67     
==========================================
+ Hits         8083     8105      +22     
- Misses       3468     3513      +45     
Impacted Files Coverage Δ
zcash_client_backend/src/fees/fixed.rs 70.14% <ø> (ø)
zcash_client_backend/src/fees/zip317.rs 80.58% <ø> (ø)
zcash_client_backend/src/keys.rs 88.55% <ø> (ø)
zcash_extensions/src/transparent/demo.rs 76.62% <ø> (ø)
zcash_primitives/src/consensus.rs 49.28% <ø> (ø)
...h_primitives/src/transaction/components/orchard.rs 84.39% <ø> (ø)
zcash_primitives/src/transaction/fees/fixed.rs 87.50% <ø> (ø)
zcash_primitives/src/transaction/mod.rs 78.13% <ø> (ø)
zcash_primitives/src/transaction/builder.rs 61.53% <35.61%> (-13.31%) ⬇️
zcash_client_backend/src/data_api/wallet.rs 87.01% <100.00%> (ø)
... and 1 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment thread zcash_primitives/src/transaction/builder.rs Outdated
…ror::OrchardAnchorNotAvailable`

Co-authored-by: str4d <thestr4d@gmail.com>
Comment thread zcash_primitives/src/transaction/builder.rs Outdated
Comment thread zcash_primitives/src/transaction/builder.rs Outdated
Comment thread zcash_primitives/src/transaction/builder.rs Outdated
Comment thread zcash_primitives/src/transaction/builder.rs Outdated
Comment thread zcash_primitives/src/transaction/builder.rs Outdated
Comment thread zcash_primitives/src/transaction/builder.rs Outdated
Comment thread zcash_primitives/src/transaction/builder.rs Outdated
Comment thread zcash_primitives/src/transaction/builder.rs Outdated
Comment thread zcash_primitives/CHANGELOG.md Outdated
Comment thread zcash_primitives/CHANGELOG.md Outdated
Copy link
Copy Markdown
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@str4d str4d merged commit 5e7eea7 into zcash:main Jun 23, 2023
@nuttycom nuttycom deleted the add_orchard_to_transaction_builder branch June 23, 2023 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants