Pagination queries for `balances` endpoint #2490

rafal-ch · 2024-12-10T11:29:03Z

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

…tion_for_balances

xgreenx · 2024-12-13T00:15:27Z

crates/fuel-core/src/schema/balance.rs

@@ -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")]


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=)

It also makes this change breaking

Updated in fd2b019

xgreenx · 2024-12-13T00:33:56Z

crates/fuel-core/src/service/adapters/graphql_api/off_chain.rs

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

-    fn balances(
-        &self,
+    fn balances<'a>(


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?

These two commits should solve the problem

…tion_for_balances

rafal-ch · 2024-12-13T16:37:21Z

Converting to draft, some rework is needed here.

…tion_for_balances

xgreenx · 2025-01-02T23:58:36Z

crates/fuel-core/src/service/adapters/graphql_api/off_chain.rs

+                    .into_boxed()
+            }
+            (IterDirection::Forward, Some(asset_id)) => {
+                let start = (asset_id != *base_asset_id)


If asset_id == base_asset_id, you will skip base asset while we should return it, since it is starting entry=)

Ok, I was wondering why it actually works... :)

It turned out that even if we return the bass asset id here, it'll be skipped by the logic inside query_pagination here. Internally, it kinda changes the notion of start to actually mean after.

So now the updated behavior always includes the start entry in the output, even though it'll be filtered by the query. While it won't change the returned balances, it is now consistent with other pagination queries. This consistency might be actually important because the query code makes some further calculations about has_next_page and other flags.

…tion_for_balances

AurelienFT

Very good usage of rust syntax and bricks. Liked it :)

AurelienFT · 2025-01-10T11:05:40Z

crates/fuel-core/src/schema/balance.rs

@@ -79,9 +79,11 @@ impl BalanceQuery {
        Ok(balance)
    }

-    // TODO: This API should be migrated to the indexer for better support and


This is not something we want to do in the future anymore ? In my mind it's still something we want to do in the future to decouple both use cases of the node.

AurelienFT · 2025-01-10T11:11:42Z

crates/fuel-core/src/schema/balance.rs

-    #[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 \


The cost computation is hard to understand for me, maybe it's normal but it might requires a comment. What I don't understand :

Why are we making pay for first * child_complexity + last * child_complexity. Example : (4 * 10) + (9 * 10) = 130. Inner question : Why are we paying if the first is far away and why are we paying again for all the numbers from 0 to last some where already included in the first computation.

What is logic to me is two solutions :

Either we want people to pay for numbers before their first and so it would be :

(query_costs().storage_read + last.unwrap_or_default() as usize) * child_complexity

(because last is higher and include all the precents numbers)

Either we just want to pay for the interval :

(query_costs().storage_read + (last.unwrap_or_default() as usize - first.unwrap_or_default() as usize)) * child_complexity

AurelienFT · 2025-01-10T11:14:39Z

crates/fuel-core/src/service/adapters/graphql_api/off_chain.rs

+                    let start = (asset_id != *base_asset_id)
+                        .then_some(CoinBalancesKey::new(owner, &asset_id));


Why are we redoing the comparaison here as we are already in the else

Suggested change

let start = (asset_id != *base_asset_id)

.then_some(CoinBalancesKey::new(owner, &asset_id));

let start = Some(CoinBalancesKey::new(owner, &asset_id));

AurelienFT · 2025-01-10T11:17:05Z

crates/fuel-core/src/service/adapters/graphql_api/off_chain.rs

