Dapps Typed Data request improvements #21333
There are no files selected for viewing
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| (ns status-im.contexts.wallet.wallet-connect.utils.typed-data | ||
| (:require [clojure.string :as string] | ||
| [status-im.constants :as constants] | ||
| [status-im.contexts.wallet.wallet-connect.utils.networks :as networks] | ||
| [status-im.contexts.wallet.wallet-connect.utils.rpc :as rpc] | ||
| [utils.number :as number])) | ||
|
|
||
| (declare flatten-data) | ||
|
|
||
| (defn- format-flattened-key | ||
| [k] | ||
| (cond | ||
| (keyword? k) (name k) | ||
| (number? k) (str k) | ||
| (string? k) k | ||
| :else "unsupported-key")) | ||
|
|
||
| (defn- flatten-map | ||
| [data path] | ||
| (reduce-kv (fn [acc k v] | ||
| (->> (format-flattened-key k) | ||
| (conj path) | ||
| (flatten-data v acc))) | ||
| [] | ||
| data)) | ||
|
|
||
| (defn- flatten-vec | ||
| [data path] | ||
| (->> data | ||
| (map-indexed vector) | ||
| (reduce (fn [acc [idx v]] | ||
| (->> (str idx) | ||
| (conj path) | ||
| (flatten-data v acc))) | ||
| []))) | ||
|
|
||
| (defn flatten-data | ||
| "Recursively flatten a map or vector into a flat vector. | ||
|
|
||
| e.g. `[[[\"person\" \"first-name\"] \"Rich\"] | ||
| [[[\"person\" \"last-name\"] \"Hickey\"]]]`" | ||
| ([value] | ||
| (flatten-data value [] [])) | ||
| ([value acc path] | ||
| (cond | ||
| (map? value) (into acc (flatten-map value path)) | ||
| (vector? value) (into acc (flatten-vec value path)) | ||
| :else (conj acc [path value])))) | ||
|
|
||
| (defn format-fields | ||
| "Format the fields into maps with `:label` & `:value`, where the label | ||
| is the flattened keys joined with a separator | ||
|
|
||
| e.g. `{:label \"person: first-name:\" :value \"Rich\"}`" | ||
| [data separator] | ||
| (mapv (fn [[kv v]] | ||
| {:label (-> separator | ||
| (string/join kv) | ||
| (str separator) | ||
| string/trim) | ||
| :value v}) | ||
| data)) | ||
|
|
||
| (defn flatten-typed-data | ||
| "Flatten typed data and prepare it for UI" | ||
| [typed-data] | ||
| (-> typed-data | ||
| (select-keys [:domain :message]) | ||
| flatten-data | ||
| (format-fields ": "))) | ||
|
There was a problem hiding this comment. Is the format Do you know if there's any interest in improving the designs or if any concerns were raised? There was a problem hiding this comment. 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. |
||
|
|
||
| (defn get-chain-id | ||
| "Returns the `:chain-id` from typed data if it's present and if the EIP712 domain defines it. Without | ||
| the `:chain-id` in the domain type, it will not be signed as part of the typed-data." | ||
| [typed-data] | ||
| (let [chain-id-type? (->> typed-data | ||
| :types | ||
| :EIP712Domain | ||
| (some #(= "chainId" (:name %)))) | ||
| data-chain-id (-> typed-data | ||
| :domain | ||
| :chainId | ||
| number/parse-int)] | ||
| (when chain-id-type? | ||
| data-chain-id))) | ||
|
|
||
| (defn sign | ||
| [password address data chain-id-eip155 version] | ||
| (let [legacy? (= version :v1) | ||
| chain-id (networks/eip155->chain-id chain-id-eip155)] | ||
| (rpc/wallet-safe-sign-typed-data data | ||
| address | ||
| password | ||
| chain-id | ||
| legacy?))) | ||
|
|
||
| (defn typed-data-request? | ||
| [method] | ||
| (contains? | ||
| #{constants/wallet-connect-eth-sign-typed-v4-method | ||
| constants/wallet-connect-eth-sign-typed-method} | ||
| method)) | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| (ns status-im.contexts.wallet.wallet-connect.utils.typed-data-test | ||
| (:require | ||
| [cljs.test :refer-macros [deftest is testing]] | ||
| [status-im.contexts.wallet.wallet-connect.utils.typed-data :as sut])) | ||
|
|
||
| (deftest flatten-data-test | ||
| (testing "correctly returns the fallback when key is not string/keyword/number" | ||
| (is (match? [[["unsupported-key"] "value"]] | ||
| (sut/flatten-data {[1 2] "value"})))) | ||
|
|
||
| (testing "correctly flattens a simple map" | ||
| (is (match? [[["zero"] 0] | ||
| [["one"] 1] | ||
| [["2"] 2]] | ||
| (sut/flatten-data {:zero 0 | ||
| "one" 1 | ||
| 2 2})))) | ||
|
|
||
| (testing "correctly flattens a simple vec" | ||
| (is (match? [[["0"] "zero"] [["1"] "one"]] | ||
| (sut/flatten-data ["zero" "one"])))) | ||
|
|
||
| (testing "correctly flattens a nested map" | ||
| (is (match? [[["nested" "child"] "child value"] | ||
| [["nested" "nested" "0"] "child one"] | ||
| [["nested" "nested" "1"] "child two"] | ||
| [["flat"] "child value"]] | ||
| (sut/flatten-data {:nested {:child "child value" | ||
| :nested ["child one" "child two"]} | ||
| :flat "child value"})))) | ||
|
|
||
| (testing "correctly flattens a nested vector" | ||
| (is (match? [[["0" "flat"] 1] | ||
| [["0" "nested-vector" "0"] 1] | ||
| [["0" "nested-vector" "1"] 2] | ||
| [["0" "nested-map" "one"] 1]] | ||
| (sut/flatten-data [{:flat 1 | ||
| :nested-vector [1 2] | ||
| :nested-map {:one 1}}]))))) | ||
|
|
||
| (deftest flatten-typed-data-test | ||
| (testing "successfully extracts, flattens and formats the typed data" | ||
| (is (match? [{:label "domain: chain-id:" :value 1} | ||
| {:label "message: to: address:" :value "0x"} | ||
| {:label "message: from: address:" :value "0x"} | ||
| {:label "message: amount:" :value "0x"}] | ||
| (sut/flatten-typed-data {:domain {:chain-id 1} | ||
| :types {:Tx [{:name "to" | ||
| :type "address"}]} | ||
| :message {:to {:address "0x"} | ||
| :from {:address "0x"} | ||
| :amount "0x"}}))))) |
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
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.
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.
@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.
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.
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.