Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 134
[PAY-2689] Send tips via SDK (behind feature flag)#8090
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.
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
d12cb824ca6f617108b7fe8e0b00ec14b71f2dc6adb49de6551ade07ec4df943c5076db7defe0File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -25,8 +25,22 @@ const connections = config.solanaEndpoints.map( | ||
| (endpoint) => new Connection(endpoint) | ||
| ) | ||
| const delay = async (ms: number) => { | ||
| return await new Promise((resolve) => setTimeout(() => resolve, ms)) | ||
| const delay = async (ms: number, options?: { signal: AbortSignal }) => { | ||
| const signal = options?.signal | ||
| return new Promise<void>((resolve, reject) => { | ||
| if (signal?.aborted) { | ||
| reject() | ||
| } | ||
| const listener = () => { | ||
| clearTimeout(timer) | ||
| reject() | ||
| } | ||
| const timer = setTimeout(() => { | ||
| signal?.removeEventListener('abort', listener) | ||
| resolve() | ||
| }, ms) | ||
| signal?.addEventListener('abort', listener) | ||
| }) | ||
| } | ||
| const getFeePayerKeyPair = (feePayerPublicKey?: PublicKey) => { | ||
| @@ -104,54 +118,75 @@ const sendTransactionWithRetries = async ({ | ||
| logger: Logger | ||
| }) => { | ||
| const serializedTx = transaction.serialize() | ||
| const connection = connections[0] | ||
| const createRetryPromise = async (): Promise<void> => { | ||
| let retryCount = 0 | ||
| while (true) { | ||
| await delay(RETRY_DELAY_MS) | ||
| // Explicitly not awaited, sent in the background | ||
| logger.info({ retryCount }, 'Attempting send...') | ||
| try { | ||
| Promise.any( | ||
| connections.map((connection) => | ||
| connection.sendRawTransaction(serializedTx, sendOptions) | ||
| ) | ||
| ) | ||
| } catch (error) { | ||
| logger.warn( | ||
| { error, retryCount, rpcEndpoint: connection.rpcEndpoint }, | ||
| `Failed retry...` | ||
| let retryCount = 0 | ||
| const createRetryPromise = async (signal: AbortSignal): Promise<void> => { | ||
| while (!signal.aborted) { | ||
| Promise.any( | ||
| connections.map((connection) => | ||
| connection.sendRawTransaction(serializedTx, { | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know this is existing code but wow is this agressive 😅 | ||
| skipPreflight: true, | ||
| maxRetries: 0, | ||
| ...sendOptions | ||
| }) | ||
| ) | ||
| } | ||
| ).catch((error) => { | ||
| logger.warn({ error, retryCount }, `Failed retry...`) | ||
| }) | ||
| await delay(RETRY_DELAY_MS) | ||
| retryCount++ | ||
| } | ||
| } | ||
| const createTimeoutPromise = async () => { | ||
| const createTimeoutPromise = async (signal: AbortSignal) => { | ||
| await delay(RETRY_TIMEOUT_MS) | ||
| logger.error('Timed out sending transaction') | ||
| if (!signal.aborted) { | ||
| logger.error('Timed out sending transaction') | ||
| } | ||
| } | ||
| const start = Date.now() | ||
| const connection = connections[0] | ||
| const abortController = new AbortController() | ||
| try { | ||
| if (!sendOptions?.skipPreflight) { | ||
| const simulatedRes = await connection.simulateTransaction(transaction) | ||
| if (simulatedRes.value.err) { | ||
| logger.error( | ||
| { error: simulatedRes.value.err }, | ||
| 'Transaction simulation failed' | ||
| ) | ||
| throw simulatedRes.value.err | ||
| } | ||
| } | ||
| const res = await Promise.race([ | ||
| createRetryPromise(), | ||
| connection.confirmTransaction(confirmationStrategy, commitment), | ||
| createTimeoutPromise() | ||
| ]) | ||
| const end = Date.now() | ||
| const elapsedMs = end - start | ||
| const res = await Promise.race([ | ||
| createRetryPromise(abortController.signal), | ||
| connection.confirmTransaction( | ||
| { ...confirmationStrategy, abortSignal: abortController.signal }, | ||
| commitment | ||
| ), | ||
| createTimeoutPromise(abortController.signal) | ||
| ]) | ||
| if (!res || res.value.err) { | ||
| if (!res || res.value.err) { | ||
| throw res?.value.err ?? 'Transaction polling timed out.' | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels like it might create false positives about timeouts. Should we not have the timeout promise explicitly return a value here and reserve the null-coalescing for something like "Unknown..." so that we know it's an unhandled case? | ||
| } | ||
| logger.info({ commitment }, 'Transaction sent successfully') | ||
| return confirmationStrategy.signature | ||
| } catch (error) { | ||
| logger.error({ error }, 'Transaction failed to send') | ||
| throw error | ||
| } finally { | ||
| // Stop the other operations | ||
| abortController.abort() | ||
| const end = Date.now() | ||
| const elapsedMs = end - start | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was always taught to use | ||
| logger.info( | ||
| { error: res?.value.err ?? 'timeout', commitment, elapsedMs }, | ||
| 'Transaction failed to send' | ||
| { elapsedMs, retryCount }, | ||
| 'sendTransactionWithRetries completed.' | ||
| ) | ||
| throw new Error('Transaction failed to send') | ||
| } | ||
| logger.info({ commitment, elapsedMs }, 'Transaction sent successfully') | ||
| return confirmationStrategy.signature | ||
| } | ||
| export const relay = async ( | ||
| @@ -188,7 +223,10 @@ export const relay = async ( | ||
| const signature = base58.encode(transaction.signatures[0]) | ||
| const logger = res.locals.logger.child({ signature }) | ||
| logger.info('Sending transaction...') | ||
| logger.info( | ||
| { rpcEndpoints: connections.map((c) => c.rpcEndpoint) }, | ||
| 'Sending transaction...' | ||
| ) | ||
| const confirmationStrategy = { ...strategy, signature } | ||
| await sendTransactionWithRetries({ | ||
| transaction, | ||
| @@ -220,6 +258,11 @@ export const relay = async ( | ||
| await forwardTransaction(logger, formattedResponse) | ||
| logger.info('Request finished.') | ||
| } catch (e) { | ||
| next(e) | ||
| if (!res.writableEnded && e) { | ||
| res.status(500).send({ error: e }) | ||
| next() | ||
| } else { | ||
| next(e) | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -57,7 +57,7 @@ export class BorshString extends Layout<string> { | ||
| getSpan(b: Uint8Array, offset = 0): number { | ||
| if (!b) { | ||
| return this.maxLength | ||
| return u32().span + this.maxLength | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels like a magic fix. Can you add a comment or at least comment here on what this is fixing? ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's actually more just semantics - previously, specifying a "max size" for the "BorshString" class was the max encoded size, meaning that it capped the size of the resulting encoded value. With this change, the "max size" becomes the max length of the actual string value. This aligns more closely to what was used (and why that bug was introduced) since using max length was meant to ensure that the string value was less than 32 bytes for the sake of generating account address seeds from it. | ||
| } | ||
| const length = u32().decode(b, offset) | ||
| return u32().span + length | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also not sure what the layout is in this package, but seems like utils should go outside this module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair. I think I like it co-located for now, but if this module grows a bit more or other modules want to use it I'll bring it out