-
Notifications
You must be signed in to change notification settings - Fork 250
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
[#1411] Refactor AccountBalance to use Balance for transparent funds #1570
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1570 +/- ##
==========================================
- Coverage 61.63% 61.63% -0.01%
==========================================
Files 148 148
Lines 18836 18853 +17
==========================================
+ Hits 11610 11620 +10
- Misses 7226 7233 +7 ☔ View full report in Codecov by Sentry. |
zcash_client_backend/src/data_api.rs
Outdated
|
||
/// Adds the specified value to the unshielded total, checking for overflow of | ||
/// the total account balance. | ||
#[deprecated( | ||
note = "this function is deprecated. Please use the `Balance::add_spendable_value` on the unshielded field instead instead." | ||
)] | ||
pub fn add_unshielded_value(&mut self, value: NonNegativeAmount) -> Result<(), BalanceError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is unclear whether we should retain this method at all. It may be better to make a breaking change and delete it. The problem is that we cannot know what the intent was, because previously only the total was tracked. The value to be added might be spendable, pending change, pending non-change, or some combination.
Option 1: assume that the value to be added is pending non-change, and document the assumption.
/// Adds the specified value to the unshielded total, checking for overflow of | |
/// the total account balance. | |
#[deprecated( | |
note = "this function is deprecated. Please use the `Balance::add_spendable_value` on the unshielded field instead instead." | |
)] | |
pub fn add_unshielded_value(&mut self, value: NonNegativeAmount) -> Result<(), BalanceError> { | |
/// Adds the specified value to the pending non-change unshielded balance, | |
/// checking for overflow of the total account balance. | |
/// | |
/// Note: Previously only the *total* unshielded balance was tracked. In this | |
/// implementation, we assume that the intent was to add the value to the | |
/// unshielded balance as pending non-change. This assumption may or may not | |
/// be correct. | |
#[deprecated( | |
note = "this function is deprecated. Please use [`AccountBalance::with_unshielded_balance_mut`] \ | |
with a closure that calls the appropriate `add_*_value` method(s) of [`Balance`] on its \ | |
argument." | |
)] | |
pub fn add_unshielded_value(&mut self, value: NonNegativeAmount) -> Result<(), BalanceError> { |
Option 2: delete this method, which requires a breaking version update.
/// Adds the specified value to the unshielded total, checking for overflow of | |
/// the total account balance. | |
#[deprecated( | |
note = "this function is deprecated. Please use the `Balance::add_spendable_value` on the unshielded field instead instead." | |
)] | |
pub fn add_unshielded_value(&mut self, value: NonNegativeAmount) -> Result<(), BalanceError> { |
Either way the alternatives need to be documented in the release notes (see later suggestions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good either way. I decided to maintain functionality (proved by passing the existing tests) because it's what a non maintainer contributor should do. But I can proceed with the option you folks consider more appropriate.
edit: I'm a big fan of removing code, specially this kind of code that ended up being a "half-baked" solution in the new context of transparent ZEC support in librustzcash. But I understand if maintainers decide is not the right moment for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -383,16 +383,61 @@ pub(crate) fn get_transparent_balances<P: consensus::Parameters>( | |||
pub(crate) fn add_transparent_account_balances( | |||
conn: &rusqlite::Connection, | |||
mempool_height: BlockHeight, | |||
min_confirmations: u32, | |||
account_balances: &mut HashMap<AccountId, AccountBalance>, | |||
) -> Result<(), SqliteClientError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
) -> Result<(), SqliteClientError> { | |
) -> Result<(), SqliteClientError> { | |
// TODO (#issue number): distinguish between pending change and pending non-change. | |
(if this is not intended to be fixed in this PR).
zcash_primitives/CHANGELOG.md
Outdated
- Implemented changes to refactor AccountBalance to use Balance for transparent funds | ||
(see issue #1411). `AccountBalance` now has an `unshielded` value that uses Balance. | ||
The current implementation maintains retrocompatibility with the `unshielded` value | ||
represeted with a `NonNegativeAmount`. There are values that are pending to be | ||
implemented such as change tracking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Implemented changes to refactor AccountBalance to use Balance for transparent funds | |
(see issue #1411). `AccountBalance` now has an `unshielded` value that uses Balance. | |
The current implementation maintains retrocompatibility with the `unshielded` value | |
represeted with a `NonNegativeAmount`. There are values that are pending to be | |
implemented such as change tracking. | |
- Refactored `AccountBalance` to use `Balance` for transparent funds (issue #1411). | |
`AccountBalance` now has an `unshielded_balance()` that uses `Balance`. This does | |
not currently distinguish between pending change and non-change; the pending value | |
is all counted as non-change. |
Is the intention to fix the latter issue in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! I'm figuring out how to do it 😅. Also I will add more tests.
There was a problem hiding this comment.
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.
Thank you for the thorough review! I'll be addressing these comments shortly. |
the appropriate `add_*_value` method(s) of `Balance` on its argument. | ||
Note that the appropriate method(s) depend on whether the funds are | ||
spendable, pending change, or pending non-change (previously, only the | ||
total unshielded value was tracked). | ||
## [0.19.0] - 2024-10-02 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## [0.19.0] - 2024-10-02 | |
## [0.19.0] - 2024-10-02 |
There needs to be a blank line between sections in Markdown.
`AccountBalance` now has an `unshielded_balance()` that uses `Balance`. This does | ||
not currently distinguish between pending change and non-change; the pending value | ||
is all counted as non-change. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(just a nit)
…unds will eventually close zcash#1411
- remove method instead of deprecating it. - Formatting - CHANGELOG entries
3734cda
to
68b9725
Compare
@@ -105,6 +105,22 @@ where | |||
st.wallet().get_transparent_balances(account_id, height_2), | |||
Ok(h) if h.get(taddr) == Some(&value) | |||
); | |||
// TODO: Find our why this does not work. | |||
|
|||
// let unshielded_balance = st.wallet().get_wallet_summary(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will eventually close #1411
I don't know if someone had taken this Issue yet. It's a Ready to work on item according to the DAG.