-
Notifications
You must be signed in to change notification settings - Fork 358
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
Fix erc721 transferFrom
comment
#341
Fix erc721 transferFrom
comment
#341
Conversation
docs/ERC721.md
Outdated
@@ -434,7 +434,7 @@ None. | |||
|
|||
Transfers `tokenId` token from `from_` to `to`. | |||
|
|||
> Note that this function should be used instead of `safeTransferFrom` to transfer tokens. Exercise caution as tokens sent to a contract that does not support ERC721 can be lost forever. | |||
> Usage of this method is discouraged, use [`safeTransferFrom`](#safetransferfrom) whenever possible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it discouraged? They're used in different scenarios and some people even prefer this method to avoid calling arbitrary contracts and thus prevent side effects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point! And thank you for the article^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we mention something here?
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/IERC721.sol#L84
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just asked Fran, he confirmed it's an old comment and he's not sure about that recommendation. let's not discourage it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, sir. Sounds good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good!
docs/ERC721.md
Outdated
@@ -160,15 +160,15 @@ await signer.send_transaction( | |||
|
|||
### Token Transfers | |||
|
|||
EIP721 discourages the use of `transferFrom` and favors `safeTransferFrom` in regard to token transfers. The safe function adds the following conditional logic: | |||
This library includes `transferFrom` and `safeTransferFrom` to transfer NFTs. If using `transferFrom`, note that THE CALLER IS RESPONSIBLE TO CONFIRM THAT THE RECIPIENT IS CAPABLE OF RECEIVING NFTS OR ELSE THEY MAY BE PERMANENTLY LOST. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about bold text instead of uppercase?
Co-authored-by: Martín Triay <martriay@gmail.com>
Resolves #340.