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.

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

zcash_client_sqlite: Start documenting the table structures #1429

Merged
merged 4 commits into from
Jun 18, 2024
Merged

Conversation

str4d
Copy link
Contributor

@str4d str4d commented Jun 18, 2024

The existing pins for the current database structure are moved out of the verify_schema test, so they can serve the dual purpose of documentating the database structure.

The documentation needs significantly more detail than this PR currently includes; this is intended as a starting point from which we can continually improve our documentation going forward.

@str4d str4d added A-documentation A-wallet Area: light wallet backend. labels Jun 18, 2024
Copy link

codecov bot commented Jun 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.03%. Comparing base (87e2308) to head (cf1922c).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1429      +/-   ##
==========================================
+ Coverage   63.01%   63.03%   +0.02%     
==========================================
  Files         126      127       +1     
  Lines       14772    14784      +12     
==========================================
+ Hits         9308     9319      +11     
- Misses       5464     5465       +1     

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

Copy link
Contributor

@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.

Added suggestions for documentation to replace TODOs.

zcash_client_sqlite/src/wallet/db.rs Outdated Show resolved Hide resolved
zcash_client_sqlite/src/wallet/db.rs Outdated Show resolved Hide resolved
zcash_client_sqlite/src/wallet/db.rs Outdated Show resolved Hide resolved
///
/// - The `cached_transparent_receiver_address` column contains the transparent receiver
/// component of the UA. It is cached directly in the table to make account lookups for
/// transparent outputs more efficient, enabling joins to [`TABLE_UTXOS`].
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll need to figure out what to do here with respect to transparent addresses that are not associated with the seed, in order to support the import of zcashd wallets. Also this might be changing with the addition of ZIP 320? We need to end up being consistent about how we handle transparent addresses; it might ultimately make more sense to break those out into a separate table that has a nullable foreign key relationship to this one or something.

zcash_client_sqlite/src/wallet/db.rs Outdated Show resolved Hide resolved
zcash_client_sqlite/src/wallet/db.rs Outdated Show resolved Hide resolved
zcash_client_sqlite/src/wallet/db.rs Outdated Show resolved Hide resolved
Co-authored-by: Kris Nuttycombe <kris@nutty.land>
Copy link
Contributor

@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 cf1922c

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. So much better! <3

@str4d str4d merged commit dd3a557 into main Jun 18, 2024
28 checks passed
@str4d str4d deleted the zcs-db-docs branch June 18, 2024 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documentation A-wallet Area: light wallet backend.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants