Skip to content

Conversation

@sahar-fehri
Copy link
Contributor

@sahar-fehri sahar-fehri commented Jan 8, 2026

Description

Total volume was displaying incorrect values for ERC20 tokens on EVM chains.
Example:
Raw totalVolume from API: 587,265 (already in USD)
Displayed value: $1.8B (incorrectly multiplied by ETH conversion rate ~$3,087)
Expected value: ~$587K

The formatMarketDetails function was multiplying all market data values by conversionRate for all EVM assets (isEvmAssetSelected). However:
Native assets (ETH): API returns market data in native units → conversion needed
ERC20 tokens: API returns market data already in fiat (vsCurrency) → no conversion needed

Changelog

CHANGELOG entry: Fixed a bug where total volume was displaying the wrong value in asset details page

Related issues

Fixes:

Manual testing steps

Feature: my feature name

  Scenario: user [verb for user action]
    Given [describe expected initial app state]

    When user [verb for user action]
    Then [describe expected outcome]

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Note

Fixes incorrect fiat values in market details by applying conversion only when needed.

  • Refactors formatMarketDetails to accept needsConversion and optional conversionRate, applying a multiplier only for cached EVM data; leaves API-fetched (fiat) data unchanged
  • Updates TokenDetails.tsx to compute needsConversion based on cache source and network, simplify conversionRate selection, and pass new options into formatMarketDetails
  • Adjusts tests to new behavior, adds locale/fiat cases, and removes snapshot assertions/files in MarketDetailsList.test.tsx and TokenDetails.test.tsx

Written by Cursor Bugbot for commit c85ae41. This will update automatically on new commits. Configure here.

@sahar-fehri sahar-fehri requested a review from a team as a code owner January 8, 2026 17:19
@github-actions
Copy link
Contributor

github-actions bot commented Jan 8, 2026

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@github-actions github-actions bot added size-M and removed size-S labels Jan 9, 2026
locale: i18n.locale,
currentCurrency,
isEvmAssetSelected: !isNonEvmAsset,
isNativeAsset: !isNonEvmAsset && Boolean(asset.isNative || asset.isETH),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think this assertion is true:
isNativeAsset: !isNonEvmAsset && Boolean(asset.isNative || asset.isETH),

The name is isNativeAsset and you are saying that only !isNonEvmAsset can be native, what about nonEVMAssets?

I would have kept the property isEvmAssetSelected and added a new property called isNativeAsset equal to Boolean(asset.isNative || asset.isETH)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a naming confusing issue ^^"
The current code:
isNativeAsset: !isNonEvmAsset && Boolean(asset.isNative || asset.isETH)
Actually means: "Is this an EVM native asset that needs conversion?"
But the name suggests: "Is this any native asset?" (which would include SOL)

I can refactor this and keep isEvmAssetSelected

Non evm prices come from the snap and should already come in user's currency

@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.17%. Comparing base (d672a66) to head (0a768e1).
⚠️ Report is 20 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main   #24331       +/-   ##
===========================================
- Coverage   79.28%   69.17%   -10.11%     
===========================================
  Files        4065     4066        +1     
  Lines      107173   107260       +87     
  Branches    21830    21860       +30     
===========================================
- Hits        84971    74202    -10769     
- Misses      16248    27442    +11194     
+ Partials     5954     5616      -338     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2026

🔍 Smart E2E Test Selection

  • Selected E2E tags: SmokeAssets
  • Risk Level: medium
  • AI Confidence: 85%
click to see 🤖 AI reasoning details

The changes modify the TokenDetails component and marketDetails utility function, which are responsible for displaying market data (market cap, volume, all-time high/low, fully diluted market cap) for tokens in the Asset Overview screen.

Key changes:

  1. TokenDetails.tsx: Refactored the conversion rate logic to use needsConversion flag instead of isEvmAssetSelected. The new logic determines if data needs conversion based on whether it's cached EVM data (needs conversion) vs API-fetched data (already in fiat).

  2. marketDetails.ts: Changed the API from isEvmAssetSelected + conversionRate to needsConversion + optional conversionRate. Uses a multiplier approach that only applies conversion when both needsConversion is true AND conversionRate is provided.

  3. Test updates: Tests were updated to reflect the new API and snapshot tests were removed in favor of explicit assertions.

The changes are well-scoped to the asset display functionality and have comprehensive unit test coverage. The risk is medium because:

  • The changes affect how market data values are displayed to users
  • Currency conversion logic changes could result in incorrect values being shown
  • However, the logic is straightforward and well-tested

SmokeAssets is the appropriate tag as it covers "Asset management and display, NFTs, token details" which directly matches the TokenDetails component being modified.

View GitHub Actions results

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 9, 2026

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants