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

feat: add sql query to obtain balance #3446

Merged

Conversation

hansieodendaal
Copy link
Contributor

@hansieodendaal hansieodendaal commented Oct 11, 2021

Description

  • Replaced the get_balance function containing multiple sql queries with a single raw sql query.
  • Removed redundant code (fn fetch_pending_outgoing_outputs) as a result of this change.
  • Added more test points for time-locked balance.
  • Note: To properly test transaction validation in async fn test_txo_validation(), access to the backend to obtain pending incoming transactions is needed via fn fetch_pending_incoming_outputs, although that function is not used in production code anymore. If it is also removed other methods will have to be added to the backend to obtain the data for testing. Retaining the current function was chosen in lieu of adding other code.

Motivation and Context

Get balance used a lot of RAM and was really slow due to multiple database interactions.

Comparison of old vs. new query time performance for a wallet with 251,000 UTXOs in the database shown below:

  • Full scale

image

  • Y-axis zoomed in

image

How Has This Been Tested?

  • Unit tests
  • System level tests

Replaced the `get_balance` function containing multiple sql queries with a
single raw sql query.
let balance_query_result = if let Some(val) = tip {
let balance_query = sql_query(
"SELECT coalesce(sum(value), 0) as amount, 'available_balance' as category \
FROM outputs WHERE status = ? \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should take the mined heights into account when the tip is specified

Suggested change
FROM outputs WHERE status = ? \
FROM outputs WHERE status = ? and mined_height < $val and deleted_at_height > $val\

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a job for the validation code though, I think it is cleaner that this function is based purely on the status.

Copy link
Contributor

@philipr-za philipr-za left a comment

Choose a reason for hiding this comment

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

LGTM

let balance_query_result = if let Some(val) = tip {
let balance_query = sql_query(
"SELECT coalesce(sum(value), 0) as amount, 'available_balance' as category \
FROM outputs WHERE status = ? \
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a job for the validation code though, I think it is cleaner that this function is based purely on the status.

@@ -1047,6 +1036,104 @@ impl OutputSql {
.first::<OutputSql>(conn)?)
}

/// Return the available, time locked, pending incoming and pending outgoing balance
pub fn get_balance(tip: Option<u64>, conn: &SqliteConnection) -> Result<Balance, OutputManagerStorageError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

It will be clearer what the tip argument is for if we rename it to something like tip_for_timelock_calculation. Currently it is not clear what the tip is for and that it actually affects whether the timelock balance is returned or not. The comment should also make the behaviour of the function clear when the tip is provided or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will make work these two comments into a new small PR after the merge.

#[sql_type = "diesel::sql_types::Text"]
category: String,
}
let balance_query_result = if let Some(val) = tip {
Copy link
Contributor

Choose a reason for hiding this comment

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

the generic val here makes reading the code difficult below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As above

@aviator-app aviator-app bot merged commit e23ceec into tari-project:development Oct 12, 2021
@hansieodendaal hansieodendaal deleted the ho_get_balance_query branch October 12, 2021 12:35
aviator-app bot pushed a commit that referenced this pull request Oct 12, 2021
Description
---
Fix confusing names in `get_balance` functions

Motivation and Context
---
Comments as per PR #3446

How Has This Been Tested?
---
cargo clippy
cargo formal all
sdbondi added a commit to sdbondi/tari that referenced this pull request Oct 12, 2021
* development:
  fix: fix confusing names in get_balance functions (tari-project#3447)
  feat: add sql query to obtain balance (tari-project#3446)
  fix: u64->i64->u64 conversion; chain split height as u64 (tari-project#3442)
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.

None yet

3 participants