Skip to content
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

Merged
merged 6 commits into from
Dec 1, 2023
Merged

Conversation

silasbw
Copy link
Contributor

@silasbw silasbw commented Dec 1, 2023

No description provided.

@silasbw
Copy link
Contributor Author

silasbw commented Dec 1, 2023

I'll wire up to CICD in a follow-on.

Copy link
Contributor

@cajubelt cajubelt left a 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(
Copy link
Contributor

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',
Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor

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

@silasbw silasbw enabled auto-merge (squash) December 1, 2023 18:33
@silasbw silasbw merged commit c1d3bed into main Dec 1, 2023
5 checks passed
@silasbw silasbw deleted the mix0 branch December 1, 2023 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants