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.

Sign up for GitHub

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

Fixed bug with fiat value displayed as 0 for some tokens in receive field on Swap Page #21402

Merged
merged 1 commit into from
Oct 10, 2024
Merged

Fixed bug with fiat value displayed as 0 for some tokens in receive field on Swap Page #21402

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions src/status_im/subs/wallet/wallet.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,14 +470,19 @@
(rf/reg-sub
:wallet/tokens-filtered
:<- [:wallet/tokens]
(fn [{:keys [by-symbol market-values-per-token details-per-token]}
:<- [:profile/currency]
(fn [[{:keys [by-symbol market-values-per-token details-per-token prices-per-token]} currency]
[_ {:keys [query chain-ids hide-token-fn]}]]
(let [tokens (->> by-symbol
(map (fn [token]
(-> token
(assoc :market-values
(get market-values-per-token (:symbol token)))
(assoc :details (get details-per-token (:symbol token))))))
(let [token-symbol (keyword (:symbol token))
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with everybody mentioning malli can be a powerful ally. But more important than that to me is that we are not following even the basic practice and guideline of testing non-trivial subs.

The whole graph of non-trivial subs should be tested with deftest-sub, so that changes in the db layer can be caught up by different tests. The tests also provide an automatic runtime verification layer to make sure schemas are correct.

The tests would also provide live examples of data to play in the REPL. Overall, my comment is that malli schemas can help, but it would be good to first sort out the basics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ilmotta, I fully agree with the point on testing.

One moment I can think of is that artificial data that we use as rf db placeholder in tests can become outdated when the db structure is modified. And if that happens, the subs tests will remain valid even if subs fail on real rf db with modified structure.

Wondering if we could consider having Malli schemas for rf db content. If we have it, we can validate that data in tests hasn't degraded. And also the data can be automatically generated from specs the first time when you writing tests and modified for the test purpose.
Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

@vkjr @clauxx @seanstrom @ulisesmac I think it's about time we explore schemas for certain parts of the app-db. We have malli for a long time already in the repo. Malli is a proven solution in many Clojure projects. We don't need to use in the entire db, I wouldn't tbh.

For example, we might want to spec only parts that are changing very often to give us some protection in the middle of the battle. Or we might do the opposite to play safe now and only have schemas for parts of the app-db that are stable and proven. Adding schemas later, when implementation is done is like adding tests later, their value is greatly diminished. So the time is ripe!

I think the only way to protect us against stale schemas and stale data is to have quick integration level tests that call the RPC endpoints and validate the responses using the same schemas we use for unit tests and for development. If we connect all these dots we get a powerful system with runtime validation.

On a tangent, I shared with @seanstrom these days a dirty proof of concept to generate malli schemas from any Go struct. This is 100% possible. There's a chance we can use go generate to create EDN files with the schemas and help us in this endeavor to add schemas at the boundaries, which is where the Clojure masters recommend us we focus on.

Copy link
Contributor

Choose a reason for hiding this comment

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

Validating certain inputs of the app-db sounds good but probably not enough. Shouldn't we instead validate the subscription's output? IMO validating some basic subs would be better.

Again, I agree on not validating everything 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Validating certain inputs of the app-db sounds good but probably not enough.

@ulisesmac I see schemas for the app-db would cause a positive, compounding effect in the rest of the system.

The first level is data arriving from status-go. That data usually end up being written to the app-db and become the results of root subscriptions. If we nail this part correctly we have a solid foundation to build the rest. Layer-2 subs benefit immediately from schemas for the app-db. Layer-3 subs we need to categorize in 2 groups at least. The first degree layer-3 subs (the ones using extractors) would also benefit from app-db schemas because the inputs would already be spec'ed. Many of these subs won't need new schemas for their output because many do simple transformations, like filtering.

The second degree layer-3 subs (is there a better name for this?) tend to be the most complicated in the app, they are the ones where inputs are other layer-3 subs. These subs would also benefit from schemas in the app-db. It's hard to explain, but here's my take: all the data from these subs is invariably derived from root subscriptions. If we have schemas for the app-db we will be able to heavily reuse parts of the schemas when describing input signals and output. For example, a layer-3 sub would add 2 keys to a map, and we would be able to reuse a schema from the app-db by using malli's :merge, or sometimes layer-3 subs just group and filter the data, but the results are still the same "things" we spec'ed in the app-db. I see reusability everywhere.

The other benefit of spec'ing the app-db is that it would protect the event layer, which is where a lot of critical stuff happens and where our first mistakes/misconceptions are made.

Okay, but my prediction is that it doesn't matter where we start, if we start to define schemas for subscriptions we will invariably, and eventually define schemas for the app-db. I imagine this: let's say I spec a layer-3 sub, but then I want to spec its inputs. But if I describe the inputs, then I already described the extractors' output. If I described the extractors' output, then I already described parts of the app-db. So in the end, we will eventually bubble up and start spec'ing the app-db, so why not start from there and rip more benefits?

My main concern is that if we don't use schemas to validate RPC endpoint responses we will eventually have a lot of red warnings saying the data don't conform to schemas and devs will start to say schemas are not worth maintaining because they only give guarantees when running the entire application and manually interacting with the app, which is true.

With quo components this problem is not that big of a deal because the data arriving to most components is not directly tied to domain entities.

Copy link
Contributor

Choose a reason for hiding this comment

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

The other benefit of schemas for the app-db is to generate samples of valid data for unit tests, for REPL experimentation and for generating data for quickly seeing how the UI would look like with some randomness and to check the performance.

I had a lot of fun generating random notifications, which are quite large maps.

price (get-in prices-per-token [token-symbol currency])]
(-> token
(assoc :market-values
(get market-values-per-token token-symbol))
(assoc :details (get details-per-token token-symbol))
(assoc-in [:market-values-per-currency currency :price]
price)))))
(filter (fn [{:keys [chain-id]}]
(some #{chain-id} chain-ids)))
(remove #(when hide-token-fn
Expand Down