Dapps Typed Data request improvements #21333

clauxx · 2024-09-26T11:33:53Z

Summary

Improved the UI of the wallet connect typed data request by flattening the data and displaying it to the user, as seen in the screenshot below.

Steps to test

Open Status
Connect to dapp (e.g. https://react-app.walletconnect.com/ will work on mainnet only)
Generate a eth_signTypedData_v4 request
See the request data

status: ready

status-im-auto · 2024-09-26T11:39:13Z

Jenkins Builds

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`dac775d`	#2	2024-09-26 11:39:12	~4 min	`tests`	📄`log`
✔️	`dac775d`	#2	2024-09-26 11:41:12	~6 min	`android-e2e`	🤖`apk` 📲
✔️	`dac775d`	#2	2024-09-26 11:42:47	~8 min	`android`	🤖`apk` 📲
✔️	`dac775d`	#2	2024-09-26 11:47:58	~13 min	`ios`	📱`ipa` 📲

✔️	`ad19102`	#4	2024-10-01 08:47:47	~4 min	`tests`	📄`log`
✔️	`ad19102`	#4	2024-10-01 08:49:33	~6 min	`android-e2e`	🤖`apk` 📲
✔️	`ad19102`	#4	2024-10-01 08:50:00	~6 min	`android`	🤖`apk` 📲
✔️	`ad19102`	#4	2024-10-01 08:52:23	~8 min	`ios`	📱`ipa` 📲

mohsen-ghafouri

Nice work 🚀

ilmotta · 2024-09-26T19:46:42Z

src/status_im/contexts/wallet/wallet_connect/utils/typed_data.cljs

+  (-> typed-data
+      (select-keys [:domain :message])
+      flatten-data
+      (format-fields ": ")))


Is the format from: wallets: 1: copied from other products or is this a standard we are creating ourselves for Status? I find hard to imagine users can figure out the indexes and the flattening going on, but maybe not, things are crazy in terms of usability when signing.

Do you know if there's any interest in improving the designs or if any concerns were raised?

I think we're doing small iterations on this. There is no standard around typed data, but there are definitely improvements to be made. Aside from the representation of arrays, which I agree that is not ideal, is the inclusion of custom types instead (or in addition) to key names, as it is now. The custom types should give more context around what the piece of data represents and the context it lives in. For now though, we decided move away from json but keep it simple at the same time.

ilmotta · 2024-09-26T19:47:00Z

src/status_im/contexts/wallet/wallet_connect/utils/typed_data.cljs

