Skip to content

zcash_client_backend: Generalize & extend wallet metadata query API#1580

Merged
nuttycom merged 2 commits into
zcash:mainfrom
nuttycom:wallet/multi_output_dynamic_min_split
Nov 13, 2024
Merged

zcash_client_backend: Generalize & extend wallet metadata query API#1580
nuttycom merged 2 commits into
zcash:mainfrom
nuttycom:wallet/multi_output_dynamic_min_split

Conversation

@nuttycom
Copy link
Copy Markdown
Collaborator

No description provided.

@nuttycom nuttycom force-pushed the wallet/multi_output_dynamic_min_split branch 3 times, most recently from 98093e1 to 9ca4f50 Compare October 21, 2024 15:52
@nuttycom nuttycom force-pushed the wallet/multi_output_dynamic_min_split branch 2 times, most recently from cb12eb7 to 3fe1cdb Compare October 25, 2024 15:50
@nuttycom nuttycom marked this pull request as ready for review October 25, 2024 21:45
str4d

This comment was marked as outdated.

@nuttycom nuttycom dismissed str4d’s stale review October 25, 2024 22:29

This was a test.

@nuttycom nuttycom marked this pull request as draft October 26, 2024 22:53
@nuttycom nuttycom force-pushed the wallet/multi_output_dynamic_min_split branch 2 times, most recently from 37f993e to 600b5e6 Compare November 1, 2024 17:48
@nuttycom nuttycom changed the title Wallet/multi output dynamic min split zcash_client_backend: Generalize & extend wallet metadata query API Nov 1, 2024
@nuttycom nuttycom marked this pull request as ready for review November 1, 2024 17:49
@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 1, 2024

Codecov Report

Attention: Patch coverage is 69.68504% with 77 lines in your changes missing coverage. Please review.

Project coverage is 56.38%. Comparing base (8b49ca8) to head (00cafa3).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
zcash_client_backend/src/data_api/testing/pool.rs 62.50% 21 Missing ⚠️
zcash_client_backend/src/data_api.rs 60.00% 20 Missing ⚠️
zcash_client_sqlite/src/wallet/common.rs 77.92% 17 Missing ⚠️
zcash_client_backend/src/fees.rs 66.66% 9 Missing ⚠️
zcash_client_sqlite/src/lib.rs 33.33% 6 Missing ⚠️
zcash_client_backend/src/fees/common.rs 71.42% 2 Missing ⚠️
zcash_client_backend/src/data_api/testing.rs 94.44% 1 Missing ⚠️
zcash_client_sqlite/src/error.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1580      +/-   ##
==========================================
+ Coverage   56.30%   56.38%   +0.07%     
==========================================
  Files         148      148              
  Lines       19062    19232     +170     
==========================================
+ Hits        10733    10844     +111     
- Misses       8329     8388      +59     

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

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.

Reviewed 530d06b

Comment thread components/zcash_protocol/src/value.rs
Comment thread components/zcash_protocol/src/value.rs
Comment thread zcash_client_backend/src/data_api.rs
Comment thread zcash_client_backend/src/data_api.rs Outdated
Comment thread zcash_client_backend/src/data_api.rs
Comment thread zcash_client_sqlite/src/wallet/common.rs
Comment thread zcash_client_backend/src/data_api.rs
Comment thread zcash_client_backend/src/data_api.rs Outdated
Comment thread zcash_client_backend/src/data_api.rs Outdated
Comment thread zcash_client_sqlite/src/wallet/common.rs Outdated
This generalizes the previous account metadata query API to be able to
represent more complex queries, and also to return note totals in
addition to note counts.
@nuttycom nuttycom force-pushed the wallet/multi_output_dynamic_min_split branch from 7dab03a to 3352671 Compare November 13, 2024 20:22
@nuttycom
Copy link
Copy Markdown
Collaborator Author

force-pushed to address comments from code review.

str4d
str4d previously approved these changes Nov 13, 2024
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 3352671 after minor comments, Clippy lints, and rustfmt errors are fixed.

Comment thread zcash_client_backend/src/fees.rs Outdated
Comment thread zcash_client_sqlite/CHANGELOG.md Outdated
Comment thread zcash_client_sqlite/src/error.rs Outdated
Comment thread zcash_client_sqlite/src/wallet/common.rs Outdated
Comment thread zcash_client_sqlite/src/wallet/common.rs Outdated
Comment thread components/zcash_protocol/src/value.rs Outdated
Comment thread components/zcash_protocol/src/value.rs Outdated
@nuttycom nuttycom force-pushed the wallet/multi_output_dynamic_min_split branch from b87558f to 01b0d1c Compare November 13, 2024 20:58
@nuttycom nuttycom requested review from daira and str4d November 13, 2024 21:00
Co-authored by: Jack Grig <jack@electriccoin.co>
Co-authored-by: Daira-Emma Hopwood <daira@jacaranda.org>
@nuttycom nuttycom force-pushed the wallet/multi_output_dynamic_min_split branch from 01b0d1c to 00cafa3 Compare November 13, 2024 21:19
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.

Re-utACK 00cafa3

@nuttycom nuttycom added this pull request to the merge queue Nov 13, 2024
Merged via the queue into zcash:main with commit be4bc23 Nov 13, 2024
@nuttycom nuttycom deleted the wallet/multi_output_dynamic_min_split branch November 13, 2024 23:25
{
// Creates a transaction that sends the specified value from the given account to
// the provided recipient address, using a greedy input selector and the default
// mutli-output change strategy.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo


// Spend half of each one of our notes, so that we can get a distribution of sent note values.
// FIXME: This test is currently excessively specialized to the `zcash_client_sqlite::WalletDb`
// implmentation of the `InputSource` trait. A better approach would be to create a test input
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo

Copy link
Copy Markdown
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.

Post-hoc ACK. There are a couple of typos in comments.

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