[#21483] fix: update collectible amounts after sending #21867
Conversation
Jenkins BuildsClick to see older builds (40)
|
mohsen-ghafouri
force-pushed
the
fix/collectible-amount-update
branch
from
ddde37e to
c1cb39d
Compare
mohsen-ghafouri
force-pushed
the
fix/collectible-amount-update
branch
4 times, most recently
from
b321c72 to
1bac99f
Compare
mohsen-ghafouri
requested review from
smohamedjavid,
shivekkhurana,
vkjr and
briansztamfater
shivekkhurana
added
wallet-core
|
Thank you for work! |
|
Hi @churik, I will do it today |
mohsen-ghafouri
force-pushed
the
fix/collectible-amount-update
branch
2 times, most recently
from
466083d to
a3e5655
Compare
|
@smohamedjavid could you please review when you can? |
mohsen-ghafouri
force-pushed
the
fix/collectible-amount-update
branch
from
a3e5655 to
a364057
Compare
| :wallet/build-transaction-for-collectible-route | ||
| (fn [{:keys [db]}] | ||
| (let [last-request-uuid (get-in db [:wallet :ui :send :last-request-uuid])] | ||
| {:db (update-in db [:wallet :ui :send] dissoc :transaction-for-signing) | ||
| (let [last-request-uuid (get-in db [:wallet :ui :send :last-request-uuid]) | ||
| collectible-unique-id (get-in db [:wallet :ui :send :collectible :unique-id])] | ||
| {:db (-> | ||
| db | ||
| (update-in [:wallet :ui :send] dissoc :transaction-for-signing) | ||
| (assoc-in [:wallet :ui :collectibles :pending collectible-unique-id] true)) |
There was a problem hiding this comment.
IMO wallet/transaction-success or wallet/transactions-sent-signal-received would be a better place to add this.
There was a problem hiding this comment.
@smohamedjavid i wanted to add it in better place but as we do cleanup after closing the modal, we don't have access to this data after that. here it was the only option IMO.
There was a problem hiding this comment.
If I'm not mistaken, the transaction clean-up events are dispatched inside the wallet/transaction-success event. We could get this same information on that event and update the app-db.
One other reason to move this assoc-in to one of those events is that we build the transaction as soon as the user is on the TX confirmation/summary screen. If the user somehow decides to leave the flow, then this app-db update is misleading.
There was a problem hiding this comment.
I can see :wallet/clean-selected-collectible being called in places like closing the send view or selecting another account. IMO, storing the pending collectible here carries less risk, and since we store very light data in the pending section, any misleadings wouldn’t cause critical issues. WDYT?
There was a problem hiding this comment.
I'm sorry. The word misleading misleads 😅 . I meant to say that updating the app-db even before the transaction is placed is overhead, as this update is done when the user goes to the TX summary/confirmation screen, and the user can go back to wallet home from there (w/o submitting the TX). So, the app-db update without the actual transaction getting placed is something we can avoid.
I agree that the :wallet/clean-selected-collectible event is called as part of page (TX flow screens) unmounts. But, the TX flow screen is dismissed in wallet/end-transaction-flow event, which is dispatched part of wallet/transaction-success event where we will have access to collectible sent. That's why I was suggesting that place. WDYT?
There was a problem hiding this comment.
@smohamedjavid :wallet/clean-selected-collectible it's not only called in TX flow screen, it can be called from other places, please check the usage, it triggers if user close some views (like select asset view, send from view, etc).
so having some no really pending transaction in app-db has less burden than missing an update IMO. wdyt?
There was a problem hiding this comment.
My point is if user while waiting to get transaction-success event, try to navigate around and it will cause that collectible data being missed.
There was a problem hiding this comment.
so having some no really pending transaction in app-db has less burden than missing an update IMO. wdyt?
I was concerned if the user leaves the send flow without placing the TX. Then, this will trigger fetching that particular collectible data by ID, which we will update the app-db (in the accounts collectible list and the detailed collectible data for the detail screen) with the same data. It's fine, we will explore this later to make it efficient.
My point is if user while waiting to get transaction-success event, try to navigate around and it will cause that collectible data being missed.
I agree. It's something we should not allow until we get the TX response (success/failed).
There was a problem hiding this comment.
yes i agree, this is not the perfect solution but the only workaround i found for now. we can definitely improve it. thank for all your feedbacks 🙏
mohsen-ghafouri
force-pushed
the
fix/collectible-amount-update
branch
from
3f448b8 to
80083b1
Compare
|
@briansztamfater @smohamedjavid Could you please check it again? |
| :wallet/build-transaction-for-collectible-route | ||
| (fn [{:keys [db]}] | ||
| (let [last-request-uuid (get-in db [:wallet :ui :send :last-request-uuid])] | ||
| {:db (update-in db [:wallet :ui :send] dissoc :transaction-for-signing) | ||
| (let [last-request-uuid (get-in db [:wallet :ui :send :last-request-uuid]) | ||
| collectible-unique-id (get-in db [:wallet :ui :send :collectible :unique-id])] | ||
| {:db (-> | ||
| db | ||
| (update-in [:wallet :ui :send] dissoc :transaction-for-signing) | ||
| (assoc-in [:wallet :ui :collectibles :pending collectible-unique-id] true)) |
There was a problem hiding this comment.
so having some no really pending transaction in app-db has less burden than missing an update IMO. wdyt?
I was concerned if the user leaves the send flow without placing the TX. Then, this will trigger fetching that particular collectible data by ID, which we will update the app-db (in the accounts collectible list and the detailed collectible data for the detail screen) with the same data. It's fine, we will explore this later to make it efficient.
My point is if user while waiting to get transaction-success event, try to navigate around and it will cause that collectible data being missed.
I agree. It's something we should not allow until we get the TX response (success/failed).
mohsen-ghafouri
force-pushed
the
fix/collectible-amount-update
branch
from
e3fd46d to
38fbbd1
Compare
100% of end-end tests have passedPassed tests (11)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
Class TestWalletMultipleDevice:
Class TestCommunityOneDeviceMerged:
|
fixes #21483
Summary
Multicollectible amount is not updated after transaction until relogin
Areas that maybe impacted
Steps to test
Note for Test
As you can see in the video, receiving the updated balance may take some time (in my case, approximately 50 seconds). Once a signal confirming the ownership transfer is received, the balance will be updated accordingly
Result
Simulator.Screen.Recording.-.iPhone.13.-.2025-01-07.at.21.38.45.mp4
status: ready