CSV Airdrop Collectible transfer support

[schmanu]

CSV Airdrop Collectible transfer support

• adds support for multisending ERC721 and ERC1155 tokens.
  This feature was requested by multiple users.
• improves the performance of the summary of transfers by using react window. For larger files the browser performance dropped a lot before.
• adds a help button which shows FAQ in a modal

Type

• Update

Compatible Networks

- Mainnet
- Rinkeby
- xDai
- Polygon
- BSC

Code for review: https://github.com/bh2smith/safe-airdrop

IPFS hash/App URL

QmYpnhEbxi2EdY3YaFnP7VCeDziVHdcJ88ZfmrrpP9mCRU
https://cloudflare-ipfs.com/ipfs/QmYpnhEbxi2EdY3YaFnP7VCeDziVHdcJ88ZfmrrpP9mCRU

Desired date none

I will be gone starting Dec 6th for 4 weeks without access to the project. I will only be able to comment

Disclaimer

This update is part of a safe grant

[bh2smith]

Depending on how the review goes and possible change requests that may arise, I could potentially take over to get it to the finish line in the meantime.

[yagopv]

Hi! 👋🏻

We were taking a look to the DApp. It works great!!. We could only test ERC721 because safe web currently don't support ERC1155 as collectible.

We only notice some minor things:

• If the id of the NFT does not match the stored one the app fails to show some feedback about what's going on. No clue for the user about the problem being with an id. In big files could be difficult to guess
• Edge case. I notice that I can add the same NFT n times and send to the same safe address i'm in. I can complete the transaction and will be charged for it
• When trying to use a file with more or less 1500 transactions it fails. With 1200-1300 is fine. Seems to reach some infura API limit or something like that. In the Network panel you can see it easily as a lot of requests are failing. Like this one:

curl 'https://rinkeby.infura.io/v3/{infura-api-key}' \
  -H 'sec-ch-ua: " Not A;Brand";v="99", "Chromium";v="96", "Google Chrome";v="96"' \
  -H 'Referer: http://localhost:3000/' \
  -H 'sec-ch-ua-mobile: ?0' \
  -H 'User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/96.0.4664.110 Safari/537.36' \
  -H 'sec-ch-ua-platform: "macOS"' \
  -H 'Content-Type: application/json' \
  --data-raw '{"jsonrpc":"2.0","method":"eth_call","params":[{"to":"0x00000000000c2e074ec69a0dfb2997ba6c7d2e1e","data":"0x0178b8bf358b36f7e029bb96817cc6cb90230efffe0bdd336410f6905f0b98dde8594a69"},"latest"],"id":"1"}' \
  --compressed

I'm attaching the ok and ko files plus a video:

csv-airdrop-error.csv
csv-airdrop-ok.csv
https://user-images.githubusercontent.com/1576776/146749221-140de030-34eb-44f1-9d0b-c74e7bf063dc.mov

And at last, super minor thing 🙂, you have this duplicated useSafeAppsSDK usage here

This tests were made on Rinkeby

[bh2smith]

Thanks for the great feedback! Some of those items should be easy fixes however others are impractical in practise (e.g. it would never actually be possible to send that many NFTs at once we should put a hard limit on the size of the file and return an error). The number of transfers is bounded above by the block gas limit. I have done benchmarks in the past for erc20 transfers and found that about 250 uses about 7.5 million gas (note this is based on my not so good memory) which was 50% of the block gas limit at the time. We could put a hard cap on the number of records at, say, 400.

We will look closer at your detailed feedback after evaluating and fixing fixing.

I just wanted to get the idea of hard limit out there now so you can tell me if this seems reasonable to you.

[yagopv]

For me seems reasonable. You can import the first n (hard limit) and warn the user about what was done for example

[schmanu]

Hey @yagopv,
Thanks alot for the detailed feedback!

Some issues like balance checks or duplicate transfers (for erc721) are not implemented yet. But I got them on my mental backlog. Will do that after my vacation.

Sending the same id multiple times should always result in at least an warning. Probably even error. But only for erc721 as erc1155 can be fungible.

