Skip to content
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

Agent: add ERC721Receiver support #1058

Merged
merged 5 commits into from
Jan 14, 2020
Merged

Agent: add ERC721Receiver support #1058

merged 5 commits into from
Jan 14, 2020

Conversation

izqui
Copy link
Contributor

@izqui izqui commented Dec 18, 2019

Close #1036, see aragonone/product#251

To do:

  • Tests
  • Decide on release strategy (SaintFame needs this ASAP)

@izqui izqui requested a review from sohkai December 18, 2019 02:19
@auto-assign auto-assign bot requested a review from bpierre December 18, 2019 02:19
@izqui izqui removed the request for review from bpierre December 18, 2019 02:21
@izqui izqui changed the base branch from master to next December 23, 2019 18:58
@@ -56,6 +57,7 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
event RemoveProtectedToken(address indexed token);
event PresignHash(address indexed sender, bytes32 indexed hash);
event SetDesignatedSigner(address indexed sender, address indexed oldSigner, address indexed newSigner);
event ReceiveERC721(address indexed token, address operator, address from, uint256 tokenId, bytes data);
Copy link
Contributor

Choose a reason for hiding this comment

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

Might as well index the operator and from

apps/agent/contracts/Agent.sol Show resolved Hide resolved
@@ -35,6 +35,7 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
uint256 public constant PROTECTED_TOKENS_CAP = 10;

bytes4 private constant ERC165_INTERFACE_ID = 0x01ffc9a7;
bytes4 private constant ERC721_RECEIVED = 0x150b7a02; // bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe rename the variable to ERC721_RECEIVED_INTERFACE_ID to be consistent with the ERC165_INTERFACE_ID.

apps/agent/contracts/Agent.sol Show resolved Hide resolved
@izqui izqui changed the base branch from next to master December 23, 2019 19:02
Copy link
Contributor

@sohkai sohkai left a comment

Choose a reason for hiding this comment

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

LGTM!

@sohkai sohkai changed the base branch from master to next December 23, 2019 19:24
@izqui izqui merged commit 3c0ab41 into next Jan 14, 2020
@izqui izqui deleted the agent-nft branch January 14, 2020 08:32
sohkai added a commit that referenced this pull request Mar 11, 2020
* Agent: add ERC721Receiver support

* Lint

* Test

* Address review comments

* cosmetic: small whitespace changes

Co-authored-by: Brett Sun <qisheng.brett.sun@gmail.com>
ramilexe pushed a commit to ConsiderItDone/aragon-apps that referenced this pull request Dec 9, 2021
* Agent: add ERC721Receiver support

* Lint

* Test

* Address review comments

* cosmetic: small whitespace changes

Co-authored-by: Brett Sun <qisheng.brett.sun@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants