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: Solve fetchDomainBalance lambda issues #3914

Merged
merged 1 commit into from
Dec 12, 2024

Conversation

mmioana
Copy link
Contributor

@mmioana mmioana commented Dec 11, 2024

Description

There were some issues discovered while doing the prod deployment, causing for the displayed values to be always 0.
The main one was due to the used fallback value for the chainId property. Given we don't really want to be a parametrisable property, this value has been removed from the lambda's input values. Also, given we want to use this value to filter out the coins list from CoinGecko this value has been renamed to supportedNetwork and will be set in NetworkConfig.

Another issue we might face is not having the token used for an action already stored in the DB - which we need to determine the number of decimals - so, we should use the getTokenFromEverywhere. This query relies on the fetchTokenFromChain lambda that will try to find the token if it is not already stored and then save it in the DB.

Introduced apiTokenIdsData in ExchangeRatesService only for optimisation, in order not to make extra calls for getting the whole coins list from CoinGecko.

Added the usage of the checksummed token address only to prevent some errors.

As a summary, the following issues for the fetchDomainBalance lambda were fixed in this PR

  • Remove chainId argument
  • Use checksummed token address
  • Replace chainId usage with the resolved network variable
  • Replace getToken with getTokenFromEverywhere query

Testing

TODO: Just make sure the values shown for the Total cards and the Total in and out cards are still properly shown.
You can also try running this query

query MyQuery {
  getDomainBalance(
    input: {colonyAddress: "<COLONY_ADDRESS_HERE>", timeframePeriod: 4, timeframeType: DAILY}
  ) {
    total
    totalIn
    totalOut
    timeframe {
      key
      value {
        total
        totalIn
        totalOut
      }
    }
  }
}

Please also try changing the query's input values and make sure everything still works as expected ✨

Remove chainId argument
Use checksummed token address
Replace chainId usage with the resolved network variable
Replace getToken with getTokenFromEverywhere query
@mmioana mmioana requested a review from a team as a code owner December 11, 2024 10:35
@mmioana mmioana self-assigned this Dec 11, 2024
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.

Changes seem very sensible and straightforward 💯
The query returns data ✔️ (for multiple timeframes too)
image

Cards still work as they should ✔️
image
image

Switching teams also constantly worked, creating a payment invalidated the query and changed the numbers ✔️

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.

Nice work, the code changes look sensible and everything works as described:

Screenshot 2024-12-11 at 18 32 43

Screenshot 2024-12-11 at 18 32 52

Screenshot 2024-12-11 at 18 33 14

It would be nice if there was a bit more information in the PR description to describe exactly what problem this PR aims to resolve, especially as there is no linked issue. The code changes make sense though and I think I can understand the intent behind the changes, but just a bit of extra background info on the issue would be helpful in the future! :)

@mmioana
Copy link
Contributor Author

mmioana commented Dec 11, 2024

You're right @iamsamgibbs! Somehow in my head is was pretty clear what I tried to solve, but didn't put my thoughts down in this PR's description, which now I have adjusted 💯

@iamsamgibbs
Copy link
Contributor

Amazing, thank you very much @mmioana that is much clearer now!

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.

The values from the GraphQL response corresponds with the data on frontend. Awesome work @mmioana ! ✨

Screenshot 2024-12-12 at 23 21 50 Screenshot 2024-12-12 at 23 22 03

@mmioana mmioana merged commit 7382c26 into master Dec 12, 2024
2 checks passed
@mmioana mmioana deleted the fix/fetch-domain-balance-issues branch December 12, 2024 15:35
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.

4 participants