-
Notifications
You must be signed in to change notification settings - Fork 986
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(wallet)_: Send flow for collectibles #21871
Conversation
Jenkins BuildsClick to see older builds (4)
|
(defn remove-duplicates-in-ownership | ||
[ownership] | ||
(->> ownership | ||
(reduce (fn [acc {:keys [address timestamp] :as owner}] | ||
(let [existing-owner (get acc address)] | ||
(if (or (nil? existing-owner) (> timestamp (:timestamp existing-owner))) | ||
(assoc acc address owner) | ||
acc))) | ||
{}) | ||
vals)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes, the status-go returns duplicate data in the list, we do a cleanup based on the address and timestamp of the balance.
collectible-tx? (send-utils/tx-type-collectible? | ||
(-> db :wallet :ui :send :tx-type)) | ||
collectible (when collectible-tx? | ||
(-> db :wallet :ui :send :collectible)) | ||
one-collectible? (when collectible-tx? | ||
(= (collectible.utils/collectible-balance collectible) 1))] | ||
(= (collectible.utils/collectible-balance collectible sender) 1))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Picking the right balance of the collectible to send.
Previously, we took the first balance in the list, which may not be the right balance of the selected account.
(fn [{:keys [db]} [{:keys [ignore-entry-point?]}]] | ||
(let [entry-point-wallet-home? (= (get-in db [:wallet :ui :send :entry-point]) :wallet-stack) | ||
multiple-owners? (get-in db [:wallet :ui :send :collectible-multiple-owners?]) | ||
transaction-type (get-in db [:wallet :ui :send :tx-type])] | ||
(when (or ignore-entry-point? | ||
(and entry-point-wallet-home? (not multiple-owners?)) | ||
(not entry-point-wallet-home?)) | ||
{:db (update-in db | ||
[:wallet :ui :send] | ||
dissoc | ||
:collectible | ||
:collectible-multiple-owners? | ||
:token-display-name | ||
:amount | ||
(when (send-utils/tx-type-collectible? transaction-type) | ||
:tx-type))})))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We skip cleaning the selected collectible on navigating back (the user will land on the From
screen) from the select recipient screen if the collectible is present in multiple accounts.
(rf/dispatch [:wallet/select-from-account | ||
{:address address | ||
:network-details network-details | ||
:stack-id :screen/wallet.select-from | ||
:start-flow? true}])) | ||
:start-flow? (not collectible-tx?)}])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's a collectible tx, the flow is already started.
Hey @smohamedjavid, would you mind also check and advice about this issue?#21483 also my draft PR #21867 (not working properly yet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
62% of end-end tests have passed
Failed tests (3)Click to expandClass TestWalletMultipleDevice:
Class TestCommunityOneDeviceMerged:
Passed tests (5)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMerged:
|
aa4871e
to
84bedc6
Compare
75% of end-end tests have passed
Expected to fail tests (2)Click to expandClass TestWalletMultipleDevice:
Passed tests (6)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
|
([{:keys [ownership]} address] | ||
(let [balance (some #(when (= address (:address %)) | ||
(js/parseInt (:balance %))) | ||
ownership)] | ||
(if (js/Number.isNaN balance) 0 balance)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a helper for parsing numbers that allows to pass a default utils.number/parse-int
hi @smohamedjavid thank you for PR. Unfortunately, one issue has been found, but it's okay if it will be fixed in a separate follow-up. Let me know if this will be addressed in this PR, otherwise, the PR can be merged |
Signed-off-by: Mohamed Javid <[email protected]>
@VolodLytvynenko - I will fix it in a follow-up PR 👍 |
84bedc6
to
c679008
Compare
fixes #21727
Summary
This PR fixes
Collectibles.send.flow.mp4
Testing notes
This PR doesn't fix the balance of the collectible after it's been sent. It's handled in different issue: #21483.
Platforms
Steps to test
Prerequisites: An account with ERC721 and ERC1155 collectibles
ERC721
ERC1155
status: ready