[#18736] add address to watch using an ENS #19043
Conversation
ulisesmac
requested review from
briansztamfater,
smohamedjavid,
J-Son89,
ibrkhalil and
a team
| @@ -114,16 +114,15 @@ | |||
|
|
|||
| (defn- local-suggestions-list | |||
| [] | |||
| (fn [] | |||
There was a problem hiding this comment.
Just removing an extra fn [] I found, not related to this PR
| (defn validate-message | ||
| [addresses] | ||
| (fn [s] | ||
| (cond | ||
| (or (= s nil) (= s "")) nil | ||
| (contains? addresses s) (i18n/label :t/address-already-in-use) | ||
| (not (or (validation/eth-address? s) | ||
| (validation/ens-name? s))) (i18n/label :t/invalid-address) | ||
| :else nil))) | ||
| (defn- validate-address | ||
| [known-addresses s] | ||
| (cond | ||
| (or (nil? s) (= s "")) nil | ||
| (contains? known-addresses s) (i18n/label :t/address-already-in-use) | ||
| (not (or (validation/eth-address? s) | ||
| (validation/ens-name? s))) (i18n/label :t/invalid-address))) | ||
|
|
There was a problem hiding this comment.
Just changed the function to not return another function, I've seen the previous pattern, although it's completely ok, causes some bugs/confusion while using it
| (if @validation-msg | ||
| [quo/info-message | ||
| {:accessibility-label :error-message | ||
| :size :default | ||
| :icon :i/info | ||
| :type :error | ||
| :style style/info-message} | ||
| @validation-msg] | ||
| [activity-indicator activity-state])]])))) |
There was a problem hiding this comment.
This if was a when, while using when, two messages could be displayed under certain circumstances, because we have two components and the back-end response comes some time later.
The message is the following:
I think we should only have a single component, having two makes the code more error prone. A bigger refactor on this component might be needed, but I didn't do it here because I wanted to keep the diff focused.
Jenkins BuildsClick to see older builds (19)
|
ulisesmac
force-pushed
the
18736-watch-only-address-by-ens
branch
from
8227dc2
to
f11d730
Compare
| (rf/reg-event-fx | ||
| :wallet/get-address-details | ||
| (fn [{:keys [db]} [address-or-ens]] | ||
| (let [request-params [(chain/chain-id db) address-or-ens] | ||
| ens? (string/includes? address-or-ens ".")] | ||
| {:db (-> db | ||
| (assoc-in [:wallet :ui :add-address-to-watch :activity-state] :scanning) | ||
| (assoc-in [:wallet :ui :add-address-to-watch :validated-address] nil)) | ||
| :fx [(if ens? | ||
| [:json-rpc/call | ||
| [{:method "ens_addressOf" | ||
| :params request-params | ||
| :on-success [:wallet/get-address-details] | ||
| :on-error [:wallet/ens-not-found]}]] | ||
| [:json-rpc/call | ||
| [{:method "wallet_getAddressDetails" | ||
| :params request-params | ||
| :on-success [:wallet/store-valid-address-activity address-or-ens] | ||
| :on-error #(log/info "failed to get address details" | ||
| {:error % | ||
| :event :wallet/get-address-details})}]])]}))) |
There was a problem hiding this comment.
Part of the main solution, I'm calling "ens_addressOf" to first resolve the given ens to an address
| (defn- validate-address | ||
| [known-addresses s] | ||
| (cond | ||
| (or (nil? s) (= s "")) nil |
There was a problem hiding this comment.
nit: (= s "") --> (string/blank? s)
There was a problem hiding this comment.
thanks @ibrkhalil !
Actually, I already tried it, but they are different and I didn't want to change the logic.
blank will say true when it's " " or "\n \n" too, I thought it was cheaper to just comapre it with = and ""
| {:db (assoc-in db | ||
| [:wallet :ui :watch-address-activity-state] | ||
| (if hasActivity :has-activity :no-activity))})) | ||
| (rf/reg-event-fx |
There was a problem hiding this comment.
hmm, so this file is really starting to bloat.
@OmarBasem, @briansztamfater & I discussed this week of further splitting the wallet into sub-contexts
Mostly based off the figma keys, but we can have others that better fit.
e.g something like
src/status_im/contexts/wallet/
|-/accounts
|-/events.cljs
|-/view.cljs
|-/style.cljs
|-/add-account
|-/events.cljs
|-/view.cljs
|-/style.cljs
|-/create-account
|-/add-account-to-watch
etc..
We have this for Send & Collectibles already too.
Anyway, I'm not saying to do this refactor here, but perhaps we can at least create the appropriate ns for these events added and move them to that file and the others can be moved in a refactor.
Also maybe these events are common enough to wallet that they should be in this ns. 🤷♂️
There was a problem hiding this comment.
@J-Son89
Totally agree!
Actually I noticed this while adding these events, but noticed it's only happening on Collectibles and wasn't sure.
But yeah, I'll move them. mirrorring figma structure is a nice idea 👍
94% of end-end tests have passedFailed tests (2)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (45)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
|
|
hey @ulisesmac could you please rebase the current PR and resolve existing conflicts? |
| (validation/ens-name? s))) (i18n/label :t/invalid-address) | ||
| :else nil))) | ||
| (defn- validate-address | ||
| [known-addresses s] |
There was a problem hiding this comment.
can we have a more descriptive variable name for s?
ulisesmac
force-pushed
the
18736-watch-only-address-by-ens
branch
from
73f0dca
to
4338c5b
Compare
|
@VolodLytvynenko I've already rebased and solved the conflicts |
|
@J-Son89 I followed your suggested code structure and pushed the changes |
96% of end-end tests have passedFailed tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (46)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
|
|
hi @ulisesmac thank you for PR. No issues from my side. PR is ready to be merged |
ulisesmac
force-pushed
the
18736-watch-only-address-by-ens
branch
from
6a4dd19
to
c7529bb
Compare
fixes #18736
Summary
This PR adds the ability to register an address to watch using an ENS, also some some small bugs.
Check the following example,
sonnyf.ethexists andsonnyff.ethdoesn't exist,.Before:
Screencast.from.2024-02-28.12-48-32.webm
Now:
Screencast.from.2024-02-28.12-56-16.webm
Screencast.from.2024-02-28.13-11-37.webm
Platforms
Steps to test
status: ready