-
Notifications
You must be signed in to change notification settings - Fork 11.7k
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
Align ERC721 Receiver with current ERC721 standard. #1047
Conversation
Adds a second address field to onERC721Received onERC721Received(address,address,uint256,bytes) Updates the function signature to 0x150b7a02 from 0xf0b9e5ba
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.
Thanks for the update! Just an extra comment to update and then we can merge.
@@ -21,12 +21,14 @@ contract ERC721Receiver { | |||
* transfer. This function MUST use 50,000 gas or less. Return of other | |||
* than the magic value MUST result in the transaction being reverted. | |||
* Note: the contract address is always the message sender. | |||
* @param _from The sending address | |||
* @param _operator The address which called `safeTransferFrom` function | |||
* @param _from The address which previously owned the token | |||
* @param _tokenId The NFT identifier which is being transfered | |||
* @param _data Additional data with no specified format | |||
* @return `bytes4(keccak256("onERC721Received(address,uint256,bytes)"))` |
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.
please update the text here as well
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.
Done. Also fixed the "This function MUST use 50,000 gas or less." statement, which is no longer true.
@@ -4,7 +4,7 @@ import "./ERC721Receiver.sol"; | |||
|
|||
|
|||
contract ERC721Holder is ERC721Receiver { | |||
function onERC721Received(address, uint256, bytes) public returns(bytes4) { | |||
function onERC721Received(address, address, uint256, bytes) public returns(bytes4) { |
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.
Q: What is the purpose of this file? would ERC721Receiver not be the more expressive implementation?
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.
The "Receiver" file is the interface as defined in the standard; the "Holder" contract is the minimal implementation of that interface, so I think we have to leave this as is.
I'm not a huge fan of interfaces not being clearly named as such; I don't particularly like the "Holder" name here, but I don't have a deep enough view of the whole OpenZeppelin stack to guarantee that any change I make would actually be an improvement, so I'll leave this one to others.
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.
Ah, I realize that now, yeah. I do think we'd benefit from an I
prefix being applied to all of the interfaces, but that's a larger change. Cheers
Removed "Must use 50,000 gas or less" Corrected the function signature
#1032 is safe to close after this gets merged. |
🚀 Description
The ERC-721 onERC721Received function changed. It's now
onERC721Received(address,address,uint256,bytes)
. This makes the function signature0x150b7a02
(it was 0xf0b9e5ba)npm run lint:all:fix
).