For erc1155: if you wanna test those a Gnosis safe can hold these tokens. Only displaying these is not supported yet.
Spoilers: we currently only send these using safeTransferFrom. I planned to optimize this to use batchSafeTransferFrom for each individual contract.

As noted I'm currently not able to work on this. I will implement changes in early January.

Cheers from Colombia :)

[schmanu]

Hi @yagopv!

A new release has been published. It should cover all your review issues + more:

• Balance and duplicate checks for erc721 transfers. This change reworked the balance checks by using the safe api to fetch all balances on the start of the app instead of using getBalance() contract calls
• Drain safe functionality (new button to generate a csv file with all safe assets incl. erc721 tokens)
• A hard cap at 400 transactions per file. For now an error will be thrown explaining why this isn't possible
• Some UI tweaks: Breadcrumb Element for separating CSV form and summary tables

Deployed at: https://ipfs.infura.io/ipfs/QmZGXxZd9GEkCHFT4W2Lq8tai7kK2hGf3FqSTTGEAG5KkB

If you have any follow up issues or questions, please let me know here.

Have a great sunday!

[yagopv]

Hi !!

Checked again. Love the new Drain Safe feature ❤️. Pretty cool !!

I can see the new warnings about adding duplicates or tokens whose ids doesn't exist in the safe. Would like to comment about even with the warning I can complete the transaction. I don't know if this is intentional but i can send anyway a wrong token or duplicate ones and to the same address they belong.

So fine for me if you want to leave the warnings as they are right now and left the user decide if they want to complete the transaction.

Aside note: I had a weird error as well that I think is related to react-dropzone/react-dropzone#1103. I couldn't make the file dialog appear pressing the dropzone or button but after updating Chrome it worked as expected. The curious thing is that i was on Chrome 96 arm that as the issue suggest seems to be the solution 😩. Anyway, seems related to react-dropzone or Chrome. I tested in Firefox and Edge and is working fine and as well asked 2 colleagues for checking on Chrome and worked for them as well.

[schmanu]

I can see the new warnings about adding duplicates or tokens whose ids doesn't exist in the safe. Would like to comment about even with the warning I can complete the transaction. I don't know if this is intentional but i can send anyway a wrong token or duplicate ones and to the same address they belong.

This is true and I could change it to an error or add a sticky error message above the submit button reminding users that they try to send more assets than they have in their balance. This would be similiar to the This transaction will most likely fail. To save gas costs, avoid creating the transaction. message which is presented by the gnosis safe when trying to submit the transaction.

[schmanu]

Hey @yagopv,

I played around a bit and improved the sloppy error handling to display the state better at all times.
Would this work for you?

This should clearly point users towards the risks of submitting a possibly wrong transaction and the new header icon makes sure people don't miss the errors / warnings which previously disappeared too quickly / easily.

If this works I'll merge and deploy this.

[yagopv]

I think is really nice! Works for me

[yagopv]

When you have the new version please add a comment with the IPFS and my colleague from the QA team @JagoFigueroa will finish the testing. Thanks!!

[schmanu]

Hey @JagoFigueroa,

I deployed a new release containing the rework of the Message Display.

The release contains the deployed IPFS Hash / URL.

[JagoFigueroa]

Hey guys! hats off to you, the app is phenomenal ;)

All green from my side, the app has worked for me as expected in all the supported chains.

Here are a couple of small improvements that I can think of for the future:

• I think it would be neat to either allow chain prefixes on the CSV file or ignore them when performing a drain. As when doing so with an address with chain prefix it automatically populates the CSV with it, making it invalid:

• The summary for native tokens on Xdai, Polygon & BSC shows them as ETH and I think it would be nice to provide feedback per native token

[bh2smith]

Great feedback! Thanks very much. We will surely put those on our todo list. In fact, I have already created issues

1. Allow/Support Chain Prefixes in CSV bh2smith/safe-airdrop#337
2. Correct Native Token Symbol on all networks bh2smith/safe-airdrop#338

Thanks again! 🥂