Skip to content
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.

Sign up for GitHub

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

feat: support sending multi collectibles #20045

Merged
merged 9 commits into from
May 17, 2024
Merged

feat: support sending multi collectibles #20045

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
16d09b5
update constants
BalogunofAfrica May 15, 2024
75d9dd9
set tx-type to based on multi or single collectible
BalogunofAfrica May 15, 2024
b080936
add ERC1155TransferTx to param list
BalogunofAfrica May 15, 2024
42bada9
rename tx-type for collectibles
BalogunofAfrica May 15, 2024
d710a87
add utils for collectibles
BalogunofAfrica May 16, 2024
4c81703
prefix contract-type with wallet
BalogunofAfrica May 16, 2024
6688a89
fix schema error for amount input
BalogunofAfrica May 16, 2024
6a4cfc2
refactor: rename is-collectible? util
BalogunofAfrica May 16, 2024
4dacc9e
namespace the keys for the type set in [:wallet :ui :send :tx-type]
BalogunofAfrica May 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/quo/components/wallet/amount_input/schema.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@
[:props
[:map {:closed true}
[:status {:optional true} [:maybe [:enum :default :error]]]
[:on-change-text {:optional true} [:maybe fn?]]
[:on-inc-press {:optional true} [:maybe fn?]]
[:on-dec-press {:optional true} [:maybe fn?]]
[:container-style {:optional true} [:maybe :map]]
[:min-value {:optional true} [:maybe :int]]
[:max-value {:optional true} [:maybe :int]]
Expand Down
7 changes: 7 additions & 0 deletions src/status_im/constants.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,11 +506,18 @@
(def ^:const send-type-stickers-buy 4)
(def ^:const send-type-bridge 5)
(def ^:const send-type-erc-721-transfer 6)
(def ^:const send-type-erc-1155-transfer 7)

(def ^:const bridge-name-transfer "Transfer")
(def ^:const bridge-name-erc-721-transfer "ERC721Transfer")
(def ^:const bridge-name-erc-1155-transfer "ERC1155Transfer")
(def ^:const bridge-name-hop "Hop")

(def ^:const wallet-contract-type-uknown 0)
(def ^:const wallet-contract-type-erc-20 1)
(def ^:const wallet-contract-type-erc-721 2)
(def ^:const wallet-contract-type-erc-1155 3)

(def ^:const alert-banner-height 40)

(def ^:const status-hostname "status.app")
Expand Down
4 changes: 2 additions & 2 deletions src/status_im/contexts/wallet/events.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@
{:db (-> db
(assoc-in [:wallet :ui :send :token] token)
(assoc-in [:wallet :ui :send :to-address] to-address)
(assoc-in [:wallet :ui :send :tx-type] :bridge))
(assoc-in [:wallet :ui :send :tx-type] :tx/bridge))
:fx [[:dispatch
[:wallet/wizard-navigate-forward
{:current-screen stack-id
Expand All @@ -259,7 +259,7 @@

(rf/reg-event-fx :wallet/start-bridge
(fn [{:keys [db]}]
{:db (assoc-in db [:wallet :ui :send :tx-type] :bridge)
{:db (assoc-in db [:wallet :ui :send :tx-type] :tx/bridge)
:fx [[:dispatch [:open-modal :screen/wallet.bridge-select-asset]]]}))

(rf/reg-event-fx :wallet/select-bridge-network
Expand Down
33 changes: 25 additions & 8 deletions src/status_im/contexts/wallet/send/events.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,8 @@
{:prefix prefix
:testnet-enabled? testnet-enabled?
:goerli-enabled? goerli-enabled?})
collectible-tx? (= (-> db :wallet :ui :send :tx-type) :collectible)
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?
Expand Down Expand Up @@ -235,13 +236,19 @@
:collectible
:token-display-name
:amount
(when (= transaction-type :collectible) :tx-type))})))
(when (send-utils/tx-type-collectible?
transaction-type)
:tx-type))})))

