Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[#1411] Refactor AccountBalance to use Balance for transparent funds #1570

Merged
merged 3 commits into from
Dec 7, 2024
Merged

[#1411] Refactor AccountBalance to use Balance for transparent funds #1570

merged 3 commits into from
Dec 7, 2024

Conversation

pacu
Copy link
Contributor

@pacu pacu commented Oct 13, 2024 •
edited by daira
Loading

fixes #1411

@codecov Codecov
Copy link

codecov bot commented Oct 17, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 94.89796% with 5 lines in your changes missing coverage. Please review.

Project coverage is 56.73%. Comparing base (c33ad67) to head (bd45812).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
...client_backend/src/data_api/testing/transparent.rs 97.46% 2 Missing ⚠️
zcash_client_sqlite/src/wallet/transparent.rs 50.00% 2 Missing ⚠️
zcash_client_sqlite/src/wallet.rs 50.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1570      +/-   ##
==========================================
+ Coverage   56.39%   56.73%   +0.34%     
==========================================
  Files         149      149              
  Lines       19466    19540      +74     
==========================================
+ Hits        10977    11086     +109     
+ Misses       8489     8454      -35

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

daira
daira requested changes Oct 17, 2024
View reviewed changes
Copy link
Contributor

@daira daira left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Changes required.

Note that tests passed despite the bug of not checking the overflow invariant in AccountBalance::add_unshielded_value. The suggested replacement code obviously does maintain the invariant because it only uses other public methods, but still we might want more test coverage here.

@pacu
Copy link
Contributor Author

pacu commented Oct 18, 2024

Thank you for the thorough review! I'll be addressing these comments shortly.

@pacu pacu force-pushed the feature/transparent-balance branch from 3734cda to 68b9725 Compare October 22, 2024 22:25
@pacu pacu force-pushed the feature/transparent-balance branch from d49ebd3 to f706b33 Compare October 24, 2024 22:30
@pacu pacu marked this pull request as ready for review October 24, 2024 22:30
@pacu pacu requested a review from daira October 24, 2024 22:30
daira
daira requested changes Oct 25, 2024
View reviewed changes
Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

Changes needed.

@pacu pacu force-pushed the feature/transparent-balance branch from f706b33 to da30040 Compare October 26, 2024 00:01
@daira daira mentioned this pull request Oct 26, 2024
Closed
daira
daira reviewed Oct 26, 2024
View reviewed changes
Copy link
Contributor

@daira daira left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The changes to AccountBalance should be documented in the CHANGELOG for zcash_client_backend, not zcash_primitives. (This was fixed.)

Also, the fix to zcash_client_sqlite's implementation of get_wallet_summary, along with its remaining limitations, should be documented in the CHANGELOG for zcash_client_sqlite: (This was also fixed.)

### Fixed
- `zcash_client_sqlite::WalletDb`'s implementation of
  `zcash_client_backend::data_api::WalletRead::get_wallet_summary` has been
  fixed to take account of `min_confirmations` for transparent balances.
  Note that this implementation treats `min_confirmations == 0` the same
  as `min_confirmations == 1` for both shielded and transparent TXOs.
  It also does not currently distinguish between pending change and
  non-change; the pending value is all counted as non-change (issue
  [#1592](https://github.com/zcash/librustzcash/issues/1592)).

daira
daira previously approved these changes Nov 5, 2024
View reviewed changes
Copy link
Contributor

@daira daira left a comment •
edited
Loading

Choose a reason for hiding this comment

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

utACK. (The complaint about redundancy in the test code is non-blocking.)

@pacu pacu requested review from nuttycom and daira November 5, 2024 02:16
@pacu
Copy link
Contributor Author

pacu commented Dec 4, 2024

oh no! a wild conflicting commit appeared!

@pacu pacu force-pushed the feature/transparent-balance branch from 0bf347e to f6c36fc Compare December 4, 2024 22:32
@pacu
[zcash#1411] Refactor AccountBalance to use Balance for transparent f…
bc63767 
…unds

closes zcash#1411

- Adds min_confirmations to transparent account balances
- Adds a new `transparent_balance_spendability` test to verify
transparent funds spendability.
- cargo clippy + cargo fmt

Refactor: Extract Method on lambda `check_balance`

Fix transparent_balance_spendability tests and `check_balance` to
properly verify `min_confirmations` assumptions

Fix comment typo

Improve test documentation

PR Suggestions

cargo fmt and cargo clippy
@pacu pacu force-pushed the feature/transparent-balance branch from f6c36fc to bc63767 Compare December 4, 2024 23:27
@pacu
Copy link
Contributor Author

pacu commented Dec 4, 2024

Squashed commits and rebased to main afterwards.

daira
daira previously approved these changes Dec 5, 2024
View reviewed changes
Copy link
Contributor

@daira daira left a comment •
edited
Loading

Choose a reason for hiding this comment

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

utACK with minor non-blocking suggestion.

daira
daira previously approved these changes Dec 5, 2024
View reviewed changes
Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

utACK (+ doc-only self-ACK for 19dedb7)

@daira daira enabled auto-merge December 5, 2024 12:15
@daira daira added this pull request to the merge queue Dec 5, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 5, 2024
@daira daira force-pushed the feature/transparent-balance branch from 886f72f to 3f80212 Compare December 7, 2024 15:55
daira
daira previously approved these changes Dec 7, 2024
View reviewed changes
Copy link
Contributor

@daira daira left a comment •
edited
Loading

Choose a reason for hiding this comment

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

utACK bc63767 + doc-only self-ACK for 3f80212.
Changes since last ACK for 19dedb7 are only to the zcash_client_backend changelog.

@daira
Fix intra-doc links.
bd45812 
Signed-off-by: Daira-Emma Hopwood <[email protected]>
daira
daira approved these changes Dec 7, 2024
View reviewed changes
Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

doc-only self-ACK for bd45812

@daira daira merged commit 3725313 into zcash:main Dec 7, 2024
29 checks passed
@daira daira mentioned this pull request Dec 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daira daira daira approved these changes

@nuttycom nuttycom Awaiting requested review from nuttycom

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor AccountBalance to use Balance for transparent funds
3 participants
@pacu @nuttycom @daira