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

Pagination queries for balances endpoint #2490

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

rafal-ch
Copy link
Contributor

@rafal-ch rafal-ch commented Dec 10, 2024

Closes #2023

TODO:

  • Add integration test due to custom logic related to base asset id
  • Make it work properly with base asset id

Description

This PR enables the paginated queries for balances, but only if the balance indexation is enabled. Otherwise, the attempt to send paginated query will result with Can not use pagination when balances indexation is not available error.

Checklist

  • New behavior is reflected in tests

Before requesting review

  • I have reviewed the code myself

@rafal-ch rafal-ch marked this pull request as ready for review December 11, 2024 10:27
@rafal-ch rafal-ch requested a review from a team December 11, 2024 10:27
@@ -79,8 +79,6 @@ impl BalanceQuery {
Ok(balance)
}

// TODO: This API should be migrated to the indexer for better support and
// discontinued within fuel-core.
#[graphql(complexity = "query_costs().balance_query")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#[graphql(complexity = "query_costs().balance_query")]
#[graphql(complexity = "query_costs().balance_query + query_costs().storage_iterator \
+ (query_costs().storage_read + first.unwrap_or_default() as usize) * child_complexity \
+ (query_costs().storage_read + last.unwrap_or_default() as usize) * child_complexity\")]

We need to limit the number of requested balances=)

Copy link
Collaborator

Choose a reason for hiding this comment

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

It also makes this change breaking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in fd2b019

@@ -233,15 +233,48 @@ impl OffChainDatabase for OffChainIterableKeyValueView {
}
}

fn balances(
&self,
fn balances<'a>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, it is a tricky function because of how we handle balances.

Hmm, do you remember why we decided to skip balances during iteration and add them at the begin?

I remember: if you don't have coins but have messages, they will not appear during the iteration.

The problem with the current solution is when start == base_asset_id. The current code will return an incorrect iterator.

Either we need to handle it differently, or come up with another solution. It would be nice if we could insert base asset balance into the mid of the iterator. Maybe we could change the indexation and make it always add empy indexation for the base asset(like entry in the balances table with 0 amount just for balances)?

Then during iteration we call always fetch its value by using self.base_asset_balance?

Copy link
Contributor Author

@rafal-ch rafal-ch Dec 20, 2024

Choose a reason for hiding this comment

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

These two commits should solve the problem

@rafal-ch rafal-ch marked this pull request as draft December 13, 2024 16:35
@rafal-ch
Copy link
Contributor Author

Converting to draft, some rework is needed here.

@rafal-ch rafal-ch added the breaking A breaking api change label Dec 20, 2024
@rafal-ch rafal-ch marked this pull request as ready for review December 20, 2024 09:28
@rafal-ch rafal-ch requested a review from a team December 20, 2024 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A breaking api change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add pagination support for balances endpoint on GraphQL
2 participants