(rf/reg-event-fx
:wallet/set-collectible-to-send
(fn [{db :db} [{:keys [collectible current-screen]}]]
(let [collection-data (:collection-data collectible)
collectible-data (:collectible-data collectible)
contract-type (:contract-type collectible)
tx-type (if (= contract-type constants/wallet-contract-type-erc-1155)
:tx/collectible-erc-1155
:tx/collectible-erc-721)
collectible-id (get-in collectible [:id :token-id])
one-collectible? (= (collectible.utils/collectible-balance collectible) 1)
token-display-name (cond
Expand All @@ -255,7 +262,7 @@
(update-in [:wallet :ui :send] dissoc :token)
(assoc-in [:wallet :ui :send :collectible] collectible)
(assoc-in [:wallet :ui :send :token-display-name] token-display-name)
(assoc-in [:wallet :ui :send :tx-type] :collectible))
(assoc-in [:wallet :ui :send :tx-type] tx-type))
recipient-set? (-> db :wallet :ui :send :recipient)]
{:db (cond-> collectible-tx
one-collectible? (assoc-in [:wallet :ui :send :amount] 1))
Expand Down Expand Up @@ -314,16 +321,17 @@
amount-in (send-utils/amount-in-hex amount (if token token-decimal 0))
from-address wallet-address
disabled-from-chain-ids disabled-from-chain-ids
disabled-to-chain-ids (if (= transaction-type :bridge)
disabled-to-chain-ids (if (= transaction-type :tx/bridge)
(filter #(not= % bridge-to-chain-id) network-chain-ids)
(filter (fn [chain-id]
(not (some #(= chain-id %)
receiver-networks)))
network-chain-ids))
from-locked-amount {}
transaction-type-param (case transaction-type
:collectible constants/send-type-erc-721-transfer
:bridge constants/send-type-bridge
:tx/collectible-erc-721 constants/send-type-erc-721-transfer
:tx/collectible-erc-1155 constants/send-type-erc-1155-transfer
:tx/bridge constants/send-type-bridge
constants/send-type-transfer)
balances-per-chain (when token (:balances-per-chain token))
token-available-networks-for-suggested-routes
Expand Down Expand Up @@ -455,6 +463,14 @@
:TokenID token-id
:ChainID to-chain-id))

(= bridge-name constants/bridge-name-erc-1155-transfer)
Copy link
Contributor

@ilmotta ilmotta May 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't reviewed PRs that evolved this send.events namespace, but the fact that it has zero coverage is not good. This namespace is not trivial and I'd normally reject as a reviewer. The re-frame architecture was built to help make this layer testable and it's the top-priority recommended by re-frame to test https://github.com/day8/re-frame/blob/5bd82b3d6625af1fac2b21ba0cd5bab448e44ffe/docs/Testing.md#what-to-test.

I know it's not a tech debt from your PR and at this stage it might be too much work to attach attack this debt. But, it's never too late to play the boy scout rule Always leave your code cleaner than you found it.

Perhaps something you guys could figure out in future changes to this events namespace.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ilmotta I'll consider it for new PRs :)

cc: @status-im/wallet-mobile-devs

(assoc :ERC1155TransferTx
(assoc tx-data
:Recipient to-address
:TokenID token-id
:ChainID to-chain-id
:Amount amount-in))

(= bridge-name constants/bridge-name-transfer)
(assoc :TransferTx tx-data)

Expand Down Expand Up @@ -494,8 +510,9 @@
from-address (get-in db [:wallet :current-viewing-account-address])
transaction-type (get-in db [:wallet :ui :send :tx-type])
transaction-type-param (case transaction-type
:collectible constants/send-type-erc-721-transfer
:bridge constants/send-type-bridge
:tx/collectible-erc-721 constants/send-type-erc-721-transfer
:tx/collectible-erc-1155 constants/send-type-erc-1155-transfer
:tx/bridge constants/send-type-bridge
constants/send-type-transfer)
token (get-in db [:wallet :ui :send :token])
collectible (get-in db [:wallet :ui :send :collectible])
Expand Down
9 changes: 6 additions & 3 deletions src/status_im/contexts/wallet/send/flow_config.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
(ns status-im.contexts.wallet.send.flow-config)
(ns status-im.contexts.wallet.send.flow-config
(:require
[status-im.contexts.wallet.send.utils :as send-utils]))

(defn- collectible-selected?
[db]
(let [collectible-stored (-> db :wallet :ui :send :collectible)
tx-type (-> db :wallet :ui :send :tx-type)]
(and (some? collectible-stored)
(= tx-type :collectible))))
(send-utils/tx-type-collectible? tx-type))))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you checking for collectible tx type that way.. you seem to be checking for

(def ^:private collectible-tx-set
  #{:tx/collectible-erc-721
    :tx/collectible-erc-1155})

if it contains :collectible, what is the benefit of that over (= tx-type :collectible) ?

