feat: activities in a browser wallet #19405
Conversation
vkjr
requested review from
a team,
alexjba,
caybro,
micieslak and
noeliaSD
as code owners
Jenkins BuildsClick to see older builds (92)
|
vkjr
force-pushed
the
feat/browser-account-activity
branch
from
9474a38 to
856b50e
Compare
vkjr
changed the title
vkjr
force-pushed
the
feat/browser-account-activity
branch
from
856b50e to
cd18b9b
Compare
vkjr
changed the title
vkjr
force-pushed
the
feat/browser-account-activity
branch
2 times, most recently
from
c9d8cb9 to
c672022
Compare
fix: comments fixed
vkjr
force-pushed
the
feat/browser-account-activity
branch
from
c672022 to
b4cd6e8
Compare
caybro
left a comment
caybro
left a comment
There was a problem hiding this comment.
Nice work overall, a bit unsure about the overall architecture, esp. regarding the stores
storybook/stubs/AppLayouts/Browser/stores/BrowserActivityStore.qml
Outdated
Show resolved
Hide resolved
| import StatusQ.Core.Theme | ||
| import StatusQ.Core.Utils as SQUtils | ||
|
|
||
| import SortFilterProxyModel |
| readonly property bool isLightTheme: Theme.palette.name === Constants.lightThemeName | ||
| property color animatedBgColor | ||
| property int txType: walletRootStore.getTransactionType(root.modelData) | ||
| property int txType: activityStore.getTransactionType(root.modelData) |
There was a problem hiding this comment.
| property int txType: activityStore.getTransactionType(root.modelData) | |
| readonly property int txType: activityStore.getTransactionType(root.modelData) |
| } | ||
|
|
||
| function getDappDetails(chainId, contractAddress) { | ||
| return undefined |
There was a problem hiding this comment.
The general, currently used approach is to mock stores, it's our cut-off line between ui and nim side.
For details please refer to: https://github.com/status-im/status-desktop/blob/master/guidelines/QML_ARCHITECTURE_GUIDE.md
@caybro, could you please elaborate? |
|
@caybro, @micieslak - please take a look again, review notes fixed. |
| overview: root.browserWalletStore.dappBrowserAccount | ||
| communitiesStore: null | ||
| currencyStore: SharedStores.CurrenciesStore {} | ||
| networksStore: SharedStores.NetworksStore {} |
There was a problem hiding this comment.
These 2 stores are global, ie you should not recreate them but pass them down all the way from AppMain
|
|
||
| // TODO: https://github.com/status-im/status-desktop/issues/15329 | ||
| // Get DApp data from the backend | ||
| function getDappDetails(chainId, contractAddress) { |
There was a problem hiding this comment.
Wondering if it would make sense to have a specific Utils file for this context?
There was a problem hiding this comment.
Sure! I see Utils as a place for static, global, and generic helpers. If this is more context-specific, we could consider creating a dedicated utils file for that context instead. WDYT?
|
@noeliaSD, could you please check the last commit, at |
noeliaSD
left a comment
noeliaSD
left a comment
There was a problem hiding this comment.
Great job! I’ve reviewed the latest commit and you’ve followed the pattern we discussed very well. I’ve added a few comments and suggestions for improvements, along with some guidance for the next steps to fully refine the architecture according to our guidelines. These can be addressed in separate follow-up tasks.
| browserActivityStore: root.browserActivityStore | ||
| accounts: root.browserWalletStore.accounts | ||
| overview: root.browserWalletStore.dappBrowserAccount | ||
| activityStore: root.browserActivityStore |
There was a problem hiding this comment.
As you mentioned, this should follow the same improvement approach: pass only the specific data needed to the Menu instead of the full store. I’m completely fine with addressing this in a separate task.
| } | ||
| onOpenWalletMenu: Global.openPopup(browserWalletMenu) | ||
| onOpenWalletMenu: { | ||
| // Initialize activity filters before opening popup |
There was a problem hiding this comment.
These kinds of data preparations likely belong in the adaptors layer, which I didn’t mention earlier. It’s fine to keep it here for now, and if you don't mind, we can create a separate task to improve this part as well. We can sync again to define the proper approach, if you wish. For technical reference, you can check the adaptors section here: https://github.com/status-im/status-desktop/blob/master/guidelines/QML_ARCHITECTURE_GUIDE.md#adaptors
| overview: root.browserWalletStore.dappBrowserAccount | ||
| activityStore: root.browserActivityStore | ||
| transactionActivityStatus: root.browserActivityStore.transactionActivityStatus | ||
| currencyStore: root.currencyStore |
There was a problem hiding this comment.
The same consideration applies to both currencyStore and networkStore as well.
|
|
||
| required property BrowserStores.BrowserWalletStore browserWalletStore | ||
| required property BrowserStores.BrowserActivityStore browserActivityStore | ||
| required property var accounts |
There was a problem hiding this comment.
It would be helpful to add a brief comment or description clarifying the expected roles for this accounts model.
| required property BrowserStores.BrowserWalletStore browserWalletStore | ||
| required property BrowserStores.BrowserActivityStore browserActivityStore | ||
| required property var accounts | ||
| required property var overview |
There was a problem hiding this comment.
Also here. It would be helpful some description and if it's a model, which are the expected roles.
| required property var accounts | ||
| required property var overview | ||
| required property var activityStore | ||
| required property var transactionActivityStatus |
There was a problem hiding this comment.
Are the possible values of this expected to be of type int?
| signal transactionFilterUpdateRequested() | ||
| signal collectiblesModelUpdateRequested() | ||
| signal recipientsModelUpdateRequested() | ||
| signal connectedAccountDeleted() |
There was a problem hiding this comment.
As a naming suggestion to keep the pattern consistent, you could use deleteConnectedAccountRequested(). But feel free to decide what fits best!
There was a problem hiding this comment.
I removed those signals because there is no actual reason to use them, we can do connection to activity status object directly in the BrowserLayout.
| downloadsStore: BrowserStores.DownloadsStore {} | ||
| browserRootStore: BrowserStores.BrowserRootStore {} | ||
| browserWalletStore: BrowserStores.BrowserWalletStore {} | ||
| browserActivityStore: BrowserStores.BrowserActivityStore { |
There was a problem hiding this comment.
As we discussed offline, it would be good to create a separate task to refactor the Browser Stores location and instantiation into a new ui/app/AppLayouts/stores/Browser folder, all related stores should then be instantiated inside ui/app/AppLayouts/stores/RootStore.qml, with a corresponding reference added in appMain.
6 participants
What does the PR do
Implements activities list in a browser wallet.
Affected areas
browser_section/module.nimBrowserActivityStore.qmladded for reusingHistoryView.qmlandTransactionDelegate.qmlHistoryView.qmlandTransactionDelegate.qmlpropertywalletRootStorewas renamed toactivityStore, removed wallet-specific importsArchitecture compliance
My PR is consistent with this document: QML Architecture Guidelines
Screencapture of the functionality
Fixes #19273