Selected networks are not shown as preferred if a multichain wallet with preferred networks is scanned on the 'send to' page #19775 #19899
Conversation
This file contains 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
mmilad75
requested review from
briansztamfater,
ilmotta,
3esmit,
vkjr,
OmarBasem,
ulisesmac and
shivekkhurana
and removed request for
3esmit
Jenkins BuildsClick to see older builds (24)
|
ilmotta
approved these changes
May 3, 2024
There was a problem hiding this comment.
LGTM
Just one important note from my part, even though the regexes were extracted as constants, that doesn't give us any guarantee they are correct. Such complex regexes should definitely be covered by unit tests before we merge code IMO. This regx-multichain-address is not an easy one to grasp, but even if it was simpler I think it should be covered by tests. The macro cljs.test/are is our friend :)
|
would be nice to have a video if possible showing the behaviour here 🙏 |
J-Son89
approved these changes
May 6, 2024
Sure 👍 |
Done |
83% of end-end tests have passedFailed tests (7)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletMultipleDevice:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (43)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMerged:
|
29% of end-end tests have passedFailed tests (5)Click to expandClass TestWalletMultipleDevice:
Class TestDeepLinksOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (2)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
|
Hi @mmilad75 thank you for PR. Take a look at a found issue PR_ISSUE 1: 're-matches must match against the string' error is shown when an invalid eth QR is scannedSteps:
Actual result:'re-matches must match against the string' error is shown error_scan.mp4Expected result:No error is shown. If the invalid QR is scanned, then 'Oops this QR does not contain the address ' OS:IOS, Android Devices:
Logs |
|
@VolodLytvynenko Can you please test again? It should be fine now |
0% of end-end tests have passedFailed tests (51)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletOneDevice:
Class TestDeepLinksOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletMultipleDevice:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
|
VolodLytvynenko
force-pushed
the
milad/19775-fix-scanned-address
branch
from
ac9ab33
to
6119423
Compare
83% of end-end tests have passedFailed tests (7)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestWalletMultipleDevice:
Class TestDeepLinksOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletOneDevice:
Expected to fail tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (43)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
|
|
Discussing specification related to 'ethereum' prefix support https://discord.com/channels/624634427930312714/1225795996555022478/1237744788267073586 |
This conversation is way beyond the scope of this ticket. Even if we want to support EIP-155 for the scanner, we need to add it to different part of the app (like when a user tries to add a EIP-155 address manually instead of scanning a qr-code). If we decide to add to support them, we need to create a separate ticket for it. |
OmarBasem
approved these changes
May 9, 2024
hi @mmilad75 Probably, we are going to use the approach as MetaMask does. Addresses that contain the 'ethereum' prefix will be recognized by our scanner, but only the address without the 'ethereum' prefix will be fetched into the 'Sent to' field, which helps the user to continue the sending flow. Steps:
Actual result in PR:The address is not recognized by a scanner develope_qr.mp4How it works in develop and meta mask:The address is recognized and fetched without 'ethereum' prefix nightly.mp4This means that merging this PR can cause the breaking of the feature that supports fetching scanned addresses with the 'ethereum' prefix, which works now in develop. Sorry for the confusion. I should have investigated and clarified this before creating this task: #19775 task. Let's wait for John reply here https://discord.com/channels/624634427930312714/1225795996555022478/1237744788267073586 |
mmilad75
force-pushed
the
milad/19775-fix-scanned-address
branch
from
3dace2c
to
3f63898
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.
fixes #19775
The issue was that the scanner was scanning the text successfully, but the filter we have in the
scan_accountwas removing the prefixes of the address. I replaced it with the correct regex.RPReplay_Final1715001529.MP4