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

[#18777] Fix exception while calling money/mul on nil values #19065

Closed
wants to merge 2 commits into from

Conversation

ulisesmac
Copy link
Contributor

@ulisesmac ulisesmac commented Feb 29, 2024

fixes #18777

Summary

This PR solves the exception thrown, but I wasn't able to replicate the problem.

It seems the user's tokens lack of the market values, or sometimes other token values are nil. This PR makes the funciton mul able to work with nil and treats it as 0 (same as Clojure's *).

Testing notes

When the issue is replicated, some balances should become zero, the difference is the app isn't crashing now.

Steps to test

Please follow the steps reported in the issue description, the exception shouldn't be thrown, I couldn't replicate it.

status: ready

@ulisesmac ulisesmac requested review from smohamedjavid, J-Son89 and a team February 29, 2024 18:12
@ulisesmac ulisesmac self-assigned this Feb 29, 2024
@status-im-auto
Copy link
Member

status-im-auto commented Feb 29, 2024

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 4982684 #2 2024-02-29 18:20:51 ~7 min tests 📄log
✔️ 4982684 #2 2024-02-29 18:22:24 ~9 min android-e2e 🤖apk 📲
✔️ 4982684 #2 2024-02-29 18:22:25 ~9 min android 🤖apk 📲
✔️ 4982684 #2 2024-02-29 18:23:27 ~10 min ios 📱ipa 📲

Copy link
Member

@Parveshdhull Parveshdhull left a comment

Choose a reason for hiding this comment

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

Hi @ulisesmac, Thank you very much for fixing the issue.

@Parveshdhull
Copy link
Member

Parveshdhull commented Mar 1, 2024

I think the main problems are in utils.money and status-im.contexts.wallet.common.utils. We have so many js function calls there, without nil checks, I think we should fix them first.

#18532 (comment)

@J-Son89 Should we create an issue, so that we fix these files in one go, instead of applying fixes when we get exceptions?

@J-Son89
Copy link
Contributor

J-Son89 commented Mar 1, 2024

I think the main problems are in utils.money and status-im.contexts.wallet.common.utils. We have so many js function calls there, without nil checks, I think we should fix them first.

#18532 (comment)

@J-Son89 Should we create an issue, so that we fix these files in one go, instead of applying fixes when we get exceptions?

Yeah makes sense, we should also make sure there's sufficient test coverage on these too

@ulisesmac
Copy link
Contributor Author

I'm closing this PR because this same fix has been already applied in develop in 7d6b2ac

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

"times() not number: null" error occurs after returning from the backround
5 participants