Add new indexer API to getAccountAllTransactionVersions and use it in getAccountTransactions #364

0xaptosj · 2024-04-15T01:10:25Z

Description

Add getAllAccountTransactionVersions which returns versions of all transaction versions related to the given address.
Add getAllAccountTransactions which returns all transactions related to given address instead of only ones sent by address. This makes the result has same length as calling getAccountTransactionsCount.

This is a solution to #352. There doesn't exist an indexer API that returns the aggregated result. So I have to first call an indexer API to get all user txs' versions, then call full node API to get tx detail on each of them.

Test Plan

Updated unit test that lets user 1 transfer APT to user 2 then user 2 transfer APT back to user 1. Calling getAccountTransactions on both users should all return 2 txs.

Related Links

0xaptosj · 2024-04-15T01:46:42Z

src/types/generated/operations.ts

@@ -55,7 +55,6 @@ export type CurrentTokenOwnershipFieldsFragment = {
    token_properties: any;
    token_standard: string;
    token_uri: string;
-    decimals: any;


this is the result of running indexer-codegen

This is a breaking change - have we talked with Indexer team on why they removed it?

0xmaayan

I am not sure about this solution. This can increase the dapp server requests and can potentially lead to rate limit or just worst performance (like the Explorer that suffers from a bad performance - site load, etc)

I think we should push for an Indexer solution @bowenyang007

0xmaayan · 2024-04-15T11:52:40Z

CHANGELOG.md

@@ -21,6 +21,8 @@ All notable changes to the Aptos TypeScript SDK will be captured in this file. T
 - [`Breaking`] Change any generate transaction function to return `SimpleTransaction` or `MultiAgentTransaction` instance
 - Adds `getUserTransactionHash` which can generate a transaction hash after signing, but before submission
 - Add function to create resource address locally
+- Add `getAccountAllTransactionVersions` which returns versions of all transactions related to the given address.


These 2 should be under a new dated release version

0xmaayan · 2024-04-15T11:54:02Z

src/types/generated/operations.ts

@@ -55,7 +55,6 @@ export type CurrentTokenOwnershipFieldsFragment = {
    token_properties: any;
    token_standard: string;
    token_uri: string;
-    decimals: any;


This is a breaking change - have we talked with Indexer team on why they removed it?

gregnazario

I'd like to see an indexer only solution for a single request that is paginated if we choose to go down this route.

Otherwise, this is way too costly.

gregnazario · 2024-04-15T19:40:19Z

src/internal/account.ts

+  // TODO: Ideally indexer should provide one API that returns all transactions details in one go
+  // But now we have to query all transaction versions first and then query each transaction by version
+  const versions = await getAccountAllTransactionVersions({ aptosConfig, accountAddress, options });
+  const results = [];
+  for (const version of versions) {
+    const tx = getTransactionByVersion({ aptosConfig, ledgerVersion: version });
+    results.push(tx);
+  }
+  return Promise.all(results);
+}


This significantly changes the behavior of this function from being a paginated API that is associated with an account, to having a non-paginated API across two different services.

If this is the case that this is important, it should do whatever query the explorer and wallets do, or have a dedicated query for it.

Additionally, this changes what is a 1 network call, to 1 + n network calls.

In a limited network environment, it would be slow or eat up all your bandwidth.

0xaptosj · 2024-04-15T20:46:21Z

discussion happening in this thread

0xaptosj · 2024-04-15T23:36:00Z

Synced with Bowen offline, tldr: i think we have to keep this slow approach, because user_transactions table would be gone within a month cuz it's too expensive. Indexer team might come up with a better solution in the future but for now this is the recommended way by them.

0xaptosj · 2024-04-16T00:11:22Z

Not sure why decimals field is removed in some queries, as I don't see it being removed on hasura. I added them back manually. We should probably investigate it later.

0xmaayan · 2024-04-16T08:45:50Z

src/internal/account.ts

-  return paginateWithCursor<{}, TransactionResponse[]>({
+  // TODO: Ideally indexer should provide one API that returns all transactions details in one go
+  // But now we have to query all transaction versions first and then query each transaction by version
+  const versions = await getAccountAllTransactionVersions({ aptosConfig, accountAddress, options });


can we keep this query as is, and add another query like getAllAccountUserTransactions that does this multiple queries? and mention it on the function comment

0xmaayan · 2024-04-16T09:42:37Z

Not sure why decimals field is removed in some queries, as I don't see it being removed on hasura. I added them back manually. We should probably investigate it later.

might be related #314
Ideally, we should not add anything manually but generate whatever is on Hasura so we wont introduce any weird/wrong behavior

…er-txs

0xaptosj · 2024-04-24T18:17:44Z

tests/e2e/api/account.test.ts

+
+      // sleep for 500 ms to ensure the txns are in the ledger
+      await new Promise((resolve) => {
+        setTimeout(resolve, 500);


without the sleep tx2 wouldn't be included, pretty weird considering i already waitForTransaction on tx2

0xaptosj requested a review from a team as a code owner April 15, 2024 01:10

0xaptosj changed the title ~~add new apt to get all user tx versions and update get user txs~~ add new api to get all user tx versions and update get user txs Apr 15, 2024

0xaptosj force-pushed the j/new-indexer-api-get-all-user-txs branch 2 times, most recently from 6864cc4 to e27dd3b Compare April 15, 2024 01:34

0xaptosj changed the title ~~add new api to get all user tx versions and update get user txs~~ Add new indexer API to get all user related txs' versions and use it in get all user txs Apr 15, 2024

0xaptosj commented Apr 15, 2024

View reviewed changes

0xaptosj force-pushed the j/new-indexer-api-get-all-user-txs branch from e27dd3b to 11b9531 Compare April 15, 2024 06:31

0xaptosj changed the title ~~Add new indexer API to get all user related txs' versions and use it in get all user txs~~ Add new indexer API to get all user related tx versions and use it in get all user txs Apr 15, 2024

0xaptosj changed the title ~~Add new indexer API to get all user related tx versions and use it in get all user txs~~ Add new indexer API to get all user related transaction versions and use it in get all user txs Apr 15, 2024

0xaptosj changed the title ~~Add new indexer API to get all user related transaction versions and use it in get all user txs~~ Add new indexer API to getAccountAllTransactionVersions and use it in getAccountTransactions Apr 15, 2024

0xmaayan requested changes Apr 15, 2024

View reviewed changes

gregnazario requested changes Apr 15, 2024

View reviewed changes

0xmaayan reviewed Apr 16, 2024

View reviewed changes

0xaptosj added 4 commits April 24, 2024 09:58

add new api to get all user tx versions and update get user txs

d11efab

update changelog

d7a5a88

restore decimals

618c144

new api

b6ba219

0xaptosj force-pushed the j/new-indexer-api-get-all-user-txs branch from 00e8b6f to 618c144 Compare April 24, 2024 17:07

0xaptosj added 8 commits April 24, 2024 10:08

remove comment

1a7c5d0

update test

dd1bbb7

Merge branch 'j/new-get-all-tx-api' into j/new-indexer-api-get-all-us…

f8b61cf

…er-txs

rename

67803ad

Update CHANGELOG.md

434c494

update test

cfaf6d3

nit

34be240

nit

67ea495

0xaptosj added 2 commits April 24, 2024 11:09

fix test

d39548c

fix linting

a6ac473

0xaptosj commented Apr 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new indexer API to getAccountAllTransactionVersions and use it in getAccountTransactions #364

Add new indexer API to getAccountAllTransactionVersions and use it in getAccountTransactions #364

0xaptosj commented Apr 15, 2024 •

edited

Loading

0xaptosj Apr 15, 2024

0xmaayan Apr 15, 2024

0xmaayan left a comment

0xmaayan Apr 15, 2024

0xaptosj Apr 16, 2024

0xmaayan Apr 15, 2024

gregnazario left a comment

gregnazario Apr 15, 2024

0xaptosj commented Apr 15, 2024

0xaptosj commented Apr 15, 2024

0xaptosj commented Apr 16, 2024

0xmaayan Apr 16, 2024 •

edited

Loading

0xmaayan commented Apr 16, 2024

0xaptosj Apr 24, 2024

Add new indexer API to getAccountAllTransactionVersions and use it in getAccountTransactions #364

Are you sure you want to change the base?

Add new indexer API to getAccountAllTransactionVersions and use it in getAccountTransactions #364

Conversation

0xaptosj commented Apr 15, 2024 • edited Loading

Description

Test Plan

Related Links

0xaptosj Apr 15, 2024

Choose a reason for hiding this comment

0xmaayan Apr 15, 2024

Choose a reason for hiding this comment

0xmaayan left a comment

Choose a reason for hiding this comment

0xmaayan Apr 15, 2024

Choose a reason for hiding this comment

0xaptosj Apr 16, 2024

Choose a reason for hiding this comment

0xmaayan Apr 15, 2024

Choose a reason for hiding this comment

gregnazario left a comment

Choose a reason for hiding this comment

gregnazario Apr 15, 2024

Choose a reason for hiding this comment

0xaptosj commented Apr 15, 2024

0xaptosj commented Apr 15, 2024

0xaptosj commented Apr 16, 2024

0xmaayan Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

0xmaayan commented Apr 16, 2024

0xaptosj Apr 24, 2024

Choose a reason for hiding this comment

0xaptosj commented Apr 15, 2024 •

edited

Loading

0xmaayan Apr 16, 2024 •

edited

Loading