feat(trade): cache quotes #5473
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
WalkthroughThe changes streamline several modules in the cowswap-frontend codebase by removing redundant trade quote reset functionality and simplifying state management. The reset logic, related hooks, and props have been removed from advanced orders and trade widget components. A new modular hook, Changes
Sequence Diagram(s)sequenceDiagram
participant TW as TradeWidget
participant TQM as TradeQuoteManager
participant FAP as FetchAndProcessQuote
participant AT as Atom (TradeQuotes State)
TW->>TQM: Initiate trade quote request
TQM->>FAP: fetchAndProcessQuote(params)
FAP-->>TQM: Return API response or error
TQM->>AT: Update trade quote state
TQM-->>TW: Return updated quote data
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| const cowAnalytics = useCowAnalytics() | ||
| const debouncedTradeAmountAnalytics = useMemo(() => createDebouncedTradeAmountAnalytics(cowAnalytics), [cowAnalytics]) | ||
|
|
||
| const updateAdvancedOrdersState = useUpdateAdvancedOrdersRawState() | ||
|
|
||
| const onCurrencySelection = useCallback( | ||
| (field: Field, currency: Currency | null) => { | ||
| // Reset the output field until we fetch quote for new selected token | ||
| // This is to avoid displaying wrong amounts in output field |
There was a problem hiding this comment.
This was an execessive amount reset which caused sell amount resetting to 1 when you change sell token
|
|
||
| const onSwitchTokens = useCallback(() => { | ||
| onSwitchTokensDefault() | ||
| resetTradeQuote() |
There was a problem hiding this comment.
Also an excessive quote resetting, this is already handled in useTradeQuotePolling
![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
🧹 Nitpick comments (5)
apps/cowswap-frontend/src/modules/volumeFee/state/volumeFeeAtom.ts (1)
49-58: Code simplification improves readability.The refactoring removes unnecessary variable assignment and debug logging while maintaining the same logical behavior for correlated token detection.
Consider adding a comment explaining that this function checks if the trading pair consists of correlated tokens that should skip fee calculation:
// If there are two tokens in the list, it means that it is a pair correlated token } else { + // Check if both tokens in the pair are correlated (exist in the same correlation group) return tokens[inputCurrencyAddress] && tokens[outputCurrencyAddress] }apps/cowswap-frontend/src/modules/tradeQuote/services/fetchAndProcessQuote.ts (2)
24-24: Confirm loading-state logicCalling
tradeQuoteManager.setLoading(hasParamsChanged)on every fetch attempt might introduce flicker if multiple rapid calls occur. Consider throttling or debouncing quotes for improved user experience when the parameters change repeatedly in quick succession.
39-39: Improve error loggingUsing
console.logfor production-level error reporting is limited. Consider integrating a structured logging or monitoring service (e.g., Sentry) for better observability of quote failures in real-world usage.apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts (2)
32-34: Guard against undefined sellToken more explicitly.
The code retrievessellTokenfromquoteParamsand immediately creates a manager. While there's a check at line 48, you might consider proactively logging a warning or providing fallback behavior whensellTokenis undefined.
65-66: Consider adding localized error handling.
fetchQuotedelegates tofetchAndProcessQuote, which handles errors via the manager. If additional fallback or user notifications are needed, ensure they’re triggered within this function or via the manager.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
apps/cowswap-frontend/src/modules/advancedOrders/hooks/useAdvancedOrdersActions.ts(2 hunks)apps/cowswap-frontend/src/modules/trade/containers/TradeWidget/TradeWidgetUpdaters.tsx(1 hunks)apps/cowswap-frontend/src/modules/trade/containers/TradeWidget/index.tsx(1 hunks)apps/cowswap-frontend/src/modules/trade/containers/TradeWidget/types.ts(0 hunks)apps/cowswap-frontend/src/modules/trade/state/receiveAmountInfoAtom.ts(1 hunks)apps/cowswap-frontend/src/modules/tradeQuote/hooks/useResetTradeQuote.ts(0 hunks)apps/cowswap-frontend/src/modules/tradeQuote/hooks/useSetTradeQuoteParams.ts(1 hunks)apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuote.ts(2 hunks)apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuoteManager.ts(1 hunks)apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts(5 hunks)apps/cowswap-frontend/src/modules/tradeQuote/hooks/useUpdateTradeQuote.ts(0 hunks)apps/cowswap-frontend/src/modules/tradeQuote/index.ts(1 hunks)apps/cowswap-frontend/src/modules/tradeQuote/services/fetchAndProcessQuote.ts(1 hunks)apps/cowswap-frontend/src/modules/tradeQuote/state/tradeQuoteAtom.ts(2 hunks)apps/cowswap-frontend/src/modules/tradeQuote/state/tradeQuoteInputAtom.ts(1 hunks)apps/cowswap-frontend/src/modules/tradeQuote/utils/quoteUsingSameParameters.ts(2 hunks)apps/cowswap-frontend/src/modules/volumeFee/state/volumeFeeAtom.ts(1 hunks)libs/tokens/src/hooks/tokens/unsupported/useUnsupportedTokens.ts(1 hunks)
💤 Files with no reviewable changes (3)
- apps/cowswap-frontend/src/modules/tradeQuote/hooks/useResetTradeQuote.ts
- apps/cowswap-frontend/src/modules/trade/containers/TradeWidget/types.ts
- apps/cowswap-frontend/src/modules/tradeQuote/hooks/useUpdateTradeQuote.ts
✅ Files skipped from review due to trivial changes (1)
- apps/cowswap-frontend/src/modules/tradeQuote/hooks/useSetTradeQuoteParams.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Cypress
🔇 Additional comments (30)
libs/tokens/src/hooks/tokens/unsupported/useUnsupportedTokens.ts (2)
4-6: Good refactoring to improve code quality and state management.Adding
DEFAULT_UNSUPPORTED_TOKENS_STATEas a constant makes the code more maintainable and follows the best practice of centralizing default values rather than creating them inline. This approach will make it easier to modify the default state in the future if needed.
9-9: Great improvement for consistency.Using the constant instead of an inline empty object ensures consistent default state throughout the application. This change aligns well with the PR's objective of improving state management around trading functionality.
apps/cowswap-frontend/src/modules/tradeQuote/state/tradeQuoteInputAtom.ts (1)
5-5: Type definition looks good for supporting multiple quote management.The addition of this type alias provides a clear semantic meaning for strings representing sell token addresses, which will be useful for the quote caching functionality.
apps/cowswap-frontend/src/modules/tradeQuote/utils/quoteUsingSameParameters.ts (2)
1-1: Import of LatestAppDataDocVersion enhances type safety.Adding this type import ensures proper type checking when working with app data structures.
50-51: Successfully implemented hook ignoring in quote caching.The updated destructuring now properly ignores both quote metadata and hooks when comparing app data, which aligns with the PR objective of modifying quote triggering logic to ignore hooks in appData. This will help prevent unnecessary quote refreshes when only hooks change.
Adding the type assertion to
decoded as LatestAppDataDocVersionimproves type safety while destructuring.apps/cowswap-frontend/src/modules/tradeQuote/index.ts (1)
7-7:❓ Verification inconclusive
Good implementation of centralized quote management.
Exporting the new
useTradeQuoteManagerhook provides a more comprehensive approach to quote management, likely facilitating the caching functionality mentioned in the PR objectives.
🏁 Script executed:
#!/bin/bash # Check if any components still try to import the removed hooks echo "Checking for any remaining imports of removed hooks..." rg "useResetTradeQuote|useUpdateTradeQuote" --type ts --type tsxLength of output: 213
Below is a revised verification script that uses glob patterns instead of file type flags. This should correctly search both
.tsand.tsxfiles for any lingering imports of the removed hooks. Please run the following script and attach the results for further confirmation:#!/bin/bash echo "Verifying that there are no lingering references to removed hooks in .ts and .tsx files..." rg "useResetTradeQuote|useUpdateTradeQuote" -g "*.ts" -g "*.tsx"Once you confirm that no output is produced (i.e. no stale imports exist), we can finalize the review comment approval.
apps/cowswap-frontend/src/modules/trade/containers/TradeWidget/index.tsx (1)
11-11: Simplified params management aligns with caching strategy.The removal of
tradeQuoteStateOverridefrom the destructured params is part of the broader caching implementation strategy. This change streamlines the component by removing unnecessary state override functionality.apps/cowswap-frontend/src/modules/advancedOrders/hooks/useAdvancedOrdersActions.ts (2)
32-32: Simplified dependency array after removing quote reset logic.The dependency array has been properly reduced after removing the quote reset functionality, which supports the caching mechanism being implemented.
54-54: Simplified token switching implementation.Direct usage of
useSwitchTokensPlaceswithout an intermediate function streamlines the code while maintaining the override behavior for output currency amount. This change allows quotes to persist when switching tokens back and forth, supporting the caching functionality.apps/cowswap-frontend/src/modules/trade/containers/TradeWidget/TradeWidgetUpdaters.tsx (2)
1-1: Removed unused imports along with state override functionality.Properly removing the
useEffectimport after its associated hook was removed keeps the codebase clean.
42-42:❓ Verification inconclusive
Verify quote polling behavior with caching implementation.
The TradeQuoteUpdater is only added when quote polling is enabled. Ensure this behavior aligns with the new caching mechanism to prevent situations where cached quotes aren't refreshed when they should be.
🏁 Script executed:
#!/bin/bash # Check implementation of TradeQuoteUpdater to understand how it interacts with the new caching system rg -A 10 "export function TradeQuoteUpdater" --type typescriptLength of output: 99
I'll need to gather the missing details by querying the repository using a revised search pattern. The previous command failed due to an unrecognized file type ("typescript"). Please run the following shell script to search for the definition of
TradeQuoteUpdaterin the relevant TypeScript/TSX files:Once you run this script, please verify that the implementation of
TradeQuoteUpdatercorrectly integrates with the new caching mechanism. In particular, ensure that when quote polling is enabled (i.e., when<TradeQuoteUpdater />is rendered), it triggers a refresh of quotes as required by the caching logic.Next Steps:
- Verify that the refreshed quotes logic is executed when polling is enabled.
- Confirm that cached quotes do not persist erroneously when they should be updated.
If after running this new script all behavior aligns with the expected caching updates, we can close this review comment. Until then, please double-check the integration manually.
🏁 Script executed:
#!/bin/bash echo "Searching for TradeQuoteUpdater in TSX files:" rg -A 10 "export function TradeQuoteUpdater" -g "*.tsx" echo "Searching for TradeQuoteUpdater in TS files:" rg -A 10 "export function TradeQuoteUpdater" -g "*.ts"Length of output: 1227
Action Required: Verify Refresh Logic for Cached Quote Polling
The current implementation in
apps/cowswap-frontend/src/modules/trade/containers/TradeWidget/TradeWidgetUpdaters.tsxconditionally renders<TradeQuoteUpdater />based on the disabled state of quote polling. Our search confirms thatTradeQuoteUpdateris defined in
apps/cowswap-frontend/src/modules/tradeQuote/updaters/TradeQuoteUpdater/index.tsxand usesuseTradeQuotePolling()along with other related hooks.Please ensure that this polling behavior correctly integrates with the new caching mechanism. In particular, verify that when quote polling is enabled (and
<TradeQuoteUpdater />is rendered):
- The polling (via
useTradeQuotePolling) triggers a refresh of quotes even when cached quotes exist.- The caching logic (likely tied to
useTradeQuoteand related hooks) correctly invalidates or updates cached quotes as needed to avoid stale data.If the current behavior does not refresh cached quotes when required, further adjustments will be necessary to align the polling with the caching update strategy.
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuoteManager.ts (5)
14-19: Well-designed interface for quote management.The
TradeQuoteManagerinterface provides a clean and comprehensive API for managing trade quotes, with clear methods for setting loading states, resetting, and handling responses and errors.
29-35: Effective loading state management with parameter change awareness.The
setLoadingmethod intelligently handles the loading state based on whether parameters have changed, only clearing the response when necessary. This approach preserves cached responses during loading when possible.
41-45: Good error handling with specific processing for unsupported tokens.The error handling specifically processes unsupported token errors, which is important for providing a good user experience when users attempt to trade non-supported tokens.
21-62:❓ Verification inconclusive
Ensure quote caching implementation meets testing requirements.
The PR objectives specify testing scenarios for WETH to COW trading pair. This implementation caches quotes by sell token address, which should satisfy the requirements for caching when switching between tokens. However, verify this behavior meets all test cases described in the PR, particularly step 4 where switching back to WETH should display the price instantly.
🏁 Script executed:
#!/bin/bash # Check how the tradeQuotesAtom is structured to understand how quotes are stored by token rg -A 15 "tradeQuotesAtom" --type typescriptLength of output: 79
Action Required: Validate Trade Quote Caching Mechanism
- The implementation caches quotes keyed by the sell token address, which appears to meet the PR's objectives.
- An initial search using
rgwith--type typescriptproduced an error. Please re-run using--type ts(as shown above) to inspect the internal structure oftradeQuotesAtom.- Manually verify that when switching back to WETH, the cached quote (price) is displayed instantly, aligning with test step 4.
🏁 Script executed:
#!/bin/bash # Re-run the search command for tradeQuotesAtom using the recognized "ts" file type for TypeScript files rg -A 15 "tradeQuotesAtom" --type tsLength of output: 7499
Action: Confirm that the quote caching behavior satisfies test step 4 requirements
Based on our review of the code, the caching mechanism stores trade quotes keyed by a lowercased token address and retrieves them correctly in the
useTradeQuotehook. In particular:
- The
updateTradeQuoteAtomfunction in apps/cowswap-frontend/src/modules/tradeQuote/state/tradeQuoteAtom.ts lowercases the sell token address and merges updates appropriately.- The
useTradeQuotehook fetches the cached quote using the lowercased address (viagetCurrencyAddress(inputCurrency).toLowerCase()), which should ensure that switching back to WETH displays the previously fetched price instantly.Please ensure that integration tests—especially the scenario where switching back to WETH (as defined in step 4 of the PR requirements) should immediately show the cached quote—confirm this behavior.
46-57:❓ Verification inconclusive
Verify loading state behavior for non-optimal quotes.
The
onResponsemethod only setsisLoading: falsewhen the quote is optimal. This might lead to a perpetual loading state for non-optimal quotes. Ensure this is the intended behavior and that there's a mechanism to eventually update the loading state for all quotes.
🏁 Script executed:
#!/bin/bash # Check usage of PriceQuality enum to understand the implications of only setting isLoading to false for optimal quotes rg -A 3 "PriceQuality" --type typescript # Check if there's any other mechanism that updates the loading state for non-optimal quotes rg "isLoading: false" --type typescript --glob "*tradeQuote*"Length of output: 174
Subject: Verify isLoading Behavior for Non-Optimal Quotes
The original review comment raised a concern that in the
onResponsemethod,isLoading: falseis only set when the quote is optimal. This could potentially leave non-optimal quote states perpetually loading unless another mechanism updates them.To clarify this behavior, please run the above shell script. It uses more precise file filters (using
*.ts) to search:
- For usage of the
PriceQualityenum,- For instances where
isLoading: falseis set in trade quote modules, and- For the definition of the
updatefunction that might clarify howisLoadingis managed.Based on the output:
- If another mechanism or later update sets
isLoadingto false for non-optimal quotes, then the behavior is as intended.- If not, this might indicate that non-optimal quotes could indeed remain in a loading state indefinitely, which may require a fix.
Please verify the script outputs to ensure the desired behavior is fully understood.
🏁 Script executed:
#!/bin/bash # Re-run the command using a glob pattern for TypeScript files echo "Searching for PriceQuality usage in TypeScript files:" rg -A 3 "PriceQuality" -g "*.ts" echo -e "\nSearching for any instance where 'isLoading: false' is set in tradeQuote-related files:" rg "isLoading: false" -g "*tradeQuote*.ts" echo -e "\nSearching for the definition of the update function to check its behavior regarding isLoading:" rg -n "function update(" -g "*.ts"Length of output: 9075
Subject: Verify Loading State for Non-Optimal Quotes
Our cursory investigation confirms that in the
onResponsemethod (inapps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuoteManager.ts), the update only includes settingisLoading: falsewhen the quote is optimal. The searches indicate that aside from an initial state set intradeQuoteAtom.ts, there isn’t an obvious alternate update path for non-optimal quotes.Please manually verify the following:
- That non-optimal quotes (when
requestParams.priceQuality !== PriceQuality.OPTIMAL) are eventually updated to clear the loading state.- Whether the
updatefunction (or any subsequent process) applies anisLoading: falseupdate for non-optimal quotes in another code path.If this behavior is intentional (for example, if non-optimal quotes are handled via a different mechanism), adding an inline comment to clarify would be beneficial. Otherwise, a fix might be needed to ensure that non-optimal quotes do not remain in a perpetual loading state.
apps/cowswap-frontend/src/modules/tradeQuote/services/fetchAndProcessQuote.ts (1)
9-11: Validate concurrency approachUsing
onlyResolvesLastfor bothgetFastQuoteandgetOptimalQuoteis an elegant solution to avoid stale requests. However, ensure there isn’t any unforeseen concurrency scenario where additional quote methods or side effects might bypass the protective logic.apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuote.ts (1)
27-28: Anticipate potential address variationsWhen indexing into
tradeQuoteswithgetCurrencyAddress(inputCurrency).toLowerCase(), ensure addresses are consistently normalized across the app. If different cases or chain-based address formats slip through, they could lead to missing quotes.apps/cowswap-frontend/src/modules/trade/state/receiveAmountInfoAtom.ts (1)
17-17: Validate indexing logic
tradeQuotes[getCurrencyAddress(inputCurrency).toLowerCase()]assumesinputCurrencyis always valid. If an unsupported or undefined currency arrives, the function might yield unintended behavior. Consider additional safeguards or fallbacks for invalid inputs.apps/cowswap-frontend/src/modules/tradeQuote/state/tradeQuoteAtom.ts (2)
30-30: Good shift to multiple quotesSwitching from a single
tradeQuoteAtomtotradeQuotesAtomimproves support for multiple tokens. This design is cleaner and more flexible for multi-quote scenarios.
32-62: Review overshadowing logicThe check for ignoring a “fast” quote when an “optimal” quote finishes first (lines 41–45) is a helpful approach to prevent overwriting a more accurate quote. However, ensure that all relevant states (e.g., partial re-fetch, or chain-specific quotes) are considered so that valid quotes are not inadvertently discarded.
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts (9)
5-5: New import is consistent with usage.
This import ofPriceQualityaligns well with the usage in lines 87 and 88.
15-17: Ensure consistent usage of newly introduced modules.
AddinguseTradeQuoteManagerandfetchAndProcessQuotecentralizes quote-handling logic, which is a clear improvement. Make sure all old references to the previous quote functions are removed or updated elsewhere in the codebase to avoid confusion.
48-48: Good defensive check.
Early return if the manager is missing is a solid safeguard, ensuring we don’t proceed with incomplete data.
52-55: Validate the zero-amount condition.
Resetting the manager whenquoteParamsis falsy or the amount is'0'seems logical. Confirm that this won’t unintentionally clear quote state in use cases where the user is still inputting a value.
58-58: Skipping unsupported tokens is appropriate.
Good check to prevent wasted API calls for unsupported tokens.
68-68: Consolidate fetching logic is a nice simplification.
fetchAndUpdateQuoteconsolidates the old multiple quote-fetching calls. This improves maintainability, but double-check no prior overrides or special handling got lost during refactoring.
87-88: Parallel fetching may introduce race conditions.
InvokingfetchQuotefor both FAST and OPTIMAL quotes in quick succession could overlap. EnsuretradeQuoteManagerconcurrency handling is robust so that the final state always reflects the correct quote.
117-117: Resetting the manager on quote expiration avoids repeated checks.
PerformingtradeQuoteManager.reset()once expired is helpful. This logic effectively prevents indefinite expired state checks.
129-129: Dependency array might cause extra effect runs.
AddingtradeQuoteManagerto dependencies ensures that re-instantiating the manager triggers the effect. Confirm this won't cause unintended re-fetches or reset loops.
| @@ -56,16 +56,6 @@ const shouldSkipFeeAtom = atom<boolean>((get) => { | |||
| return tokens[inputCurrencyAddress] && tokens[outputCurrencyAddress] | |||
| } | |||
| }) | |||
|
|
|||
| if (isCorrelated) { | |||
| console.debug('[Volume Fee] Skipping fee for correlated tokens', { | |||
There was a problem hiding this comment.
Just remove noisy log
apps/cowswap-frontend/src/modules/tradeQuote/utils/quoteUsingSameParameters.ts
Outdated
Show resolved
Hide resolved
| fetchStartTimestamp: number | ||
| } | ||
|
|
||
| export async function fetchAndProcessQuote( |
There was a problem hiding this comment.
This was extracted from useTradeQuotePolling, no business logic changes here
| onResponse(data: OrderQuoteResponse, requestParams: FeeQuoteParams, fetchStartTimestamp: number): void | ||
| } | ||
|
|
||
| export function useTradeQuoteManager(sellTokenAddress: SellTokenAddress | undefined): TradeQuoteManager | null { |
There was a problem hiding this comment.
Extracted the tradeQuotesAtom state updates logic in TradeQuoteManager in order to make it more clear what are the updates doing.
| const priorityTokenAddresses = usePriorityTokenAddresses() | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
tradeQuoteStateOverride was not in use, just deleted it
| } | ||
| }) | ||
| }) | ||
| export const tradeQuotesAtom = atom<Record<SellTokenAddress, TradeQuoteState | undefined>>({}) |
There was a problem hiding this comment.
Here we have the most important changes. Changes the state from single TradeQuoteState to a map.
There was a problem hiding this comment.
I didn't get why the key is the sell token?
There was a problem hiding this comment.
Basically, I replicated the state from the legacy: https://github.com/cowprotocol/cowswap/blob/release/2024-07-02/apps/cowswap-frontend/src/legacy/state/price/reducer.ts#L26
Sorry, I din't your question, what is supposed to be a key?
If you are worried about other parameters such as amount, kind, etc., the quote refetches when any of them are changed.
The caching only works when you switch token places or change sell token forth and back without amount changes (see "To Test" in the PR description.)
There was a problem hiding this comment.
Hey @shoom3301 , thank you!
Some issues (differences between barn version and the current one):
- when a quote is resulted with error, it should be also cached (like it is on Barn). In the current PR a quote is triggered again when flip tokens in the fields
- In the current PR a quote is triggered when a slippage amount is changed in the settings, while in Bart it is not
- Refreshing quote state: may hang for too long (up to 1 minute). Might it be possible to force it to run faster?
- In the current PT caching is applied when flip a token pair on limit orders page while on Barn it is not. I'm not sure about it, maybe we should leave it as it is.. or should fix this?
- TWAP: when change a price protection value on the form, a new quote is not triggered while on Bart it is.
- Hooks (as you mentioned): adding-removing a hook does not force a quote recalculation on the form, but it should.
Thanks.
|
@elena-zh thank you!
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/cowswap-frontend/src/modules/tradeQuote/utils/quoteUsingSameParameters.ts (1)
39-43: Consider optimizing the appData comparison.The
compareAppDataWithoutQuoteDatafunction correctly handles string comparison, but it performs multiple operations (decoding JSON, removing properties, re-stringifying) for every quote comparison. For high-frequency trading scenarios, this could impact performance.Consider adding a memo/cache mechanism or a more efficient comparison algorithm if this becomes a bottleneck.
apps/cowswap-frontend/src/modules/swap/updaters/QuoteObserverUpdater/index.tsx (2)
45-46: Consider clarifying the reset condition for better readability.The condition with multiple negations (
(!hasQuoteError && !isQuoteUpdating)) can be a bit difficult to parse at first glance.You could improve readability by restructuring the condition:
- // Reset the opposite field when the quote is updating or has an error - if ((!hasQuoteError && !isQuoteUpdating) || !orderKind) return + // Reset the opposite field when the quote is updating or has an error + if (!orderKind) return + + // Only proceed with reset if there's an error or the quote is updating + if (!hasQuoteError && !isQuoteUpdating) return
41-43: Update the comment to reflect the expanded reset logic.The comment above the useEffect hook only mentions resetting when the quote is updating, but the implementation now also resets when there's an error.
- /** - * Reset the opposite field when the quote is updating - */ + /** + * Reset the opposite field when: + * 1. The quote is updating, or + * 2. There's an error with the quote + */apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuoteManager.ts (1)
25-61: Consider adding caching validation.Since this PR reintroduces quotes caching, it would be beneficial to add a method or condition that validates whether a cached quote is still valid (not just based on expiration but also on other factors like price volatility).
You could add a
isQuoteStillValidhelper function within the hook that checks not only expiration but also considers other conditions:export function useTradeQuoteManager(sellTokenAddress: SellTokenAddress | undefined): TradeQuoteManager | null { const update = useSetAtom(updateTradeQuoteAtom) const processUnsupportedTokenError = useProcessUnsupportedTokenError() + // Helper to determine if a cached quote is still valid + const isQuoteStillValid = useCallback( + (quote: OrderQuoteResponse | null, params: FeeQuoteParams | null): boolean => { + if (!quote || !params) return false + + // Check if quote is expired + const isExpired = isQuoteExpired({ response: quote, quoteParams: params }) + if (isExpired) return false + + // Add additional validation logic here if needed + // e.g., check if market conditions have changed significantly + + return true + }, + [] + ) return useMemo( // ...rest of the implementation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/cowswap-frontend/src/modules/swap/updaters/QuoteObserverUpdater/index.tsx(2 hunks)apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuoteManager.ts(1 hunks)apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts(5 hunks)apps/cowswap-frontend/src/modules/tradeQuote/utils/quoteUsingSameParameters.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Setup
- GitHub Check: Cypress
🔇 Additional comments (25)
apps/cowswap-frontend/src/modules/tradeQuote/utils/quoteUsingSameParameters.ts (4)
1-1: Good addition for handling nullish values.Adding the
Nullishtype import enhances type safety and allows for better handling of undefined or null parameters.
12-16: Improved robustness with null checking.The function signature change to accept
Nullish<FeeQuoteParams>and the early return for nullish parameters are good defensive programming practices that prevent potential runtime errors when comparing properties of null/undefined objects.This change aligns with the PR objective of reintroducing quotes caching, making the comparison function more robust for edge cases during initialization or data loading.
48-58: LGTM - Preserves existing quote metadata handling.The
removeQuoteMetadatafunction efficiently removes only the quote-specific metadata while preserving other app data, which is crucial for proper quote caching without affecting other functionality.
18-33:❓ Verification inconclusive
Note that hooks in appData are no longer being omitted.
Based on the past review comments, there was a discussion about omitting
hooksdata from appData comparisons to prevent unnecessary quote fetching when switching between tokens with permit support. However, I don't see this optimization in the current implementation.From the PR objectives, it was mentioned that part of this PR involves "modifying quote triggering logic to ignore hooks in appData". If that's still intended, the
compareAppDataWithoutQuoteDataorremoveQuoteMetadatafunctions might need additional modifications.
🏁 Script executed:
#!/bin/bash # Check if there are any changes to hooks handling in the PR # Search for hooks-related code in appData files echo "Searching for hooks-related code in appData files:" rg -A 5 "hooks" --glob "**/*appData*.ts" # Check the AppDataInfoUpdater implementation mentioned in the past comments echo "Checking AppDataInfoUpdater implementation:" fd -e ts -e tsx AppDataInfoUpdater --exec cat {}Length of output: 4512
Action Required: Update AppData Comparison to Omit Hooks
The current implementation inquoteUsingSameParameters.tsstill performs a full comparison ofappData, including the hooks, which contradicts the PR objective of ignoring hooks to prevent unnecessary quote fetching (especially when handling tokens with permit support). The search results inapps/cowswap-frontend/src/modules/appData/utils/appDataFilter.tsconfirm that hooks are still part of the data structure and are not omitted. Please consider updating either thecompareAppDataWithoutQuoteDatafunction or the associatedremoveQuoteMetadatalogic to explicitly filter out hooks, ensuring that the optimization discussed in past reviews is properly applied. Also, verify that this update does not inadvertently affect other parts of the application relying on completeappDatastructures.apps/cowswap-frontend/src/modules/swap/updaters/QuoteObserverUpdater/index.tsx (4)
21-21: Enhanced error handling for quote errors.Adding this flag to track quote errors helps ensure the UI is updated appropriately when there are issues with the trade quote.
45-46: Logic updated to reset fields on quote errors.The condition now resets the opposite field when there's a quote error, which is consistent with the quote caching feature being reintroduced in this PR. This helps ensure that fields are properly reset when quote data is invalid.
53-53: Dependency array correctly updated with the new hasQuoteError dependency.Adding
hasQuoteErrorto the useEffect dependency array ensures the effect reruns when error status changes, making the component more reactive to quote errors.
30-31:❓ Verification inconclusive
Verify that quote caching works correctly with this reset logic.
The comment on line 30 mentions the importance of the
isQuoteUpdatingcheck. Since we're modifying reset behavior to account for errors as well, confirm this doesn't interfere with the caching mechanism.
🏁 Script executed:
#!/bin/bash # Check for other components or hooks that might be affected by this change # Look for related files that use quote updating logic rg -A 2 -B 2 "isQuoteUpdating|hasQuoteError|tradeQuote.isLoading" --type ts --type tsx # Check for tests that might need updating with the new logic rg -A 2 "QuoteObserverUpdater" --type testLength of output: 188
Below is an updated review comment that reflects the need for a closer manual verification of the caching mechanics after our reset logic change. Since our initial automated search using file type flags did not yield results (due to errors with unrecognized file types), please re-run the following shell scripts manually or verify the related behavior across your codebase:
#!/bin/bash # Re-run search for quote updating logic usages across the repository. rg -A 2 -B 2 "isQuoteUpdating" . # Re-run search for any instance of QuoteObserverUpdater usage, including tests. rg -A 2 "QuoteObserverUpdater" .Attention: Verify Quote Caching in Updated Reset Logic
- The
isQuoteUpdatingcheck is critical to ensuring that the amount updates correctly following a reset.- With the new reset behavior now handling errors in addition to the normal reset flow, confirm that this change does not interfere with the existing quote caching mechanism.
- Please ensure that any tests or components relying on quote caching are reviewed and updated as needed.
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuoteManager.ts (6)
1-13: Imports look appropriate and well-organized.All necessary dependencies are imported, and the imports are logically grouped by source (external libraries first, followed by local imports). The file structure follows project conventions.
14-19: Interface is well-defined with clear method signatures.The
TradeQuoteManagerinterface provides a clean, consistent API for managing trade quotes. Each method has a specific purpose and appropriate parameters.
21-24: Good hook initialization with proper dependency management.The hook accepts an optional sell token address and initializes necessary state management tools. This is consistent with the PR objective of reintroducing quotes caching.
25-38: Implementation ofsetLoadingandresetmethods look good.Both methods update the trade quote state appropriately. The
setLoadingmethod intelligently clears the response only when parameters have changed, which supports the caching functionality mentioned in the PR objectives.
39-45: Error handling is implemented correctly.The
onErrormethod properly updates the state and processes unsupported token errors. This is crucial for preventing the application from getting stuck in an error state, which aligns with the PR objective to avoid the "Quote expired" state.
46-57: Optimal quote handling enhances caching capabilities.The
onResponsemethod includes special handling for optimal quotes by conditionally updating the loading state. This is key to the PR's objective of restoring quotes caching functionality.apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts (11)
5-5: Import of PriceQuality is appropriate.This import is necessary for specifying the quality of quotes to fetch (FAST or OPTIMAL).
15-15: Import of useTradeQuoteManager is correctly added.This enables the use of the new centralized trade quote management functionality.
17-17: Import update for fetchAndProcessQuote is appropriate.The import now includes the TradeQuoteFetchParams type, which improves type safety when calling this function.
32-34: Extract sellToken and initialize tradeQuoteManager.These lines extract the sell token from the quote parameters and initialize the trade quote manager. This is a crucial part of the caching implementation since quotes are cached per sell token.
48-55: Improved null checking and reset logic.The code now properly checks for the existence of tradeQuoteManager before attempting to use it, and resets the state when necessary. This prevents potential null reference errors and ensures clean state transitions.
62-64: Simplified quote fetching logic.The fetchQuote function provides a cleaner abstraction for fetching quotes, making the code more maintainable. This approach aligns with the project's goal to improve the quote caching mechanism.
65-76: Improved quote caching logic.The logic for determining when to fetch a new quote has been refined. The code now properly skips quote updates when using the same parameters, which is central to the PR's objective of reintroducing quotes caching.
84-85: Streamlined quote fetching implementation.The code now uses the simplified fetchQuote function for both fast and optimal quotes, reducing code duplication. This makes the implementation more maintainable and consistent.
114-114: Appropriate use of tradeQuoteManager.reset().Using the manager's reset method to clear the quote state before fetching a new quote after expiration is a clean approach. This helps prevent the application from getting stuck in a "Quote expired" state as mentioned in the PR objectives.
123-130: Updated dependency array with tradeQuoteManager.The dependency array correctly includes tradeQuoteManager, ensuring the effect reruns when the manager changes. This is important for maintaining correct behavior during token switching as described in the PR testing procedure.
74-74:❓ Verification inconclusive
Verify that caching behavior works as expected for the WETH to COW trading pair.
The comment indicates that quote updating should be skipped only when there's no quote response, but the condition includes
hasCachedResponse || hasCachedError. Ensure this aligns with the expected caching behavior for the WETH to COW pair as mentioned in the PR objectives.
🏁 Script executed:
#!/bin/bash # Search for tests related to quote caching for WETH-COW pair rg -l "WETH.*COW.*cache|COW.*WETH.*cache" --type tsLength of output: 53
Action Required: Verify Handling of Cached Errors for WETH-to-COW Trading Pair
The current logic in
useTradeQuotePolling.tsuses the conditionif ((hasCachedResponse || hasCachedError) && quoteUsingSameParameters(currentQuoteParams, quoteParams)) {to decide whether to skip updating the quote. According to the PR objectives, quote updates should be skipped only when there is a valid cached response. This raises the question of whether a cached error should also prevent a new quote fetch for the WETH to COW pair.
- Review the Expected Behavior: Confirm if caching an error is intended to block a quote update or if only a successfully retrieved quote should be cached.
- Manual Verification Recommended: Since automated searches did not reveal tests validating this caching behavior for the WETH-to-COW pair, please verify manually that handling cached errors in this manner aligns with the intended functionality.
- Potential Adjustment: If only valid responses should prevent updates, consider amending the condition to check solely
hasCachedResponse.
|
@elena-zh 2nd case is fixed now (for eth-flow) |
|
Hey @shoom3301 , everything looks good for me besides 1 issue: quote refreshing still takes a bit longer than I'd expect. F.e. on Barn it takes up to 10 seconds, in the PR it takes 30sec-1 minute :( |
|
@elena-zh can you check it again please? Just pushed a fix |
|
@elena-zh I pushed another fix. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts (2)
45-49: Consider preserving cached quotes when switching tabs.The current implementation resets the trade quote manager whenever the window isn't visible. This might impact user experience when they return to the tab as they'll need to wait for a fresh quote to be fetched. Consider preserving cached quotes and only refreshing them if they've expired.
useEffect(() => { - if (!isWindowVisible && tradeQuoteManager) { + if (!isWindowVisible && tradeQuoteManager && isQuoteExpired(tradeQuoteRef.current)) { tradeQuoteManager.reset() } }, [isWindowVisible, tradeQuoteManager])
118-120: Consider refactoring the quote expiration handling.When a quote expires, the code resets the entire trade quote manager and then immediately tries to fetch a new quote. Consider implementing a more targeted approach that just marks the specific quote as expired without a full reset.
- tradeQuoteManager.reset() + // Just mark the current quote as expired instead of a full reset + tradeQuoteManager.markAsExpired(currentQuote.quoteParams?.priceQuality)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Cypress
🔇 Additional comments (7)
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts (7)
34-34: Efficient implementation of the trade quote manager.The code now uses a dedicated manager to handle quote state, which is a cleaner separation of concerns. Initializing with the currency address is a good approach for caching by token.
56-59: Reset handling is properly implemented.The code correctly handles the reset case when there are no quote parameters or the amount is zero.
66-68: Simplified quote fetching architecture.The code now uses a more streamlined approach to fetch quotes by delegating to a central service function. This improves maintainability and follows the single responsibility principle.
69-90: Caching implementation aligns with PR objectives.The implementation now properly caches quotes based on parameters, which addresses the PR goal of reintroducing quotes caching. The condition on line 78 ensures that quotes are only refetched when necessary, improving performance and user experience.
78-80: Effective cache utilization prevents unnecessary quote fetches.This condition is the core of the caching mechanism - it prevents fetching new quotes when parameters haven't changed and there's already a response or error. This is exactly what's needed to meet the PR objective of caching quotes for the same token pairs.
82-85: Check for online status and window visibility is correct.The code properly avoids fetching quotes when the browser is offline or the tab is not active, which saves resources and prevents unnecessary API calls.
88-89:❓ Verification inconclusive
Verify that both fast and optimal quotes are correctly processed.
The code fetches both fast and optimal quotes when needed. Make sure the
tradeQuoteManagercan handle multiple quote types correctly and that they're properly displayed in the UI.
🏁 Script executed:
#!/bin/bash # Check if the tradeQuoteManager correctly handles both fast and optimal quotes # Look for the implementation of useTradeQuoteManager to understand how different quote types are stored fd -t f "useTradeQuoteManager.ts" -x cat {} # Check if the UI displays both fast and optimal quotes rg -A 10 -B 10 "fastQuote|optimalQuote" --glob "*.tsx"Length of output: 2531
Action: Verify UI Integration for Fast and Optimal Quotes
The backend handling looks correct—fast quotes are processed as interim updates while optimal quotes trigger the final update (stopping the loading state via
onResponse). However, our search for explicit references to “fastQuote” or “optimalQuote” in the UI components returned no results. Please manually verify that:
- The UI correctly reflects the temporary fast quote update and then properly transitions to display the optimal quote.
- The displayed data from
tradeQuoteManager(usingupdateTradeQuoteAtom) is handled as expected.
|
@anxolin sorry, there is a pending comment for you and I merged the PR. |
Summary
Fixes #5471
Before we used to have quotes caching and it was removed in the recent refactoring.
In this PR I added it back.
To Test
On Sepolia:
Also, please check that we don't stuck in "Quote expired" state.
Also, I changed the quote triggering logic. Now we ignore hooks in appData, it might affect Hooks store.
Summary by CodeRabbit
New Features
Refactor
Improvements