Refactoring: removed duplications in swap rf-db access paths #22314
Conversation
vkjr
requested review from
briansztamfater,
clauxx,
alwx,
seanstrom,
flexsurfer and
ulisesmac
Jenkins BuildsClick to see older builds (13)
|
|
@flexsurfer, @alwx, @seanstrom, @ulisesmac, would you mind to take a look? |
| [status-im.contexts.wallet.sheets.buy-network-selection.view :as buy-network-selection] | ||
| [status-im.contexts.wallet.sheets.select-asset.view :as select-asset] | ||
| [utils.i18n :as i18n])) | ||
|
|
||
| (rf/reg-event-fx :wallet.buy-crypto/select-provider | ||
| (fn [{:keys [db]} [{:keys [account provider recurrent?]}]] | ||
| (let [swap-network (get-in db [:wallet :ui :swap :network])] | ||
| (let [{swap-network :network} (get-in db db/swap)] |
| @@ -786,7 +786,7 @@ | |||
| (fn [{:keys [db]} | |||
| [{sent-transactions :sentTransactions | |||
| send-details :sendDetails}]] | |||
| (let [swap? (get-in db [:wallet :ui :swap])] | |||
| (let [swap? (get-in db db/swap)] | |||
There was a problem hiding this comment.
Small question, do we always dissoc this state when it's not used? or is possible that it could be an empty map?
There was a problem hiding this comment.
better to have swap? as bool, so it should use seq
There was a problem hiding this comment.
@seanstrom, well we should dissoc it always but I can imagine that sometimes it is not the case due to bug.
@flexsurfer, sorry I didn't quite get what do you mean, could you please elaborate? 🙏
There was a problem hiding this comment.
i mean (get-in db [:wallet :ui :swap]) is a map right? but swap? should be bool
| (def swap [:wallet :ui :swap]) | ||
|
|
There was a problem hiding this comment.
Small nit:
swap could be a confusing name since it can be read almost like a verb, like "we're about to swap the db" or something. We could consider re-naming it to something like swap-ui or wallet-swap. What do you think?
There was a problem hiding this comment.
why not
path-swap?
Yeah that's better, but maybe reverse to swap-path, so when we have more paths, it's easier to find with the editor.
There was a problem hiding this comment.
@clauxx, @seanstrom, @flexsurfer, we can have a namespace db-path in a file db-path.cljs with that constant defined (and others later). That will result in more idiomatic db-path/swap. wdyt?
| :pending | ||
| (= tx-status constants/transaction-status-failed) | ||
| :failed) | ||
| {:keys [swap-approval-transaction-id]} (get-in db db/swap) |
There was a problem hiding this comment.
wondering if (conj db/swap :swap-approval-transaction-id) is better since we avoid a long let binding alignment. wdyt?
There was a problem hiding this comment.
Within let it would be longer actually:
{:keys [swap-approval-transaction-id]} (get-in db db-path/swap)
swap-approval-transaction-id (get-in db (conj db/swap :swap-approval-transaction-id))There was a problem hiding this comment.
But it won't force all bindings to be displaced too far.
It's fine, just I don't like (as others, I guess) when all the code is significantly moved to the right
@ulisesmac, I think we can update more places in code to make it "guideline by example" first and than put to words in a document if needed, wdyt? :) |
|
@flexsurfer, changes made, could you please take a look? |
| @@ -795,7 +796,9 @@ | |||
| (fn [{:keys [db]} | |||
| [{sent-transactions :sentTransactions | |||
| send-details :sendDetails}]] | |||
| (let [swap? (get-in db [:wallet :ui :swap])] | |||
| (let [swap? (-> db | |||
There was a problem hiding this comment.
@flexsurfer, thanks for catching! fixed
VolodLytvynenko
force-pushed
the
chore/db-paths-2
branch
from
ca55e5f to
a526e4b
Compare
Well, if this is an approach already agreed by the team, I think it should be in the guidelines so that all code added follows it. Also, maybe we should clarify that this is the approach recommended when possible, idk if there could be some event handlers where this is hard to achieve. wdyt? cc: @ilmotta |
36% of end-end tests have passedFailed tests (7)Click to expandClass TestWalletMultipleDevice:
Class TestWalletOneDevice:
Expected to fail tests (2)Click to expandClass TestWalletCollectibles:
Passed tests (5)Click to expandClass TestWalletCollectibles:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
|
Hi @vkjr no issues from my side. PR can be merged. Here are the checked flows: How the Flow is Started Used Networks: Conditions on Swap Setup ERC-20 Transactions Additional tests: |
@vkjr @ulisesmac, @seanstrom, @alwx, @flexsurfer, @clauxx, just one observation from this PR. The PR solution is completely fine for reducing duplication, but it's also solving a problem that exists from the choice of nesting maps in the app db. If we used less nesting and more root keys with good namespacing and qualification, we would:
Essentially the guideline suggestion from @ulisesmac is a good one in the sense that this nesting pattern is growing in usage in the repo per the majority preference, but wouldn't be necessary if we kept the app db flat (or more flat, as it used to be in the past). |
Not sure I can agree with such direct conclusion. I tend to think that problem solved by this PR is not a result of nesting itself but a result of its incorrect usage. Big refactorings could occur for any approach including using fully-flat structure, only the reasons would be different. So there are pros and cons of every approach as usually :)
And aim of this PR is to facilitate the further improvements of database by reducing constants duplication and simplify future changes. So I consider it a step in the direction that would suit us all :) |
@vkjr It's not my intention to go back to the original discussions about flat versus nesting that we had already, although the topics are related. About "incorrect usage", I don't see where the incorrect usage was, just suboptimal Clojure code. The changes I see in the PR are:
The other change in the PR was to extract the path to a constant, which is something we wouldn't do if we didn't nest so much in the app-db. It would make no sense to extract to a constant a root key for instance, so I think my conclusion is reasonable. Btw, when I mention "root keys" I don't mean full flattening everywhere. There's always a middle ground.
It's quite different to me. The language server would easily and safely rename the root key in one go in all places. I'm not thinking in terms of big/small refactor. The renaming of a root key is simple and safe because it's globally unique. It would be a quick & easy PR to merge, no need to ask for manual QA either. Renaming nested keys requires a careful analysis of all potential uses of each individual nested key and because Clojure is so dynamic, it's an error prone approach and cumbersome. This is one tradeoff.
But is this really a problem in practice? We could discuss examples to compare how things would look like and how much worse one approach is compared to the other. We also have namespaced keys to our disposal and they support aliasing the qualification part just as a normal import of a var like in
This doesn't seem to be a problem in general because only mounted subscriptions will be compared. We also know the app performed well for years with the previous approach avoiding nesting. Speaking of performance, a tiny detail, I recommend adding the
Correct 👍🏼 that's our consensus and I agree with it. Well designed root keys can be quite convenient to consume. They can have the same stem, thus offer autocompletion like in I'm curious about the inconveniences of a more flat structure, perhaps I'm envisioning something different than the not so great experience you had. But again, I don't want to start long discussions about flat vs nested.
We should be mindful about the already significant overhead that is to read code in status-mobile because of the numerous ways of doing the same thing, unfinished refactors, etc. The pattern introduced in this PR raises the important question about when to use it, as @ulisesmac pointed out. I would encourage us to avoid introducing patterns without careful evaluation if it would scale well throughout the rest of the repo. |
Good point, thanks! We have 2 main discussion points right now:
Flat vs NestedHere we probably need a clarification because as you mentioned we can envision different things. When I envision nested structure, I think about structure I provided as an example in application layers document.
So with these clarifications we can compare if you envision similar things when we talk about flat and nested. Is my vision is different from yours? When to use introduced approachShame, I missed the latest question from @ulisesmac in this PR, thanks for bringing it up. Main thought:Taking into account that I also feel that we shouldn't need constants for more than 2-keywords paths in ideal rf-db, I can imagine it would be equally convenient (or even more) to use namespace-prefixed keys like Wdyt? cc @ulisesmac |
|
Always enjoyable to discuss technical topics with you @vkjr
The problem is that the states related to the messenger deserve some love to be consistent and are not a good indicator of what things could look like if the structure was still kept flat(ish). Qualifications could be better used, and we could avoid global names like I think we mostly agree on the direction. As you said, the app-db layers might be able to unnest and separate concerns well enough that it won't be a problem anymore.
Complete flatness is something that I think is undesirable. I envision something like root keys being small tables, properly indexed and normalized, where other parts of the app-db can more or less efficiently join. These so called "tables" could be the immediate children of the layers you've been advocating and that can work quite well. The UI state as in for example
I agree with you 100% on this.
This would be a great upside from the reestructure of the app-db you are trying to bring forward 👍 |
Thanks, it's likewise 🎩
Fully agree on this, I expected those "tables" to be especially wide-used for the And In my imagination they always no deeper than 2nd level of nesting: {:domain {:accounts {"account-1-id" {.. account data ..}
"account-2-id" {.. account data ..}}
:messages {"message-1-id" {.. message data ..}
"message-2-id" {.. message data ..}}
:tokens {...}
:chats {...}}}Which mean they also can be a prefixed root-keys 👍 |
Summary
Problem
We duplicate same
rf-dbpath sections multiple times in our codebase. For example:wallet :ui :swapin code sections similar to this one:That means that any database refactoring will be a pain. In fact it is the same problem as sprinkling some magic constant around the code - tons of places will need to change if constants changed.
Lets check how things are with the send constants:
124 occurences!
Decision
This PR reimplements suggestions proposed by @flexsurfer in #22288 - combining
update-inwithassoc/dissocand proper destructuring to get rid of key sequence duplication. Plus It introduces constantdb/swapfor[:wallet :ui :swap]db path.Review notes
This PR only introduces constant for
swapdb path. If you will find it useful the approach can be spread to more code areasTesting notes
Swaps internals changed, they need testing
Platforms
Areas that may be impacted
Functional
Steps to test
status: ready