-
Notifications
You must be signed in to change notification settings - Fork 78
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
chore(@desktop/wallet): Unifying the various "TokensTypes" across the app #12654
Conversation
Jenkins BuildsClick to see older builds (40)
|
5ca7e3a
to
ab9764d
Compare
a36f1d7
to
98e3788
Compare
a1c641e
to
83cdcf8
Compare
98e3788
to
56ea0f9
Compare
83cdcf8
to
4f2fd95
Compare
dfb875d
to
36db2e7
Compare
4f2fd95
to
b73e352
Compare
8b287f6
to
58ce5e1
Compare
b73e352
to
ea79cfa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great change!
Just conflicts to fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌🏼I left some questions inline.
@@ -84,9 +84,10 @@ proc initShard*(cluster: int = -1, index: int = -1): Shard = | |||
result.cluster = cluster | |||
result.index = index | |||
|
|||
# ToDo: Will be streamlined to single TokenType under https://github.com/status-im/status-desktop/pull/12654/files | |||
type NewTokenType* {.pure.} = enum | |||
type TokenType* {.pure.} = enum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about how to avoid growing these catch-all types
files. One way could be to follow the domain-driven design and not mix different topics (messaging with wallet). Does it make sense to have this into a blockchain_types.nim
and the current types.nim
into messaging_types.nim
. Will this bring some clarity to the code for a newcomer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stefandunca the reasoning for this is that eventually community tokens and wallet tokens are "tokens" and we are treating them even more so after the are listing the community tokens with account assets. hope that makes sense
ea79cfa
to
d6603b5
Compare
d6603b5
to
d1837df
Compare
fixes #12501
What does the PR do
We found that TokenTypes was declared at multiple places in the app and this is an attempt to unify it into one
different versions found on nim side:
src/backend/activity.nim --> Used for activity as this was the only one that depended on backend for the values we kept those untouched
src/app_service/service/community/dto/community.nim --> used for community permission
New place:
src/app_service/common/types.nim
Affected areas
Community permissions, transaction activity , SendModal
StatusQ checklist
Screenshot of functionality (including design for comparison)