Skip to content
This repository has been archived by the owner on Dec 31, 2024. It is now read-only.

F fix pan conversion rate #604

Merged
merged 4 commits into from
Sep 12, 2021
Merged

F fix pan conversion rate #604

merged 4 commits into from
Sep 12, 2021

Conversation

mohammadranjbarz
Copy link
Collaborator

@mohammadranjbarz mohammadranjbarz commented Aug 29, 2021

in: 'query',
description: 'could be hourly',
},
],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@aminlatifi
it's not related to conversion rate, but I think it was good to refactor this part

@mohammadranjbarz
Copy link
Collaborator Author

mohammadranjbarz commented Sep 1, 2021

@aminlatifi
I couldn't find any solution for PAN price, when coingecko dont return price in that range time, I just added some validations for /conversionRates endpoint , before user could send any invalid symbol and would get response , but I restricted the user input,
and I think it's better to return 0 when we cant fetch PAN price, before we were sending 1.

but if you have any idea I can implement it ( I didn't find any good solution)

@mohammadranjbarz mohammadranjbarz marked this pull request as ready for review September 1, 2021 07:47
@@ -41,9 +41,32 @@ function getTokenBySymbol(symbol) {
return tokensBySymbols[symbol] || { symbol };
}

const isSymbolInTokenWhitelist = symbol => {
return Boolean(
Copy link
Member

Choose a reason for hiding this comment

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

Let's create a set which includes the symbols. The set should be initialized once when the application start

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

const getValidSymbols = () => {
const symbols = [];
getWhiteListTokens().forEach(token => {
if (!symbols.includes(token.symbol)) {
Copy link
Member

Choose a reason for hiding this comment

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

The same set can be used here too

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

@aminlatifi aminlatifi left a comment

Choose a reason for hiding this comment

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

See the comments

@aminlatifi aminlatifi merged commit 7b08ef6 into develop Sep 12, 2021
@aminlatifi aminlatifi deleted the f_fix_pan_conversion_rate branch September 12, 2021 09:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants