-
Notifications
You must be signed in to change notification settings - Fork 2
Paul/coinrank #88
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
Paul/coinrank #88
Conversation
c8cf9a1 to
18e884c
Compare
src/coinrankEngine.ts
Outdated
| import { setAsync, slackMessage } from './utils/dbUtils' | ||
| import { logger, snooze } from './utils/utils' | ||
|
|
||
| const client = createClient() |
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.
Don't need to create another client
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.
Makes sense. done
src/coinrankEngine.ts
Outdated
| const LOOP_DELAY = 45000 | ||
|
|
||
| export const coinrankEngine = async (): Promise<void> => { | ||
| await console.log('hello') |
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.
Remove testing remnant
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.
done
src/coinrankEngine.ts
Outdated
| try { | ||
| const lastUpdate = new Date().toISOString() | ||
| const { uri } = config.providers.coingecko | ||
| let markets: any = [] |
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.
Could be typed as CoinrankMarkets instead of any
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. Done
src/coinrankEngine.ts
Outdated
| const marketsPage = asCoingeckoMarkets(reply) | ||
| markets = [...markets, ...marketsPage] | ||
| } catch (e) { | ||
| console.log('ugh') |
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.
Let's let the cleaner throw so we don't update the redis list with incomplete data
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.
Makes sense. done
src/exchangeRateRouter.ts
Outdated
|
|
||
| // Update redis cache | ||
| const redisData: CoinrankRedis = { | ||
| markets: data, |
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.
This only saves the the sliced data for the requested fiat. Convert everything in the USD list to the selected currency before saving it and returning to the user
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.
I don't understand. All USD market are queried by the engine. The API endpoint only writes non-USD markets after doing the conversion. The converted value is returned to the user and saved in redis.
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 data saved to redis is only a subset of the USD data (sliced at 378 and saved in line 392) and once it is saved subsequent requests for the same fiat code will no longer be null at line 361 which skips any conversion functionality.
Instead, take markets in line 377 and convert all of them to the requested fiat -> save to redis -> slice and return to user
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.
understood. makes sense.
src/exchangeRateRouter.ts
Outdated
| .send(`Invalid length param: ${length}. Must be between 1-100`) | ||
| return | ||
| } | ||
| const fiatSuffix = fiatCode.split(':')[1] |
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.
Nitpick, just use the fiatCode as is for the key. And replace any hardcoded USD/iso:USD value with the default fiat from the config
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.
ok. note there are other hardcoded iso:USD occurences in the file
18e884c to
ee67703
Compare
src/exchangeRateRouter.ts
Outdated
|
|
||
| // Update redis cache | ||
| const redisData: CoinrankRedis = { | ||
| markets: data, |
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 data saved to redis is only a subset of the USD data (sliced at 378 and saved in line 392) and once it is saved subsequent requests for the same fiat code will no longer be null at line 361 which skips any conversion functionality.
Instead, take markets in line 377 and convert all of them to the requested fiat -> save to redis -> slice and return to user
ee67703 to
157c4ee
Compare
src/indexEngines.ts
Outdated
| await setupDatabase(config.couchUri, ratesDbSetup) | ||
| ratesEngine().catch(e => logger('ratesEnginee failure', e)) | ||
| uidEngine().catch(e => logger('uidEnginee failure', e)) | ||
| // ratesEngine().catch(e => logger('ratesEnginee failure', e)) |
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.
turd here
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.
damn. the whole file change is a turd. how'd that sneak in there. will remove
src/indexEngines.ts
Outdated
| ratesEngine().catch(e => logger('ratesEnginee failure', e)) | ||
| uidEngine().catch(e => logger('uidEnginee failure', e)) | ||
| // ratesEngine().catch(e => logger('ratesEnginee failure', e)) | ||
| // uidEngine().catch(e => logger('uidEnginee failure', e)) |
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.
another turd
157c4ee to
e34ebad
Compare
Uh oh!
There was an error while loading. Please reload this page.