-
Notifications
You must be signed in to change notification settings - Fork 984
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
Metamask address support #20844
Metamask address support #20844
Conversation
Jenkins BuildsClick to see older builds (43)
|
It's WIP or not? 🤔 |
@J-Son89, not anymore) |
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.
Hi @vkjr
Thanks for your PR!
I left a some comments
(defn- is-text-a-status-url-for-path? | ||
[text path] | ||
(some #(string/starts-with? text %) (router/path-urls path))) | ||
(some #(string/starts-with? text %) (router/prepend-status-urls path))) |
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.
Booleans in Clojure don't start with the prefix is-
, it's enough if we just provide the question mark at the end
(validation/eth-address? address) [:wallet/address-validation-success address] | ||
:else [:wallet/address-validation-failed address]) | ||
(<= (count address) 0) [:wallet/address-validation-failed address] | ||
(utils-address/eip-3770-address? address) [:wallet/address-validation-success 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.
👍
src/utils/address.cljs
Outdated
;; EIP-3770 is a format used by Status and described here: https://eips.ethereum.org/EIPS/eip-3770 | ||
(def regx-eip-3770-address #"^(?:(?:eth:|arb1:|oeth:)(?=:|))*0x[0-9a-fA-F]{40}$") | ||
(def regx-metamask-address #"^ethereum:(0x[0-9a-fA-F]{40})@(0x1|0xa|0xa4b1)$") | ||
(def regx-address-contains #"(?i)0x[a-fA-F0-9]{40}") |
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.
💯
src/utils/address.cljs
Outdated
"0xa" "oeth:" | ||
nil)) | ||
|
||
(defn is-metamask-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.
Consider what I mentioned before about the is-
prefix in Clojure
src/utils/address.cljs
Outdated
(defn metamask-address->status-address | ||
[metamask-address] | ||
(when-let [[_ address metamask-network-suffix] (is-metamask-address? metamask-address)] | ||
(when-let [status-network-prefix (eip-155-suffix->eip-3770-prefix metamask-network-suffix)] | ||
(str status-network-prefix 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.
IMO the usage of is-metamaks-address?
isn't a good practice for future maintenance.
I understand that is-metamaks-address?
will return the result of re-find
, which is a vector of the matching regex groups, but since this function ends with ?
it's better to always treat it as if its return value is a boolean.
What I'd suggest is to create a function that performs the string splitting you are performing with re-find
and another function to just check if an address is a metamask one or not.
wdyt?
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.
@ulisesmac, I fully agree with you, the naming doesn't match current usecase and it is better to split function.
And with all other comments too, thanks a lot for reviewing!
src/utils/address.cljs
Outdated
:else | ||
nil)) |
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.
We don't need :else nil
for cond
since it already returns nil
when there isn't a matching clause.
src/utils/address_test.cljs
Outdated
|
||
(def valid-metamask-addresses | ||
["ethereum:0x38cf6E0Ba4C4530735616e1Ee7ff5FbCB726fBd2@0x1" | ||
"ethereum:0x38cf6E0Ba4C4530735616e1Ee7ff5FbCB726fBd2@0xa4b1" | ||
"ethereum:0x38cf6E0Ba4C4530735616e1Ee7ff5FbCB726fBd2@0xa"]) | ||
|
||
(def invalid-metamask-addresses | ||
["ethe:0x38cf6E0Ba4C4530735616e1Ee7ff5FbCB726fBd2@0x1" | ||
":0x38cf6E0Ba4C4530735616e1Ee7ff5FbCB726fBd2@0xa4b1" | ||
"0x38cf6E0Ba4C4530735616e1Ee7ff5FbCB726fBd2@0xa" | ||
"ethereum:0x38cf6E0Ba4C4530735616e1Ee7ff5FbCB726fBd2@0x1d" | ||
"ethereum:0x38cf6E0Ba4C4530735616e1Ee7ff5FbCB726fBd20xa4b1" | ||
"ethereum:0x38cf6E0Ba4C4530735616e1Ee7ff5FbCB726fBd2:0xa" | ||
"ethereum:0x38cf6E0Ba4C4530735616e1Ee7ff5FbCB726fBd2"]) | ||
|
||
(def metamask-to-status | ||
[{:metamask "ethereum:0x38cf6E0Ba4C4530735616e1Ee7ff5FbCB726fBd2@0x1" | ||
:status "eth:0x38cf6E0Ba4C4530735616e1Ee7ff5FbCB726fBd2"} | ||
{:metamask "ethereum:0x38cf6E0Ba4C4530735616e1Ee7ff5FbCB726fBd2@0xa4b1" | ||
:status "arb1:0x38cf6E0Ba4C4530735616e1Ee7ff5FbCB726fBd2"} | ||
{:metamask "ethereum:0x38cf6E0Ba4C4530735616e1Ee7ff5FbCB726fBd2@0xa" | ||
:status "oeth:0x38cf6E0Ba4C4530735616e1Ee7ff5FbCB726fBd2"} | ||
{:metamask "ethe:0x38cf6E0Ba4C4530735616e1Ee7ff5FbCB726fBd2@0x1" :status nil} | ||
{:metamask ":0x38cf6E0Ba4C4530735616e1Ee7ff5FbCB726fBd2@0xa4b1" :status nil} | ||
{:metamask "0x38cf6E0Ba4C4530735616e1Ee7ff5FbCB726fBd2@0xa" :status nil} | ||
{:metamask "ethereum:0x38cf6E0Ba4C4530735616e1Ee7ff5FbCB726fBd2@0x1d" :status nil} | ||
{:metamask "ethereum:0x38cf6E0Ba4C4530735616e1Ee7ff5FbCB726fBd20xa4b1" :status nil} | ||
{:metamask "ethereum:0x38cf6E0Ba4C4530735616e1Ee7ff5FbCB726fBd2:0xa" :status nil} | ||
{:metamask "ethereum:0x38cf6E0Ba4C4530735616e1Ee7ff5FbCB726fBd2" :status nil}]) | ||
|
||
(deftest is-metamask-address?-test | ||
(testing "Check valid metamask addresses" | ||
(dorun | ||
(for [address valid-metamask-addresses] | ||
(is (utils.address/is-metamask-address? address))))) | ||
(testing "Check invalid metamask addresses" | ||
(dorun | ||
(for [address invalid-metamask-addresses] | ||
(is (not (utils.address/is-metamask-address? address))))))) | ||
|
||
(deftest metamask-address->status-address-test | ||
(testing "Check metamask to status address conversion is valid" | ||
(dorun | ||
(for [{metamask-address :metamask | ||
status-address :status} metamask-to-status] | ||
(is (= status-address (utils.address/metamask-address->status-address metamask-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.
Thank you for these tests! 👍 💯
57% of end-end tests have passed
Failed tests (3)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestWalletMultipleDevice:
Passed tests (4)Click to expandClass TestWalletOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
|
@vkjr Thank you for the PR. Here are found issues: ISSUE 1: User navigated to overview wallet page when MetaMask QR is Scanned by Universal ScannerSteps:
Actual Result:The user is navigated to the wallet overview page after scanning the MetaMask QR code. wallet.mp4Expected Result:The app should recognize the MetaMask QR code and show the drawer with the scanned address. Devices:Pixel 7a, Android 13 Additional info:The QR with "ethereum" prefix is not recognized on the Send to page as well "Oops this is not QR" toast is shown qr_scanning.mp4 |
@VolodLytvynenko , thanks for finding, checking! |
Hi @vkjr, the PR looks great. The QR code issue mentioned in this comment seems to be related to the Metamask plugin. The mobile version usually shares other QRs, which can be scanned well using a build of this PR. The issue can be fixed in a follow-up, but if you think it’s better to address it in this PR, feel free to do so. Otherwise, PR can be merged |
@VolodLytvynenko, qr code from metamask plugin contains text |
@vkjr The best way to address this issue is to ignore the "ethereum:" prefix and only extract the address body, like |
@VolodLytvynenko, I added support to ethereum addresses without suffixes. Could you please briefly take a look, just to make sure all is good? |
71% of end-end tests have passed
Failed tests (2)Click to expandClass TestWalletMultipleDevice:
Passed tests (5)Click to expandClass TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
|
@VolodLytvynenko, are the results of endtoend testing acceptable? |
@vkjr Yes. e2e are ok. PR can be merged. Thank you! |
@VolodLytvynenko, thank you! |
fixes #19986
Summary
This PR adds support for metamask addresses, they now can be scanned from QR codes and converted to Status addresses.
Small additional changes:
utils.address
namespaceReview notes
Metamask stores addresses in QR codes with the following format:
ethereum:0x38cf6E0Ba4C4530735616e1Ee7ff5FbCB726fBd2@0xa
ethereum: - prefix described in ethereum/EIPs#67
0xa - chain ID suffix described in EIP-155
Testing notes
Bunch of qr codes for quick testing:
status-qr-multichain
status-qr-profile
metamask-qr-@0xa4b1
metamask-qr-@0xa
metamask-qr-@0x1
Platforms
Areas that maybe impacted