-
Notifications
You must be signed in to change notification settings - Fork 15
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
Conversation
Remove chainId argument Use checksummed token address Replace chainId usage with the resolved network variable Replace getToken with getTokenFromEverywhere query
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this 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:
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! :)
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 💯 |
Amazing, thank you very much @mmioana that is much clearer now! |
There was a problem hiding this 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 ! ✨
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 fromCoinGecko
this value has been renamed tosupportedNetwork
and will be set inNetworkConfig
.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 thefetchTokenFromChain
lambda that will try to find the token if it is not already stored and then save it in theDB
.Introduced
apiTokenIdsData
inExchangeRatesService
only for optimisation, in order not to make extra calls for getting the whole coins list fromCoinGecko
.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
Testing
TODO: Just make sure the values shown for the
Total
cards and theTotal in and out
cards are still properly shown.You can also try running this query
Please also try changing the
query
's input values and make sure everything still works as expected ✨