Wallet - Add malli spec to Quo Wallet components (batch 1) #18354 #18707
Conversation
mmilad75
requested review from
smohamedjavid,
ilmotta,
clauxx,
ulisesmac and
J-Son89
| [:metrics? {:optional true} [:maybe :boolean]] | ||
| [:on-press {:optional true} [:maybe fn?]]]) | ||
|
|
||
| (def ?amount |
There was a problem hiding this comment.
I think we should mark internal schemas private
There was a problem hiding this comment.
I like how you split up the schemas 🙌
Jenkins BuildsClick to see older builds (40)
|
| [quo.components.wallet.account-overview.style :as style] | ||
| [quo.foundations.colors :as colors] | ||
| [quo.theme :as quo.theme] | ||
| [react-native.core :as rn] | ||
| [schema.core :as schema] |
There was a problem hiding this comment.
It could be a function that would wrap instrumentation, but it feels boilerplate-ish to have in each. Would be easier if we used schema/=>, then from the component-schema file we could just require the view and instrument it, but there was an issue with that that I don't remember now from our initial conversations.
There was a problem hiding this comment.
I don't quite understand the problem of requiring schema.core, since that's the namespace exposing instrument.
There was a problem hiding this comment.
There was a problem hiding this comment.
hauahahaha any small time make big time!
| @@ -142,8 +142,8 @@ | |||
| :epoch-number-mainnet "181,329" | |||
| :epoch-number-optimism "181,329" | |||
| :epoch-number-arbitrum "181,329" | |||
| :optimism-progress-percentage "10" | |||
There was a problem hiding this comment.
curious, is this a result of adding the schema? :)
There was a problem hiding this comment.
Yes, some use cases were wrong and I fixed them
| [:name [:maybe :string]] | ||
| [:address [:maybe :string]] | ||
| [:emoji [:maybe :string]] | ||
| [:customization-color [:maybe [:or :string :keyword]]]]] |
There was a problem hiding this comment.
Are these keys always going to be passed and sometimes may be (explicitly) nil or are they optional? If so, should add {:optional true}
| @@ -60,7 +60,7 @@ | |||
| :data cards | |||
| :horizontal true | |||
| :separator [rn/view {:style style/separator}] | |||
| :render-fn quo/account-card | |||
| :render-fn (fn [item] [quo/account-card item]) | |||
There was a problem hiding this comment.
It was throwing malli errors before this. I don't know why 🤔
There was a problem hiding this comment.
We should probably understand why because we should be able to just pass quo/account-card without wrapping.
There was a problem hiding this comment.
@ilmotta The original flat-list implementation expects a function to be passed.
Just saying again that we should revisit our wrapper 👁️
There was a problem hiding this comment.
But quo/account-card is a function, so it surprises me that we need to wrap it in a dummy closure.
There was a problem hiding this comment.
Just saying again that we should revisit our wrapper 👁️
Just saying... I see you! Yes, that wrapper function seems very inefficient with the creation of anonymous functions over and over.
| [:=> | ||
| [:catn | ||
| [:props | ||
| [:map |
There was a problem hiding this comment.
Since we didn't yet arrive at a decision on open/close maps this is not critical, but since this component's props are simpler I'd suggest we close the map to keep it consistent with the rest of the schemas.
There was a problem hiding this comment.
This one is used in some places that receives data directly from backend, so a lot of extra props are passed to it. It's better to let it be open for now
There was a problem hiding this comment.
+1 for open.
- Backend passes props that the component might not need
- Quo preview passes
@statewhile calling the component, which may contain extra kv pairs.
| [:catn | ||
| [:props | ||
| [:map | ||
| [:account |
There was a problem hiding this comment.
Do you see value in extracting account to schema.common ?
There was a problem hiding this comment.
Where to put schemas is always difficult to decide. From one angle, I prefer to think schema.common is the place for schemas more decoupled from domains. Something where we would describe a big integer, a public key, a theme, a style map, and so on. But an account schema could live outside schema.common. This would help us separate things based on bounded contexts. Otherwise there's a tendency common stuff can absorb anything.
An account can mean other things besides a wallet account. Maybe if we want to define a schema in the global registry, it should be in a different namespace, so the keyword in the registry would be :schema.wallet/account.
Or maybe we shouldn't couple quo components with domain concepts, even though they look like they depend on them. This would lead to a bit of duplication of schemas, but then we would be able to evolve schemas independently from quo. Just a tradeoff.
Long train of thought, just ideas on the table @shivekkhurana.
There was a problem hiding this comment.
There are two schools of typings that I have observed:
-
Move all types to their own ns
This helps avoid circular deps. that may arise in future -
Keep types close to implementation
Better docs, easier to implement as no extra files are needed, available at the point of execution.
I personally prefer 2 (ie local types), but there are some domain models that will be repeated. Account being one of them maybe?
So in my head, I assumed schema.common to absorb any type that appears more than once.
There was a problem hiding this comment.
to absorb any type that appears more than once.
I also prefer option 2 @shivekkhurana whenever possible and only use the global registry if really necessary.
Maybe the word common is not the best word when I picked it up, because it implies any schema common to two or more namespaces can already live up there. We're already starting to have discussions about having schemas for data crossing the status-go <-> mobile barrier (e.g. in contract tests and events), which means it won't take long until we have lots of common schemas (complicated ones, not like quo schemas) that are all representing entities that are not tied to any particular part of the app.
Putting them all under the keyword :schema.common/* can work for a while, but something tells me we'll refactor later on and break them apart by domains, so instead of :schema.common/wallet-account we would have :schema.wallet/account. Of course, I'm guessing about this supposed future, we may never get there and so using the :schema.common/ qualification is totally fine now.
| [:map | ||
| [:account-name {:optional true} [:maybe :string]] | ||
| [:currency-change {:optional true} [:maybe :string]] | ||
| [:current-value {:optional true} [:maybe :string]] |
There was a problem hiding this comment.
Shouldn't this be :number ?
There was a problem hiding this comment.
Good point. :string is too loose to represent numbers. For instance, if the value is a string because it's an encoded big integer, then it would be better to at least create an alias to represent this, e.g. :schema.common/raw-bigint. I'm not sure if that's the case here, but the :string schema is also a good example of a type that doesn't help me figure out how to deal with it.
In the future, we could also consider only passing actual numbers throughout the app, even if some of them are representations of big numbers and need to be decoded as such. I did this in the past and it can bring sanity to the whole system. But we would need to make sure all numbers are decoded (from status-go responses) when written to the app-db.
| [:currency-change {:optional true} [:maybe :string]] | ||
| [:current-value {:optional true} [:maybe :string]] | ||
| [:percentage-change {:optional true} [:maybe :string]] | ||
| [:time-frame-string {:optional true} [:maybe :string]] |
There was a problem hiding this comment.
Folks, I've been thinking, it would be good if we arrived at a guideline for predicates in schemas sooner rather than later.
Malli accepts all predicates to be functions or keywords (if they've been added to the registry). The benefit of predicates as functions is that we get compile time errors from the CLJS compiler. Many times, I made typos like :sting instead of :string, but this will cause a runtime error later on or no error at all if there's no test code exercising the schema (which would lead to merging invalid schemas sometimes).
It's nice to be able to represent schemas as data. They could be easily serialized, but we don't use this feature.
Therefore, I'm inclined to prefer functions as predicates, not keywords. Instead of :string, it would be string?. It's just as terse, but with compile-time protection.
Do you have another perspective? Or does this sound reasonable for a guideline in schema/README?
| @@ -60,7 +60,7 @@ | |||
| :data cards | |||
| :horizontal true | |||
| :separator [rn/view {:style style/separator}] | |||
| :render-fn quo/account-card | |||
| :render-fn (fn [item] [quo/account-card item]) | |||
There was a problem hiding this comment.
We should probably understand why because we should be able to just pass quo/account-card without wrapping.
| [quo.components.wallet.account-overview.style :as style] | ||
| [quo.foundations.colors :as colors] | ||
| [quo.theme :as quo.theme] | ||
| [react-native.core :as rn] | ||
| [schema.core :as schema] |
There was a problem hiding this comment.
I don't quite understand the problem of requiring schema.core, since that's the namespace exposing instrument.
|
|
||
| (def ?schema | ||
| [:=> | ||
| [:catn |
There was a problem hiding this comment.
Not a suggestion for change @mmilad75, but rather a perception to incite possible guidelines.
Maybe we should prefer cat (not catn) for function schemas with just one argument, as is the case for most components. It adds an extra indentation and when these schemas fail it will be just as clear why. One might say it's nice for consistency because catn leads to more readable errors, but to me it seems superfluous to use it.
Maybe the best is to just keep the option so the developer use whatever they prefer, but we shouldn't feel forced to use catn all the time IMO.
| [:catn | ||
| [:props | ||
| [:map | ||
| [:account |
There was a problem hiding this comment.
Where to put schemas is always difficult to decide. From one angle, I prefer to think schema.common is the place for schemas more decoupled from domains. Something where we would describe a big integer, a public key, a theme, a style map, and so on. But an account schema could live outside schema.common. This would help us separate things based on bounded contexts. Otherwise there's a tendency common stuff can absorb anything.
An account can mean other things besides a wallet account. Maybe if we want to define a schema in the global registry, it should be in a different namespace, so the keyword in the registry would be :schema.wallet/account.
Or maybe we shouldn't couple quo components with domain concepts, even though they look like they depend on them. This would lead to a bit of duplication of schemas, but then we would be able to evolve schemas independently from quo. Just a tradeoff.
Long train of thought, just ideas on the table @shivekkhurana.
| [:map | ||
| [:account-name {:optional true} [:maybe :string]] | ||
| [:currency-change {:optional true} [:maybe :string]] | ||
| [:current-value {:optional true} [:maybe :string]] |
There was a problem hiding this comment.
Good point. :string is too loose to represent numbers. For instance, if the value is a string because it's an encoded big integer, then it would be better to at least create an alias to represent this, e.g. :schema.common/raw-bigint. I'm not sure if that's the case here, but the :string schema is also a good example of a type that doesn't help me figure out how to deal with it.
In the future, we could also consider only passing actual numbers throughout the app, even if some of them are representations of big numbers and need to be decoded as such. I did this in the past and it can bring sanity to the whole system. But we would need to make sure all numbers are decoded (from status-go responses) when written to the app-db.
| | `:currency-change` | A value representing the change of the value of the selected account in the selected currency. (default `nil`) | ||
| | `:percentage-change` | A value representing the change of the value of the selected account in percentage relative to it's previous value. (default `nil`)" | ||
| (quo.theme/with-theme view-internal)) | ||
| (quo.theme/with-theme |
46% of end-end tests have passedFailed tests (25)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (22)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
|
|
@status-im/mobile-qa Please check the e2e result |
48% of end-end tests have passedFailed tests (13)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Passed tests (12)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
Hey @mmilad75, thanks for the PR! We are experiencing the data delivery issue, which is the reason for most of the e2e failures. They are not related to the PR. |
fixes #18354
The following components are updated:
Quo / Wallet