feat(correlated-tokens): use new CMS model Correlated Tokens #5427
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
WalkthroughThe changes update the volume fee handling by replacing the tax-free assets functionality with correlated tokens. In the frontend, the updater components and associated imports have been renamed and modified to fetch and process correlated token data. New state management has been introduced via a Jotai atom for correlated tokens, while the tax-free assets atom has been removed. Additionally, the dependency for Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Component
participant Updater as CorrelatedTokensUpdater
participant API as API Endpoint (/correlated-tokens)
participant Atom as correlatedTokensAtom
App->>Updater: Render updater
Updater->>API: Request correlated tokens data
API-->>Updater: Return token data
Updater->>Atom: Update state with correlated tokens
Poem
🪧 TipsChatThere are 3 ways to chat with Cod 8000 eRabbit:
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 (
|
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/cowswap-frontend/src/modules/volumeFee/state/correlatedTokensAtom.ts (1)
6-8: Consider using more specific types.Instead of using generic
stringtypes, consider creating more specific types or importing them from the SDK:-type Address = string -type Symbol = string +import { Address } from '@cowprotocol/types' +type Symbol = string // TODO: Consider creating a more specific type for token symbolsapps/cowswap-frontend/src/modules/volumeFee/updaters/CorrelatedTokensUpdater.tsx (1)
43-71: Enhance error handling for better debugging.The error handling could be more informative:
-console.error('Failed to fetch correlated tokens', error) +console.error('[CorrelatedTokensUpdater] Failed to fetch correlated tokens:', { + error, + method, + timestamp: new Date().toISOString(), +})apps/cowswap-frontend/src/modules/volumeFee/state/volumeFeeAtom.ts (1)
49-58: Add JSDoc comments to explain the correlation logic.The correlation logic handles two cases but lacks documentation:
+/** + * Checks if the input and output currencies are correlated based on two rules: + * 1. Single asset rule: If only one asset is listed, it's a global correlated token + * 2. Pair rule: If two assets are listed, both must be present to be correlated + */ return correlatedTokens.some((tokens) => { const addresses = Object.keys(tokens) if (addresses.length === 1) { return addresses[0] === inputCurrencyAddress || addresses[0] === outputCurrencyAddress } else { return tokens[inputCurrencyAddress] && tokens[outputCurrencyAddress] } })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
apps/cowswap-frontend/src/modules/application/containers/App/Updaters.tsx(2 hunks)apps/cowswap-frontend/src/modules/volumeFee/index.ts(1 hunks)apps/cowswap-frontend/src/modules/volumeFee/state/correlatedTokensAtom.ts(1 hunks)apps/cowswap-frontend/src/modules/volumeFee/state/taxFreeAssetsAtom.ts(0 hunks)apps/cowswap-frontend/src/modules/volumeFee/state/volumeFeeAtom.ts(3 hunks)apps/cowswap-frontend/src/modules/volumeFee/updaters/CorrelatedTokensUpdater.tsx(4 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (1)
- apps/cowswap-frontend/src/modules/volumeFee/state/taxFreeAssetsAtom.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Cypress
- GitHub Check: Setup
🔇 Additional comments (6)
package.json (2)
75-75: LGTM! Version bump aligns with the new Correlated Tokens model.MOO! 🐮 This update to
@cowprotocol/cms@^0.11.0is exactly what we need to support the new Correlated Tokens model. The change is consistent with our PR objectives to replace the Tax Free Assets model.
75-75:❓ Verification inconclusive
Verify the new CMS package version.
MOO! 🐮 Let me check if this version of the CMS package is secure and stable.
🏁 Script executed:
#!/bin/bash # Description: Check for security advisories and latest versions of the @cowprotocol/cms package # Check npm registry for latest versions npm view @cowprotocol/cms versions --json # Check for any security advisories npm audit @cowprotocol/cms@^0.11.0Length of output: 948
Verify the @cowprotocol/cms dependency update and perform a manual audit check
- The npm registry output confirms that version 0.11.0 of the CMS package exists.
- The automated audit command failed due to a missing lockfile (ENOLOCK). This prevents an automatic security verification.
- Action Required: Please ensure that a proper lockfile (e.g., package-lock.json) is available and re-run the audit or manually verify that no security issues exist for version 0.11.0.
apps/cowswap-frontend/src/modules/volumeFee/index.ts (1)
5-5: LGTM! Export updated to reflect new model.The export has been correctly updated from
TaxFreeAssetsUpdatertoCorrelatedTokensUpdater.apps/cowswap-frontend/src/modules/volumeFee/state/correlatedTokensAtom.ts (1)
10-13: LGTM! Good use of persistent storage.The atom is well-implemented with:
- Proper versioning in the storage key
- Type safety through PersistentStateByChain
- Correct initialization using mapSupportedNetworks
apps/cowswap-frontend/src/modules/volumeFee/updaters/CorrelatedTokensUpdater.tsx (1)
77-88: LGTM! Robust state reduction logic.The reduction logic correctly:
- Validates required attributes
- Maps to supported chains
- Maintains type safety
apps/cowswap-frontend/src/modules/application/containers/App/Updaters.tsx (1)
18-18: MOOving from TaxFreeAssets to CorrelatedTokens looks good! 🐮The replacement of
TaxFreeAssetsUpdaterwithCorrelatedTokensUpdateris clean and maintains the same module structure.Also applies to: 98-98
| const shouldSkipFeeAtom = atom<boolean>((get) => { | ||
| const { chainId } = get(walletInfoAtom) | ||
| const tradeState = get(derivedTradeStateAtom) | ||
| const taxFreeAssetsState = get(taxFreeAssetsAtom) | ||
| const correlatedTokensState = get(correlatedTokensAtom) | ||
|
|
||
| if (!tradeState) return false | ||
|
|
||
| const taxFreeAssets = taxFreeAssetsState[chainId] | ||
| const correlatedTokens = correlatedTokensState[chainId] | ||
|
|
||
| if (!taxFreeAssets) return false | ||
| if (!correlatedTokens) return false | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the chainId handling.
The code assumes chainId will be available. Consider adding a safety check:
const shouldSkipFeeAtom = atom<boolean>((get) => {
const { chainId } = get(walletInfoAtom)
+ if (!chainId) return false
const tradeState = get(derivedTradeStateAtom)
const correlatedTokensState = get(correlatedTokensAtom)Let's verify chainId handling across the codebase:
🏁 Script executed:
#!/bin/bash
# Search for chainId usage in volume fee related files
rg -A 2 'chainId' --type tsLength of output: 120296
Action Required: Add ChainId Safety Check in volumeFeeAtom.ts
It looks like the code in apps/cowswap-frontend/src/modules/volumeFee/state/volumeFeeAtom.ts currently assumes that chainId is always available. Our codebase shows multiple instances where we explicitly check whether chainId exists before proceeding, so it would be safer to add a similar check here to prevent potential runtime issues.
Please consider updating your code as follows:
const shouldSkipFeeAtom = atom<boolean>((get) => {
- const { chainId } = get(walletInfoAtom)
+ const { chainId } = get(walletInfoAtom)
+ if (!chainId) return false
const tradeState = get(derivedTradeStateAtom)
const correlatedTokensState = get(correlatedTokensAtom)This change ensures consistency with our other modules and adds a necessary guard against missing chainId.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const shouldSkipFeeAtom = atom<boolean>((get) => { | |
| const { chainId } = get(walletInfoAtom) | |
| const tradeState = get(derivedTradeStateAtom) | |
| const taxFreeAssetsState = get(taxFreeAssetsAtom) | |
| const correlatedTokensState = get(correlatedTokensAtom) | |
| if (!tradeState) return false | |
| const taxFreeAssets = taxFreeAssetsState[chainId] | |
| const correlatedTokens = correlatedTokensState[chainId] | |
| if (!taxFreeAssets) return false | |
| if (!correlatedTokens) return false | |
| const shouldSkipFeeAtom = atom<boolean>((get) => { | |
| const { chainId } = get(walletInfoAtom) | |
| if (!chainId) return false | |
| const tradeState = get(derivedTradeStateAtom) | |
| const correlatedTokensState = get(correlatedTokensAtom) | |
| if (!tradeState) return false | |
| const correlatedTokens = correlatedTokensState[chainId] | |
| if (!correlatedTokens) return false | |
| }); |
| @@ -95,7 +95,7 @@ export function Updaters() { | |||
| <LpTokensWithBalancesUpdater /> | |||
| <VampireAttackUpdater /> | |||
| <BalancesCombinedUpdater /> | |||
| <TaxFreeAssetsUpdater /> | |||
| <CorrelatedTokensUpdater /> | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify feature flag integration
Let's ensure the isSafeAppFeeEnabled feature flag is properly integrated within the CorrelatedTokensUpdater.
🏁 Script executed:
#!/bin/bash
# Search for feature flag usage in the CorrelatedTokensUpdater
rg -A 5 "isSafeAppFeeEnabled" "apps/cowswap-frontend/src/modules/volumeFee"
# Search for any Safe fee related logic in the updater
ast-grep --pattern 'function CorrelatedTokensUpdater() {
$$$
}'Length of output: 8892
Feature Flag Check Missing in CorrelatedTokensUpdater
It looks like the updater isn’t directly checking the isSafeAppFeeEnabled flag—even though that flag is used in the safe fee atom (see apps/cowswap-frontend/src/modules/volumeFee/state/safeAppFeeAtom.ts). As a result, the updater always fetches correlated tokens regardless of whether safe fee mode is enabled. Please verify that this behavior is intentional. If the updater should run only when the feature is on, add a conditional check (or gate its rendering from the parent container) to prevent unnecessary updates when the feature flag is disabled.
- File:
apps/cowswap-frontend/src/modules/volumeFee/updaters/CorrelatedTokensUpdater.tsx— Consider adding an early return or conditional SWR call based onisSafeAppFeeEnabled. - File:
apps/cowswap-frontend/src/modules/volumeFee/state/safeAppFeeAtom.ts— The flag is correctly retrieved here; ensure the overall flow aligns with the updater’s behavior.
| return assets.includes(inputCurrencyAddress) && assets.includes(outputCurrencyAddress) | ||
| return tokens[inputCurrencyAddress] && tokens[outputCurrencyAddress] |
There was a problem hiding this comment.
Now the values are stored as an object, faster access.
| acc[chainId].push(tokenIds) | ||
| acc[chainId].push(item.attributes.tokens as CorrelatedTokens) |
There was a problem hiding this comment.
Expect the data to already be formatted as a json object with address -> symbol.
Symbol is mostly for debugging, has no use as of now.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/cowswap-frontend/src/modules/volumeFee/updaters/CorrelatedTokensUpdater.tsx (1)
63-71: Let's make our error messages more helpful! 🚨While the error handling is good, we could make the messages more descriptive for easier debugging.
Consider adding more context to error messages:
-console.error('Failed to fetch correlated tokens', error) +console.error('Failed to fetch correlated tokens from CMS', { error, method: '/correlated-tokens' }) -console.error('Failed to fetch correlated tokens', e) +console.error('Failed to fetch correlated tokens from CMS - network error', { error: e, method: '/correlated-tokens' })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/cowswap-frontend/src/modules/volumeFee/updaters/CorrelatedTokensUpdater.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Setup
- GitHub Check: Cypress
🔇 Additional comments (4)
apps/cowswap-frontend/src/modules/volumeFee/updaters/CorrelatedTokensUpdater.tsx (4)
3-3: MOOving in the right direction with these type changes! 🐮The transition from tax-free assets to correlated tokens is well-structured with proper type definitions from the CMS schema.
Also applies to: 11-11, 13-13
15-28: These settings are udderly perfect! 🐄The caching strategy with a 10-minute update interval and SWR configuration is well-maintained, ensuring efficient data fetching while preventing excessive CMS load.
30-32: Looking sharp with that new component name! 🎯Clean transition from TaxFreeAssetsUpdater to CorrelatedTokensUpdater with proper state management using Jotai atoms.
77-85: Let's verify the tokens format! 🔍Based on previous feedback, we need to ensure the tokens are formatted as address -> symbol objects.
Could you confirm that
item.attributes.tokensfollows the expected format? The type should enforce:type CorrelatedTokens = { [address: string]: string // where string is the token symbol }
|
Report too large to display inline |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/cowswap-frontend/src/modules/volumeFee/updaters/CorrelatedTokensUpdater.tsx (1)
79-82: Let's beef up our error handling! 🥩Consider providing more specific error messages when network or tokens data is missing, rather than silently skipping the item.
if (!item.attributes?.network?.data?.attributes?.chainId || !item.attributes?.tokens) { + console.warn('Skipping correlated token item due to missing data:', { + hasNetwork: !!item.attributes?.network?.data?.attributes?.chainId, + hasTokens: !!item.attributes?.tokens, + itemId: item.id + }) return acc }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/cowswap-frontend/src/modules/volumeFee/updaters/CorrelatedTokensUpdater.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Setup
- GitHub Check: Cypress
🔇 Additional comments (5)
apps/cowswap-frontend/src/modules/volumeFee/updaters/CorrelatedTokensUpdater.tsx (5)
3-3: MOOving in the right direction with these type changes! 🐮The type definitions and imports have been cleanly updated to use the CMS schema for correlated tokens. This ensures type safety and maintainability.
Also applies to: 11-11, 13-13
84-90: Smart move with the token address normalization! 🎯Good catch on normalizing token addresses to lowercase. This prevents potential mismatches due to address casing.
30-32:✅ Verification successful
Let's verify our feature flag integration! 🚩
Since this component is part of the feature flag
isSafeAppFeeEnabledworkflow, we should verify the integration.
🏁 Script executed:
#!/bin/bash # Search for feature flag usage rg -l "isSafeAppFeeEnabled" --type tsLength of output: 105
Feature Flag Integration Verified 🚩
Great work — the search confirms that our feature flag,
isSafeAppFeeEnabled, is in use via thesafeAppFeeAtom.tsfile, which aligns with our PR objective of updating tax-free assets to correlated tokens while maintaining similar behavior. No additional changes are needed in theCorrelatedTokensUpdater.tsxcomponent regarding feature flag handling. For extra peace of mind, please ensure to test both enabled and disabled states of the feature flag in your staging environment.
46-59:❓ Verification inconclusive
Let's optimize our API query! 🚀
The pagination size is set to 500 items. We should consider implementing cursor-based pagination if the dataset grows beyond this limit.
🏁 Script executed:
#!/bin/bash # Check the current number of correlated token pairs curl -s "$(grep -o 'https://[^"]*' .env | grep cms)/api/correlated-tokens?pagination[pageSize]=1" | jq '.meta.pagination.total'Length of output: 167
API Pagination Optimization – Manual Verification Needed
It seems our attempt to verify the total count via the provided script didn’t work because the expected.envfile wasn’t found. Without that critical configuration, we couldn’t fetch the CMS domain to complete the request and get reliable data.
- Action Required: Please manually verify whether the
/api/correlated-tokensendpoint ever returns more than 500 items.- If It Does: Consider implementing cursor-based pagination for improved scalability as the dataset grows.
Let's ensure the API configuration is set up correctly so we can accurately assess if the 500-item limit is sufficient.
15-22:❓ Verification inconclusive
Let's verify our grazing schedule! 🌾
The 10-minute update interval is maintained from the previous implementation. Let's ensure this is still appropriate for the new correlated tokens endpoint.
🌐 Web query:
What is the recommended cache/update interval for token correlation data in DeFi applications?💡 Result:
The recommended cache/update interval for token correlation data in DeFi applications depends on balancing real-time accuracy with computational efficiency. Based on insights from decentralized systems and caching best practices:
Dynamic Jitter-Based Refresh:
For systems requiring frequent updates (e.g., price oracles), implement a refresh interval randomized between 50%–90% of the token’s expiration window. For example:
- If correlation data has a validity period of 10 minutes, refresh every 5–9 minutes with jitter to distribute load spikes[3][6].
High-Frequency Market Considerations:
Token correlations in volatile markets may require updates as frequently as every 1–5 minutes to reflect rapid price movements[8]. However, use:
- Predictive cache warming to preload anticipated data during peak volatility[6].
- Hierarchical caching for multi-tiered access (e.g., short-term in-memory caches + longer-term storage)[9].
Protocol-Specific Rebalancing:
Align with DeFi index protocols like the DeFi Pulse Index (DPI), which updates quarterly but monitors metrics like circulating supply weekly[1]. For correlation stability:
- Revalidat 9E88 e cached data after major protocol events (e.g., governance votes or collateral adjustments)[5].
Optimization via Token Pruning:
Use techniques like DaTo pruning to prioritize high-dynamic tokens in cached datasets, reducing compute overhead while maintaining accuracy[4].Implementation Example:
def get_cache_interval(expiry_minutes=10): import random min_refresh = 0.5 * expiry_minutes * 60 # 50% of expiry in seconds max_refresh = 0.9 * expiry_minutes * 60 return random.uniform(min_refresh, max_refresh)This code generates randomized refresh intervals within the recommended range[3][6].
Citations:
- 1: https://indexcoop.com/products/defi-pulse-index
- 2: https://www.chaincatcher.com/en/article/2155969
- 3: https://www.raygesualdo.com/posts/caching-auth-tokens-with-distributed-refresh/
- 4: https://arxiv.org/html/2501.00375v1
- 5: https://economics.uc3m.es/wp-content/uploads/2023/10/NewDefi_300823.pdf
- 6: https://www.akira.ai/blog/kv-caches-and-time-to-first-token
- 7: https://archive.devcon.org/archive/watch/5/a-review-of-short-term-interest-rates-of-tokens-in-the-ethereum-defi-space-why-they-exist-and-how-to-interact-with-those-smart-contracts/?playlist=Devcon+5
- 8: https://www.sciencedirect.com/science/article/pii/S1042443122000610
- 9: https://users.cs.utah.edu/~rajeev/cs7820/papers/micro06_pr.pdf
- 10: https://www.zabbix.com/documentation/current/en/manual/web_interface/definitions
Reassessing the Update Interval for Correlated Tokens
Our current static 10-minute refresh interval comes from the previous design. However, caching best practices for DeFi token correlation data suggest that—in systems with a 10-minute data validity—a dynamic, jitter-based refresh between roughly 5 to 9 minutes could be more optimal. This approach helps balance up-to-date data accuracy with load distribution, especially during market volatility.
Please review whether:
- The current fixed 10-minute interval meets the endpoint’s real-time responsiveness needs.
- A dynamic approach with randomized intervals (e.g., 5–9 minutes) might offer better performance under volatile conditions.
Keep grazing carefully, CoW!
Summary
Replaces
Tax Free AssetswithCorrelated TokensCMS modelBehaviour should be identical to previous version.
To Test
isSafeAppFeeEnabledSummary by CodeRabbit
New Features
Refactor
Chores