Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Show token prices #1382
Show token prices #1382
Changes from 31 commits
0b03759
2942280
22b4d13
324edbe
e1f77e1
2d6de29
ea7aa8f
c5b477a
88f356a
3e5f170
59dd067
e5afb36
6781722
8633535
a3783d4
97930a9
9252d6e
50e148d
88899cf
2c92f7b
873df06
15e1564
7c03888
b207d66
e70037d
91c0326
2248280
621771c
96420f8
dfaa285
b5a93f5
2f85687
7720d94
daf34df
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Perhaps we should be converting this
cacheTime
from "minutes" to "seconds" hehThere 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.
Maybe we should just specify the env var value in miliseconds?
config.now
gives milliseconds and I feel like it's more appropriate to not convert minutes to milliseconds here. WDYT?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.
config.now
gives seconds (https://github.com/decent-dao/fractal-interface/pull/1382/files#diff-284b3930e569c4259e85b685d36c139226ef2a210eaeb25f614b752fd1b2e910R197)We did name the env var
TOKEN_PRICE_CACHE_INTERVAL_MINUTES
, which specifies that the value should beMINUTES
. Minutes are a lot easier to reason about than milliseconds, when setting configs in an env var.I think the cleaner solution is to convert the value to seconds when setting the config property here: https://github.com/decent-dao/fractal-interface/pull/1382/files#diff-284b3930e569c4259e85b685d36c139226ef2a210eaeb25f614b752fd1b2e910R198
My TL;DR is that:
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.
@mudrila i just pushed up the fix, no worries