Show ENS name only when confirmed #10422
Conversation
Pull Request Checklist
|
jrainville
changed the title
Jenkins BuildsClick to see older builds (25)
|
|
I fixed and updated. Thanks @cammellos and @flexsurfer |
| (when (= hash transaction-hash) | ||
| ; TODO Return from the loop once we find a match | ||
| (when (= transaction-success true) | ||
| (re-frame/dispatch [:update-ens-tx-state :success username custom-domain? hash]) |
There was a problem hiding this comment.
here are two things, events handlers MUST be pure, re-frame/dispatch is not pure, so you should use :dispatch fx, but we have internal rule to do not use :dispath , so you should use fx/merge here and instead re-frame/dispatch use (update-ens-tx-state cofx ...)
There was a problem hiding this comment.
We are getting there, nice work! This is not the easiest of the tasks to start with :)
So, now we basically have an effectful handler (btw any time you see doseq in clojure you know that is used for side effects).
We want to turn this into a pure function, as per what Mr Surfer said.
The way we do this in re-frame is instead of actually run the side-effects in the handler (i.e (re-frame/dispatch []), we return a map with the side effects to run.
So for example if you want to make an HTTP request, you would return from the event-handler some like:
{:db {....} :make-http-request [:param]}
And then register an handler:
(re-frame/reg-fx :make-http-request
(fn [params] ...))
https://github.com/day8/re-frame/blob/master/docs/Effects.md
In this case you are running side-effects only in order to dispatch other events, and run their event handlers.
As a general rule, we avoid having having event-handlers dispatching if they don't have to, and we instead accumulate the results of other event-handlers. To give you an example:
Say you have:
(fx/defn handle-a
{:events [::event-a]}
[cofx]
(do-stuff-a))
and
(fx/defn handle-b
{:events [::event-b]}
[cofx]
(do-stuff-b))
Which reacts to::event-a and ::event-b.
Then you have a new event ::event-c which needs to run the same logic as both ::event-a and ::event-b.
One way you could do this (and this is what we did before) is to have something like this:
(fx/defn handle-c
{:events [::event-c]}
[cofx]
{:db (modify-db-for-c cofx)
:dispatch-n [[::event-a] [::event-b]]})
So essentially the handler of ::event-c is further dispatching two events, similarly to the function you have above (the difference is that here is a pure function, and we run the side-effects in an handler which is provided by re-frame).
Eventually we moved to a different pattern, instead of further dispatching, we call the handlers directly and cumulate the changes, so we have something like:
(fx/defn handle-c
{:events [::event-c]}
[cofx]
(fx/merge cofx
{:db (modify-db-for-c cofx)
(handle-a)
(handle-b)))
Which is equivalent (ish) in terms of code. This has some benefits, is more explict, easier to test, easier to follow the logic, and some drawbacks, handlers take longer to run (so perfomance might be an issue, as they don't yeild control for rendering), they couple logic more heavily and adds complexity in accumlating them (fx/merge is our own macro to merge effects).
Regadless, most time for now we use this strategy.
So in your case, I would start by pulling all the objects you are interested in:
instead of:
(let [registrations (get db :ens/registrations)]
(doseq [[hash {:keys [state username custom-domain?]}] registrations]
(when (or (= state :dismissed) (= state :submitted))
you can filter them, and already get those that match the transaction-hash in transfers:
(let [set-of-transactions-hash (reduce (fn [acc {:keys [hash]}] (conj acc hash)) #{} transfers)
registrations (filter #(This function should match against the set, submitted and dismissed) (get db :ens/registrations)]
Which should give the "actionable" transactions (i.e those that match transfers and are dismissed or submitted.
The it's the tricky bit, you want to comulate the fxs.
How we do this is to use the form:
(apply fx/merge cofx [vector of fxs])
apply in clojure works similarly to apply in javascripts, minus the binding as that's not a thing in clojure, but they are effectively the same.
so what you want to do now, is to have a function that handles a single transaction and instead of dispatching calls the right handler:
(fx/defn whatever [cofx transaction] ;; This is an actionable transaction
(cond
(:transaction-success transactions) ;; Is this two conditions mutually exclusive or a transaction (:transaction-success transactions) can be true an :failed ?
(fx/merge cofx (ens/update-ens-transaction-state..) (ens/save-username))
(= :failed (:type transaction)
(ens/update-ens-transacation-state ...))))
And finally you can comulate those and apply them, so roughly your code woudl be something like:
(let [set-of-transactions-hash (build-a-set-of-transactions-for-fast-loopkup)
registrations (filter-only-those-you-are-interested-in)
fxs (map wathever registrations)]
(apply fx/merge cofx fxs))
And that should do the tricky, there are other issues that might crop up, depending on how the code is structured. It would also be useful to add a few tests for this function, as it would be pure so you can easily make sure what you get in the db is what you expect.
This is very tricky stuff actually, very brave choice to start with! and sorry if I am too verbose or there's stuff you already know, let me know if you have any question or something does not make sense.
There was a problem hiding this comment.
Yeah i couldn't do it better than Mr. Cammellos, thanks
There was a problem hiding this comment.
@cammellos WOW! What a great explanation. Don't worry about being too verbose everything you said there is useful to me.
Also, it's not exactly my first issue tackled in react-status, but it's definitely the first that is that big haha.
Thanks again. I'm gonna fix it up!
|
Awesome job @jrainville! This is a really important fix. |
jrainville
force-pushed
the
fix/show-ens-name-when-confirmed
branch
from
d37e48e
to
ffb6523
Compare
|
FYI, I just rebased on top of develop, but I wasn't able to test the changes yet, since my testing environment had to be reinstalled. |
|
@jrainville cool! regarding the UI, you can take a browse through these designs here, the pending state for a name is indicated by changing the subtitle prop of the list item to say 'Transaction pending…'
adjacent there you'll find all the states necessary |
|
@jrainville can you please rebase it one more time? |
jrainville
force-pushed
the
fix/show-ens-name-when-confirmed
branch
from
ffb6523
to
7c15a43
Compare
|
Rebased. Should I do the design changes as @errorists commented in this PR or another one? |
|
I'd say if you can manage the design changes by Friday, please include it in this PR, else let me know and I'll create a separate issue with your name on it;) |
|
@flexsurfer fixed in c1dd922 |
98% of end-end tests have passedFailed tests (2)Click to expand
Passed tests (93)Click to expand |
95% of end-end tests have passedFailed tests (3)Click to expand
Passed tests (54)Click to expand |
94% of end-end tests have passedFailed tests (5)Click to expand
Passed tests (74)Click to expand |
98% of end-end tests have passedFailed tests (2)Click to expand
Passed tests (93)Click to expand |
|
@jrainville thanks for fix! To reproduce:
When I want to register new ENS then it works fine (and |
|
@jrainville hmm, that's possible, yes. I was tested develop based on where it was all fine 9a11412 and just tested latest develop 9d76913 which is also looks good to me. Could you rebase the very latest develop (and fix conflict) please? |
jrainville
force-pushed
the
fix/show-ens-name-when-confirmed
branch
from
c1dd922
to
7eb2245
Compare
|
@Serhy you were right, the issue was indeed in my PR. Somehow, while I tested develop, I probably didn't reset the app correctly. Anyway, I rebased and fixed the issue. You can test again to confirm. |
|
Looks good to me, @jrainville thanks! Tested on iOS13 and Android 9. |
cammellos
force-pushed
the
fix/show-ens-name-when-confirmed
branch
from
7eb2245
to
08ff2ed
Compare
fix: only save the ENS username when the TX is confirmed feat: show when ens registration are in progress, done or errored feat: implement new design for the ENS registration Signed-off-by: Andrea Maria Piana <andrea.maria.piana@gmail.com>
Summary
Fixes #9912
The problem was that the ENS name was added to the list straight when the TX was sent, but the TX could then error and you'd have no idea, plus the name would still display.
Instead, we register the name in a list with the TX hash and watch for changes in transactions, when the TX completes (error or success), we show it in the ENS page.
The look of it is not super pretty, I did with what I know, but feel free to tell me how to improve the look and the code style.
In progress:
Success:
Review notes
One thing that needs to be improved, and I don't know how, is that the state is only updated when we go on the page itself.
Also, I don't think it's related to my PR, but sometimes, the
addressthat is gotten from:multiaccount/current-accountis not correct and it is not even part of my mulriaccount. I think it's also what causes the "unknown address" issue when sending a TX.Platforms
I only tested in the Android Simulator and in Ropsten.
Areas that maybe impacted
The profile is impacted. When registering the name, you need to go back to the ENS page (or wait on it) for the name on the profile to finally appear.
Steps to test
Register a new ENS name
status: ready