feat: support sending multi collectibles #20045
Conversation
BalogunofAfrica
requested review from
briansztamfater,
J-Son89,
OmarBasem and
ulisesmac
|
|
||
| (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/contract-type-erc-1155) | ||
| :collectible-multi |
There was a problem hiding this comment.
I know there will be cases where we sent multiple ERC721 tokens in the same multi-transaction, for example batch send for community tokens which are ERC-721 but treated as the same token. Is :collectible-multi some defined naming in the wallet domain? If not, I would suggest to just specify the token standard in the tx-type i.e. :collectible-erc721 or :collectible-erc1155, IMO more understandable and extensible naming. WDYT?
There was a problem hiding this comment.
Yeah that makes sense.
Jenkins BuildsClick to see older builds (36)
|
BalogunofAfrica
force-pushed
the
feat/support-sending-multi-collectibles
branch
from
8093e96 to
8e2f5e6
Compare
87% of end-end tests have passedFailed tests (5)Click to expandClass TestWalletMultipleDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (45)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestWalletOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
|
a question, the schema error that pops up in the video - did you address that? 🤔 |
src/status_im/constants.cljs
Outdated
| (def ^:const bridge-name-hop "Hop") | ||
|
|
||
| (def ^:const contract-type-uknown 0) |
There was a problem hiding this comment.
should we specify what type of contract? 🤔
There was a problem hiding this comment.
That value is for unknown contract: https://github.com/status-im/status-go/blob/develop/services/wallet/common/const.go#L34
There was a problem hiding this comment.
I more meant, this is in a very high level file. For that reason contract could mean a plethora of things.
Perhaps smart_contract_... is more appropriate
There was a problem hiding this comment.
I think a prefix of wallet also works, just to stay similar to the go side which uses add its under the wallet namespace.
| @@ -138,7 +138,8 @@ | |||
| {:prefix prefix | |||
| :testnet-enabled? testnet-enabled? | |||
| :goerli-enabled? goerli-enabled?}) | |||
| collectible-tx? (= (-> db :wallet :ui :send :tx-type) :collectible) | |||
| collectible-tx? (contains? #{:collectible-erc-721 :collectible-erc-1155} | |||
There was a problem hiding this comment.
perhaps we put this set in a constant somewhere?
BalogunofAfrica
force-pushed
the
feat/support-sending-multi-collectibles
branch
from
8e2f5e6 to
643ec7b
Compare
|
Hi @BalogunofAfrica thank you for PR. No issues from my side. PR is ready to be merged |
@BalogunofAfrica - we have a due process for pr reviews. After this it makes sense to ask for QA, before is costly of resources as it increases friction on dev changes as it can likely lead to a waste of further QA resources. I see that this pr is small but it is important we all stick to the process we agreed to so we can keep the quality high. |
BalogunofAfrica
force-pushed
the
feat/support-sending-multi-collectibles
branch
from
643ec7b to
15ff3e2
Compare
Addressed here b842e0f |
BalogunofAfrica
force-pushed
the
feat/support-sending-multi-collectibles
branch
3 times, most recently
from
a95551c to
516b77a
Compare
BalogunofAfrica
force-pushed
the
feat/support-sending-multi-collectibles
branch
from
516b77a to
d3921d8
Compare
|
|
||
| (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)))) |
There was a problem hiding this comment.
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) ?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Totally makes sense for me now.
Thanks @BalogunofAfrica for the explanation, btw, I think the new name for the function is better 👍
BalogunofAfrica
force-pushed
the
feat/support-sending-multi-collectibles
branch
2 times, most recently
from
ee07565 to
5f1383a
Compare
| (defn tx-type-collectible? | ||
| [tx-type] | ||
| (contains? collectible-tx-set tx-type)) |
There was a problem hiding this comment.
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 :)
| @@ -455,6 +463,14 @@ | |||
| :TokenID token-id | |||
| :ChainID to-chain-id)) | |||
|
|
|||
| (= bridge-name constants/bridge-name-erc-1155-transfer) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks @ilmotta I'll consider it for new PRs :)
cc: @status-im/wallet-mobile-devs
BalogunofAfrica
force-pushed
the
feat/support-sending-multi-collectibles
branch
from
5f1383a to
4dacc9e
Compare
Fixes
#19143
Summary
Adds support for ERC1155 (Multi collectibles) send transaction
Adds constants for contract type as seen in status-go https://github.com/status-im/status-go/blob/develop/services/wallet/common/const.go#L31-L38
Platforms
Functional
Steps to test
Simulator.Screen.Recording.-.iPhone.11.Pro.-.2024-05-15.at.16.05.36.mp4
status: ready