feat: allow quotes that take longer than 5 seconds to keep going if we get nothing else - #10372
Conversation
📝 WalkthroughWalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI
participant API as swapperApi.getTradeRates
participant Swapper as Swapper.getTradeRates
participant Utils as timeoutMonadicWithOriginal
UI->>API: Request batch trade rates
loop per swapper
API->>Swapper: getTradeRates()
Swapper->>Utils: timeoutMonadicWithOriginal(promise, timeoutMs, TimeoutError)
Utils-->>Swapper: { timed, original }
Swapper-->>API: RateResult { Ok/Err, swapperName, fallback?: original }
end
API->>API: Inspect unprocessed results
alt all primary results missing && any timed-out
API->>API: Await fallback for timed-out swappers
API->>API: Process late quotes from fallback
else
API->>API: Process quotes from timed results
end
API-->>UI: Aggregated quotes by swapperName
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
b784288 to
8ea88ef
Compare
8ea88ef to
c00f851
Compare
premiumjibles
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/utils/src/timeout.ts (1)
34-50: Document invariants for fallback (non-cancelable, expected not to reject)Add TSDoc clarifying that fallback is the original non-cancelable promise and is expected to resolve to a monadic Result (never reject). This matches the intent noted earlier.
export const timeoutMonadicWithFallback = <Left, Right>( + /** + * Races `promise` against a timeout, returning: + * - `result`: the raced Promise<Result<Ok, Err>> that Err-times out after `timeoutMs` + * - `fallback`: the original, non-cancelable `promise` for late consumption (must resolve to Result and not reject) + * + * Note: consumers should either await `fallback` when needed or attach a no-op catch upstream to avoid unhandled rejections + * if a non-monadic implementation accidentally rejects. + */ promise: Promise<Result<Left, Right>>, timeoutMs: number, timeoutRight: Right, ): { result: Promise<Result<Left, Right>>; fallback: Promise<Result<Left, Right>> } => {src/state/apis/swapper/swapperApi.ts (1)
153-167: Two-phase fetch/process makes the fallback decision possibleSplitting into unprocessedSwapperResults then processing is a pragmatic trade-off; measured overhead is negligible.
🧹 Nitpick comments (5)
packages/utils/src/timeout.ts (1)
34-50: Reduce duplication: build on timeoutMonadic instead of re-implementing the raceReuse the existing timeoutMonadic for the result branch to keep behavior consistent and DRY.
Apply:
export const timeoutMonadicWithFallback = <Left, Right>( promise: Promise<Result<Left, Right>>, timeoutMs: number, timeoutRight: Right, ): { result: Promise<Result<Left, Right>>; fallback: Promise<Result<Left, Right>> } => { - return { - result: Promise.race([ - promise, - new Promise<Result<Left, Right>>(resolve => - setTimeout(() => { - resolve(Err(timeoutRight) as Result<Left, Right>) - }, timeoutMs), - ), - ]), - fallback: promise, - } + return { + result: timeoutMonadic<Left, Right>(promise, timeoutMs, timeoutRight), + fallback: promise, + } }packages/swapper/src/swapper.ts (2)
62-69: Guard against potential unhandled rejections on the fallback pathIf any swapper accidentally rejects (anti-pattern), the unawaited fallback can trigger an unhandled rejection. Attach a no-op catch to suppress noise while preserving the original promise semantics for downstream consumers.
- const { result, fallback } = timeoutMonadicWithFallback<TradeRate[], SwapErrorRight>( + const { result, fallback } = timeoutMonadicWithFallback<TradeRate[], SwapErrorRight>( swapper.getTradeRate(getTradeRateInput, deps), quoteTimeoutMs, makeSwapErrorRight({ code: TradeQuoteError.Timeout, message: `quote timed out after ${quoteTimeoutMs / 1000}s`, }), ) + // Ensure no unhandled rejection if fallback isn't consumed downstream + void fallback.catch(() => undefined)
71-76: Name the variable for what it is (rateResult), not quoteMinor clarity tweak to avoid confusing rate vs quote terminology.
- const quote = await result + const rateResult = await result return { - ...quote, + ...rateResult, fallback, swapperName, }packages/swapper/src/types.ts (1)
615-618: Clarify fallback semantics on RateResultAdd a short doc to ensure future consumers understand when and how to consume fallback.
export type RateResult = Result<TradeRate[], SwapErrorRight> & { swapperName: SwapperName - fallback?: Promise<Result<TradeRate[], SwapErrorRight>> + /** + * Original, non-cancelable promise for late rates when initial fetch timed out. + * Expected to resolve to a monadic Result (do not reject). + * Consumers should only await when no other quotes are available. + */ + fallback?: Promise<Result<TradeRate[], SwapErrorRight>> }src/state/apis/swapper/swapperApi.ts (1)
183-197: Fallback await path is correct; consider making the predicate a named helper for readabilityNo functional change; just readability if you touch this again.
- const rateResultWithFallback = - noQuotes && hasTimeoutQuote && isTimeout && rateResult?.fallback - ? { ...(await rateResult.fallback), swapperName } - : rateResult + const shouldUseFallback = + noQuotes && hasTimeoutQuote && isTimeout && Boolean(rateResult?.fallback) + const rateResultWithFallback = shouldUseFallback + ? { ...(await rateResult!.fallback!), swapperName } + : rateResult
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/swapper/src/swapper.ts(3 hunks)packages/swapper/src/types.ts(1 hunks)packages/utils/src/timeout.ts(1 hunks)src/state/apis/swapper/swapperApi.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
packages/swapper/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/swapper.mdc)
packages/swapper/**/*.ts: Use TypeScript with explicit types (e.g., SupportedChainIds) in all swapper-related files.
Use camelCase for variables and functions, PascalCase for types and interfaces, and kebab-case for filenames in swapper-related files.
Files:
packages/swapper/src/types.tspackages/swapper/src/swapper.ts
packages/swapper/src/{constants,types}.ts
📄 CodeRabbit inference engine (.cursor/rules/swapper.mdc)
Register new swappers in packages/swapper/src/constants.ts and add them to the SwapperName enum in packages/swapper/src/types.ts.
Files:
packages/swapper/src/types.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
**/*.{ts,tsx}: ALWAYS use Result<T, E> pattern for error handling in swappers and APIs
ALWAYS use Ok() and Err() from @sniptt/monads for monadic error handling
ALWAYS use custom error classes from @shapeshiftoss/errors
ALWAYS provide meaningful error codes for internationalization
ALWAYS include relevant details in error objects
ALWAYS wrap async operations in try-catch blocks
ALWAYS use AsyncResultOf utility for converting promises to Results
ALWAYS provide fallback error handling
ALWAYS use timeoutMonadic for API calls
ALWAYS provide appropriate timeout values for API calls
ALWAYS handle timeout errors gracefully
ALWAYS validate inputs before processing
ALWAYS provide clear validation error messages
ALWAYS use early returns for validation failures
ALWAYS log errors for debugging
ALWAYS use structured logging for errors
ALWAYS include relevant context in error logs
Throwing errors instead of using monadic patterns is an anti-pattern
Missing try-catch blocks for async operations is an anti-pattern
Generic error messages without context are an anti-pattern
Not handling specific error types is an anti-pattern
Missing timeout handling is an anti-pattern
No input validation is an anti-pattern
Poor error logging is an anti-pattern
Using any for error types is an anti-pattern
Missing error codes for internationalization is an anti-pattern
No fallback error handling is an anti-pattern
Console.error without structured logging is an anti-pattern
**/*.{ts,tsx}: ALWAYS use camelCase for variables, functions, and methods
ALWAYS use descriptive names that explain the purpose for variables and functions
ALWAYS use verb prefixes for functions that perform actions
ALWAYS use PascalCase for types, interfaces, and enums
ALWAYS use descriptive names that indicate the structure for types, interfaces, and enums
ALWAYS use suffixes like Props, State, Config, Type when appropriate for types and interfaces
ALWAYS use UPPER_SNAKE_CASE for constants and configuration values
ALWAYS use d...
Files:
packages/swapper/src/types.tspackages/utils/src/timeout.tssrc/state/apis/swapper/swapperApi.tspackages/swapper/src/swapper.ts
**/swapper/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
**/swapper/**/*.{ts,tsx}: AVOID throwing within swapper API implementations; return Err() instead. UI layers may still throw (caught by React error boundaries).
ALWAYS use makeSwapErrorRight for swapper errors
ALWAYS use TradeQuoteError enum for error codes in swapper errors
ALWAYS provide detailed error information in swapper errors
Files:
packages/swapper/src/types.tssrc/state/apis/swapper/swapperApi.tspackages/swapper/src/swapper.ts
**/*
📄 CodeRabbit inference engine (.cursor/rules/naming-conventions.mdc)
**/*: ALWAYS use appropriate file extensions
Flag files without kebab-case
Files:
packages/swapper/src/types.tspackages/utils/src/timeout.tssrc/state/apis/swapper/swapperApi.tspackages/swapper/src/swapper.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/react-best-practices.mdc)
USE Redux only for global state shared across multiple places
Files:
packages/swapper/src/types.tspackages/utils/src/timeout.tssrc/state/apis/swapper/swapperApi.tspackages/swapper/src/swapper.ts
🧠 Learnings (15)
📓 Common learnings
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/error-handling.mdc:0-0
Timestamp: 2025-08-03T22:09:37.542Z
Learning: Applies to **/*.{ts,tsx} : ALWAYS use timeoutMonadic for API calls
📚 Learning: 2025-08-03T22:09:37.542Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/error-handling.mdc:0-0
Timestamp: 2025-08-03T22:09:37.542Z
Learning: Applies to **/swapper/**/*.{ts,tsx} : ALWAYS use TradeQuoteError enum for error codes in swapper errors
Applied to files:
packages/swapper/src/types.tssrc/state/apis/swapper/swapperApi.tspackages/swapper/src/swapper.ts
📚 Learning: 2025-07-24T09:43:11.699Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/swapper.mdc:0-0
Timestamp: 2025-07-24T09:43:11.699Z
Learning: Applies to packages/swapper/**/*.ts : Use TypeScript with explicit types (e.g., SupportedChainIds) in all swapper-related files.
Applied to files:
packages/swapper/src/types.tssrc/state/apis/swapper/swapperApi.tspackages/swapper/src/swapper.ts
📚 Learning: 2025-07-24T09:43:11.699Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/swapper.mdc:0-0
Timestamp: 2025-07-24T09:43:11.699Z
Learning: Applies to packages/swapper/src/{constants,types}.ts : Register new swappers in packages/swapper/src/constants.ts and add them to the SwapperName enum in packages/swapper/src/types.ts.
Applied to files:
packages/swapper/src/types.tssrc/state/apis/swapper/swapperApi.tspackages/swapper/src/swapper.ts
📚 Learning: 2025-07-24T09:43:11.699Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/swapper.mdc:0-0
Timestamp: 2025-07-24T09:43:11.699Z
Learning: Applies to packages/swapper/src/swappers/*/{*.ts,endpoints.ts} : Avoid side effects in swap logic within swapper implementation files.
Applied to files:
packages/swapper/src/types.tssrc/state/apis/swapper/swapperApi.tspackages/swapper/src/swapper.ts
📚 Learning: 2025-08-03T22:09:37.542Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/error-handling.mdc:0-0
Timestamp: 2025-08-03T22:09:37.542Z
Learning: Applies to **/swapper/**/*.{ts,tsx} : ALWAYS use makeSwapErrorRight for swapper errors
Applied to files:
packages/swapper/src/types.tssrc/state/apis/swapper/swapperApi.tspackages/swapper/src/swapper.ts
📚 Learning: 2025-07-24T09:43:11.699Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/swapper.mdc:0-0
Timestamp: 2025-07-24T09:43:11.699Z
Learning: Applies to packages/swapper/src/swappers/*/{*.ts,endpoints.ts} : Include comments explaining swap logic in swapper implementation files.
Applied to files:
packages/swapper/src/types.tssrc/state/apis/swapper/swapperApi.tspackages/swapper/src/swapper.ts
📚 Learning: 2025-08-03T22:09:37.542Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/error-handling.mdc:0-0
Timestamp: 2025-08-03T22:09:37.542Z
Learning: Applies to **/*.{ts,tsx} : ALWAYS use Result<T, E> pattern for error handling in swappers and APIs
Applied to files:
packages/swapper/src/types.ts
📚 Learning: 2025-08-03T22:09:37.542Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/error-handling.mdc:0-0
Timestamp: 2025-08-03T22:09:37.542Z
Learning: Applies to **/*.{ts,tsx} : ALWAYS use timeoutMonadic for API calls
Applied to files:
packages/utils/src/timeout.tspackages/swapper/src/swapper.ts
📚 Learning: 2025-08-03T22:09:37.542Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/error-handling.mdc:0-0
Timestamp: 2025-08-03T22:09:37.542Z
Learning: Applies to **/*.{ts,tsx} : Missing timeout handling is an anti-pattern
Applied to files:
packages/utils/src/timeout.ts
📚 Learning: 2025-07-31T03:51:48.479Z
Learnt from: premiumjibles
PR: shapeshift/web#10154
File: src/state/apis/swapper/helpers/swapperApiHelpers.ts:57-60
Timestamp: 2025-07-31T03:51:48.479Z
Learning: In src/state/apis/swapper/helpers/swapperApiHelpers.ts, the getState parameter in processQuoteResultWithRatios uses `() => unknown` type instead of `() => ReduxState` to avoid type compatibility issues elsewhere in the codebase.
Applied to files:
src/state/apis/swapper/swapperApi.ts
📚 Learning: 2025-07-24T09:43:11.699Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/swapper.mdc:0-0
Timestamp: 2025-07-24T09:43:11.699Z
Learning: Applies to packages/swapper/src/swappers/*/{*.ts,endpoints.ts} : Leverage shared utilities (e.g., executeEvmTransaction, checkEvmSwapStatus) in swapper implementations when applicable.
Applied to files:
src/state/apis/swapper/swapperApi.tspackages/swapper/src/swapper.ts
📚 Learning: 2025-08-03T22:09:37.542Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/error-handling.mdc:0-0
Timestamp: 2025-08-03T22:09:37.542Z
Learning: Applies to **/swapper/**/*.{ts,tsx} : ALWAYS provide detailed error information in swapper errors
Applied to files:
src/state/apis/swapper/swapperApi.tspackages/swapper/src/swapper.ts
📚 Learning: 2025-07-24T09:43:11.699Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/swapper.mdc:0-0
Timestamp: 2025-07-24T09:43:11.699Z
Learning: Applies to packages/swapper/src/swappers/*/{*.ts,endpoints.ts} : All swappers must conform to the Swapper and SwapperApi interfaces defined in packages/swapper/src/types.ts.
Applied to files:
src/state/apis/swapper/swapperApi.ts
📚 Learning: 2025-07-24T09:43:11.699Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/swapper.mdc:0-0
Timestamp: 2025-07-24T09:43:11.699Z
Learning: Applies to packages/swapper/src/index.ts : Export unique functions/types from packages/swapper/src/index.ts if needed.
Applied to files:
packages/swapper/src/swapper.ts
🧬 Code graph analysis (2)
src/state/apis/swapper/swapperApi.ts (2)
src/state/apis/swapper/types.ts (1)
TradeQuoteError(39-39)src/state/apis/swapper/helpers/swapperApiHelpers.ts (1)
processQuoteResultWithRatios(56-86)
packages/swapper/src/swapper.ts (2)
packages/utils/src/timeout.ts (1)
timeoutMonadicWithFallback(34-50)packages/swapper/src/types.ts (2)
TradeRate(450-454)SwapErrorRight(82-88)
🔇 Additional comments (4)
packages/swapper/src/swapper.ts (1)
31-41: Monadic timeout + i18n-ready error code looks goodTimeout handling uses monadic Result and TradeQuoteError.Timeout with context in the message. Aligned with error-handling guidelines.
src/state/apis/swapper/swapperApi.ts (3)
4-4: Importing TradeQuoteError here is appropriateKeeps error-code checks centralized and avoids stringly-typed comparisons.
199-203: Processing with ratios on the (possibly) fallback-augmented result is spot onThis feeds downstream quote creation uniformly regardless of timeout fallback.
169-175: No direct Ok([]) in any swapper—empty/no-route always ErrA search across all
packages/swapper/src/swappersfor literalOk([])returns no matches, and every swapping implementation either:
- Returns
Err(...)on missing routes or failures (e.g. network errors, no-route found), or- Builds an array by pushing at least one element before returning
Ok(...)Thus an
Ok([])case cannot arise, and the fallback logic innoQuoteswill not be inadvertently skipped.
c00f851 to
3be96fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/utils/src/timeout.ts (1)
34-50: Clarify generics/naming and document contract.
- The
Rightparameter name paired withErr(timeoutRight)is counterintuitive. Consider<Ok, ErrT>withtimeoutErr: ErrT, or add TSDoc clarifying “Right = error”.- Add a brief TSDoc stating the input promise is expected to resolve to
Result(not reject). If callers may pass rejecting promises, note that unhandled rejections can occur when the timeout wins.Here’s a naming/doc pass:
-export const timeoutMonadicWithFallback = <Left, Right>( - promise: Promise<Result<Left, Right>>, - timeoutMs: number, - timeoutRight: Right, -): { result: Promise<Result<Left, Right>>; fallback: Promise<Result<Left, Right>> } => { +/** + * Races a monadic Result promise against a timeout. On timeout, returns Err(timeoutErr). + * Also returns the original promise as `fallback` for optional late consumption. + * NOTE: Promise is expected not to reject; it should resolve to Result<Ok, ErrT>. + */ +export const timeoutMonadicWithFallback = <Ok, ErrT>( + promise: Promise<Result<Ok, ErrT>>, + timeoutMs: number, + timeoutErr: ErrT, +): { result: Promise<Result<Ok, ErrT>>; fallback: Promise<Result<Ok, ErrT>> } => { return { result: Promise.race([ promise, - new Promise<Result<Left, Right>>(resolve => + new Promise<Result<Ok, ErrT>>(resolve => setTimeout(() => { - resolve(Err(timeoutRight) as Result<Left, Right>) + resolve(Err(timeoutErr) as Result<Ok, ErrT>) }, timeoutMs), ), ]), fallback: promise, } }src/state/apis/swapper/swapperApi.ts (2)
183-198: Return shape inconsistency; prefer consistent payload to simplify consumers.Returning
{ data: {} }from the inner mapper diverges from the{ swapperName, quotes }shape and forces downstream guards. Return{ swapperName, quotes: [] }instead for uniformity.- if (promiseResult.status !== 'fulfilled') return { data: {} } + if (promiseResult.status !== 'fulfilled') return { swapperName: undefined as unknown as SwapperName, quotes: [] } const { swapperName, rateResult } = promiseResult.value ... - if (rateResultWithFallback === undefined) { - return { data: {} } - } + if (rateResultWithFallback === undefined) { + return { swapperName, quotes: [] } + }Optionally rename
promiseResult→settledResultfor clarity.
199-203: Processing with fallback-fed RateResult is correct. Consider avoiding second allSettled for minor perf/clarity.You can map over
unprocessedSwapperResultsandawait Promise.all(...)since the inner mapper already handles per-item errors. Net effect is simpler control flow.If desired, I can provide a small refactor PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/swapper/src/swapper.ts(3 hunks)packages/swapper/src/types.ts(1 hunks)packages/utils/src/timeout.ts(1 hunks)src/state/apis/swapper/swapperApi.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/swapper/src/types.ts
- packages/swapper/src/swapper.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
**/*.{ts,tsx}: ALWAYS use Result<T, E> pattern for error handling in swappers and APIs
ALWAYS use Ok() and Err() from @sniptt/monads for monadic error handling
ALWAYS use custom error classes from @shapeshiftoss/errors
ALWAYS provide meaningful error codes for internationalization
ALWAYS include relevant details in error objects
ALWAYS wrap async operations in try-catch blocks
ALWAYS use AsyncResultOf utility for converting promises to Results
ALWAYS provide fallback error handling
ALWAYS use timeoutMonadic for API calls
ALWAYS provide appropriate timeout values for API calls
ALWAYS handle timeout errors gracefully
ALWAYS validate inputs before processing
ALWAYS provide clear validation error messages
ALWAYS use early returns for validation failures
ALWAYS log errors for debugging
ALWAYS use structured logging for errors
ALWAYS include relevant context in error logs
Throwing errors instead of using monadic patterns is an anti-pattern
Missing try-catch blocks for async operations is an anti-pattern
Generic error messages without context are an anti-pattern
Not handling specific error types is an anti-pattern
Missing timeout handling is an anti-pattern
No input validation is an anti-pattern
Poor error logging is an anti-pattern
Using any for error types is an anti-pattern
Missing error codes for internationalization is an anti-pattern
No fallback error handling is an anti-pattern
Console.error without structured logging is an anti-pattern
**/*.{ts,tsx}: ALWAYS use camelCase for variables, functions, and methods
ALWAYS use descriptive names that explain the purpose for variables and functions
ALWAYS use verb prefixes for functions that perform actions
ALWAYS use PascalCase for types, interfaces, and enums
ALWAYS use descriptive names that indicate the structure for types, interfaces, and enums
ALWAYS use suffixes like Props, State, Config, Type when appropriate for types and interfaces
ALWAYS use UPPER_SNAKE_CASE for constants and configuration values
ALWAYS use d...
Files:
packages/utils/src/timeout.tssrc/state/apis/swapper/swapperApi.ts
**/*
📄 CodeRabbit inference engine (.cursor/rules/naming-conventions.mdc)
**/*: ALWAYS use appropriate file extensions
Flag files without kebab-case
Files:
packages/utils/src/timeout.tssrc/state/apis/swapper/swapperApi.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/react-best-practices.mdc)
USE Redux only for global state shared across multiple places
Files:
packages/utils/src/timeout.tssrc/state/apis/swapper/swapperApi.ts
**/swapper/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
**/swapper/**/*.{ts,tsx}: AVOID throwing within swapper API implementations; return Err() instead. UI layers may still throw (caught by React error boundaries).
ALWAYS use makeSwapErrorRight for swapper errors
ALWAYS use TradeQuoteError enum for error codes in swapper errors
ALWAYS provide detailed error information in swapper errors
Files:
src/state/apis/swapper/swapperApi.ts
🧠 Learnings (13)
📓 Common learnings
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/error-handling.mdc:0-0
Timestamp: 2025-08-03T22:09:37.542Z
Learning: Applies to **/*.{ts,tsx} : ALWAYS use timeoutMonadic for API calls
📚 Learning: 2025-08-03T22:09:37.542Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/error-handling.mdc:0-0
Timestamp: 2025-08-03T22:09:37.542Z
Learning: Applies to **/*.{ts,tsx} : ALWAYS use timeoutMonadic for API calls
Applied to files:
packages/utils/src/timeout.ts
📚 Learning: 2025-08-03T22:09:37.542Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/error-handling.mdc:0-0
Timestamp: 2025-08-03T22:09:37.542Z
Learning: Applies to **/*.{ts,tsx} : Missing timeout handling is an anti-pattern
Applied to files:
packages/utils/src/timeout.ts
📚 Learning: 2025-08-03T22:09:37.542Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/error-handling.mdc:0-0
Timestamp: 2025-08-03T22:09:37.542Z
Learning: Applies to **/swapper/**/*.{ts,tsx} : ALWAYS use TradeQuoteError enum for error codes in swapper errors
Applied to files:
src/state/apis/swapper/swapperApi.ts
📚 Learning: 2025-07-24T09:43:11.699Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/swapper.mdc:0-0
Timestamp: 2025-07-24T09:43:11.699Z
Learning: Applies to packages/swapper/src/swappers/*/{*.ts,endpoints.ts} : Avoid side effects in swap logic within swapper implementation files.
Applied to files:
src/state/apis/swapper/swapperApi.ts
📚 Learning: 2025-07-24T09:43:11.699Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/swapper.mdc:0-0
Timestamp: 2025-07-24T09:43:11.699Z
Learning: Applies to packages/swapper/src/swappers/*/{*.ts,endpoints.ts} : Include comments explaining swap logic in swapper implementation files.
Applied to files:
src/state/apis/swapper/swapperApi.ts
📚 Learning: 2025-07-31T03:51:48.479Z
Learnt from: premiumjibles
PR: shapeshift/web#10154
File: src/state/apis/swapper/helpers/swapperApiHelpers.ts:57-60
Timestamp: 2025-07-31T03:51:48.479Z
Learning: In src/state/apis/swapper/helpers/swapperApiHelpers.ts, the getState parameter in processQuoteResultWithRatios uses `() => unknown` type instead of `() => ReduxState` to avoid type compatibility issues elsewhere in the codebase.
Applied to files:
src/state/apis/swapper/swapperApi.ts
📚 Learning: 2025-08-03T22:09:37.542Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/error-handling.mdc:0-0
Timestamp: 2025-08-03T22:09:37.542Z
Learning: Applies to **/swapper/**/*.{ts,tsx} : ALWAYS use makeSwapErrorRight for swapper errors
Applied to files:
src/state/apis/swapper/swapperApi.ts
📚 Learning: 2025-08-03T22:09:37.542Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/error-handling.mdc:0-0
Timestamp: 2025-08-03T22:09:37.542Z
Learning: Applies to **/swapper/**/*.{ts,tsx} : ALWAYS provide detailed error information in swapper errors
Applied to files:
src/state/apis/swapper/swapperApi.ts
📚 Learning: 2025-07-24T09:43:11.699Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/swapper.mdc:0-0
Timestamp: 2025-07-24T09:43:11.699Z
Learning: Applies to packages/swapper/src/swappers/*/{*.ts,endpoints.ts} : Leverage shared utilities (e.g., executeEvmTransaction, checkEvmSwapStatus) in swapper implementations when applicable.
Applied to files:
src/state/apis/swapper/swapperApi.ts
📚 Learning: 2025-07-24T09:43:11.699Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/swapper.mdc:0-0
Timestamp: 2025-07-24T09:43:11.699Z
Learning: Applies to packages/swapper/**/*.ts : Use TypeScript with explicit types (e.g., SupportedChainIds) in all swapper-related files.
Applied to files:
src/state/apis/swapper/swapperApi.ts
📚 Learning: 2025-07-24T09:43:11.699Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/swapper.mdc:0-0
Timestamp: 2025-07-24T09:43:11.699Z
Learning: Applies to packages/swapper/src/{constants,types}.ts : Register new swappers in packages/swapper/src/constants.ts and add them to the SwapperName enum in packages/swapper/src/types.ts.
Applied to files:
src/state/apis/swapper/swapperApi.ts
📚 Learning: 2025-07-24T09:43:11.699Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/swapper.mdc:0-0
Timestamp: 2025-07-24T09:43:11.699Z
Learning: Applies to packages/swapper/src/swappers/*/{*.ts,endpoints.ts} : Validate inputs and log errors for debugging in swapper implementations.
Applied to files:
src/state/apis/swapper/swapperApi.ts
🧬 Code graph analysis (1)
src/state/apis/swapper/swapperApi.ts (2)
src/state/apis/swapper/types.ts (1)
TradeQuoteError(39-39)src/state/apis/swapper/helpers/swapperApiHelpers.ts (1)
processQuoteResultWithRatios(56-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install and Cache
🔇 Additional comments (4)
packages/utils/src/timeout.ts (1)
34-50: Solid utility addition; matches monadic timeout behavior while exposing a fallback.The shape
{ result, fallback }is appropriate and minimally invasive.src/state/apis/swapper/swapperApi.ts (3)
4-4: Correct import for Timeout detection.Using
TradeQuoteErrorto discriminate timeouts is the right approach.
153-168: Two-phase fetch (gather → process) enables clean fallback semantics.Collecting raw results before processing makes the “no quotes + has timeouts” check possible with negligible overhead.
176-182: Timeout detection LGTM.Accurately scopes fallback to when at least one timeout occurred.
…e get nothing else
3be96fa to
5dfe5ea
Compare
0xApotheosis
left a comment
0xApotheosis
left a comment
There was a problem hiding this comment.
One nitpick on naming, and possibly one coderabbit comment to action - otherwise looking good. Modified BULK_FETCH_RATE_TIMEOUT_MS to test the way results are handled, looking good.
|
@0xApotheosis actioned both here. Agree the renaming is clearer, i've never been a huge fan of "left" and "right" naming 😆 . 5322561 |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/state/apis/swapper/swapperApi.ts (1)
169-176: Fix confirmed: Ok([]) now counts as “no quotes”.This closes the fallback suppression edge case. Nice.
🧹 Nitpick comments (2)
src/state/apis/swapper/swapperApi.ts (2)
4-4: Ensure TradeQuoteError is a runtime enum (not a type) and avoid name collision.We use
TradeQuoteError.Timeoutas a runtime value below. Please confirm@shapeshiftoss/swapperexports it as an enum/value. If there’s any collision with localTradeQuoteErrortypes, consider aliasing to make intent explicit.Example alias (only if needed):
-import { getTradeQuotes, getTradeRates, TradeQuoteError } from '@shapeshiftoss/swapper' +import { getTradeQuotes, getTradeRates, TradeQuoteError as SwapperTradeQuoteError } from '@shapeshiftoss/swapper'And then use
SwapperTradeQuoteError.Timeoutat the call sites.
184-195: Avoid unbounded waits on fallback; guard with try/catch.Awaiting the original (fallback) promise can hang indefinitely, stalling RTK Query resolution when all swappers time out. Add a try/catch so a failing fallback doesn’t turn this branch into a rejected promise, and consider capping the fallback wait (e.g., 20–30s) to prevent indefinite hangs.
Apply this minimal safety net within the current block:
- const isTimeout = - rateResult?.isErr() && rateResult.unwrapErr().code === TradeQuoteError.Timeout - const rateResultWithFallback = - noQuotes && hasTimeoutQuote && isTimeout && rateResult?.fallback - ? { ...(await rateResult.fallback), swapperName } - : rateResult + const isTimeout = + rateResult?.isErr() && rateResult.unwrapErr().code === TradeQuoteError.Timeout + let rateResultWithFallback = rateResult + if (noQuotes && hasTimeoutQuote && isTimeout && rateResult?.fallback) { + try { + rateResultWithFallback = { ...(await rateResult.fallback), swapperName } + } catch (_e) { + // keep original timed-out result; proceed without fallback + } + }Optional (outside this hunk): wrap
rateResult.fallbackwith a longer-but-finite timeout to avoid indefinite hangs (e.g.,FALLBACK_RATE_TIMEOUT_MS = 30000) using your existing timeout helper.Also applies to: 200-203
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/swapper/src/swapper.ts(3 hunks)packages/utils/src/timeout.ts(1 hunks)src/state/apis/swapper/swapperApi.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/utils/src/timeout.ts
- packages/swapper/src/swapper.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
**/*.{ts,tsx}: ALWAYS use Result<T, E> pattern for error handling in swappers and APIs
ALWAYS use Ok() and Err() from @sniptt/monads for monadic error handling
ALWAYS use custom error classes from @shapeshiftoss/errors
ALWAYS provide meaningful error codes for internationalization
ALWAYS include relevant details in error objects
ALWAYS wrap async operations in try-catch blocks
ALWAYS use AsyncResultOf utility for converting promises to Results
ALWAYS provide fallback error handling
ALWAYS use timeoutMonadic for API calls
ALWAYS provide appropriate timeout values for API calls
ALWAYS handle timeout errors gracefully
ALWAYS validate inputs before processing
ALWAYS provide clear validation error messages
ALWAYS use early returns for validation failures
ALWAYS log errors for debugging
ALWAYS use structured logging for errors
ALWAYS include relevant context in error logs
Throwing errors instead of using monadic patterns is an anti-pattern
Missing try-catch blocks for async operations is an anti-pattern
Generic error messages without context are an anti-pattern
Not handling specific error types is an anti-pattern
Missing timeout handling is an anti-pattern
No input validation is an anti-pattern
Poor error logging is an anti-pattern
Using any for error types is an anti-pattern
Missing error codes for internationalization is an anti-pattern
No fallback error handling is an anti-pattern
Console.error without structured logging is an anti-pattern
**/*.{ts,tsx}: ALWAYS use camelCase for variables, functions, and methods
ALWAYS use descriptive names that explain the purpose for variables and functions
ALWAYS use verb prefixes for functions that perform actions
ALWAYS use PascalCase for types, interfaces, and enums
ALWAYS use descriptive names that indicate the structure for types, interfaces, and enums
ALWAYS use suffixes like Props, State, Config, Type when appropriate for types and interfaces
ALWAYS use UPPER_SNAKE_CASE for constants and configuration values
ALWAYS use d...
Files:
src/state/apis/swapper/swapperApi.ts
**/swapper/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
**/swapper/**/*.{ts,tsx}: AVOID throwing within swapper API implementations; return Err() instead. UI layers may still throw (caught by React error boundaries).
ALWAYS use makeSwapErrorRight for swapper errors
ALWAYS use TradeQuoteError enum for error codes in swapper errors
ALWAYS provide detailed error information in swapper errors
Files:
src/state/apis/swapper/swapperApi.ts
**/*
📄 CodeRabbit inference engine (.cursor/rules/naming-conventions.mdc)
**/*: ALWAYS use appropriate file extensions
Flag files without kebab-case
Files:
src/state/apis/swapper/swapperApi.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/react-best-practices.mdc)
USE Redux only for global state shared across multiple places
Files:
src/state/apis/swapper/swapperApi.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/error-handling.mdc:0-0
Timestamp: 2025-08-03T22:09:37.542Z
Learning: Applies to **/*.{ts,tsx} : ALWAYS use timeoutMonadic for API calls
📚 Learning: 2025-08-03T22:09:37.542Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/error-handling.mdc:0-0
Timestamp: 2025-08-03T22:09:37.542Z
Learning: Applies to **/swapper/**/*.{ts,tsx} : ALWAYS use TradeQuoteError enum for error codes in swapper errors
Applied to files:
src/state/apis/swapper/swapperApi.ts
📚 Learning: 2025-07-24T09:43:11.699Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/swapper.mdc:0-0
Timestamp: 2025-07-24T09:43:11.699Z
Learning: Applies to packages/swapper/src/swappers/*/{*.ts,endpoints.ts} : Avoid side effects in swap logic within swapper implementation files.
Applied to files:
src/state/apis/swapper/swapperApi.ts
📚 Learning: 2025-07-31T03:51:48.479Z
Learnt from: premiumjibles
PR: shapeshift/web#10154
File: src/state/apis/swapper/helpers/swapperApiHelpers.ts:57-60
Timestamp: 2025-07-31T03:51:48.479Z
Learning: In src/state/apis/swapper/helpers/swapperApiHelpers.ts, the getState parameter in processQuoteResultWithRatios uses `() => unknown` type instead of `() => ReduxState` to avoid type compatibility issues elsewhere in the codebase.
Applied to files:
src/state/apis/swapper/swapperApi.ts
📚 Learning: 2025-07-24T09:43:11.699Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/swapper.mdc:0-0
Timestamp: 2025-07-24T09:43:11.699Z
Learning: Applies to packages/swapper/src/swappers/*/{*.ts,endpoints.ts} : Include comments explaining swap logic in swapper implementation files.
Applied to files:
src/state/apis/swapper/swapperApi.ts
📚 Learning: 2025-08-03T22:09:37.542Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/error-handling.mdc:0-0
Timestamp: 2025-08-03T22:09:37.542Z
Learning: Applies to **/swapper/**/*.{ts,tsx} : ALWAYS use makeSwapErrorRight for swapper errors
Applied to files:
src/state/apis/swapper/swapperApi.ts
📚 Learning: 2025-08-03T22:09:37.542Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/error-handling.mdc:0-0
Timestamp: 2025-08-03T22:09:37.542Z
Learning: Applies to **/swapper/**/*.{ts,tsx} : ALWAYS provide detailed error information in swapper errors
Applied to files:
src/state/apis/swapper/swapperApi.ts
📚 Learning: 2025-07-24T09:43:11.699Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/swapper.mdc:0-0
Timestamp: 2025-07-24T09:43:11.699Z
Learning: Applies to packages/swapper/src/swappers/*/{*.ts,endpoints.ts} : Leverage shared utilities (e.g., executeEvmTransaction, checkEvmSwapStatus) in swapper implementations when applicable.
Applied to files:
src/state/apis/swapper/swapperApi.ts
📚 Learning: 2025-08-03T22:09:37.542Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/error-handling.mdc:0-0
Timestamp: 2025-08-03T22:09:37.542Z
Learning: Applies to **/swapper/**/*.{ts,tsx} : AVOID throwing within swapper API implementations; return Err() instead. UI layers may still throw (caught by React error boundaries).
Applied to files:
src/state/apis/swapper/swapperApi.ts
📚 Learning: 2025-07-24T09:43:11.699Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/swapper.mdc:0-0
Timestamp: 2025-07-24T09:43:11.699Z
Learning: Applies to packages/swapper/src/swappers/*/{*.ts,endpoints.ts} : Verify chain ID filtering in filterAssetIdsBySellable and filterBuyAssetsBySellAssetId methods.
Applied to files:
src/state/apis/swapper/swapperApi.ts
🧬 Code graph analysis (1)
src/state/apis/swapper/swapperApi.ts (2)
src/state/apis/swapper/types.ts (1)
TradeQuoteError(39-39)src/state/apis/swapper/helpers/swapperApiHelpers.ts (1)
processQuoteResultWithRatios(56-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Call / Static
🔇 Additional comments (2)
src/state/apis/swapper/swapperApi.ts (2)
153-167: Good split: fetch all rates first, then process.Using
Promise.allSettledto stage results before processing is clear and keeps concurrency. LGTM.
177-183: Timeout detection logic is sound.Checks for
Timeouton fulfilled results only; that’s appropriate here.
2 participants
Description
Let's quotes through in the swapper even if they take longer than 5 seconds if we have no other quotes
Issue (if applicable)
closes #10371
Risk
Medium-high risk, code is fairly surgical and should only kick in on the edge case where there's a timeout and no quote results but this is still important code so marking as medium-high but it's potentially high.
Testing
Engineering
Easier to test this if you monkey patch BULK_FETCH_RATE_TIMEOUT_MS to be lower
Operations
Screenshots (if applicable)
Easiest to test this on quotes that only have thor swaps. But Thor was down at the time of testing so I lowered the timeout all the way down to 500ms. Which on my internet would make all quotes fail. Notice it still works because no quotes came through and it's resorting to no timeout.
https://jam.dev/c/7ab7124a-f0e0-4073-8b64-36e9230e0456
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Breaking Changes