Safe-react: Sending collectibles Tx always fails in ethereum network

[francovenica]

Env and safe: https://pr1537--safereact.review.gnosisdev.com/rinkeby/app/#/safes/0x9913B9180C20C6b0F21B6480c84422F6ebc4B808/transactions

Tried transfer 2 CK to another safe created recently.

Etherscan of the 2 attempts
https://rinkeby.etherscan.io/tx/0xc39899150beb3138b4ae596a51ba7425dbc05d4425be245c1435cd003903d42a
https://rinkeby.etherscan.io/tx/0x49993a44ecac024e1592d00ee75495d3f57afe51e3ce059cb95a84871b760da4

[fernandomg]

Since the fix of #1463, we're no longer falling back to transfer or transferFrom.

That particular NFT (CK in Rinkeby) has not safeTransferFrom method implemented.

@rmeissner, @dasanra, @tschubotz

Shall we fallback to transferFrom if not safeTransferFrom is available?

[dasanra]

@fernandomg thank you for this feedback. We will discuss it on monday. At least we know why it's happening

[fernandomg]

Given that now we have the chance to know beforehand that a tx may fail (thanks to #1523).

With that info, we should try to re-build the tx with transferFrom for the NFTs.

---

Still, I'm concerned about using transferFrom since we may be sending the tx to a non-ERC721TokenReceiver contract.

[tschubotz]

@fernandomg @pablofullana I just checked the contracts of the top ERC721 collectibles that we have in Gnosis Safe. Literally all of them properly implement safeTransferFrom. Just CryptoKitties doesn't.

I'm against falling automatically back to transferFrom whenever estimation fails with safeTransferFrom since it's the point of safeTransferFrom to fail, when the recipient doesn't implement IERC721TokenReceiver.

So I propose just putting in an exception for CryptoKitties, i.e. use transferFrom just for the CK contracts and use safeTransferFrom for any other collectible.

CK on mainnet: https://etherscan.io/address/0x06012c8cf97bead5deae237070f9587f8e7a266d
CK on rinkeby: https://rinkeby.etherscan.io/address/0x16baf0de678e52367adc69fd067e5edd1d33e3bf

What do you think?

Also cc @rmeissner @dasanra

[fernandomg]

It looks valid to me, as it seems that the CK is the only popular NFT that didn't implement the ERC721 standard.

Shall we warn the user about a potential token lost if the counterpart is a contract? Or that's too much?

[tschubotz]

Shall we warn the user about a potential token lost if the counterpart is a contract? Or that's too much?

I think that's too much to be honest. I don't know of any wallet that does that and it seems to work fine :)