'No matching clause' error is shown if ENS is entered into 'sent to' page #19741 #19957
Conversation
mmilad75
requested review from
ulisesmac,
briansztamfater,
clauxx,
ilmotta,
shivekkhurana,
vkjr and
J-Son89
and removed request for
ulisesmac
Jenkins BuildsClick to see older builds (41)
|
| :ens ens | ||
| :address (eip55/address->checksum result) | ||
| :networks [:ethereum :optimism] | ||
| :full-address (str "eth:opt:" (eip55/address->checksum result))}] |
There was a problem hiding this comment.
I see this `"eth:opt:" string as a magical string. It could be a constant or better yet IMO, a function that converts a sequence of network keywords into their shorthand versions. I can't remember, but I think such a function even exists already, but don't trust my memory.
There was a problem hiding this comment.
I wrote some functions to convert network ids to address prefix:
(def network->short-name
{constants/mainnet-network-name constants/mainnet-short-name
constants/optimism-network-name constants/optimism-short-name
constants/arbitrum-network-name constants/arbitrum-short-name})
(def short-name->network
{constants/mainnet-short-name constants/mainnet-network-name
constants/optimism-short-name constants/optimism-network-name
constants/arbitrum-short-name constants/arbitrum-network-name})
(defn short-names->network-preference-prefix
[short-names]
(str (string/join ":" short-names) ":"))
(defn network-preference-prefix->network-names
[prefix]
(as-> prefix $
(string/split $ ":")
(map short-name->network $)))These were in wallet.common.utils ns. But my PR was not merged because I was moved to another team and Ibrahim started working on that feature.
Agreed that we should have a namespace to manage addresses. They are not strings. They just look like strings.
There was a problem hiding this comment.
That's great @shivekkhurana, exactly what I had in mind. Thanks
There was a problem hiding this comment.
I'm gonna copy your code and put it in my PR @shivekkhurana
There was a problem hiding this comment.
status-im.contexts.wallet.common.utils.networks This will be a better namespace to include them in.
| @@ -167,7 +170,9 @@ | |||
| :disabled? (not valid-ens-or-address?) | |||
| :on-press #(rf/dispatch | |||
| [:wallet/select-send-address | |||
| {:address @input-value | |||
| {:address (if local-suggestion-address | |||
There was a problem hiding this comment.
You can or the conditions instead of using if. A bit shorter.
| valid-ens-or-address? (boolean (rf/sub [:wallet/valid-ens-or-address?])) | ||
| local-suggestions (rf/sub [:wallet/local-suggestions]) | ||
| local-suggestion-address (:full-address (first local-suggestions)) | ||
| {:keys [color]} (rf/sub [:wallet/current-viewing-account])] |
There was a problem hiding this comment.
I know it's not from this PR, but subscribing to :wallet/current-viewing-account just to get the color is not good, especially because that sub and its inputs signals are quite heavy in processing. The better approach would be to have a sub that returns only the color of the current-viewing-account from the input of sub :wallet/accounts.
There was a problem hiding this comment.
I'm sub-inspector Shivek. You have been found in possession of {:keys [color]} (rf/sub ...), which is an illegal substance. You have the right to resolve this.
There was a problem hiding this comment.
I created this wallet/current-viewing-account->color which only returns the color
There was a problem hiding this comment.
Why isn't it called wallet/current-viewing-account-color ?
The -> is a convention for a pure transformation function.
There was a problem hiding this comment.
Done. Thanks for letting me know
| (let [selected-tab (or (rf/sub [:wallet/send-tab]) (:id (first tabs-data))) | ||
| valid-ens-or-address? (boolean (rf/sub [:wallet/valid-ens-or-address?])) | ||
| local-suggestions (rf/sub [:wallet/local-suggestions]) | ||
| local-suggestion-address (:full-address (first local-suggestions)) |
There was a problem hiding this comment.
Just a suggestion: it's better to have a new sub that returns only what the view needs. In this case, any change to the output of :wallet/local-suggestions will force Reagent to re-process the entire component, but you only care about the full-address of the first suggestion. Maybe it's not a big deal here, but these details can easily pile up in terms of performance.
Not sure if it helps, but the mindset I put myself in is to not think of subs as normal functions. When we subscribe in the component, we're forming a dependency graph of subscriptions for the duration of the component lifecycle. This has a cost and affects re-renders, not like normal function calls that we can sprinkle anywhere and usually only care about what they do.
There was a problem hiding this comment.
Very good point. Done
There was a problem hiding this comment.
btw @mmilad75, this is a known problem we have throughout the codebase. I wrote a ton of code not following my own suggestion here, I guess because it took me a long time to understand the performance implications in mobile versus web development (which is where I came from using re-frame). Now we should gradually improve things or not make them worse if we can.
| :ens ens | ||
| :address (eip55/address->checksum result) | ||
| :networks [:ethereum :optimism] | ||
| :full-address (str "eth:opt:" (eip55/address->checksum result))}] |
There was a problem hiding this comment.
I wrote some functions to convert network ids to address prefix:
(def network->short-name
{constants/mainnet-network-name constants/mainnet-short-name
constants/optimism-network-name constants/optimism-short-name
constants/arbitrum-network-name constants/arbitrum-short-name})
(def short-name->network
{constants/mainnet-short-name constants/mainnet-network-name
constants/optimism-short-name constants/optimism-network-name
constants/arbitrum-short-name constants/arbitrum-network-name})
(defn short-names->network-preference-prefix
[short-names]
(str (string/join ":" short-names) ":"))
(defn network-preference-prefix->network-names
[prefix]
(as-> prefix $
(string/split $ ":")
(map short-name->network $)))These were in wallet.common.utils ns. But my PR was not merged because I was moved to another team and Ibrahim started working on that feature.
Agreed that we should have a namespace to manage addresses. They are not strings. They just look like strings.
| valid-ens-or-address? (boolean (rf/sub [:wallet/valid-ens-or-address?])) | ||
| local-suggestions (rf/sub [:wallet/local-suggestions]) | ||
| local-suggestion-address (:full-address (first local-suggestions)) | ||
| {:keys [color]} (rf/sub [:wallet/current-viewing-account])] |
There was a problem hiding this comment.
I'm sub-inspector Shivek. You have been found in possession of {:keys [color]} (rf/sub ...), which is an illegal substance. You have the right to resolve this.
|
@shivekkhurana @ilmotta Please have another look. I resolved your comments |
|
@mmilad75 All good, except for this |
Done |
| :address (eip55/address->checksum result) | ||
| :networks [constants/ethereum-network-name constants/optimism-network-name] | ||
| :full-address (str (network-utils/short-names->network-preference-prefix | ||
| [(get network-utils/network->short-name |
There was a problem hiding this comment.
Nice improvement @mmilad75. What follows is just a light suggestion, something I'd do. Maybe you like the overall idea, even if you don't want to apply it now.
This part can be more general/functional and you can reuse the sequence of networks too. While thinking about this suggestion, it forced me to think about what result really means as a word. The goal in this refactor was to use the principle of "tell don't ask", but applied to a functional paradigm.
Tell-Don't-Ask is a principle that helps people remember that object-orientation is about bundling data with the functions that operate on that data. It reminds us that rather than asking an object for data and acting on that data, we should instead tell an object what to do.
Translated to our case here, it's like rather than asking for individual pieces of data and assembling them together, make that a verb (function) and tell it to do its thing.
(defn- resolved-address->prefixed-address
[address networks]
(let [prefixes (->> networks
(map network-utils/network->short-name)
network-utils/short-names->network-preference-prefix)]
(str prefixes (eip55/address->checksum address))))
(rf/reg-event-fx :wallet/set-ens-address
(fn [{:keys [db]} [result ens]]
(let [networks [constants/ethereum-network-name constants/optimism-network-name]
suggestion (if result
[{:type item-types/address
:ens ens
:address (eip55/address->checksum result)
:networks networks
:full-address (resolved-address->prefixed-address result networks)}]
[])]
{:db (-> db
(assoc-in [:wallet :ui :search-address :local-suggestions] suggestion)
(assoc-in [:wallet :ui :search-address :valid-ens-or-address?] (boolean result)))})))
There was a problem hiding this comment.
This is a very great suggestion, thanks man. Every time I learn a lot from you 🙏 @ilmotta
| {:keys [color]} (rf/sub [:wallet/current-viewing-account])] | ||
| (let [selected-tab (or (rf/sub [:wallet/send-tab]) (:id (first tabs-data))) | ||
| valid-ens-or-address? (boolean (rf/sub [:wallet/valid-ens-or-address?])) | ||
| local-suggestion-address (rf/sub [:wallet/local-suggestions->full-address]) |
There was a problem hiding this comment.
As an outsider to the wallet domain, I wondered why getting the first suggestion is important and I thought that reason could be on the name/keyword. In other words, why is the first suggestion relevant? What's the order? Why not the last? If that was in the name, it could help others understand. Also, the name is in the plural, which gives me the impression the sub will return a sequence of full addresses, and not just one.
Suggestions? :wallet/top-suggestion-full-address. Or if it's really arbitrary the choice of suggestion the name could be :wallet/first-suggestion-full-address.
There was a problem hiding this comment.
To be honest, I don't know even why it's returning an array. I thought it'd be useful later. At the moment, only one item is returned and even the implementation is for one ens match. And, each ens has only one address. So, only one item is stored in the db, but it's in an array.
| [short-names] | ||
| (str (string/join ":" short-names) ":")) | ||
|
|
||
| (defn network-preference-prefix->network-names |
There was a problem hiding this comment.
Only thing missing to me is a couple unit tests.
81% of end-end tests have passedFailed tests (8)Click to expandClass TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (42)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
|
hi @mmilad75 thank you for PR. No issues from my side. PR is ready to be merged |
mmilad75
force-pushed
the
milad/19741-fix-enter-ens
branch
from
951d93f
to
e1ee3d7
Compare
| @@ -20,3 +20,21 @@ | |||
| constants/arbitrum-mainnet-chain-id)) | |||
| (is (= (utils/network->chain-id {:network :arbitrum :testnet-enabled? true :goerli-enabled? false}) | |||
| constants/arbitrum-sepolia-chain-id)))) | |||
|
|
|||
| (deftest test-short-names->network-preference-prefix | |||
There was a problem hiding this comment.
@mmilad75, thanks for covering the code with unit tests! I think we can further improve this code.
- In our codebase, but also in many other Clojure repos out there, the name of the test var is suffixed with the
testword, not prefixed. It's just a convention. - The
testingmacro is unnecessary because we already know which function is under test based on the test var, which in our repo tend to be the name of the function under test followed by-test. Therefore, we try to use thetestingmacro for the cases where we actually need to give more context about the test. - (Totally optional, but you may prefer) You can make the tests more concise using the
aremacro. This macro is incredibly useful for testing these pure functions with a small input & output, which is the case of many utility functions. It helps make testing and adding examples cheap and if it's cheap, the greater likelihood devs will test. And even nicer, zprint will autoformat in tabular form. For example (untested):
(deftest short-names->network-preference-prefix-test
(are [expected short-names]
(= expected (utils/short-names->network-preference-prefix short-names))
"eth:" ["eth"]
"eth:opt:" ["eth" "opt"]
"eth:opt:arb1:" ["eth" "opt" "arb1"]))I see you're following the pattern in the file that existed before your PR, but if you could apply the suggestions above, that would be a nice bonus :)
There was a problem hiding this comment.
Additionally, the are macro for this sort of utility function makes adding examples so cheap that it becomes more obvious to me that the examples are lacking more sad path examples or nil/empty cases because this is where many implementations fail. Example inputs: "", ":", " ", and of course nil.
fixes #19741
Screen.Recording.2024-05-09.at.21.16.16.mov