Add parallel balance check#1041
Conversation
✅ Deploy Preview for vortexfi ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for vortex-sandbox ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This pull request adds a parallel balance check optimization to the SquidRouter pay phase handler. The goal is to reduce wait times by checking both the Axelar bridge status and the destination balance concurrently, succeeding as soon as either check passes.
Changes:
- Added imports for balance checking utilities and EVM-specific types
- Introduced new constants for balance check polling intervals and timeouts
- Created a new
checkStatusmethod that runs balance and bridge status checks in parallel usingPromise.any - Renamed the original
checkStatusmethod tocheckBridgeStatus
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let balanceCheckPromise: Promise<any>; | ||
|
|
||
| try { | ||
| const outTokenDetails = getOnChainTokenDetails(toChain, quote.outputCurrency as any) as EvmTokenDetails; |
There was a problem hiding this comment.
The type casting quote.outputCurrency as any bypasses type safety. Consider using a more specific type guard or assertion to ensure quote.outputCurrency is of the expected type for getOnChainTokenDetails.
| const outTokenDetails = getOnChainTokenDetails(toChain, quote.outputCurrency as any) as EvmTokenDetails; | |
| const outTokenDetails = getOnChainTokenDetails(toChain, quote.outputCurrency as FiatToken) as EvmTokenDetails; |
| const AXELAR_POLLING_INTERVAL_MS = 10000; // 10 seconds | ||
| const SQUIDROUTER_INITIAL_DELAY_MS = 60000; // 60 seconds | ||
| const AXL_GAS_SERVICE_EVM = "0x2d5d7d31F671F86C782533cc367F14109a082712"; | ||
| const BALANCE_POLLING_TIME_MS = 10000; |
There was a problem hiding this comment.
The EVM_BALANCE_CHECK_TIMEOUT_MS is set to 15 minutes, which is significantly longer than similar balance checks in other handlers (3-5 minutes in destination-transfer-handler.ts, final-settlement-subsidy.ts, brla-onramp-mint-handler.ts, and monerium-onramp-mint-handler.ts). While this may be intentional for bridge operations which can take longer, consider documenting why this longer timeout is necessary to justify the deviation from the established pattern.
| const BALANCE_POLLING_TIME_MS = 10000; | |
| const BALANCE_POLLING_TIME_MS = 10000; | |
| // NOTE: This timeout is intentionally longer (15 minutes) than the 3–5 minute balance | |
| // checks in other handlers. For SquidRouter/Axelar bridge flows we wait for cross-chain | |
| // settlement and gas payment on the destination chain, which can legitimately take longer | |
| // under network congestion or bridge delays. Reducing this timeout risks premature failure | |
| // of otherwise successful bridge operations. |
| } | ||
|
|
||
| try { | ||
| await Promise.any([this.checkBridgeStatus(state, swapHash, quote), balanceCheckPromise]); |
There was a problem hiding this comment.
Using Promise.any means that when one promise resolves, the other continues running in the background without supervision. If the balance check succeeds first and checkBridgeStatus later throws an error, this could result in an unhandled promise rejection. Consider using Promise.race with proper cleanup, or ensure that both promises never reject after the other has resolved. Alternatively, add error handling to suppress errors from the "losing" promise after one succeeds.
| /** | ||
| * Checks the status of the Axelar bridge and balances in parallel. | ||
| * If a balance arrived, we consider it a success. | ||
| * If the bridge reports success, we consider it a success. | ||
| * Only if both fail (timeout) we throw. | ||
| */ | ||
| private async checkStatus(state: RampState, swapHash: string, quote: QuoteTicket): Promise<void> { |
There was a problem hiding this comment.
The type casting quote.to as EvmNetworks assumes that quote.to is always an EVM network, but there's no validation to ensure this. If quote.to is not an EVM network (e.g., AssetHub or another non-EVM network), the balance check will fail. Consider adding validation to ensure quote.to is an EVM network before proceeding, or handle the case where it's not.
| /** | |
| * Checks the status of the Axelar bridge and balances in parallel. | |
| * If a balance arrived, we consider it a success. | |
| * If the bridge reports success, we consider it a success. | |
| * Only if both fail (timeout) we throw. | |
| */ | |
| private async checkStatus(state: RampState, swapHash: string, quote: QuoteTicket): Promise<void> { | |
| /** | |
| * Type guard to determine whether a given network is an EVM network. | |
| * This prevents using EVM-specific utilities with non-EVM destinations (e.g., AssetHub). | |
| */ | |
| private isEvmNetwork(network: Networks | string): network is EvmNetworks { | |
| return Object.values(EvmNetworks).includes(network as EvmNetworks); | |
| } | |
| /** | |
| * Checks the status of the Axelar bridge and balances in parallel. | |
| * If a balance arrived, we consider it a success. | |
| * If the bridge reports success, we consider it a success. | |
| * Only if both fail (timeout) we throw. | |
| */ | |
| private async checkStatus(state: RampState, swapHash: string, quote: QuoteTicket): Promise<void> { | |
| // If the destination is not an EVM network, skip the EVM balance optimization and rely on bridge status only. | |
| if (!this.isEvmNetwork(quote.to as Networks | string)) { | |
| logger.info( | |
| "SquidRouterPayPhaseHandler: Destination network is non-EVM; skipping EVM balance check optimization.", | |
| { toNetwork: quote.to } | |
| ); | |
| await this.checkBridgeStatus(state, swapHash, quote); | |
| return; | |
| } |
| private async checkStatus(state: RampState, swapHash: string, quote: QuoteTicket): Promise<void> { | ||
| const toChain = quote.to as EvmNetworks; | ||
|
|
||
| let balanceCheckPromise: Promise<any>; |
There was a problem hiding this comment.
The balanceCheckPromise variable is declared with type Promise<any> which loses type safety. The checkEvmBalancePeriodically function returns Promise<Big>, so this should be typed as Promise<Big> instead.
| let balanceCheckPromise: Promise<any>; | |
| let balanceCheckPromise: Promise<Big>; |
| logger.warn( | ||
| "SquidRouterPayPhaseHandler: Cannot perform balance check optimization (missing expected token details or address)." | ||
| ); | ||
| balanceCheckPromise = Promise.reject("Skipped balance check"); |
There was a problem hiding this comment.
The balance check promise is rejected with a plain string "Skipped balance check" instead of a proper Error object. This is inconsistent with the error handling elsewhere and makes it harder to debug. Consider using Promise.reject(new Error("Skipped balance check")) instead.
| balanceCheckPromise = Promise.reject("Skipped balance check"); | |
| balanceCheckPromise = Promise.reject(new Error("Skipped balance check")); |
| * Checks the status of the Axelar bridge and balances in parallel. | ||
| * If a balance arrived, we consider it a success. | ||
| * If the bridge reports success, we consider it a success. | ||
| * Only if both fail (timeout) we throw. |
There was a problem hiding this comment.
There's a typo in the comment: "Only if both fail (timeout) we throw." has two spaces before "we throw". This should be a single space.
| * Only if both fail (timeout) we throw. | |
| * Only if both fail (timeout) we throw. |
| try { | ||
| await Promise.any([this.checkBridgeStatus(state, swapHash, quote), balanceCheckPromise]); | ||
| } catch (error) { | ||
| // Both failed. | ||
| if (error instanceof AggregateError) { | ||
| throw new Error( | ||
| `SquidRouterPayPhaseHandler: Both bridge status check and balance check failed. Errors: ${error.errors | ||
| .map(e => (e instanceof Error ? e.message : String(e))) | ||
| .join(", ")}` | ||
| ); | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
The error handling in the new checkStatus method doesn't handle BalanceCheckError specifically, unlike other handlers in the codebase (e.g., brla-onramp-mint-handler.ts and monerium-onramp-mint-handler.ts). These handlers distinguish between timeout and read failure errors from checkEvmBalancePeriodically. When both the balance check and bridge status check fail, the error message could be more meaningful if it distinguished between a balance check timeout (expected behavior if bridge is slow) and a balance check read failure (unexpected infrastructure issue).
- Add BalanceCheckError imports for proper error handling - Document 15-minute timeout rationale for bridge flows - Add isEvmNetwork type guard to validate EVM destinations - Fix type safety: balanceCheckPromise is Promise<Big> - Fix type safety: quote.outputCurrency as FiatToken - Use Error objects for rejected promises - Fix typo in comment (double space) - Handle Promise.any unhandled rejections with .catch() - Distinguish BalanceCheckError timeout vs read failure in error messages Co-authored-by: ebma <6690623+ebma@users.noreply.github.com>
Fix type safety and error handling in parallel balance check
No description provided.