Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Remove CLNY from currency list and add try catch for CoinGecko errors #3961

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

mmioana
Copy link
Contributor

@mmioana mmioana commented Dec 17, 2024

Description

This PR addresses mainly the problem of having the CLNY token migrated to another address on Arbitrum One.
This caused the fetchTokenPriceByAddress to throw an error and render the Balances page blank, due to a missing try...catch block.
Also given that for the moment we don't have data based on which to convert amounts to CLNY, we'll hide the option from the user menu, while also refreshing the preferredCurrency stored in the user profile.
Screenshot 2024-12-17 at 19 13 18
Screenshot 2024-12-17 at 19 13 21

Testing

TODO: Let's test our changes do work as expected.

  • Step 1. Please add this at line 55 in src/components/shared/CurrencyConversion/CurrencyConversion.tsx
const conversionRate = useCurrency({
    contractAddress: '0xd611b29dc327723269bd1e53fe987ee71a24b234',
    chainId: Network.ArbitrumOne,
    conversionDenomination: SupportedCurrencies.Clny,
  });

and comment also line 79 in src/utils/currency/currency.ts
// if (isDev) return 1;

  • Step 2. Now go to http://localhost:9091/planex/balances
    You should see this error in the console, but the page should have no issue rendering.
    Screenshot 2024-12-17 at 19 25 30

  • Step 3. Awesome ✨ Now let's test the CLNY currency selection. Please comment the 36-39 lines in src/components/common/Extensions/UserNavigation/partials/UserSubmenu/partials/Currency/Currency.tsx

         .filter(
            (supportedCurrency) =>
              supportedCurrency !== SupportedCurrencies.Clny,
          )
  • Step 4. The CLNY token should be again visible in the user menu list
    Screenshot 2024-12-17 at 19 28 56
  • Step 5. Select it and open the user menu again. The value should not update to CLNY but remain as previously selected.
Screen.Recording.2024-12-17.at.19.29.47.mov
  • Step 6. You can also run this query to confirm the preferredCurrency is not updating to CLNY
query MyQuery {
 # Note: I have used leela's wallet address
  getUserByAddress(id: "0xb77D57F4959eAfA0339424b83FcFaf9c15407461") { 
    items {
      profile {
        preferredCurrency
      }
    }
  }
}
  • Step 7. Now let's try to have the CLNY already stored in the user profile's preferredCurrency.
    First, let's manually update the localStorage preferredCurrency entry to CLNY
    Screenshot 2024-12-17 at 19 37 17

Now, please run the following mutation

mutation MyMutation {
  updateProfile(
    input: {
      id: "0xb77D57F4959eAfA0339424b83FcFaf9c15407461", 
      preferredCurrency: CLNY
    }
  ) {
    preferredCurrency
    id
    displayName
  }
}
  • Step 8. Refresh the page and the selected currency should be based on the user's location
Screen.Recording.2024-12-17.at.19.39.43.mov
  • Step 9. You can run this query again and check the preferredCurrency is set to another value than CLNY
query MyQuery {
 # Note: I have used leela's wallet address
  getUserByAddress(id: "0xb77D57F4959eAfA0339424b83FcFaf9c15407461") { 
    items {
      profile {
        preferredCurrency
      }
    }
  }
}
  • Step 10. Now please select from the currencies list another one to test possible regressions
Screen.Recording.2024-12-17.at.21.33.44.mov

If you've reached so far, I can confirm your testing is completed, though feel free to explore any other scenario that comes in mind. 🎉

Diffs

Changes 🏗

  • SupportedCurrencies.Clny is not used ftm.
  • One more missing try...catch

Resolves #3965

@mmioana mmioana force-pushed the hotfix/clny-token-breaks-app branch from 00e38da to 4e0d730 Compare December 17, 2024 17:47
@mmioana mmioana changed the title Fix: Remove CLNY from currency list and add try catch for coingecko e… Fix: Remove CLNY from currency list and add try catch for CoinGecko errors Dec 17, 2024
@mmioana mmioana marked this pull request as ready for review December 17, 2024 18:44
@mmioana mmioana requested a review from a team as a code owner December 17, 2024 18:44
@mmioana mmioana self-assigned this Dec 17, 2024
@mmioana mmioana marked this pull request as draft December 17, 2024 19:17
…rrors

Fix: Remove CLNY from currency list and add try catch for coingecko errors
@mmioana mmioana force-pushed the hotfix/clny-token-breaks-app branch from 4e0d730 to 5c27714 Compare December 17, 2024 20:32
@mmioana mmioana marked this pull request as ready for review December 17, 2024 20:34
Copy link
Member

@rdig rdig left a comment

Choose a reason for hiding this comment

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

Working as expected now, after that missing try/catch

CLNY is no longer selectable, even if you have it saved within your profile or local storage

Screencast.from.2025-01-09.17-47-06.mp4

While all the other currencies work as intended and don't cause any issues

(The balance page no longer breaks on render)

Screenshot from 2025-01-09 17-48-13

Screencast.from.2025-01-09.17-48-26.mp4

Copy link
Contributor

@bassgeta bassgeta left a comment

Choose a reason for hiding this comment

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

Nice job commenting this out so it's easy to add it back when we can 👍
Modified the code as instructed and got the console error ✔️
image
Commented the CLNY filter and it showed up 👀
image

overscroll-fix-2025-01-10_10.52.36.mp4

Changed preferred currency in the API and local storage ✔️
image
And it defaults to EUR ✔️
image
image

Copy link
Contributor

@davecreaser davecreaser left a comment

Choose a reason for hiding this comment

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

Nice solution @mmioana - and really cool that you've left such helpful comments for future us too! 🤝

Tested and all working as expected:
Screenshot 2025-01-13 at 20 32 25
Screenshot 2025-01-13 at 20 33 11
Screenshot 2025-01-13 at 20 33 31
Screenshot 2025-01-13 at 20 34 09
Screenshot 2025-01-13 at 20 34 27
Screenshot 2025-01-13 at 20 35 04
Screenshot 2025-01-13 at 20 35 32
Screenshot 2025-01-13 at 20 35 55
Screenshot 2025-01-13 at 20 36 01

@mmioana mmioana merged commit 8588cdb into master Jan 13, 2025
2 checks passed
@mmioana mmioana deleted the hotfix/clny-token-breaks-app branch January 13, 2025 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[PROD] Remove CLNY from the token switcher as it breaks the colony and returns 0 values
4 participants