-
Notifications
You must be signed in to change notification settings - Fork 22
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
chore: update celo tokens info lookup table in mixpanel #301
Conversation
I'll wire up to CICD in a follow-on. |
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.
if there's a reasonable error message when the filepath is bad I think this is probly fine to merge
} | ||
|
||
const lookupTables: Record<string, string> = { | ||
'c1881b36-7300-421d-9934-73e11d3b39d8': toCsv( |
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 is this string? seems like it should be a named constant or at least explained in a code comment
|
||
const lookupTables: Record<string, string> = { | ||
'c1881b36-7300-421d-9934-73e11d3b39d8': toCsv( | ||
'../src/data/mainnet/celo-tokens-info.json', |
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 seems brittle. we've already moved tokens info jsons once, and might do it again, for instance to ts files to get static validation
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.
have you tried putting a non-existent filename here to see what happens? seems like we'd at least want a descriptive error message
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.
and maybe even a CI step checking that this file exists. which we could do if the path were an exported constant and we included a script that just looks for the file in CI
No description provided.