add value_balance to builder by AloeareV · Pull Request #352 · zcash/orchard

AloeareV · 2022-08-23T19:24:34Z

No description provided.

str4d · 2022-09-15T16:12:16Z

~~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 · 2022-09-15T16:13:25Z

+    /// The net value of the bundle. The value of all spends, minus the value
+    /// of all outputs.
+    pub fn value_balance(&self) -> i64 {


What is the use case for this method (calculating the value balance from the in-progress builder state)? It would be useful to describe intended use cases in the documentation.

nuttycom · 2022-09-15T18:58:01Z

+    pub fn value_balance(&self) -> i64 {
+        self.spends
+            .iter()
+            .map(|spend| spend.note.value().inner() as i64)


I would prefer that this return a ValueSum value, which wraps an i128 internally.

nuttycom

i64 is not sufficiently type-safe, and we already have ValueSum to represent the values of these kinds of balances.

str4d · 2022-09-15T19:22:09Z

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 · 2022-09-15T19:32:15Z

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 · 2022-09-15T19:34:26Z

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 · 2022-09-15T19:39:49Z

@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 · 2022-09-15T19:42:04Z

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.

str4d

utACK 8011e0d, but I am now the author of almost all of this PR's code, so it needs another review (because I do not trust myself to not get my lefts and rights mixed up).

codecov-commenter · 2022-09-15T19:59:36Z

Codecov Report

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

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

❗ 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

@@            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	Coverage Δ
src/builder.rs	`73.98% <0.00%> (-1.54%)`	⬇️
src/value.rs	`89.28% <0.00%> (-2.46%)`	⬇️
src/keys.rs	`81.67% <0.00%> (ø)`
src/address.rs	`40.00% <0.00%> (ø)`
src/circuit.rs	`88.66% <0.00%> (ø)`
src/lib.rs	`0.00% <0.00%> (ø)`
src/note_encryption.rs	`88.88% <0.00%> (+0.14%)`	⬆️
src/note.rs	`88.73% <0.00%> (+0.32%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

zancas · 2022-09-16T16:11:18Z

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?

nuttycom

utACK, modulo it would be good to have a test for value_balance

sellout

Just a question, not a requested change.

daira

Blocking on my review tomorrow.

daira · 2022-09-19T11:27:21Z

+    /// minus the value of all outputs.
+    ///
+    /// Useful for balancing a transaction, as the value balance of an individual bundle
+    /// can be non-zero, but a transaction may not have a positive total value balance.  


Suggested change

/// can be non-zero, but a transaction may not have a positive total value balance.

/// can be non-zero. Each bundle's value balance is added to the transparent

/// transaction value pool, which [must not have a negative value]. (If it were

/// negative, the transaction would output more value than it receives in inputs.)

///

/// [must not have a negative value]: https://zips.z.cash/protocol/protocol.pdf#transactions

I forgot to commit this before merging. See #359.

daira · 2022-09-19T12:01:50Z

+            )
+            .fold(Some(ValueSum::zero()), |acc, note_value| acc? + note_value)
+            .ok_or(OverflowError)?;
+        i64::try_from(value_balance).and_then(|i| V::try_from(i).map_err(|_| value::OverflowError))


This is correct because i64 can represent all of $[-$ MAX_MONEY, MAX_MONEY $]$.

daira

utACK; also checked src/value.rs for overflow potential.

… in zcash#352. Signed-off-by: Daira Hopwood <daira@jacaranda.org>

AloeareV mentioned this pull request Aug 24, 2022

Add orchard bundle creation to zcash_primitives::transaction::builder::Builder zcash/librustzcash#583

Closed

str4d mentioned this pull request Sep 15, 2022

Add value_balance to orchard builder #353

Closed

daira reviewed Sep 15, 2022

View reviewed changes

Comment thread src/builder.rs Outdated

str4d reviewed Sep 15, 2022

View reviewed changes

nuttycom reviewed Sep 15, 2022

View reviewed changes

nuttycom requested changes Sep 15, 2022

View reviewed changes

str4d requested changes Sep 15, 2022

View reviewed changes

Comment thread src/builder.rs Outdated

Comment thread src/value.rs Outdated

Comment thread CHANGELOG.md Outdated

add value_balance to builder

8011e0d

str4d approved these changes Sep 15, 2022

View reviewed changes

nuttycom approved these changes Sep 17, 2022

View reviewed changes

sellout reviewed Sep 18, 2022

View reviewed changes

Comment thread src/builder.rs

daira requested changes Sep 19, 2022

View reviewed changes

daira reviewed Sep 19, 2022

View reviewed changes

daira approved these changes Sep 19, 2022

View reviewed changes

daira merged commit f206b3f into zcash:main Sep 19, 2022

daira added a commit to daira/orchard that referenced this pull request Sep 19, 2022

Correct the doc comment for the Builder::value_balance method added…

899a9eb

… in zcash#352. Signed-off-by: Daira Hopwood <daira@jacaranda.org>

daira mentioned this pull request Sep 19, 2022

Correct the doc comment and add a test for Builder::value_balance #359

Merged

daira added a commit to daira/orchard that referenced this pull request Sep 19, 2022

Correct the doc comment for the Builder::value_balance method added…

fbfc8f9

… in zcash#352. Signed-off-by: Daira Hopwood <daira@jacaranda.org>

-    /// can be non-zero, but a transaction may not have a positive total value balance.
+    /// can be non-zero. Each bundle's value balance is added to the transparent
+    /// transaction value pool, which [must not have a negative value]. (If it were
+    /// negative, the transaction would output more value than it receives in inputs.)
+    ///
+    /// [must not have a negative value]: https://zips.z.cash/protocol/protocol.pdf#transactions

Conversation

AloeareV commented Aug 23, 2022

Uh oh!

Uh oh!

str4d commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

str4d Sep 15, 2022

Choose a reason for hiding this comment

Uh oh!

nuttycom Sep 15, 2022

Choose a reason for hiding this comment

Uh oh!

nuttycom left a comment

Choose a reason for hiding this comment

Uh oh!

str4d commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AloeareV commented Sep 15, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AloeareV commented Sep 15, 2022

Uh oh!

str4d commented Sep 15, 2022

Uh oh!

str4d commented Sep 15, 2022

Uh oh!

str4d left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Sep 15, 2022

Codecov Report

Uh oh!

zancas commented Sep 16, 2022

Uh oh!

nuttycom left a comment

Choose a reason for hiding this comment

Uh oh!

sellout left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

daira left a comment

Choose a reason for hiding this comment

Uh oh!

daira Sep 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daira Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

daira Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

daira left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

str4d commented Sep 15, 2022 •

edited

Loading

str4d commented Sep 15, 2022 •

edited

Loading

str4d left a comment •

edited

Loading

daira Sep 19, 2022 •

edited

Loading