@@ -0,0 +1,94 @@
+(ns status-im.contexts.wallet.wallet-connect.utils.typed-data


@clauxx, is the Desktop app integrating now or will integrate with WC? Sorry if this is an obvious question, but my point is that I feel this logic doesn't belong in clients and should be in status-go. A lot of wallet-related code is living in the wrong place in my opinion. In the not so distant past, our high-level architectural goal was to build light clients and I think the goal still stands.

Well, this is just a thought for your PR, not a suggestion for change. It would be great if the Mobile Wallet team more heavily considered this factor because we are inflating too much the client with logic. It's great that you unit tested, but this is not always the case in mobile PRs. My belief and experience with status-go is that the slowdown in iterations would only come in the beginning as people learn how to solve with this backend-first mentality.

cc'ing @shivekkhurana @smohamedjavid just to bump this point a bit more.

It's on the client for now, since this logic is directly related to the UI and how we show the typed data to the user. We are dealing with limited horizontal space on mobile, so we had to find solutions to remove nesting. On the desktop this might not be an issue, so the representation there might be different.

ilmotta · 2024-09-26T19:47:18Z

src/status_im/contexts/wallet/wallet_connect/modals/sign_message/view.cljs

+(defn display-data-view
+  []
+  (let [typed-data? (->> (rf/sub [:wallet-connect/current-request-method])
+                         (contains? typed-data-methods))]


Could be a separate subscription. typed-data-methods is static, so the result for typed-data? will be nicely cached.

ilmotta · 2024-09-26T19:47:30Z

src/status_im/contexts/wallet/wallet_connect/modals/sign_message/view.cljs

+  [{:keys [label value]}]
+  [quo/data-item
+   {:card?           false
+    :container-style (merge style/data-item


merge is known to be quite slow, so if we can avoid it the better. Here you can use assoc, or better extract the container-style map into a var because it's a static map.

ilmotta · 2024-09-26T19:47:59Z

src/status_im/contexts/wallet/wallet_connect/utils/typed_data_test.cljs

+            [["2"] 2]])))
+
+  (testing "correctly flattens a simple vec"
+    (is (= (sut/flatten-data ["zero" "one"])


The expected argument should be the first.

Example output from CIDER from another project:

(deftest some-test (is (= 1 0)))

…a-improve status-im/status-go@effde33...effde33

clauxx · 2024-10-01T10:04:57Z

@ilmotta the comments were addressed 👍

status-im-auto · 2024-10-01T12:45:26Z

71% of end-end tests have passed

Total executed tests: 7
Failed tests: 2
Expected to fail tests: 0
Passed tests: 5

IDs of failed tests: 702745,727229

Failed tests (2)

Click to expand

Rerun failed tests

Class TestWalletMultipleDevice:

1. test_wallet_send_eth, id: 727229

Device 2: Find `Text` by `xpath`: `//android.view.ViewGroup[@content-desc='container']/android.widget.TextView[@text='Ether']/../android.widget.TextView[3]`
Device 2: `Text` is `0.07559 ETH`

critical/test_wallet.py:159: in test_wallet_send_eth
    self.errors.verify_no_errors()
base_test_case.py:192: in verify_no_errors
    pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
 Sender balance is not updated on Etherscan, it is 0.4067 but expected to be 0.4068

Class TestOneToOneChatMultipleSharedDevicesNewUi:

1. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745

Device 2: Tap on found: Button
Device 2: Attempt 0 is successful clicking close-activity-center

Test setup failed: critical/chats/test_1_1_public_chats.py:40: in prepare_devices
    self.home_2.handle_contact_request(self.username_1)
../views/home_view.py:410: in handle_contact_request
    chat_element.accept_contact_request()
../views/home_view.py:162: in accept_contact_request
    self.handle_cr("accept-contact-request")
../views/home_view.py:159: in handle_cr
    ).wait_for_rendering_ended_and_click()
../views/base_element.py:163: in wait_for_rendering_ended_and_click
    self.wait_for_visibility_of_element(20)
../views/base_element.py:147: in wait_for_visibility_of_element
    raise TimeoutException(
 Device 2: Button by xpath:`//*[contains(@text, 'sender')]/ancestor::*[@content-desc='activity']/*[@content-desc="accept-contact-request"]` is not found on the screen after wait_for_visibility_of_element

Device sessions

Device 1:

Steps, video, logs

Device 2:

Steps, video, logs

Passed tests (5)

Click to expand

Class TestWalletMultipleDevice:

1. test_wallet_send_asset_from_drawer, id: 727230

Class TestCommunityOneDeviceMerged:

1. test_community_copy_and_paste_message_in_chat_input, id: 702742
Device sessions Device 1: Steps, video, logs
2. test_restore_multiaccount_with_waku_backup_remove_switch, id: 703133
Device sessions Device 1: Steps, video, logs

Class TestWalletOneDevice:

1. test_wallet_add_remove_regular_account, id: 727231
Device sessions Device 1: Steps, video, logs

Class TestCommunityMultipleDeviceMerged:

1. test_community_message_edit, id: 702843
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs

mariia-skrypnyk · 2024-10-01T13:13:35Z

Hey @clauxx!

I get an error during testing that blocks me.
Can you please take a look?

WC-PR-debug-logs.zip

my steps:

Login (Testnet but I also tried Mainnet too)
Scan https://react-app.walletconnect.com/ QR code
Generated a eth_signTypedData_v4 request

Thanks

clauxx · 2024-10-01T13:23:26Z

Hey @clauxx!

I get an error during testing that blocks me.

my steps:
1. Login (Testnet but I also tried Mainnet too)

2. Scan https://react-app.walletconnect.com/ QR code

3. Generated a eth_signTypedData_v4 request
Thanks

Hi! It will not work on testnet, since the data includes the wrong chainId (mainnet), and we check that for safety. It should work on Mainnet though. Did you make sure while connecting that both the dapp and the wallet are on mainnet? Should probably add this in the description.

status-im-auto · 2024-10-01T17:09:38Z

0% of end-end tests have passed

Total executed tests: 2
Failed tests: 2
Expected to fail tests: 0
Passed tests: 0

IDs of failed tests: 702745,727229

Failed tests (2)

Click to expand

Rerun failed tests

Class TestOneToOneChatMultipleSharedDevicesNewUi:

1. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745

Device 2: Tap on found: Button
Device 2: Attempt 0 is successful clicking close-activity-center

Test setup failed: critical/chats/test_1_1_public_chats.py:40: in prepare_devices
    self.home_2.handle_contact_request(self.username_1)
../views/home_view.py:410: in handle_contact_request
    chat_element.accept_contact_request()
../views/home_view.py:162: in accept_contact_request
    self.handle_cr("accept-contact-request")
../views/home_view.py:159: in handle_cr
    ).wait_for_rendering_ended_and_click()
../views/base_element.py:163: in wait_for_rendering_ended_and_click
    self.wait_for_visibility_of_element(20)
../views/base_element.py:147: in wait_for_visibility_of_element
    raise TimeoutException(
 Device 2: Button by xpath:`//*[contains(@text, 'sender')]/ancestor::*[@content-desc='activity']/*[@content-desc="accept-contact-request"]` is not found on the screen after wait_for_visibility_of_element

Device sessions

Device 1:

Steps, video, logs

Device 2:

Steps, video, logs

Class TestWalletMultipleDevice:

1. test_wallet_send_eth, id: 727229

Device 2: Find Text by xpath: //android.view.ViewGroup[@content-desc='container']/android.widget.TextView[@text='Ether']/../android.widget.TextView[3]
Device 2: Text is 0.07579 ETH

critical/test_wallet.py:159: in test_wallet_send_eth
    self.errors.verify_no_errors()
base_test_case.py:192: in verify_no_errors
    pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
 Sender balance is not updated on Etherscan, it is 0.4064 but expected to be 0.4065

mariia-skrypnyk · 2024-10-01T17:14:13Z

Thanks @clauxx .

I've checked on Mainnet once again (probably didn't turn Testnet switcher on dApp) 🤔.

Looks good on both platforms!

Have a tiny question regarding Network element from design here:

https://www.figma.com/design/mLyiYPXqu8KetdQONOlZpV/dApp-Interactions?node-id=1869-27020&node-type=shape_with_text&m=dev

Is it planned to be added or I should report it?
Thanks.

PR can be merged.
Failed e2e are not related.

clauxx · 2024-10-02T10:50:31Z

@mariia-skrypnyk I think it's not needed, as we're checking if the network of the session is the same as the one contained in the data (but it's not guaranteed it is), and if they're not the same we show a toast and reject the request.

clauxx added 5 commits September 25, 2024 17:17

feat: added type data flattening + tests

51a7903

feat: added typed data flattened ui

f6c9217

ref: moved typed data utils into their own ns

8f2b7db

ref: small changes & formatting

19be9b3

feat: show typed data view only for typed data requests

db935cd

clauxx requested review from ilmotta, alwx, shivekkhurana and mohsen-ghafouri September 26, 2024 11:33

clauxx self-assigned this Sep 26, 2024

Merge branch 'develop' into cl-21278-typed-data-improve

dac775d

mohsen-ghafouri approved these changes Sep 26, 2024

View reviewed changes

ilmotta approved these changes Sep 26, 2024

View reviewed changes

clauxx added 3 commits October 1, 2024 11:40

ref: moved typed-data? into sub

220b0fa

fix: tests and styling

a289009

Merge remote-tracking branch 'origin/develop' into cl-21278-typed-dat…

ad19102

…a-improve status-im/status-go@effde33...effde33

clauxx added the request-manual-qa label Oct 1, 2024

mariia-skrypnyk self-assigned this Oct 1, 2024

mariia-skrypnyk added the Tested - Issues label Oct 1, 2024

ilmotta approved these changes Oct 1, 2024

View reviewed changes

mariia-skrypnyk added Tested - OK and removed Tested - Issues labels Oct 1, 2024

clauxx merged commit 97aff27 into develop Oct 2, 2024
6 checks passed

clauxx deleted the cl-21278-typed-data-improve branch October 2, 2024 10:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dapps Typed Data request improvements #21333

Dapps Typed Data request improvements #21333

clauxx commented Sep 26, 2024 •

edited

Loading

status-im-auto commented Sep 26, 2024 •

edited

Loading

mohsen-ghafouri left a comment

ilmotta Sep 26, 2024

clauxx Oct 1, 2024

ilmotta Sep 26, 2024

clauxx Oct 1, 2024

ilmotta Sep 26, 2024

ilmotta Sep 26, 2024

ilmotta Sep 26, 2024

clauxx commented Oct 1, 2024

status-im-auto commented Oct 1, 2024

Class TestWalletMultipleDevice:

Class TestOneToOneChatMultipleSharedDevicesNewUi:

Class TestWalletMultipleDevice:

Class TestCommunityOneDeviceMerged:

Class TestWalletOneDevice:

Class TestCommunityMultipleDeviceMerged:

mariia-skrypnyk commented Oct 1, 2024 •

edited

Loading

clauxx commented Oct 1, 2024

status-im-auto commented Oct 1, 2024

Class TestOneToOneChatMultipleSharedDevicesNewUi:

Class TestWalletMultipleDevice:

mariia-skrypnyk commented Oct 1, 2024 •

edited

Loading

clauxx commented Oct 2, 2024

		@@ -0,0 +1,94 @@
		(ns status-im.contexts.wallet.wallet-connect.utils.typed-data

Dapps Typed Data request improvements #21333

Dapps Typed Data request improvements #21333

Conversation

clauxx commented Sep 26, 2024 • edited Loading

Summary

Steps to test

status-im-auto commented Sep 26, 2024 • edited Loading

Jenkins Builds

mohsen-ghafouri left a comment

Choose a reason for hiding this comment

ilmotta Sep 26, 2024

Choose a reason for hiding this comment

clauxx Oct 1, 2024

Choose a reason for hiding this comment

ilmotta Sep 26, 2024

Choose a reason for hiding this comment

clauxx Oct 1, 2024

Choose a reason for hiding this comment

ilmotta Sep 26, 2024

Choose a reason for hiding this comment

ilmotta Sep 26, 2024

Choose a reason for hiding this comment

ilmotta Sep 26, 2024

Choose a reason for hiding this comment

clauxx commented Oct 1, 2024

status-im-auto commented Oct 1, 2024

71% of end-end tests have passed

Failed tests (2)

Class TestWalletMultipleDevice:

Class TestOneToOneChatMultipleSharedDevicesNewUi:

Passed tests (5)

Class TestWalletMultipleDevice:

Class TestCommunityOneDeviceMerged:

Class TestWalletOneDevice:

Class TestCommunityMultipleDeviceMerged:

mariia-skrypnyk commented Oct 1, 2024 • edited Loading

clauxx commented Oct 1, 2024

status-im-auto commented Oct 1, 2024

0% of end-end tests have passed

Failed tests (2)

Class TestOneToOneChatMultipleSharedDevicesNewUi:

Class TestWalletMultipleDevice:

mariia-skrypnyk commented Oct 1, 2024 • edited Loading

clauxx commented Oct 2, 2024

clauxx commented Sep 26, 2024 •

edited

Loading

status-im-auto commented Sep 26, 2024 •

edited

Loading

mariia-skrypnyk commented Oct 1, 2024 •

edited

Loading

mariia-skrypnyk commented Oct 1, 2024 •

edited

Loading