-
Notifications
You must be signed in to change notification settings - Fork 90
feat: add EFP (Ethereum Follow Protocol) addresses to address book #19195
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
Conversation
Jenkins BuildsClick to see older builds (17)
|
|
Wow, nice job at a first glance! Pls rebase to get rid of the conflicts |
jrainville
left a comment
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.
Midway review. I checked all Nim files for now.
Great job! I think the biggest things are the loading props/events are not used. Also, there are way too many info logs 😅
Can you trim most of them down and maybe switch some of them to debug
src/app/modules/main/wallet_section/following_addresses/controller.nim
Outdated
Show resolved
Hide resolved
src/app/modules/main/wallet_section/following_addresses/controller.nim
Outdated
Show resolved
Hide resolved
src/app/modules/main/wallet_section/following_addresses/controller.nim
Outdated
Show resolved
Hide resolved
src/app/modules/main/wallet_section/following_addresses/controller.nim
Outdated
Show resolved
Hide resolved
src/app/modules/main/wallet_section/following_addresses/model.nim
Outdated
Show resolved
Hide resolved
src/app/modules/main/wallet_section/following_addresses/module.nim
Outdated
Show resolved
Hide resolved
src/app/modules/main/wallet_section/following_addresses/controller.nim
Outdated
Show resolved
Hide resolved
jrainville
left a comment
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.
Super nice job! I finished reviewing. I found no big problem in the QML, though it is less my area of expertise, so let's see what the experts say 😄
| id: noFollowingAddresses | ||
| Layout.fillWidth: true | ||
| Layout.preferredHeight: 44 | ||
| visible: RootStore.followingAddresses.count === 0 |
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 think we're moving away from using Singleton stores. It makes it hard to test and use the components in Sotrybook.
To "fix" this, just add a property at the top and pass the needed property from the store in the parent.
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.
Oh, I see that the existing components already use RootStore like that. I'll wait to see what the others say, since it seems the other components need refactoring anyway
| } | ||
|
|
||
| // Full-width divider above pagination (extends beyond content padding) | ||
| Rectangle { |
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 think we already have a component for Separators. @caybro @noeliaSD @micieslak please help here
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.
Yeah, Separator but it's not very useful in this context
caybro
left a comment
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 job! Just a couple of comments, mostly regarding some good QML practices and component structure
ui/app/AppLayouts/Wallet/controls/FollowingAddressesDelegate.qml
Outdated
Show resolved
Hide resolved
ui/app/AppLayouts/Wallet/controls/FollowingAddressesDelegate.qml
Outdated
Show resolved
Hide resolved
ui/app/AppLayouts/Wallet/controls/FollowingAddressesDelegate.qml
Outdated
Show resolved
Hide resolved
ui/app/AppLayouts/Wallet/panels/WalletFollowingAddressesHeader.qml
Outdated
Show resolved
Hide resolved
|
@caybro thanks for the feedback, I've made your recommended changes and rebased ontop of the latest commits. |
jrainville
left a comment
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.
Looks good from my side!
I haven't tested manually though
| self.allCollectiblesModule.load() | ||
| info "wallet-section: loading assetsModule" | ||
| self.assetsModule.load() | ||
| info "wallet-section: loading savedAddressesModule" |
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.
The info logs here can be removed as well
6407be9 to
a4a17bc
Compare
|
@caybro can you please re-review this PR? Let's get it merged |
|
I have updated the branch with latest upstream commits, and resolved a status-go conflict. |
caybro
left a comment
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.
Keep up the good work, we're getting there 💪
| import "../stores" | ||
| import ".." | ||
|
|
||
| import AppLayouts.Wallet.stores as WalletStores |
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.
You definitely don't need all those imports :)
| property var activeNetworks | ||
|
|
||
| readonly property int maxHeight: 341 | ||
| height: implicitHeight > maxHeight ? maxHeight : implicitHeight |
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.
Don't set maxHeight or height at all, the menu will figure it out
|
|
||
| StatusAction { | ||
| text: { | ||
| var savedAddr = WalletStores.RootStore.getSavedAddress(root.address) |
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.
You should extract the savedAddr, isSaved etc.. as a readonly property of this StatusAction, and evaluate it only once, instead of multiple times for each property here
|
|
||
| font.weight: Font.Medium | ||
| textPosition: StatusBaseButton.TextPosition.Left | ||
| textColor: Theme.palette.primaryColor1 |
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 think these 3 props are the default already
| id: listView | ||
| objectName: "FollowingAddressesView_followingAddresses" | ||
| anchors.fill: parent | ||
| spacing: 8 |
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.
| spacing: 8 | |
| spacing: Theme.halfPadding |
| property string lastReloadedTime | ||
|
|
||
| /* Indicates whether the content is being loaded */ | ||
| property bool loading |
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.
required
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.
hmm, required is helpful when no value makes no sense and there is no good default value and e.g. component would be rendered incorrectly because of that. In this case required is not needed imo.
| BlockchainExplorersMenu { | ||
| id: blockchainExplorersMenu | ||
| flatNetworks: root.activeNetworks | ||
| onNetworkClicked: { |
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.
| onNetworkClicked: { | |
| onNetworkClicked: (shortname, isTestnet) => { |
| visible: !!root.name | ||
| enabled: root.showButtons | ||
| type: StatusRoundButton.Type.Quinary | ||
| radius: 8 |
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.
| radius: 8 | |
| radius: Theme.radius |
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.
and above too
| } | ||
|
|
||
| title: name | ||
| objectName: name || "followingAddressDelegate" |
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.
You usually don't set it here, but rather inside the (List)view, here it's kinda useless
| import "../stores" | ||
| import ".." | ||
|
|
||
| import AppLayouts.Wallet.stores as WalletStores |
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.
Again, all those imports needed?
|
@caybro @micieslak thank you for your reviews, I've pushed your suggestions now, I appreciate the comments. let me know if I've missed anything :) |
|
@caveman-eth I'm checking with infra team why CI only triggered partially |
@igor-sirotin Ok thanks. I also don't have read perms for jenkins, so I can't see why its the checks are failing. |
|
@igor-sirotin any info on the CI runs? Is it because the status-go PR isn't merged? |
Sorry for the delay. Let's ping infra one more time. It is not related to |
micieslak
left a comment
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.
Thank you for the alignments! Looks good to me from the UI side.
I think ideally we should have some review from wallet team. @dlipicar @Khushboo-dev-cpp could you take a look?
| import QtQuick | ||
|
|
||
| QtObject { | ||
| function getContactPublicKeyByAddress(address) { |
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.
The general rule we try to follow and fully align to in the future is to keep those stubs empty. It's up to tests/pages to provided mocked impl.
|
|
||
| property bool showSavedAddresses | ||
| property bool isAccountTokensReloading | ||
| property int lastReloadTimestamp: Date.now() / 1000 |
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.
It's ok now, but when converted to non-singleton we will make it an empty stub as well, moving the mock to e.g. storybook/mocks
| id: root | ||
|
|
||
| property SharedStores.NetworkConnectionStore networkConnectionStore | ||
| property var rootStore // Injected from parent, not singleton |
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.
it could be strictly typed
| property alias starButton: starButton | ||
|
|
||
| signal openSendModal(string recipient) | ||
| signal menuRequested(string name, string address, string ensName, var tags) |
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.
basically it's not necessary (and not recommended) to pass via a signal stuff that is already known to the caller (as the caller provided it via properties).
| import QtQuick | ||
|
|
||
| QtObject { | ||
| function getContactPublicKeyByAddress(address) { |
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.
Ideally stubs should remain empty, it's up to tests/pages to provide mocks. If needed to be shared, can go to storybook/mocks as well.
|
@caveman-eth I forked your branch and opened a test PR with it here: #19446 The goal is to have the full CI suite running. There is no easy way to do it for external contributors safely. If that PR passes, we can merge your work once you rebase! Make sure to rebase the status-go PR first. We'll merge that one, then you can point to the develop commit containing your fix and we'll merge the desktop branch! |
|
It seems like both mobile builds fail on CI on your PR, even though they work in other PRs, so it's something specific here. Sadly, the build is not very nice with the logs: To get better logs, we need to use the |
|
@caveman-eth I ran the build with The It happened in three files. I pushed a commit in my own PR to fix it. You can cherry-pick it to yours: 83b827d |
|
Alright! Everything passes in #19446 now! So @caveman-eth just rebase the status-go branch. Once it's merged, rebase this one and point to develop's commit and use the commit I created here. |
@jrainville |
Those builds are "beta" we could say. They are very flaky right now and not required to merge, so it's fine |
Introduces support for EFP following addresses throughout the wallet app, including backend RPCs, service layer, controller, model, and QML UI integration. Adds new modules, views, delegates, and updates wallet section logic to display and manage onchain friends from EFP, with ENS and avatar support. Also updates SavedAddressesDelegate to handle EFP addresses and refines wallet header and layout for the new section.
…ing addresses Cleans up excessive debug logging and removes unused loading state and cache checks from the following addresses modules and service. Updates the QML views to simplify loading indicators and ensures data is loaded when the user navigates to the following addresses page. Update status-go submodule to latest commit
Extracted FollowingAddressMenu into a separate QML component and updated FollowingAddressesDelegate to use a menuRequested signal for menu actions. Improved property requirements and data flow in FollowingAddresses and FollowingAddressesView, and updated connections for refreshing and updating following addresses. Minor UI text adjustment in WalletFollowingAddressesHeader and code cleanup in RootStore. This refactor improves modularity, maintainability, and clarity of the following addresses feature.
Eliminated info log statements from the load() method in the wallet_section module to reduce console output and clean up the code.
Refactors FollowingAddresses components to use injected rootStore and activeNetworksModel, improving testability and modularity. Adds storybook pages for FollowingAddressMenu, FollowingAddressesDelegate, and FollowingAddressesView for interactive testing and development. Updates RootStore stubs and related stores to support new signals and mock data. Improves pagination, event handling, and UI consistency in the following addresses workflow.
Corrected indentation for 'avatar' and 'isFollowingAddress' properties. Advanced the status-go submodule pointer to live commit 4c3fda7768ed7d35abb187877aac5962e3ee19bc Fix spacing property to handle undefined values Updated the spacing assignment to use optional chaining
|
@jrainville I rebased and also added in your commit, but seems that the mobile builds are still failing here. |
Looks like a normal flaky error. I'll reset my PR to your changes to double check, but if all is good I'll merge 😄 |
|
Everything passed! Merging. Congrats on your amazing work @caveman-eth |
@jrainville |
What does the PR do
Adds a new "Following Addresses" tab to the wallet that displays onchain followings from the Ethereum Follow Protocol (EFP). Users can view, search, and save their EFP followings directly in Status.
Issue: #18686
Status-go dependency: status-im/status-go#7052
Features:
Technical implementation:
following_addressservice module in Nim with async RPC task handlingFollowingAddressesView- extendsRightTabBaseViewwith custom headerWalletFollowingAddressesHeader- custom header component matching SavedAddresses patternFollowingAddresses- main content component with search and paginationFollowingAddressesDelegate- individual address list itemSavedAddressActivityPopupandSavedAddressesDelegateto support following addressesAffected areas
Architecture compliance
My PR is consistent with this document: QML Architecture Guidelines
Screencapture of the functionality
Video: https://streamable.com/3y4s1f
Image:

Impact on end user
Before:
After:
How to test
Risk
Low risk. New feature with no modifications to existing wallet functionality.