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

fix: fix undefined properties issue #309

Merged
merged 1 commit into from
Dec 7, 2023

Conversation

bakoushin
Copy link
Contributor

@bakoushin bakoushin commented Dec 6, 2023

Fixing an issue introduced in #306: tokens not having isFeeCurrency property programmatically received isCoreToken: undefined property which caused RTDB update to fail.

Test plan

Updated unit tests

Related issues

Related to RET-899

Copy link
Contributor

@kathaypacific kathaypacific left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏🏻


for (const address of Object.keys(transformData)) {
expect(
Object.values(transformData[address]).every(isNotUndefined),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: perhaps it'd be the most robust just to declare the expected data in full? (in case expected properties are missing, this test would then fail)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kathaypacific I'm not sure that I interpreted your suggestion correctly, here are my thoughts so far:

the property set has a dynamic nature, depending on the character of the coin. e.g. stable coins have isFeeCurrency or isSupercharged, others do not.

we can ensure that if token is, for instance, "Celo" it should have certain properties, but it seems to me having not too much value. effectively we are making a snapshot of what's in a json file.

for basic required properties we have a schema validation, which aims to ensure that at least required values are present.

in this specific case I wanted only ensure that we are not submitting any undefined properties to RTDB, because it throws an error seeing that, and apparently there is no way to make it ignore them (although this is possible in Firestore)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay makes sense, happy for you to also leave it like this

@bakoushin bakoushin merged commit 8629ab1 into main Dec 7, 2023
8 checks passed
@bakoushin bakoushin deleted the alex/fix-undefined-properties-in-rtdb-update branch December 7, 2023 21:15
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