chore(communities-wallet): various improvements on community related transaction flows #17010
Conversation
saledjenic
requested review from
alaibe,
friofry,
dlipicar,
jrainville,
alexjba and
anastasiyaig
Jenkins BuildsClick to see older builds (102)
|
saledjenic
force-pushed
the
fix/community-minting-airdrop
branch
3 times, most recently
from
23ce959 to
fa6b1f5
Compare
saledjenic
force-pushed
the
fix/community-minting-airdrop
branch
3 times, most recently
from
b37a3ee to
a33044d
Compare
saledjenic
force-pushed
the
fix/community-minting-airdrop
branch
3 times, most recently
from
348bc1f to
6913c91
Compare
|
I have some questions about this PR When airdropping, if the 'Show fees' toggle is not switched on, nothing happens (the 'create airdrop' button does not enable). This is a bit confusing because the toggle seems an optional field; the 'create airdrop' button should be enabled when the token and the address have been selected, regardless of the fees toggle. What is the correct behavior in this case?
KeycardThe flows cannot be tested with Keycard: the minting of owner and TokenMaster seems to be stuck. Owner and TokenMaster tokens minted using keycard. Note that I cancelled the authentication via Biometrics and chose the keycard PIN. https://sepolia-optimism.etherscan.io//tx/0x7272f1087f5e2774ff151a6ab926f4b2babac34bb16a7ef68e79e72eeb65d61e Screen.Recording.2025-01-20.at.2.11.11.PM.movSomething seems stuck because the UI still shows the screen to mint the owner/tokenmaster tokens. Screen.Recording.2025-01-20.at.2.12.17.PM.movI'm also noticing some issues on this PR:
Screen.Recording.2025-01-20.at.3.12.42.PM.mov
Screen.Recording.2025-01-20.at.3.00.59.PM.movScreen.Recording.2025-01-20.at.3.02.52.PM.movI'm attaching the logs of the account that I used to test in case they are useful: Account with keycard: Account without a keycard: |
saledjenic
force-pushed
the
fix/community-minting-airdrop
branch
2 times, most recently
from
f792337 to
fe044c7
Compare
|
@virginiabalducci thanks for checking this PR.
There are some technical limitations with the old approach. Already discussed this with @benjthayer cause we do not know all the details necessary for asking for the best route and all the calculations that we need before the form is fulfilled. Also, for the difference from the old approach, the received route is valid for the entered data and that's why as the first aid I added that "fees toggle". It becomes enabled once the form is fulfilled and once it's enabled, toggling on asks for the route. All that from the UX side of things will be handled by the new design.
Actually, it's not an issue in the keycard flow itself since the tx is always executed, but was an issue in the form of an address used to add tokens later. That's fixed now, you can check it now.
As said above this PR was not about fixing things, but it's about switching to completely different way of doing things than how it was. In general, this PR aligns the desktop client with the changes we did on the statusgo side. All other issues that we had/have will be fixed after, and I am pretty sure all of them (or the majority) are on the UI (qml) side. Also it's on the UI team of the backend team, once it becomes a priority, to revise/rework the entire community part. Based on all I said here, also having in mind the complexity of this PR (including statusgo PRs) and since it's tough to rebase it if conflicts appear due to many old redundant codes which is removed in this PR, I am kindly asking you all reviewers here to review this PR (and related statusgo PRs) and finally have it merged soon. @ALL |
Thanks for the feedback @saledjenic , then from my end it's good to merge |
saledjenic
force-pushed
the
fix/community-minting-airdrop
branch
from
fe044c7 to
7a7341d
Compare
|
The issue of the contact requests is no longer present on latest commit, however the Start Chat button issue is still present, this seems to be an issue coming from the master build. It was reported here #17131 |
There was a problem hiding this comment.
Unfortunately the SubscriptionBroker is still needed for dapps with all its features. My suggestion is to drop the changes done in SubscriptionBroker and Subscription.
I see two ways of doing this, depending how much effort we're willing to put into removing flows that are no longer necessary.
The simple approach would be continue using the SubscriptionBroker. But to extend it instead of changing it, to allow single shot requests instead of periodic requests (maybe it will work out of the box if the request interval is set to 0).
It would work just as it used to work before, but the request-response ratio wouldn't be 1-1, but 1-n.
The downside is that we'll push the refactoring for later. Maybe not a downside at all since the changes needed for this PR are already substantial.
The other way would be to remove the SubscriptionBroker dependency from ui/app/AppLayouts/Communities/helpers/TransactionFeesBroker.qml. Instead of calling d.feesBroker.subscribe(subscription) in TransactionFeesBroker::subscribe to do an immediate request to the router. On top of this, we'll need to also copy some of the logic provided by SubscriptionBroker::connectToSubscriptionEvents to preserve the "managed" subscription flows and connect/disconnect from nim events when needed.
ui/app/mainui/Popups.qml
Outdated
| active: finalisePopup.contentItem.Window.window.active | ||
| } | ||
| onStopUpdatingFees: { | ||
| root.stopUpdatingFees() |
There was a problem hiding this comment.
stopUpdatingFees is not implemented.
There was a problem hiding this comment.
Also, not sure the stopUpdatingFees is needed. With the current mechanism it can be controlled by using the enabled property exposed by the subscriber.
There was a problem hiding this comment.
stopUpdatingFees is not implemented.
How do you mean this?
Also, not sure the stopUpdatingFees is needed. With the current mechanism it can be controlled by using the enabled property exposed by the subscriber.
Yes, it's needed, cause it tells the backend (statusgo) to stop updating and sending fees. At first yes, we could use the enabled property, but there is one more component added there, fees toggle, and the user can toggle it off, while the subscriber is still enabled and in that case we actually don't want to receive any updates.
There was a problem hiding this comment.
How do you mean this?
There's no stopUpdatingFees() implementation in Popups.qml
Yes, it's needed, cause it tells the backend (statusgo) to stop updating and sending fees. At first yes, we could use the enabled property, but there is one more component added there, fees toggle, and the user can toggle it off, while the subscriber is still enabled and in that case we actually don't want to receive any updates.
What this means is that there are cases where the router continues to update fees and send signals, but we don't want to propagate to QML?
I've had the impression that we want to stop the router processing when the toggle is off. I was expecting the subscriber to control this flow when it's disabled.
ui/app/mainui/Popups.qml
Outdated
| SetSignerFeesSubscriber { | ||
| id: feeSubscriber | ||
| onCalculateFees: { | ||
| const subscription = feesBroker.prepareSetSignerFeesSubscribtion(feeSubscriber) |
There was a problem hiding this comment.
Ideally the subscription object is not exposed by the broker. It was an internal detail.
Why is it a 2 steps mechanism to use the subscribe function? Can we modify the subscribe to receive the subscriber and prepare the data internally? Then the usage is quite simple: Declare the subscriber and call subscribe when needed.
There was a problem hiding this comment.
Why is it a 2 steps mechanism to use the subscribe function?
When I started integrating the backend changes, I remembered that I needed subscriptionId out of the broker. Still, I didn't check for justification of that after I cleaned the code and prepared the PR. Now, looking at the code seems no need and we can merge those two calls into a single one. Will try to update that.
| } | ||
|
|
||
| onStopUpdatingFees: { | ||
| root.stopUpdatingFees() |
There was a problem hiding this comment.
Why not controlling the deployFeeSubscriber.enabled property instead of delegating the action with a signal?
|
@alexjba thanks for the review and comments. I see the point you made, and yes, I agree and that's something we need to do, but not sure if the time is now (in this PR) since I just wanted to not touch the qml (or touch it as less as possible, just to make the app works with the new backed logic). |
Unfortunately I don't see any way out without touching qml..The main constraint is that we need the |
saledjenic
force-pushed
the
fix/community-minting-airdrop
branch
2 times, most recently
from
1cb4c07 to
91f13bb
Compare
|
@alexjba I created a new SubscriptionBrokerCommunities for community purposes specifically, so dapps are not affected by the changes. Also kept the subscriptions within the broker, cause there is no need for a subscription id out of it now. |
saledjenic
force-pushed
the
fix/community-minting-airdrop
branch
from
91f13bb to
e9744f7
Compare
…transaction flows These changes should simplify the community related tx handlings on the client side, align it with tx flows that we already have for other sending types and make it maintainable.
saledjenic
force-pushed
the
fix/community-minting-airdrop
branch
from
e9744f7 to
d274453
Compare
Corresponding
status-goPRs:These changes should simplify the community-related tx handlings on the client side, align it with tx flows that we already have for other sending types and make it maintainable.
Related issue: #16810
This PR doesn't fix issues that we already have (if any are fixed, that's a side effect, not intention). The intention of this PR is to introduce a new sending flow to the community part of the app and make it maintainable for the future.
UI for the community's settings part has to be revised and improved a lot on the UI (qml) side. I am not sure if BrokerFees will be needed at all after these changes (but they are kept because of minimizing number of changes) as well as many other approaches done there. Code can be improved and simplified a lot on the qml side.
Spotted issues:
Flows that will be covered by finishing this PR: