Skip to content
This repository was archived by the owner on Nov 10, 2023. It is now read-only.

(Fix) Support CryptoKitties NFTs#1574

Merged
fernandomg merged 8 commits intodevelopmentfrom
fix/1550-cryptoKitties-support
Nov 11, 2020
Merged

(Fix) Support CryptoKitties NFTs#1574
fernandomg merged 8 commits intodevelopmentfrom
fix/1550-cryptoKitties-support

Conversation

@fernandomg
Copy link
Contributor

This PR closes #1550, by creating logic/collectibles/utils file and move all the NFT-related helper functions into it.

generateERC721TransferTxData will decide whether the method to transfer an NFT will be transfer or safeTransferFrom, based on preset conditions where CryptoKitties tokens is taken as an exception.

transfer was used instead of transferFrom because transferFrom is not implemented in the rinkeby version of CK, and was the method used as a fallback before.

Things done:

  • moved SAFE_TRANSFER_FROM_WITHOUT_DATA_HASH const
  • moved isSendERC721Transaction function
  • moved getERC721Symbol function
  • moved isERC721Contract function
  • created getTransferMethodByContractAddress along with CK_ADDRESS const
  • created generateERC721TransferTxData function
  • refactored ReviewCollectible component to use generateERC721TransferTxData
  • updated tests

This PR will conflict with PR #1567. My suggestion is to merge that PR before, but resolution can be done any way.

 NFT-related helper functions into it

`generateERC721TransferTxData` will decide whether the method
 to transfer an NFT will be `transfer` or `safeTransferFrom`,
 based on preset conditions where CryptoKitties tokens is taken
 as an exception.

Also, `transfer` was used instead of `transferFrom`
 because `transferFrom` is not implemented in the
 rinkeby version, and was the method used as a
 fallback before.

- moved `SAFE_TRANSFER_FROM_WITHOUT_DATA_HASH` const
- moved `isSendERC721Transaction` function
- moved `getERC721Symbol` function
- moved `isERC721Contract` function
- created `getTransferMethodByContractAddress` along with `CK_ADDRESS` const
- created `generateERC721TransferTxData` function
- refactored `ReviewCollectible` component to use `generateERC721TransferTxData`
- updated tests
@fernandomg fernandomg self-assigned this Nov 5, 2020
@github-actions
Copy link

github-actions bot commented Nov 5, 2020

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link

github-actions bot commented Nov 5, 2020

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 3 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@ghost
Copy link

ghost commented Nov 5, 2020

Travis automatic deployment:
https://pr1574--safereact.review.gnosisdev.com/volta/app

@ghost
Copy link

ghost commented Nov 5, 2020

Travis automatic deployment:
https://pr1574--safereact.review.gnosisdev.com/rinkeby/app

1 similar comment
@ghost
Copy link

ghost commented Nov 5, 2020

Travis automatic deployment:
https://pr1574--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Nov 5, 2020

Travis automatic deployment:
https://pr1574--safereact.review.gnosisdev.com/volta/app

* @param {string} contractAddress
* @returns string
*/
export const getTransferMethodByContractAddress = (contractAddress: string): string => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add two tests cases? one for contractAddress = Criptokitties and other for a random collectible address

Copy link
Contributor Author

@fernandomg fernandomg Nov 6, 2020

Choose a reason for hiding this comment

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

done, thanks for the helping hand! 6e96f9d (#1574)

@fernandomg fernandomg requested a review from Agupane November 6, 2020 16:11
@ghost
Copy link

ghost commented Nov 6, 2020

Travis automatic deployment:
https://pr1574--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Nov 6, 2020

Travis automatic deployment:
https://pr1574--safereact.review.gnosisdev.com/volta/app

@ghost
Copy link

ghost commented Nov 9, 2020

Travis automatic deployment:
https://pr1574--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Nov 9, 2020

Travis automatic deployment:
https://pr1574--safereact.review.gnosisdev.com/volta/app

# Conflicts:
#	src/logic/safe/store/actions/transactions/utils/transactionHelpers.ts
#	src/logic/tokens/utils/tokenHelpers.ts
@ghost
Copy link

ghost commented Nov 10, 2020

Travis automatic deployment:
https://pr1574--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Nov 10, 2020

Travis automatic deployment:
https://pr1574--safereact.review.gnosisdev.com/volta/app

@ghost
Copy link

ghost commented Nov 11, 2020

Travis automatic deployment:
https://pr1574--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Nov 11, 2020

Travis automatic deployment:
https://pr1574--safereact.review.gnosisdev.com/volta/app

@francovenica
Copy link
Contributor

Tried just for the rinkeby version (since the CK token is only there).

Safes: https://pr1574--safereact.review.gnosisdev.com/rinkeby/app/#/safes/0xFB1fA6c1b4244191bf4Ab96d9DE74b3D8E6d6c5A/transactions
https://pr1574--safereact.review.gnosisdev.com/rinkeby/app/#/safes/0x9913B9180C20C6b0F21B6480c84422F6ebc4B808/transactions

Send the CK between safes back and forth several times. I didn't had any issues
Tried with another type of ERC721 and didn't had issues either

Looks good to me

@francovenica francovenica self-requested a review November 11, 2020 20:23
@fernandomg fernandomg merged commit 914333d into development Nov 11, 2020
@fernandomg fernandomg deleted the fix/1550-cryptoKitties-support branch November 11, 2020 21:31
@github-actions github-actions bot locked and limited conversation to collaborators Nov 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sending collectibles Tx always fails in ethereum network

4 participants