feat!: implement pagination for `Account` methods #2408

Torres-ssf · 2024-05-28T18:05:17Z

Close: Implement pagination on Wallet for getCoins / getBalances #302

Breaking Changes:

The return of the following methods was modified to support pagination:

getCoins in Provider and Account.
getBalances in Provider and Account.
getMessages in Provider and Account.

fuel-service-user · 2024-06-19T00:02:05Z

✨ A PR has been created under the rc-2408 tag on fuels-wallet repo.
FuelLabs/fuels-wallet#1383

maschad

Looks good, but I am skeptical of the tests that just assert an object is defined, I understand it's necessary to have tests for our docs, and perhaps this pattern will be removed once #2399 lands

maschad · 2024-06-19T20:07:24Z

apps/docs-snippets/src/guide/provider/pagination.test.ts

+    };
+    // #endregion pagination-2
+
+    expect(paginationArgs).toBeDefined();


I see this pattern throughout the pagination test suite but I'm not sure what the benefit of these assertions are, given that an initialized object will always be defined.

In the context of them being passed to provider.getCoins() I think an assertion that the pageInfo returned is the logical consequence of a particular paginationArgs suffices, but otherwise just asserting that a GqlPageInfo object or CursorPaginationArgs is defined doesn't offer any value imo.

@maschad The tests within the doc-snippets dir exist to be used as code snippets only. We should not rely on those tests to validate our implementation

The changes made within this PR are being tested here and here

I understand that, I elaborated a more in my final review comment , but I am suggesting that these snippet regions be incorporated into a larger test such as the ones you linked, rather than having an individual test just for the initialization of an object.

maschad · 2024-06-19T20:30:30Z

apps/docs-snippets/src/guide/provider/querying-the-chain.test.ts

-    // get all coins
-    const coins = await wallet.getCoins();
+    // fetches up to 100 coins from all assets
+    await provider.getCoins(wallet.address);


Given coins and pageInfo are being initialized in line23, how are these results being tested?

#2408 (comment)

maschad · 2024-06-19T20:31:09Z

apps/docs-snippets/src/guide/provider/querying-the-chain.test.ts

+    // #endregion get-spendable-resources-1
+
+    // #region get-spendable-resources-2
+    await wallet.getResourcesToSpend(spendableResources, excludedIds);


Should we perform an assertion on the value returned here?

#2408 (comment)

arboleya · 2024-06-20T11:00:46Z

apps/docs/src/guide/provider/pagination.md

+
+## Default Behavior
+
+If neither `assetId` nor `paginationArgs` are provided, the `getCoins` method will default to the base asset ID and return the first 100 items:


Where did this 100 come from?

Do we protect against huge numbers? (i.e. a user passes 1000000000)

cc @xgreenx

github-actions · 2024-06-20T15:32:36Z

Coverage Report:

Lines	Branches	Functions	Statements
80.9%(-0.06%)	72.45%(+0.03%)	78.27%(-0.03%)	80.98%(-0.06%)

Changed Files:

Ok	File (✨=New File)	Lines	Branches	Functions	Statements
🔴	packages/account/src/account.ts	81.15% (-3.09%)	63.93% (-3.23%)	81.81% (+0%)	80.85% (-3.07%)
🔴	packages/account/src/providers/provider.ts	68.01% (+0.44%)	60.46% (+2.18%)	73.8% (-0.61%)	67.97% (+0.16%)

arboleya

Another small detail about the PR description, can we also have before/after code snippets for the breaking changes? It can be helpful to let people know what needs to be updated without having to dig into the code.

arboleya · 2024-06-20T16:48:57Z

.fuel-core/configs/stateConfig.json

+      "nonce": "0x03d7ad03b6a14262fc7efc59a9b95afb43af702f4becdaba866364b438b0ebd3",
+      "amount": 18446744073709551615,
+      "data": "",
+      "da_height": 0


Are these message additions still mimicking the ones from fuel-core?

