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.

Sign up for GitHub

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

Add indexerTimeoutSecs Config #166

Closed
wants to merge 1 commit into from
Closed

Add indexerTimeoutSecs Config #166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions src/internal/transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {
type TransactionResponse,
WaitForTransactionOptions,
} from "../types";
import { DEFAULT_TXN_TIMEOUT_SEC } from "../utils/const";
import { DEFAULT_INDEXER_SYNC_TIMEOUT_SEC, DEFAULT_TXN_TIMEOUT_SEC } from "../utils/const";
import { sleep } from "../utils/helpers";
import { memoizeAsync } from "../utils/memoize";
import { getIndexerLastSuccessVersion } from "./general";
Expand Down Expand Up @@ -105,6 +105,7 @@ export async function waitForTransaction(args: {
const timeoutSecs = options?.timeoutSecs ?? DEFAULT_TXN_TIMEOUT_SEC;
const checkSuccess = options?.checkSuccess ?? true;
const indexerVersionCheck = options?.indexerVersionCheck ?? true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if we should update this and default to false. I feel like, only dev that relies on indexer API will probably want. But if they are only sending request to full node api, they probably want to have this off.

Thought? cc: @0xmaayan @gregnazario

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm most of the sdk queries use indexer, so if a sdk developer uses the sdk to submit transactions and then fetch data, they/sdk should wait for indexer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Only annoying thing is... if it's by default on and the indexer is delayed / broken, the dapps will completely not work.

It's a strange tradeoff based on what your dapp does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only annoying thing is... if it's by default on and the indexer is delayed / broken, the dapps will completely not work.

It's a strange tradeoff based on what your dapp does.

Yeah, I am thinking about this trade off as well. Because I would hate to sdk to be broken b/c of indexer. Maybe we should just always return, but have error field saying that the indexer failed to sync?

For example:
const result = aptos.waitForTransaction();

and result looks like following:
{
indexerSyncError: "timeout"
...
}

Or we just relies on the developer to wrap waitForTransaction in a try-catch

const indexerTimeoutSecs = options?.indexerTimeoutSecs ?? DEFAULT_INDEXER_SYNC_TIMEOUT_SEC;

let isPending = true;
let timeElapsed = 0;
Expand Down Expand Up @@ -175,7 +176,11 @@ export async function waitForTransaction(args: {
// Make sure indexer is synced with the latest ledger version
if (indexerVersionCheck) {
try {
await waitForLastSuccessIndexerVersionSync({ aptosConfig, ledgerVersion: Number(lastTxn.version) });
await waitForLastSuccessIndexerVersionSync({
aptosConfig,
ledgerVersion: Number(lastTxn.version),
timeOutSecs: indexerTimeoutSecs,
});
} catch (_e) {
throw new WaitForTransactionError(
// eslint-disable-next-line max-len
Expand All @@ -190,14 +195,15 @@ export async function waitForTransaction(args: {
}

/**
* Waits for the indexer to sync up to the ledgerVersion. Timeout is 3 seconds.
* Waits for the indexer to sync up to the ledgerVersion. Default timeout is 3 seconds.
*/
async function waitForLastSuccessIndexerVersionSync(args: {
aptosConfig: AptosConfig;
ledgerVersion: number;
timeOutSecs: number;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Timeout is one word

Suggested change
timeOutSecs: number;
timeoutSecs: number;

}): Promise<void> {
const { aptosConfig, ledgerVersion } = args;
const timeoutMilliseconds = 3000; // 3 seconds
const { aptosConfig, ledgerVersion, timeOutSecs } = args;
const timeoutMilliseconds = timeOutSecs * 1000;
const startTime = new Date().getTime();
let indexerVersion = -1;

Expand Down
1 change: 1 addition & 0 deletions src/types/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -996,6 +996,7 @@ export type WaitForTransactionOptions = {
timeoutSecs?: number;
checkSuccess?: boolean;
indexerVersionCheck?: boolean;
indexerTimeoutSecs?: number;
Comment on lines 998 to +999
Copy link
Collaborator

Choose a reason for hiding this comment

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

I want to take a moment and propose a different name for indexerVersionCheck, to waitForIndexer.

Possibly a different structure where:

waitForIndexer?: boolean,

I'm not sure we necessarily need a separate timeout for the indexer, just increasing the one to a larger number probably makes more sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree- let's not rely too much on configuration options, because too many will be overwhelming and difficult for people to find. This is more true when the preferred config/option is going to be very consistent (i.e., 95%+ of developers will expect it by default)

};
/**
* Account input type to generate an account using Legacy
Expand Down
8 changes: 8 additions & 0 deletions src/utils/const.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,14 @@ export const DEFAULT_TXN_EXP_SEC_FROM_NOW = 20;
*/
export const DEFAULT_TXN_TIMEOUT_SEC = 20;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should just increase these two to 30, instead of 20.


/**
* The default number of seconds to wait for the indexer to sync.
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets mention we use it for waitForTransaction

*
* This is the amount of time that the SDK will wait for the indexer to sync and catch up with the full node.
* Based on the processor_status's last success version.
*/
export const DEFAULT_INDEXER_SYNC_TIMEOUT_SEC = 3;

/**
* The default gas currency for the network.
*/
Expand Down
Loading