feat(wallet): connect backend to transaction progress page #18506
Conversation
Jenkins BuildsClick to see older builds (12)
|
| @@ -79,6 +79,9 @@ | |||
| "recent-history-ready" (recent-history-fetching-ended cofx event) | |||
| "fetching-history-error" (fetching-error cofx event) | |||
| "non-archival-node-detected" (non-archival-node-detected cofx event) | |||
| "pending-transaction-status-changed" {:fx [[:dispatch | |||
There was a problem hiding this comment.
add transaction progress signal.
There was a problem hiding this comment.
@smohamedjavid suggested to use the prefix "received" for signals, I think it's a good approach, wdyt?
|
|
||
| (defn content-container | ||
| [blur? keyboard-shown?] | ||
| (let [margin-bottom (if keyboard-shown? 0 (safe-area/get-bottom))] | ||
| [blur? keyboard-shown? bottom-inset] |
There was a problem hiding this comment.
@OmarBasem I think you mentioned this in another pr about this floating button page - the safe insets here are breaking the bottom margin on some pages - I made this value configurable for this reason. perhaps there's a better approach. Additionally I can move this outside this pr 👍
cc @ulisesmac
There was a problem hiding this comment.
No problem we can look into it in a separate issue
There was a problem hiding this comment.
ok, I'll remove from my changes from here to not make the mess bigger :)
| {:db (assoc-in db [:wallet :transactions tx-id] transaction-detes) | ||
| tx-ids (flatten (map (fn [[_key value]] value) transaction-hashes)) | ||
| tx-id-with-details (reduce-kv (fn [m1 chain-id v1] | ||
| (merge m1 |
There was a problem hiding this comment.
see the wallet subs below to see this structure.
Perhaps it needs some comment or something to explain why we want it this way.
There was a problem hiding this comment.
tx = transaction. I can change to transaction instead.
m1 is map1 and v1 value1. I think the best thing I can do is extract this to a helper function and have a test to define the input and output 👍
There was a problem hiding this comment.
I think the best thing I can do is extract this to a helper function
That would be better
J-Son89
requested review from
smohamedjavid,
briansztamfater,
OmarBasem,
ulisesmac,
vkjr and
mmilad75
| @@ -79,6 +79,9 @@ | |||
| "recent-history-ready" (recent-history-fetching-ended cofx event) | |||
| "fetching-history-error" (fetching-error cofx event) | |||
| "non-archival-node-detected" (non-archival-node-detected cofx event) | |||
| "pending-transaction-status-changed" {:fx [[:dispatch | |||
There was a problem hiding this comment.
@smohamedjavid suggested to use the prefix "received" for signals, I think it's a good approach, wdyt?
| (rf/reg-event-fx :wallet/clean-send-data | ||
| (fn [{:keys [db]}] | ||
| {:db (dissoc db [:wallet :ui :send])})) |
There was a problem hiding this comment.
Really good you added a clean event! 💯
Just please fix dissoc takes the keys without being inside a vector
There was a problem hiding this comment.
Now, I'm thinking you probably tried to do a dissoc-in, in that case:
(update-in db [:wallet :ui] dissoc :send)
| :id (:id transaction) | ||
| :chain-id chain-id}] | ||
| {:db (assoc-in db [:wallet :transactions tx-id] transaction-detes) | ||
| tx-ids (flatten (map (fn [[_key value]] value) transaction-hashes)) |
There was a problem hiding this comment.
This one could be (map val transaction-hashes)
There was a problem hiding this comment.
Also consider using vals
There was a problem hiding this comment.
hmm, well I do need to flatten. transacation-hashes is a map btw.
I will try vals 👍
There was a problem hiding this comment.
Oh, I mean, trying:
(flatten (vals value) transaction-hashes))
There was a problem hiding this comment.
(flatten (vals transaction-hashes)) ftw
| (defn combined-status-overview | ||
| [transaction-details] | ||
| (cond | ||
| (every? (fn [[_k v]] (= (:status v) :finalised)) transaction-details) :finalised | ||
| (some (fn [[_k v]] (= (:status v) :pending)) transaction-details) :pending | ||
| (some (fn [[_k v]] (= (:status v) :confirmed)) transaction-details) :confirmed | ||
| :else nil)) |
| @@ -13,35 +12,47 @@ | |||
| (defn titles | |||
| [status] | |||
| (case status | |||
| :sending (i18n/label :t/sending-with-elipsis) | |||
| :pending (i18n/label :t/sending-with-elipsis) | |||
There was a problem hiding this comment.
I think we have a typo in elipsis, it should use a double L
| (fn [{:keys [db]} [{:keys [message]}]] | ||
| (let [details (js->clj (js/JSON.parse message) :keywordize-keys true) | ||
| tx-hash (:hash details)] | ||
| {:db (-> db (update-in [:wallet :transactions tx-hash] assoc :status :confirmed :blocks 1))}))) |
There was a problem hiding this comment.
We can remove the -> and use db directly instead, since it's a thread-first of only one step.
BTW, why do we set blocks as 1? is it possible to set more than 1?
There was a problem hiding this comment.
yeah we will be updating the block number going forward. This is basically saying the transaction has been accepted and added to the first block. From there we do some calculations to get the next block. However that is for a follow up pr when we handle the animations etc for the various blocks on different networks.
I'll link you to the conversation in the chat and it will explain it.
src/status_im/subs/wallet/send.cljs
Outdated
| (fn [[tx-ids transactions]] | ||
| (let [send-tx-ids (set (keys transactions))] | ||
| (select-keys transactions | ||
| (for [k tx-ids :when (send-tx-ids k)] k))))) |
There was a problem hiding this comment.
I think this for is behaving like a filter:
Consider using (filter send-tx-ids tx-ids)
There was a problem hiding this comment.
hmm, will have to reconsider what exactly is being done but I think I didn't use filter for a particular reason. will check again
There was a problem hiding this comment.
checked and updated 👍
| {:db (assoc-in db [:wallet :transactions tx-id] transaction-detes) | ||
| tx-ids (flatten (map (fn [[_key value]] value) transaction-hashes)) | ||
| tx-id-with-details (reduce-kv (fn [m1 chain-id v1] | ||
| (merge m1 |
|
|
||
| (defn content-container | ||
| [blur? keyboard-shown?] | ||
| (let [margin-bottom (if keyboard-shown? 0 (safe-area/get-bottom))] | ||
| [blur? keyboard-shown? bottom-inset] |
There was a problem hiding this comment.
No problem we can look into it in a separate issue
J-Son89
force-pushed
the
jc/transaction-status-prog
branch
from
c80c17a to
6f48360
Compare
94% of end-end tests have passedExpected to fail tests (3)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (45)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
|
Hi @J-Son89, thank you for the PR. I'm not sure if this issue is caused by Goerli instability, so I'm uncertain if this issue is valid. The issue is not reproducible on other non-testnet networks. Could you check if should it be fixed? ISSUE 1: [IOS] Transaction confirmation page is not shown if goerli asset is sentSteps:
Actual result:No transaction confirmation screen navigation after Goerli network is sent transactionconfiramtion.mp4Expected result:After completing the transaction on the Goerli network, the application should navigate to the transaction confirmation screen. Logs:Additional info:For Android, the current transaction confirmation is displayed only after the 'done' button is tapped |
ISSUE 2: [Android] The transaction finalized page can't be closedSteps:
Actual result:The transaction finalized page remains open, and it cannot be closed using the 'DONE' button or [x]. finalized.mp4Expected result:The 'transaction finalized' page should be successfully closed when tapping 'DONE' or the [x] button. Log |
Hi @VolodLytvynenko - I believe that issue is because of Goerli. I tested on prod with ethereum and it worked okay for me 👌 |
|
@J-Son89, one question. When I try to send assets on Android, I see the 'Transaction Finalized' page, but this page is not shown on iOS. Should it be displayed for IOS as well? It seems it's also not shown in your description, but it can be seen in the record of this issue 2. That's why i'm confused a bit |
|
@thanks @VolodLytvynenko - let me check what's happening on android! |
Indeed, that's true. It seems to be working fine for me on Arbitrum and Optimism. It's strange, because sending Goerli on metamask works well |
J-Son89
force-pushed
the
jc/transaction-status-prog
branch
from
6f48360 to
f11e938
Compare
| @@ -46,7 +46,9 @@ | |||
| [quo/wallet-graph {:time-frame :empty}] | |||
| (when (not watch-only?) | |||
| [quo/wallet-ctas | |||
| {:send-action #(rf/dispatch [:open-modal :wallet-select-address]) | |||
| {:send-action (fn [] | |||
| (rf/dispatch [:wallet/clean-send-data]) | |||
There was a problem hiding this comment.
@ulisesmac, @briansztamfater I ran into some issues cleaning send data on navigation away from the transaction progress page. it causes a stack overflow. Anyway I found that is probably safer to add the clean event to the start of the flow so I added it here.
wdyt? 🤔
J-Son89
force-pushed
the
jc/transaction-status-prog
branch
2 times, most recently
from
ac59536 to
ef87009
Compare
|
@VolodLytvynenko I fixed the navigation issues on Android so that should be fixed now. |
58% of end-end tests have passedFailed tests (17)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (3)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (28)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
VolodLytvynenko
force-pushed
the
jc/transaction-status-prog
branch
from
ef87009 to
ee2461f
Compare
85% of end-end tests have passedFailed tests (4)Click to expandClass TestActivityMultipleDevicePR:
Class TestDeepLinksOneDevice:
Expected to fail tests (3)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (41)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
|
|
hi @J-Son89 thank you for PR. No issue from my side. PR is ready to be merged |
J-Son89
force-pushed
the
jc/transaction-status-prog
branch
from
ee2461f to
e2546eb
Compare
fixes: #18307
This pr connects the backend to the transaction progress page.
To test:
Send a transaction (any possible send flow currently available)
Go to Transaction Progress Page.
text should update as show in the video below. i.e "sending" -> "transaction confirmed"
"Transaction Finalised" will not show in this pr as that is dependent on more data being added and will be handled in a future issue.
The back button should also work on the transaction progress page and take the user to the account home page.
Screen.Recording.2024-01-15.at.11.49.54.mov