chore(@desktop/wallet): Wallet: explore streamlining "all tokens model" #12489
Conversation
Jenkins BuildsClick to see older builds (179)
|
Khushboo-dev-cpp
force-pushed
the
chore/12424
branch
from
554f700
to
ffdda6d
Compare
Khushboo-dev-cpp
force-pushed
the
chore/12424
branch
8 times, most recently
from
b9fc737
to
457be84
Compare
Khushboo-dev-cpp
changed the title
Khushboo-dev-cpp
force-pushed
the
chore/12424
branch
from
457be84
to
fb4fe9a
Compare
micieslak
force-pushed
the
chore/12424
branch
from
e18f20c
to
96baeee
Compare
Khushboo-dev-cpp
force-pushed
the
chore/12424
branch
from
96baeee
to
aa3778b
Compare
|
@everyone who will be reviewing the approach please note that these changes are just to get everyone to agree on the approach only after which will we meet all the goals we have listed above. For now we have created new services and module in order to showcase the changes and is not perfect in terms of code review yet. |
Khushboo-dev-cpp
requested review from
alaibe,
Cuteivist,
stefandunca,
saledjenic,
dlipicar and
IvanBelyakoff
| Type | ||
| # only be valid if source is custom | ||
| CommunityId | ||
| # everything below should be lazy loaded |
There was a problem hiding this comment.
I do not see where this is being loaded
There was a problem hiding this comment.
That's correct. It's a TODO and if we agree that this should be a lazy load, there will be an async method in the service, with the corresponding signal notifications and needed apis in the controller / module / model.
src/app/modules/main/wallet_section/new_tokens/token_by_symbol_model.nim
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Great job. I like the one source of truth approach and shared service data by all the models.
I am still not sure of the following aspects.
- not sure of
modelmodule's purpose given that we don't lazy load modules. A simpler classic MVVM/MVC patterns would simplify code;Controller/ViewandModelbeing directly exposed to QML and access service for data as described.- also, the
io_interfaceabstraction could be postponed until we have a second implementation and make the dependency more explicit and direct.
- also, the
- the use of broadcasting (events) for changes from service to view instead of direct dependency through interfaces or callbacks using DI?
Do you mean that model should be directly exposed to the UI instead of the module? this part is not very clear to me.
Here we are trying to showcase a way that there is a direct connection between service and model and we've protected it by introducing the interfaces. Passing the signal via view/module is still possible, however it will be the same we did so far in that case. However interface will be needed to make call to the service api still. @stefandunca please find questions/reasoning inline above |
|
Thank you @stefandunca for your input! I going to try address your points, but not sure if I fully understand your points:
I'm not sure if you really mean
Yes, it's true that now it's kind of boilerplate code. And probably we will never have second implementations here. But having those interfaces modules are testable bc we can mock their dependencies (because they depend on abstractions). And again, I know we don't have those tests. But we really should... so I would opt for at least keeping possibility of testing here.
Here, first, I'm not sure what Anyways, I'm really happy that in general we are on the same page. Regarding your concerns - I hope I'm adding some explanation here. If not, I think we could (or even should) try to clarify via further discussion to have clear mutual understanding here :) |
I'm sorry for the confusion, I meant
I agree with you that it would be great if we had a usage for this abstraction instead of making the code harder to follow and maintain. I wanted to find out if I'm the only one who finds it harder to follow the code due to those abstractions. Another reason for dropping them is that in one of our desktop meetings, I raised the need for nim tests, and the answer was that we don't want to invest time given that C++ is so close and it will bring the tests to the presentation layer.
I mean dependency injection. Basically (pass interfaces/callbacks) instead of assuming that implementation will listen to some required broadcasted messages.
The highlighted text is exactly what I asked, why not direct clear dependency through instances/callbacks instead of message broker broadcasted signals which makes it hard to understand which implementation depends on which signals that come from where? Events are a runtime aspect and not compile time aspects like interfaces/instances/callbacks.
It is more of a curiosity, I didn't dive deep in the implementation to see if there are some limitations on applying the simplifications I propose. Thanks for taking the time to share your thoughts. |
Khushboo-dev-cpp
force-pushed
the
chore/12424
branch
from
98e3788
to
56ea0f9
Compare
src/app/modules/main/wallet_section/all_tokens/flat_tokens_model.nim
Outdated
Show resolved
Hide resolved
I'd say we can create a sperate task to investigate this.. |
#12718 created this one, makes sense to do it separately! |
Khushboo-dev-cpp
force-pushed
the
chore/12424
branch
2 times, most recently
from
dfb875d
to
36db2e7
Compare
Khushboo-dev-cpp
force-pushed
the
chore/12424
branch
from
36db2e7
to
d73fc31
Compare
src/app/modules/main/wallet_section/all_tokens/flat_tokens_model.nim
Outdated
Show resolved
Hide resolved
Khushboo-dev-cpp
force-pushed
the
chore/12424
branch
from
d73fc31
to
d051678
Compare
✔️ status-desktop/prs/linux/x86_64/tests-nim/PR-12489#34 🔹 ~5 min 51 sec 🔹 d051678 🔹 📦 tests/nim package |
Khushboo-dev-cpp
force-pushed
the
chore/12424
branch
2 times, most recently
from
09b15fb
to
baaa06f
Compare
There was a problem hiding this comment.
@Khushboo-dev-cpp thanks for addressing the comments!
I've just tested the branch and I can see all listed tokens in the new Tokens List tab settings section!
Khushboo-dev-cpp
force-pushed
the
chore/12424
branch
3 times, most recently
from
355eee1
to
8b287f6
Compare
…l" spread across different sections closes #12424
Khushboo-dev-cpp
force-pushed
the
chore/12424
branch
from
8b287f6
to
58ce5e1
Compare
Closes #12424
required go PR : status-im/status-go#4165
What does the PR do
We have tried to analyze the model needed in order to implement the new Manage tokens feature (design in here) and realised we would have to create a copy of the tokens module (wallet->tokens) nested within profile->wallet. At this point we felt the need to re look at the data to check if the way of exposing data to the UI can be streamlined to avoid confusion and code duplication when using the models from the nim backend. Below are the goals and the implementation we did in order to add this improvement.
To summarise, this PR includes the initial implementation of the new design, and, equally important, a proposal for changes/improvements to the approach used so far. The proposed approach is intended to be extended to another areas in the application and, when accepted by the others, assumed as the default approach for new works. For this reason, it is important that the review of this PR is done by as wide group of developers as possible, in order to both obtain a consensus on the changes and alos continue them in subsequent works.
Goal:
SERVICE LAYER
MODEL
SortFilterProxyModel,LeftJoinModeletc... on the UI side.MODULE
QML STORES
TokensStorefor tokens list (example for this new store in this PR)Utility models(SortFilterProxyModel, LeftJoinModels etc). For a specificview'sneeds, proxies can be used directly in the component.The ideal structure for our qml stores should be as defined below but in scope of current work we will only do this for the
TokensStoreas an example:AppMainshould instantiate a singleRootStorewhich contains stores for all other "sections" (ex.ProfileStore,CommunityStore,WalletStore..). The "section" store will then contains the stores for its own "sub sections " (ex.WalletStorewill instantiateTokenStore,WalletAccountsStore,ActivityStore,TransactionStoreetc..)Covered use cases for tokens list:
Implementation:
SERVICE
The service files calls the
getTokensListAPI from status_go on init and creates token lists as mentioned in the next point. It also listens to any updates from status_go and updates itself.The
app_service/token/service.nimcontains the below structures:sourcesOfTokensList: seq[SupportedSourcesItem]This holds the various sources - Uniswap, Status, Custom, Native
flatTokenList: seq[TokenItem]This contains a flat list of all non repeating tokens and each holds information about its different sources. ex. DAI's sources will be [Uniswap, Status] as it is available on both lists.
This sources list contains foreign keys that refer to the
keyof the sourcesOfTokensList.The key in this case is the combination of chainID+Address.
tokenBySymbolList: seq[TokenBySymbolItem]This contains list of tokens grouped by symbol and also contains its various sources.
In case of community (not done yet) we will not group those by symbol due to the fact that there are scenarios where a community minted token has same symbol as a token from one of the sources list.
The key in this case is symbol if communityId is nil else the key is the address.
tokenDetailsList: Table[string, TokenDetails](TBD)Used to get description and detailed information of the token from crypto compare and will be lazy loaded.
NOTE: All of the the list items mentioned above contain a
keyproperty uniquely identifying a particular entry in the list.MODULE:
MODEL:
GENERIC PROXY MODELS:
As we want to keep the main models exposed from the backend as simple as possible we need means to do further transformations later, on UI side. It can be done using
SortFilterProxyModellibrary, but in some cases it's not sufficient. For example we need to combine data from two or more models into one model. To handle such case we introduced first two fully reusable and generic proxy models:RoleRenamingModelandLeftJoinModel(#12530). In the future we foresee adding more proxies to cover our needs regarding models transformations.