+                self.non_base_assets_first(start, owner, base_asset_id, direction)
+            }
+            (IterDirection::Reverse, Some(asset_id)) => {
+                if asset_id == *base_asset_id {


You can leverage pattern matching as you did in non_base_asset_balances don't really know if it's better or not. up to you :)

AurelienFT · 2025-01-10T11:18:09Z

crates/fuel-core/src/service/adapters/graphql_api/off_chain.rs

+            start.as_ref(),
+            Some(direction),
+        )
+        .filter_map(move |result| match result {


It would be valuable in the future to have a comment about this behavior because reading the comments on the review helped me but it's not really future proof and without it I wouldn't understand why None on base asset etc..

rafal-ch added 19 commits December 3, 2024 15:08

Unlock pagination for balances() query

d4a6596

Merge remote-tracking branch 'upstream/master' into rafal/2032_pagina…

4e0450b

…tion_for_balances

Fix typos

aa7ee75

Remove TODO since indexer is no more

449775d

Support pagination queries for balances endpoint

b2fe429

Update PR number in the changelog

4131855

Fix formatting

2dd9aef

Add test for balances pagination

05f6851

Clean up pagination test

5b010e4

Add balances_reversed_pagination_without_base_asset() test

17a7ef4

Include message coins in balance pagination tests

73f50c0

Balances pagination supports base asset id

c172d1b

Full coverage of balances pagination in integration tests

fdd2c11

Simplify balance pagination tests

6dbc7a6

Dodge a double-test glitch in test_case

32d680f

Make balances test more clear

088c50b

Test paginated balances using various chunk sizes

695f99a

Simplify the implementation of balances query

f109d1b

Update changelog

27970eb

rafal-ch marked this pull request as ready for review December 11, 2024 10:27

rafal-ch requested review from xgreenx, Dentosal and MitchTurner as code owners December 11, 2024 10:27

rafal-ch requested a review from a team December 11, 2024 10:27

Add assert for the returned chunk size

dbc32f3

xgreenx reviewed Dec 13, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/master' into rafal/2032_pagina…

f866c54

…tion_for_balances

rafal-ch marked this pull request as draft December 13, 2024 16:35

Fix the query for the base asset id

b1bfb20

rafal-ch added 6 commits December 13, 2024 18:54

Fix formatting

99e7b65

Merge remote-tracking branch 'upstream/master' into rafal/2032_pagina…

d7c3b02

…tion_for_balances

Remove not needed batch of tests

037351d

Ensure no balances are returned after the last page

3ae5222

Rename non_base_asset_balance() to non_base_asset_balances()

af64206

Update complexity for balances query

fd2b019

rafal-ch added the breaking A breaking api change label Dec 20, 2024

rafal-ch marked this pull request as ready for review December 20, 2024 09:28

rafal-ch requested a review from a team December 20, 2024 09:28

Merge remote-tracking branch 'upstream/master' into rafal/2032_pagina…

9c8ce1e

…tion_for_balances

xgreenx reviewed Jan 2, 2025

View reviewed changes

Merge remote-tracking branch 'upstream/master' into rafal/2032_pagina…

0c92b6a

…tion_for_balances

rafal-ch requested a review from rymnc as a code owner January 7, 2025 10:09

Fix the pagination start point

a943e2b

rafal-ch added the release label Jan 10, 2025

AurelienFT reviewed Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pagination queries for `balances` endpoint #2490

Pagination queries for `balances` endpoint #2490

rafal-ch commented Dec 10, 2024 •

edited

Loading

xgreenx Dec 13, 2024

xgreenx Dec 13, 2024

rafal-ch Dec 20, 2024

xgreenx Dec 13, 2024

rafal-ch Dec 20, 2024 •

edited

Loading

rafal-ch commented Dec 13, 2024

xgreenx Jan 2, 2025

rafal-ch Jan 7, 2025

AurelienFT left a comment

AurelienFT Jan 10, 2025

AurelienFT Jan 10, 2025

AurelienFT Jan 10, 2025

AurelienFT Jan 10, 2025

AurelienFT Jan 10, 2025

		let start = (asset_id != *base_asset_id)
		.then_some(CoinBalancesKey::new(owner, &asset_id));

	let start = (asset_id != *base_asset_id)
	.then_some(CoinBalancesKey::new(owner, &asset_id));
	let start = Some(CoinBalancesKey::new(owner, &asset_id));

Pagination queries for balances endpoint #2490

Are you sure you want to change the base?

Pagination queries for balances endpoint #2490

Conversation

rafal-ch commented Dec 10, 2024 • edited Loading

TODO:

Description

Checklist

Before requesting review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafal-ch Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

rafal-ch commented Dec 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AurelienFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pagination queries for `balances` endpoint #2490

Pagination queries for `balances` endpoint #2490

rafal-ch commented Dec 10, 2024 •

edited

Loading

rafal-ch Dec 20, 2024 •

edited

Loading