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.

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

Support for NFT transfers #16

Closed
bh2smith opened this issue Mar 10, 2021 · 11 comments
Closed

Support for NFT transfers #16

bh2smith opened this issue Mar 10, 2021 · 11 comments
Assignees
Labels
enhancement New feature or request

Comments

@bh2smith
Copy link
Owner

Given that NFTs have now hit the scene, it shouldn't be too difficult to support NFT airdrops.

@bh2smith
Copy link
Owner Author

bh2smith commented Mar 31, 2021

See here for an example cryptokitty transfer

However, looking at ENS domains, there is no plain old transfer function (only transferFrom) we may need to find an NFT token standard and also come up with a way to easily identify which elements of the transfer file are NFTs.

@bh2smith
Copy link
Owner Author

bh2smith commented May 4, 2021

We should probably follow the ERC721 token standard if we want to support this:

https://ethereum.org/en/developers/docs/standards/tokens/erc-721/

@bh2smith bh2smith added the enhancement New feature or request label Sep 2, 2021
@arberx
Copy link

arberx commented Sep 9, 2021

This would be awesome. Currently looking for a solution to send multiple NFTs to multiple addresses using a gnosis safe.

If someone could walk me through the code on how to support it, I could maybe implement it myself.

@bh2smith
Copy link
Owner Author

bh2smith commented Sep 9, 2021

Great to hear your enthusiasm @arberx, this will likely be a quite involved task UI-wise but is actually quite simple from a backend standpoint. I will gather a few points in the code, where we batch together the erc20 transfers for guidance, but essentially we parse the CSV with rows containing (NFT address, recipient) columns and encode each row as an NFT transfer, then throw it all into a gnosis safe "multi send" transaction and voila. We would also have to import or hard code the ERC721 contract artifacts to be able to encode the transfer method, but probably only the interface contract is needed.

Of course we should implement validations that the safe itself actually owns the tokens it seeks to transfer and give a fair warning beforehand.

From the UI side, we were thinking that NFT transfers should actually be a separate tab so to distinguish between the different CSV types, but now that I think of it, since we assume ETH transfer when the token address field is null, we could also assume NFT transfer whenever the amount field is null. This way we could actually batch together all three types of airdrops (erc20, native token and NFT)into a single transfer!

@bh2smith
Copy link
Owner Author

bh2smith commented Sep 9, 2021

This is the code block which builds transfers from a list of Payment

export function buildTransfers(transferData: Payment[]): BaseTransaction[] {
const txList: BaseTransaction[] = transferData.map((transfer) => {
if (transfer.tokenAddress === null) {
// Native asset transfer
return {
to: transfer.receiver,
value: toWei(transfer.amount, 18).toFixed(),
data: "0x",
};
} else {
// ERC20 transfer
const decimals = transfer.decimals;
const amountData = toWei(transfer.amount, decimals);
return {
to: transfer.tokenAddress,
value: "0",
data: erc20Interface.encodeFunctionData("transfer", [transfer.receiver, amountData.toFixed()]),
};
}
});
return txList;
}

Note that once the CSV is uploaded, during parsing, we convert each row into a "Payment" object.

You can see here that we are simply setting the values for a standard ethereum transaction.

  1. When native eth transfer we set the to address to recipient, the value to the amount and data to nothing.

  2. When erc20 transfer, to is the token contract address, value is 0 (because we aren't sending eth) and data is the encoded function call data of a transfer to recipient for the wei amount

  3. When NFT (i.e. when amount == null)... this would be similar to the erc20 transaction (with to as the token address) but it the data would then be encoded ERC721 transfer to recipient.

So step 3 is is essentially all that is needed to build the support that turns (token_address, recipient) into a batched transfer. There is some preprocessing involved when parsing this from the CSV for erc20s at least (we fetch the token details like symbol and decimals to display a correctly formatted list of transfers in the UI), we also verify sufficient balance - the usual. In this case, you might want to fetch some metadata for the nft like an image to display this in the interface and verify that the Safe actually owns it.

Forget about separating it into another tab, but some thought about how to display a collection of NFT transfers in the UI might be required.

@arberx
Copy link

arberx commented Sep 9, 2021

Thanks for the quick response and detailed explanation!

The contract work seems straightforward, but don't think I can commit to the front-end at the moment. Also, it seems the transaction builder app in gnosis supports erc721's although there isn't a good example...

@bh2smith
Copy link
Owner Author

bh2smith commented Sep 9, 2021

The contract work seems straightforward, but don't think I can commit to the front-end at the moment.

This is something that @schmanu might be interested in helping out with. Feel free to implement the backend anyway and let just see what happens by default without all the UI stuff.

@schmanu
Copy link
Collaborator

schmanu commented Sep 9, 2021

Yes sir! I would like to implement this in one! I'm just a bit busy at the moment. So I don't know for sure yet when I can get it done.

@bh2smith
Copy link
Owner Author

bh2smith commented Sep 9, 2021

Yes sir! I would like to implement this in one! I'm just a bit busy at the moment. So I don't know for sure yet when I can get it done.

No worries, it will likely be here for you. This issue has been open since early March anyway. I doubt that I'll ever get around to it unless I really need to send a bunch of NFTs.

@schmanu
Copy link
Collaborator

schmanu commented Oct 1, 2021

I finally have an update on this topic.

Today I built the first working version which combines NFTs and Asset Multisend.
This makes it intuitive and easy to send NFTs, ERC20 Tokens and Native transfers in one transaction using multisend.

Here is a gif showing the new UI.

nfts

schmanu added a commit that referenced this issue Oct 1, 2021
NEW UI:
* Tab-Navigation to fill out a CSV for Asset-Transfers or Collectible-Transfers
* transfer-tables are now always displayed under the CSV-Form.
* there are now two tables: For asset transfers and for collectible transfers
* submit button is at the bottom of these tables

issue #16
@schmanu schmanu self-assigned this Nov 2, 2021
bh2smith added a commit that referenced this issue Dec 3, 2021
* First working version of nft transfers

* UI changes for multisending nfts and erc20 tokens in one transaction

NEW UI:
* Tab-Navigation to fill out a CSV for Asset-Transfers or Collectible-Transfers
* transfer-tables are now always displayed under the CSV-Form.
* there are now two tables: For asset transfers and for collectible transfers
* submit button is at the bottom of these tables

issue #16

* small ui cleanup

* Cache for erc721 token info provider

* rework of nft sending ui

* one combined CSV file for nft / asset transfers
* tables are wrapped in accordions

* erc1155 support, some redesigns

* small refactoring of modal

* fixes existing unittests

* small refactoring, parser tests

* finishes up nft transfers

* Updates help text
* Validates, that the value is a integer for erc1155 transfers
* unittests for the transfer of collectibles
* unittest for decimal (invalid) erc1155
* fixes sample file

* remove unused global styles

* simplifies token_types erc1155 and erc721 to nft

* instead of providing erc1155/erc721 the token_type now is simply nft
* fixes performance problem of editor. For no reason the csvContent was held by the App and passed down to the editor
* tests for nft transfers
* updated faq

* Update src/components/FAQModal.tsx

Co-authored-by: schmanu <[email protected]>
Co-authored-by: Benjamin Smith <[email protected]>
@schmanu
Copy link
Collaborator

schmanu commented Jan 9, 2022

The newest version has ERC721 and ERC1155 transfer support!

It's release to the gnosis app list is currently under review but its already merged into main branch.

@schmanu schmanu closed this as completed Jan 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants