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: Add unstable conversions between AccountId and u32. #1557

Merged
merged 1 commit into from
Oct 7, 2024

Conversation

nuttycom
Copy link
Contributor

@nuttycom nuttycom commented Oct 3, 2024

This is necessary for third parties using the zcash_client_sqlite crate directly to be able to refer to individual account IDs without having to always use FVKs to look them up.

Copy link

codecov bot commented Oct 3, 2024

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 61.62%. Comparing base (5eebc0a) to head (cb349f8).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
zcash_client_sqlite/src/lib.rs 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1557      +/-   ##
==========================================
- Coverage   61.63%   61.62%   -0.02%     
==========================================
  Files         148      148              
  Lines       18821    18825       +4     
==========================================
  Hits        11601    11601              
- Misses       7220     7224       +4     

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

@nuttycom nuttycom force-pushed the add_accountid_conversions branch 3 times, most recently from ae012b3 to 58edf9c Compare October 5, 2024 03:08
… `u32`

This is necessary for third parties using the `zcash_client_sqlite`
crate directly to be able to refer to individual account IDs without
having to always use FVKs to look them up.
/// Constructs an `AccountId` from a bare `u32` value. The resulting identifier is not
/// guaranteed to correspond to any account stored in the database.
#[cfg(feature = "unstable")]
pub fn from_u32(value: u32) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be unsafe?

Suggested change
pub fn from_u32(value: u32) -> Self {
pub unsafe fn from_u32(value: u32) -> Self {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't unsafe, by any meaning of the term?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's up to the caller to maintain the intended invariant that AccountIds are only constructed for accounts stored in the database, which isn't checked.

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

@nuttycom nuttycom closed this Oct 7, 2024
@nuttycom nuttycom reopened this Oct 7, 2024
@nuttycom nuttycom merged commit dec3024 into main Oct 7, 2024
26 of 30 checks passed
@nuttycom nuttycom deleted the add_accountid_conversions branch October 7, 2024 02:38
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.

2 participants