Add indexerTimeoutSecs Config #166
Conversation
0xmigo
requested review from
0xmaayan,
gregnazario,
heliuchuan and
a team
as code owners
| @@ -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; | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| @@ -42,6 +42,14 @@ export const DEFAULT_TXN_EXP_SEC_FROM_NOW = 20; | |||
| */ | |||
| export const DEFAULT_TXN_TIMEOUT_SEC = 20; | |||
|
|
|||
| /** | |||
| * The default number of seconds to wait for the indexer to sync. | |||
There was a problem hiding this comment.
lets mention we use it for waitForTransaction
| @@ -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; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think that adding another timeout just gets into the level of too much configurability / probably keep it simple.
If you're a developer, you don't really care what you're waiting for, you just want it to be ready in X seconds.
Separating the two, requires extra thought and process to determine why one or the other, and plenty of room for misconfiguration.
| @@ -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; | |||
There was a problem hiding this comment.
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.
| */ | ||
| async function waitForLastSuccessIndexerVersionSync(args: { | ||
| aptosConfig: AptosConfig; | ||
| ledgerVersion: number; | ||
| timeOutSecs: number; |
There was a problem hiding this comment.
Timeout is one word
| timeOutSecs: number; | |
| timeoutSecs: number; |
| indexerVersionCheck?: boolean; | ||
| indexerTimeoutSecs?: number; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
| @@ -42,6 +42,14 @@ export const DEFAULT_TXN_EXP_SEC_FROM_NOW = 20; | |||
| */ | |||
| export const DEFAULT_TXN_TIMEOUT_SEC = 20; | |||
There was a problem hiding this comment.
We should just increase these two to 30, instead of 20.
I think thats the reason we put it in a optional field |
|
Closing this PR. Will work on a PR that fix this problem by implementing option 1 and 2 here -wait for indexer |
Description
Add a new config field to waitForTransaction
indexerTimeoutSecs. This allow user to modify the timeout for indexer sync.Test Plan
Related Links