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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Use correct asset platform when calling CoinGecko #2525

Merged
merged 4 commits into from
Jun 17, 2024

Conversation

davecreaser
Copy link
Contributor

Description

  • Use the network_id environment variable to determine which asset platform should be used while calling the CoinGecko api.

Testing

A little trickier to test properly, but there's some things you can do:

  • Go to the balances table and refresh the page with the network tab open in dev tools.
  • If you check the network tab you should find some calls to the CoinGecko api:
Screenshot 2024-06-12 at 17 30 58
  • ChangeNETWORK_ID="ganache" in your .env.local file to be NETWORK_ID="gnosis"
  • Repeat the first two steps
  • You should now see the call being made with xdai instead of arbitrum-one as the asset platform
Screenshot 2024-06-12 at 17 38 30

The main idea here is that on production, it is currently incorrectly calling with xdai as the asset platform, whereas it should be using arbitrum-one. The network_id environment variable on production is arbitrumOne.

Diffs

Changes 馃彈

  • Update the CoinGecko config to use the network_id environment variable to determine which asset platform should be used when calling the api.

Resolves #2418

@davecreaser davecreaser self-assigned this Jun 12, 2024
@davecreaser davecreaser requested review from a team as code owners June 12, 2024 16:53
@davecreaser davecreaser changed the title Fix/#2418 currency conversion wrong in production Fix: Use correct asset platform when calling CoinGecko Jun 12, 2024
@rdig
Copy link
Member

rdig commented Jun 13, 2024

@bogdan-1337 why did you make this commit 290da64 on this branch ?

Is this for arbitrum or gnosis ?

Copy link
Contributor

@iamsamgibbs iamsamgibbs left a comment

Choose a reason for hiding this comment

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

This looks good to me! Changes make sense and tested using the steps provided.

Screenshot 2024-06-13 at 10 47 34 Screenshot 2024-06-13 at 10 48 24

@rumzledz rumzledz self-requested a review June 14, 2024 11:25
Copy link
Contributor

@rumzledz rumzledz left a comment

Choose a reason for hiding this comment

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

Nice one @davecreaser , I learnt something new while reviewing this PR 馃槑

  • NETWORK_ID="ganache"
    Screenshot 2024-06-14 at 12 24 27

  • NETWORK_ID="gnosis"
    Screenshot 2024-06-14 at 12 25 11

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.

Changes look good. Initial knee jerk reaction would be to tell you to default to Gnosis, not Arbitrum since this will be merged into master and that is still a gnosis default branch.

However soon that won't be the case and I won't fight too much against this.

Screenshot from 2024-06-14 21-50-26
Screenshot from 2024-06-14 21-50-53
Screenshot from 2024-06-14 21-51-47

However I'm still curious about the change that @bogdan-1337 pushed to this branch, and especially if it needs a similar change on the master-arbitrum side

@bogdan-1337
Copy link
Collaborator

@bogdan-1337 why did you make this commit 290da64 on this branch ?

Is this for arbitrum or gnosis ?

Updated the version for the action, it was causing issues for the build on this branch, but not for other branches so not sure what that's about, but anyway good to have it updated to the new version so can be merged to any branch.

@davecreaser davecreaser merged commit d18625f into master Jun 17, 2024
6 of 9 checks passed
@davecreaser davecreaser deleted the fix/#2418-currency-conversion-wrong-in-production branch June 17, 2024 09:46
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.

Currency conversion is not working and is wrong in production
5 participants