Copy link
Contributor Author

@BalogunofAfrica BalogunofAfrica May 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tx-type for collectibles used to be just :collectible before because we supported only erc-721 collectibles. But now it is either :tx/collectible-erc-721 or :tx/collectible-erc-1155. The :collectible keyword has been renamed to :tx/collectible-erc-721.

So a simple = for :collectible would not suffice, as we have to check if the tx-type is either of the 2 supported collectible types.

cc: @ulisesmac

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally makes sense for me now.
Thanks @BalogunofAfrica for the explanation, btw, I think the new name for the function is better 👍


(defn- token-selected?
[db]
Expand All @@ -19,7 +21,8 @@
{:screen-id :screen/wallet.select-asset
:skip-step? (fn [db] (or (token-selected? db) (collectible-selected? db)))}
{:screen-id :screen/wallet.send-input-amount
:skip-step? (fn [db] (= (get-in db [:wallet :ui :send :tx-type]) :collectible))}
:skip-step? (fn [db]
(send-utils/tx-type-collectible? (get-in db [:wallet :ui :send :tx-type])))}
{:screen-id :screen/wallet.select-collectible-amount
:skip-step? (fn [db]
(or (not (collectible-selected? db))
Expand Down
16 changes: 8 additions & 8 deletions src/status_im/contexts/wallet/send/transaction_confirmation/view.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,15 @@
:weight :semi-bold
:style style/title-container
:accessibility-label :send-label}
(if (= transaction-type :bridge)
(if (= transaction-type :tx/bridge)
(i18n/label :t/bridge)
(i18n/label :t/send))]
[quo/summary-tag
{:token (if collectible? "" token-display-name)
:label (str amount " " token-display-name)
:type (if collectible? :collectible :token)
:image-source (if collectible? image-url :eth)}]]
(if (= transaction-type :bridge)
(if (= transaction-type :tx/bridge)
(map-indexed
(fn [idx path]
(let [from-network (:from path)
Expand Down Expand Up @@ -98,7 +98,7 @@
:style style/title-container
:accessibility-label :send-label}
(i18n/label :t/to)]
(if (= transaction-type :bridge)
(if (= transaction-type :tx/bridge)
[quo/summary-tag
{:type :network
:image-source (:source to-network)
Expand All @@ -107,7 +107,7 @@
[quo/summary-tag
{:type :address
:label (utils/get-shortened-address to-address)}])]
(when (= transaction-type :bridge)
(when (= transaction-type :tx/bridge)
[rn/view
{:style {:flex-direction :row
:margin-top 4}}
Expand Down Expand Up @@ -198,7 +198,7 @@
{:amount (str max-fees)
:symbol currency-symbol})}]
[data-item
{:title (if (= transaction-type :bridge)
{:title (if (= transaction-type :tx/bridge)
(i18n/label :t/bridged-to
{:network (:abbreviated-name to-network)})
(i18n/label :t/user-gets {:name (utils/get-shortened-address to-address)}))
Expand Down Expand Up @@ -249,7 +249,7 @@
:footer (when (and route (seq route))
[standard-auth/slide-button
{:size :size-48
:track-text (if (= transaction-type :bridge)
:track-text (if (= transaction-type :tx/bridge)
(i18n/label :t/slide-to-bridge)
(i18n/label :t/slide-to-send))
:container-style {:z-index 2}
Expand Down Expand Up @@ -282,12 +282,12 @@
:theme theme}]
[user-summary
{:token-display-name token-display-name
:summary-type (if (= transaction-type :bridge)
:summary-type (if (= transaction-type :tx/bridge)
:status-account
:account)
:accessibility-label :summary-to-label
:label (i18n/label :t/to-capitalized)
:account-props (if (= transaction-type :bridge)
:account-props (if (= transaction-type :tx/bridge)
from-account-props
user-props)
:network-values to-values-by-chain
Expand Down
8 changes: 8 additions & 0 deletions src/status_im/contexts/wallet/send/utils.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,3 +208,11 @@
:position-diff position-diff})))
[]
route))

(def ^:private collectible-tx-set
#{:tx/collectible-erc-721
:tx/collectible-erc-1155})

(defn tx-type-collectible?
[tx-type]
(contains? collectible-tx-set tx-type))
Comment on lines +216 to +218
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need contains?, the set can be used itself as a function to check this, but just saying in case you didn't know, I'm not saying it must be changed :)