-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: coingecko pricing provider #3
Conversation
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.
Looking good! Just one minor comment and some miscellaneous ones
packages/pricing/package.json
Outdated
"scripts": { | ||
"build": "tsc -p tsconfig.build.json", | ||
"check-types": "tsc --noEmit -p ./tsconfig.json", | ||
"clean": "rm -rf dist", |
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 is just as a bonus, if you add a trailing slash you are forcing rm
to remove a dist
folder (if it's a dist
file it's not removed):
"clean": "rm -rf dist", | |
"clean": "rm -rf dist/", |
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.
we can add this to the boilerplate
baseURL, | ||
headers: { | ||
common: { | ||
[authHeader]: apiKey, |
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.
Just in case asking, does the apiKey
expire?
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.
noup, it's long lived
const startTimestampSecs = Math.floor(startTimestampMs / 1000); | ||
const endTimestampSecs = Math.floor(endTimestampMs / 1000); |
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.
What happens if start > end? No results? API failure?
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.
aaa good catch, will add that validation
🤖 Linear
Closes GIT-67
Description
pricing
package withCoingeckoProvider
IPricingProvider
for future extensibilityChecklist before requesting a review