I suppose you added these in for testing purposes. If that's the case, could you rewrite your tests to use launchTestNode and the TestMessage utility (example)?

petertonysmith94

Couple comment 🥇

petertonysmith94 · 2024-06-21T06:03:59Z

packages/account/src/providers/provider.ts

+      /**
+       * The query parameters for this method were designed to support pagination,
+       * but the current Fuel-Core implementation does not support pagination yet.
+       */


I would assume that fuel-core will implement support for pagination in the future?

With that in mind, we could keep the CursorPaginationArgs and return a constants pageInfo object. This could reduce the breaking changes down the line.

petertonysmith94 · 2024-06-21T06:06:39Z

packages/account/src/providers/provider.ts

@@ -1392,7 +1411,7 @@ Supported fuel-core version: ${supportedVersion}.`
   * @param params - The parameters to query blocks.
   * @returns A promise that resolves to the blocks.
   */
-  async getBlocks(params: GqlGetBlocksQueryVariables): Promise<Block[]> {
+  async getBlocks(params: CursorPaginationArgs): Promise<Block[]> {


Do we want to return the PageInfo from this method?

petertonysmith94 · 2024-06-21T06:21:29Z

apps/docs/src/guide/provider/pagination.md

@@ -0,0 +1,65 @@
+# Pagination
+
+Pagination is highly efficient when dealing with large sets of data. Because of this some methods from the `Provider` class support GraphQL cursor pagination, allowing you to efficiently navigate through data chunks.


Suggested change

Pagination is highly efficient when dealing with large sets of data. Because of this some methods from the `Provider` class support GraphQL cursor pagination, allowing you to efficiently navigate through data chunks.

Pagination is highly efficient when dealing with large sets of data. Because of this, some methods from the `Provider` class support [GraphQL cursor pagination](https://graphql.org/learn/pagination/), allowing you to efficiently navigate through data chunks.

May be worth linking to the pagination section from GraphQL :)

Or potentially could go in a "Learn more" section?

petertonysmith94 · 2024-06-21T06:25:10Z

apps/docs/src/guide/provider/pagination.md

@@ -0,0 +1,65 @@
+# Pagination


May be worth adding an example of iterating over a paginated resource?

petertonysmith94 · 2024-06-21T06:33:17Z

apps/docs-snippets/src/guide/provider/querying-the-chain.test.ts


    expect(message).toBeDefined();
    expect(message?.nonce).toEqual(nonce);
-    // #endregion getMessageByNonce
+    // #endregion get-message-by-nonce-1


Unable to suggest, however, will suggest removing the expects from the snippet :)

Torres-ssf added 7 commits May 28, 2024 14:56

add pageInfo to return of getCoins and getMessages

defe281

refact getCoins on Provider class

718055b

adjusting getCoins on Account class

21364ce

refact getMessages on Provider class

638481a

adjusting getMessages on Account class

f0f3411

fixing test suites

57e2f0a

add messages to state config

abf3b71

Torres-ssf added the chore Issue is a chore label May 28, 2024

Torres-ssf self-assigned this May 28, 2024

Torres-ssf and others added 11 commits May 28, 2024 17:45

Merge branch 'master' into st/feat/implement-pagination-get-coins

b6b34ca

add pageInfo to return of getBalances

f718939

refact provider getBalances

4850e28

adjusting tests

94636b5

add changeset

3847ec2

linting

5368078

Merge branch 'master' into st/feat/implement-pagination-get-coins

e4ae168

Merge branch 'master' into st/feat/implement-pagination-get-coins

4c8c261

Merge branch 'master' into st/feat/implement-pagination-get-coins

869d410

add import

9b3c91d

Merge branch 'master' into st/feat/implement-pagination-get-coins

9a3840c

arboleya added this to the 0.x post-launch milestone Jun 12, 2024

Torres-ssf added 9 commits June 13, 2024 08:47

Merge branch 'master' into st/feat/implement-pagination-get-coins

1e79dde

Merge branch 'master' into st/feat/implement-pagination-get-coins

2dddd38

Merge branch 'master' into st/feat/implement-pagination-get-coins

c159171

Merge branch 'master' into st/feat/implement-pagination-get-coins

44b5ef0

add new doc page pagination

c66ba2c

set up pagination page at provider section

3dce4b4

update param for getBlocks

7be96e5

refact querying the chain doc page

e1bf612

fix links

c8ad228

Torres-ssf added 2 commits June 18, 2024 19:42

add nonces to spell check words

aa0f6c7

fix test case

830fa46

Torres-ssf marked this pull request as ready for review June 18, 2024 23:56

Torres-ssf requested review from digorithm, arboleya, Dhaiwat10, danielbate, nedsalk, petertonysmith94 and maschad as code owners June 18, 2024 23:56

This comment was marked as resolved.

Sign in to view

Merge branch 'master' into st/feat/implement-pagination-get-coins

b2238b0

github-actions bot added the feat Issue is a feature label Jun 19, 2024

Merge branch 'master' into st/feat/implement-pagination-get-coins

7837d8e

maschad requested changes Jun 19, 2024

View reviewed changes

arboleya reviewed Jun 20, 2024

View reviewed changes

Torres-ssf added 8 commits June 20, 2024 09:48

add tests to account test suite

3da88b2

improve tests on provider test suite

e45e3ef

remove pagination from getBalances

8caf981

edit code comment

b905970

fix doc page

6a947b4

set limit for paginated items

435c7c8

Merge branch 'master' into st/feat/implement-pagination-get-coins

8d849a5

Merge branch 'master' into st/feat/implement-pagination-get-coins

1c88a6b

arboleya reviewed Jun 20, 2024

View reviewed changes

petertonysmith94 reviewed Jun 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: implement pagination for `Account` methods #2408

feat!: implement pagination for `Account` methods #2408

Torres-ssf commented May 28, 2024 •

edited

fuel-service-user commented Jun 19, 2024 •

edited

This comment was marked as resolved.

maschad left a comment

maschad Jun 19, 2024

Torres-ssf Jun 20, 2024

maschad Jun 21, 2024

maschad Jun 19, 2024

Torres-ssf Jun 20, 2024

maschad Jun 19, 2024

Torres-ssf Jun 20, 2024

arboleya Jun 20, 2024

arboleya Jun 20, 2024

github-actions bot commented Jun 20, 2024

arboleya left a comment

arboleya Jun 20, 2024

nedsalk Jun 20, 2024 •

edited

petertonysmith94 left a comment

petertonysmith94 Jun 21, 2024

petertonysmith94 Jun 21, 2024

petertonysmith94 Jun 21, 2024

petertonysmith94 Jun 21, 2024

petertonysmith94 Jun 21, 2024

petertonysmith94 Jun 21, 2024


		## Default Behavior

		If neither `assetId` nor `paginationArgs` are provided, the `getCoins` method will default to the base asset ID and return the first 100 items:

		@@ -0,0 +1,65 @@
		# Pagination

		Pagination is highly efficient when dealing with large sets of data. Because of this some methods from the `Provider` class support GraphQL cursor pagination, allowing you to efficiently navigate through data chunks.

feat!: implement pagination for Account methods #2408

Are you sure you want to change the base?

feat!: implement pagination for Account methods #2408

Conversation

Torres-ssf commented May 28, 2024 • edited

Breaking Changes:

fuel-service-user commented Jun 19, 2024 • edited

This comment was marked as resolved.

maschad 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

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

github-actions bot commented Jun 20, 2024

arboleya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nedsalk Jun 20, 2024 • edited

Choose a reason for hiding this comment

petertonysmith94 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

Choose a reason for hiding this comment

feat!: implement pagination for `Account` methods #2408

feat!: implement pagination for `Account` methods #2408

Torres-ssf commented May 28, 2024 •

edited

fuel-service-user commented Jun 19, 2024 •

edited

nedsalk Jun 20, 2024 •

edited