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.

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

Merged
merged 1 commit into from
Nov 21, 2023

Conversation

Khushboo-dev-cpp
Copy link
Contributor

@Khushboo-dev-cpp Khushboo-dev-cpp commented Nov 7, 2023

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

type TokenType* {.pure.} = enum
  Native = 0
  ERC20 = 1,
  ERC721 = 2,
  ERC1155 = 3,
  Unknown = 4,
  ENS = 5

Affected areas

Community permissions, transaction activity , SendModal

StatusQ checklist

  • add documentation if necessary (new component, new feature)
  • update sandbox app
    • in case of new component, add new component page
    • in case of new features, add variation to existing component page
    • nice to have: add it to the demo application as well
  • test changes in both light and dark theme?

Screenshot of functionality (including design for comparison)

  • I've checked the design and this PR matches it

@status-im-auto
Copy link
Member

status-im-auto commented Nov 7, 2023

Jenkins Builds

Click to see older builds (40)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 5ca7e3a #1 2023-11-07 22:53:29 ~5 min tests/nim 📄log
✔️ 5ca7e3a #1 2023-11-07 22:56:11 ~8 min macos/aarch64 🍎dmg
✔️ ab9764d #2 2023-11-07 23:02:53 ~5 min macos/aarch64 🍎dmg
✔️ ab9764d #2 2023-11-07 23:03:14 ~5 min tests/nim 📄log
✔️ ab9764d #2 2023-11-07 23:08:08 ~10 min tests/ui 📄log
✔️ ab9764d #2 2023-11-07 23:10:36 ~12 min macos/x86_64 🍎dmg
✔️ ab9764d #2 2023-11-07 23:14:19 ~16 min linux/x86_64 📦tgz
✔️ ab9764d #2 2023-11-07 23:32:51 ~35 min windows/x86_64 💿exe
✖️ ab9764d #2 2023-11-07 23:38:37 ~41 min tests/e2e 📄log
a1c641e #3 2023-11-08 17:08:25 ~3 min macos/aarch64 📄log
a1c641e #3 2023-11-08 17:09:59 ~4 min macos/x86_64 📄log
✖️ a1c641e #3 2023-11-08 17:10:51 ~5 min tests/e2e 📄log
✔️ a1c641e #3 2023-11-08 17:10:59 ~5 min tests/nim 📄log
a1c641e #3 2023-11-08 17:13:23 ~8 min linux/x86_64 📄log
✔️ a1c641e #3 2023-11-08 17:16:37 ~11 min tests/ui 📄log
a1c641e #3 2023-11-08 17:24:17 ~19 min windows/x86_64 📄log
✔️ 83cdcf8 #4 2023-11-08 22:02:53 ~5 min macos/aarch64 🍎dmg
✔️ 83cdcf8 #4 2023-11-08 22:02:56 ~5 min tests/nim 📄log
✔️ 83cdcf8 #4 2023-11-08 22:05:30 ~8 min macos/x86_64 🍎dmg
✔️ 83cdcf8 #4 2023-11-08 22:08:18 ~10 min tests/ui 📄log
✔️ 83cdcf8 #4 2023-11-08 22:11:56 ~14 min linux/x86_64 📦tgz
✔️ 83cdcf8 #4 2023-11-08 22:24:32 ~27 min windows/x86_64 💿exe
✖️ 83cdcf8 #4 2023-11-08 22:35:20 ~38 min tests/e2e 📄log
✔️ 4f2fd95 #5 2023-11-08 22:42:12 ~4 min macos/aarch64 🍎dmg
✔️ 4f2fd95 #5 2023-11-08 22:42:57 ~5 min tests/nim 📄log
✔️ 4f2fd95 #5 2023-11-08 22:45:46 ~8 min macos/x86_64 🍎dmg
✔️ 4f2fd95 #5 2023-11-08 22:48:26 ~11 min tests/ui 📄log
✔️ 4f2fd95 #5 2023-11-08 22:52:32 ~15 min linux/x86_64 📦tgz
✔️ 4f2fd95 #5 2023-11-08 23:03:41 ~26 min windows/x86_64 💿exe
✖️ 4f2fd95 #5 2023-11-08 23:17:31 ~40 min tests/e2e 📄log
✔️ b73e352 #6 2023-11-13 21:16:03 ~5 min tests/nim 📄log
✔️ b73e352 #6 2023-11-13 21:16:10 ~5 min macos/aarch64 🍎dmg
✔️ b73e352 #6 2023-11-13 21:18:41 ~8 min macos/x86_64 🍎dmg
✔️ b73e352 #6 2023-11-13 21:21:27 ~11 min tests/ui 📄log
✔️ b73e352 #6 2023-11-13 21:25:16 ~14 min linux/x86_64 📦tgz
✔️ b73e352 #6 2023-11-13 21:39:42 ~29 min windows/x86_64 💿exe
✖️ b73e352 #6 2023-11-13 21:51:51 ~41 min tests/e2e 📄log
✔️ b73e352 #7 2023-11-15 09:49:04 ~8 min macos/x86_64 🍎dmg
✔️ b73e352 #7 2023-11-15 09:50:21 ~9 min tests/nim 📄log
b73e352 #7 2023-11-15 09:51:58 ~11 min macos/aarch64 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ ea79cfa #8 2023-11-15 10:02:53 ~5 min macos/aarch64 🍎dmg
✔️ ea79cfa #8 2023-11-15 10:05:07 ~7 min tests/nim 📄log
✔️ ea79cfa #8 2023-11-15 10:05:19 ~7 min macos/x86_64 🍎dmg
✔️ ea79cfa #8 2023-11-15 10:11:18 ~13 min tests/ui 📄log
✔️ ea79cfa #8 2023-11-15 10:12:14 ~14 min linux/x86_64 📦tgz
✔️ ea79cfa #8 2023-11-15 10:29:43 ~31 min tests/e2e 📄log
✔️ ea79cfa #8 2023-11-15 10:29:44 ~31 min windows/x86_64 💿exe
✔️ d1837df #11 2023-11-21 10:55:41 ~4 min tests/nim 📄log
✔️ d1837df #10 2023-11-21 10:59:50 ~9 min macos/x86_64 🍎dmg
✔️ d1837df #10 2023-11-21 10:59:56 ~9 min macos/aarch64 🍎dmg
✔️ d1837df #11 2023-11-21 11:01:39 ~10 min tests/ui 📄log
✔️ d1837df #10 2023-11-21 11:05:57 ~15 min linux/x86_64 📦tgz
✖️ d1837df #10 2023-11-21 11:21:53 ~31 min tests/e2e 📄log
✔️ d1837df #10 2023-11-21 11:23:07 ~32 min windows/x86_64 💿exe
✔️ d1837df #11 2023-11-21 12:02:01 ~31 min tests/e2e 📄log

@Khushboo-dev-cpp Khushboo-dev-cpp force-pushed the fix/issue-12501 branch 2 times, most recently from a1c641e to 83cdcf8 Compare November 8, 2023 21:57
@Khushboo-dev-cpp Khushboo-dev-cpp changed the title chore(@desktop/wallet): Unifying the various "TokensTypes" across the… chore(@desktop/wallet): Unifying the various "TokensTypes" across the app Nov 8, 2023
@Khushboo-dev-cpp Khushboo-dev-cpp marked this pull request as ready for review November 8, 2023 22:42
Base automatically changed from chore/12424 to master November 15, 2023 09:40
Copy link
Contributor

@Cuteivist Cuteivist left a 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

Copy link
Contributor

@stefandunca stefandunca left a 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
Copy link
Contributor

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?

Copy link
Contributor Author

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

ui/imports/utils/Constants.qml Show resolved Hide resolved
@Khushboo-dev-cpp Khushboo-dev-cpp merged commit 26f29a4 into master Nov 21, 2023
8 of 9 checks passed
@Khushboo-dev-cpp Khushboo-dev-cpp deleted the fix/issue-12501 branch November 21, 2023 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TokenType enum unification across all app layers
4 participants