Better error handling for wallet transactions + handling of missing signals #22179
+247
−113
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (39)
|
alwx
force-pushed
the
bug/swap-bridge-improvements
branch
3 times, most recently
from
b517215 to
ac07213
Compare
clauxx
approved these changes
Mar 5, 2025
|
@alwx |
|
сс @alwx - pinging for visibility, seems this Pr is here for awhile, may we take actions to move it forward? Thanks! |
|
@churik sure, just other things were more important. Updating it today. |
alwx
force-pushed
the
bug/swap-bridge-improvements
branch
2 times, most recently
from
263fc45 to
ff65daf
Compare
|
Moving it to E2E tests column (finally) |
61% of end-end tests have passedFailed tests (9)Click to expandClass TestWalletOneDevice:
Class TestWalletMultipleDevice:
Class TestWalletCollectibles:
Passed tests (14)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestProfileMultipleDevices:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestWalletCollectibles:
Class TestCommunityMultipleDeviceMerged:
Class TestWalletOneDeviceTwo:
Class TestWalletOneDevice:
|
|
@alwx could you please expand on how exactly this issue #21473 is fixed, what is the expected result? User should not face the described error anymore OR this error is handled in some other way? Same question about #21735 : do you expect that user should not face |
|
@pavloburykh the thing is when working with external providers we cannot really guarantee that things work 100% of times — sometimes the provider might be unavailable, sometimes you might reach the request limit, etc etc. But #21473 must be gone, and the whole error handling process was updated so that the errors you might be getting in the process should match those you see on desktop. |
78% of end-end tests have passedFailed tests (2)Click to expandClass TestWalletCollectibles:
Passed tests (7)Click to expandClass TestWalletOneDevice:
Class TestWalletMultipleDevice:
|
|
hi @alwx Thank you for PR. Take a look, this issue still exists PR_ISSUE 1: Unclear validation messages are shown for unsupported assets for Base on bridgeSteps:
Actual result:The validation toast is not clear on the bridge if a non-supported token is used: Expected result:Clear and user-friendly validation messages should be displayed, such as: https://www.figma.com/design/xLs1KYmF4e6WwRTZVJKeUK/Wallet?node-id=10232-108495&t=sVvADfGe0WsbBGA4-1 Devices:
|
|
@VolodLytvynenko regarding ISSUE 1: I am not sure we expect this issue #22012 to be fixed by current PR. I do not see this issue in the PR description. Why do you assume it should be fixed? Anyway, @alwx can provide more clarification on this. |
|
I'm surprised to see this issue in the current PR, as I was sure it wasn’t related to it. However, I'm unable to reproduce it in the latest development, but I can consistently reproduce it on 2 devices across different networks. Could you please take a look PR_ISSUE 2: Approval transaction is shown before bridge transaction in activity tabSteps to reproduce:
Actual result:The Approval transaction appears first (at the top). Expected result:The Bridge transaction should be shown first at the top as within blockchain OS:IOS, Android Devices:
Logshttps://drive.google.com/file/d/1eOgxGMnxgljP7VlXI636mPszvZJ5b0xv/view?usp=drive_link |
alwx
force-pushed
the
bug/swap-bridge-improvements
branch
from
f7410e9 to
f0e5a6c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Potentially fixes #21735
Fixes #21473
Also improves and simplifies error handling which is required for #22046 as well
Summary
This PR adds missing handing of missing signals and updates processing of a couple of existing signals:
wallet.router.sending-transactions-started(new)wallet.router.transactions-sentwallet.transaction.status-changed(new)In addition to that, error processing has been simplified and unified with
status-desktop— basically all the logic in:wallet/show-transaction-notificationis there to match the logic here: https://github.com/status-im/status-desktop/blob/master/src/app/modules/main/module.nim#L445Potential next steps (to be discussed)
status-desktopgoes further and use the same function to display all kinds of transaction-related notifications. Do we do more informative toasts? Do we need to duplicate the logic of indication how the transaction is moving forward?Functional
status: ready