search

zcash / orchard

Implementation of the Zcash Orchard Protocol
https://zcash.github.io/orchard/
Other
53 stars 37 forks source link

add value_balance to builder #352

Closed AloeareV closed 1 year ago

str4d commented 2 years ago

This was rebased onto a broken main (EDIT: It wasn't rebased, but the new commit that was pushed triggered a test of this merged into the broken main). It will need to be rebased once #358 is merged, which fixes the compilation problem (or the tests will need to be re-run manually by us).

str4d commented 2 years ago

we already have ValueSum to represent the values of these kinds of balances.

ValueSum is not for representing value balances; the value balance type is only ever represented abstractly in this crate. But you do raise a good point that i64 is the wrong type to use in this API.

AloeareV commented 2 years ago

Switching to ValueSum necessitated adding a Sub impl... If the balance type is only ever represented abstractly, then maybe we add some safety checks and then still return an i64? Having a new type that is only ever used in the return of this one specific function is probably not the right answer either, if you can't use it anywhere without first doing a type conversion.

AloeareV commented 2 years ago

zcash_primitives::transaction::components::amount::Amount is probably the right type in the abstract, but zcash_primitives depends on orchard, so there's a circular dependency issue there

str4d commented 2 years ago

@AloeareV the type of a value balance is intentionally abstract in this crate, to allow different downstream consumers to use their own existing types (zcash_primitives::transaction::components::amount::Amount for our crates, Zebra's own type in their case). See the documentation for the orchard::value module, where we document this extensively.

str4d commented 2 years ago

Actually, now that I think about it, this PR should add "(and [Builder::value_balance] for in-progress bundles)" to the end of the third bullet-point in that module documentation for orchard::value, now that it is adding another way that value balances are produced.

codecov-commenter commented 2 years ago

Codecov Report

Base: 88.90% // Head: 88.66% // Decreases project coverage by -0.24% :warning:

Coverage data is based on head (86b8afd) compared to base (3faab98). Patch coverage: 0.00% of modified lines in pull request are covered.

:exclamation: Current head 86b8afd differs from pull request most recent head 8011e0d. Consider uploading reports for the commit 8011e0d to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #352 +/- ## ========================================== - Coverage 88.90% 88.66% -0.25% ========================================== Files 37 38 +1 Lines 3858 3873 +15 ========================================== + Hits 3430 3434 +4 - Misses 428 439 +11 ``` | [Impacted Files](https://codecov.io/gh/zcash/orchard/pull/352?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash) | Coverage Δ | | |---|---|---| | [src/builder.rs](https://codecov.io/gh/zcash/orchard/pull/352/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash#diff-c3JjL2J1aWxkZXIucnM=) | `73.98% <0.00%> (-1.54%)` | :arrow_down: | | [src/value.rs](https://codecov.io/gh/zcash/orchard/pull/352/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash#diff-c3JjL3ZhbHVlLnJz) | `89.28% <0.00%> (-2.46%)` | :arrow_down: | | [src/keys.rs](https://codecov.io/gh/zcash/orchard/pull/352/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash#diff-c3JjL2tleXMucnM=) | `81.67% <0.00%> (ø)` | | | [src/address.rs](https://codecov.io/gh/zcash/orchard/pull/352/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash#diff-c3JjL2FkZHJlc3MucnM=) | `40.00% <0.00%> (ø)` | | | [src/circuit.rs](https://codecov.io/gh/zcash/orchard/pull/352/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash#diff-c3JjL2NpcmN1aXQucnM=) | `88.66% <0.00%> (ø)` | | | [src/lib.rs](https://codecov.io/gh/zcash/orchard/pull/352/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash#diff-c3JjL2xpYi5ycw==) | `0.00% <0.00%> (ø)` | | | [src/note\_encryption.rs](https://codecov.io/gh/zcash/orchard/pull/352/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash#diff-c3JjL25vdGVfZW5jcnlwdGlvbi5ycw==) | `88.88% <0.00%> (+0.14%)` | :arrow_up: | | [src/note.rs](https://codecov.io/gh/zcash/orchard/pull/352/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash#diff-c3JjL25vdGUucnM=) | `88.73% <0.00%> (+0.32%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

zancas commented 2 years ago

Once this PR lands, and is subsequently included in a release, zingolib can unpatch orchard. This will directly affect the development efforts of at least 3 people. Who is eligible to review @str4d 's code? Does it need to be an ECC developer?

If @AloeareV had written the code, would it now be landed?