From e190e55ce689809dd09b6ed9fef167fa35bcc19c Mon Sep 17 00:00:00 2001 From: Eric Nordelo Date: Tue, 14 May 2024 12:53:50 +0200 Subject: [PATCH] Add ERC721 hooks (#978) * feat: working in logic * feat: add empty impl * feat: finish docs * feat: update CHANGELOG * Update src/token/erc721/erc721.cairo Co-authored-by: Andrew Fleming * Update src/token/erc721/erc721.cairo Co-authored-by: Andrew Fleming * Update docs/modules/ROOT/pages/api/erc721.adoc Co-authored-by: Andrew Fleming * feat: apply review updates * docs: update comment * feat: add more tests * feat: apply review updates * feat: apply review updates --------- Co-authored-by: Andrew Fleming --- CHANGELOG.md | 3 + docs/modules/ROOT/pages/api/erc721.adoc | 198 ++++++++--- docs/modules/ROOT/pages/erc721.adoc | 2 +- src/presets/erc721.cairo | 3 +- src/tests/mocks/erc721_mocks.cairo | 6 +- src/tests/presets/test_erc721.cairo | 22 -- src/tests/token/test_erc721.cairo | 422 ++++++++++++++++++++---- src/token/erc721.cairo | 3 + src/token/erc721/erc721.cairo | 287 +++++++++++----- 9 files changed, 722 insertions(+), 224 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 657954aaa..ce2e930a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,10 +10,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- before_update and after_update hooks to ERC721Component (#978) - before_update and after_update hooks to ERC1155Component (#982) ### Changed (Breaking) +- ERC721Component internal implementation to support transfer, mint, and burn flows going through an `_update` function (#978) +- ERC721Component implementations now require an ERC721HooksTrait implementation in scope. - ERC1155Component implementations now require an ERC1155HooksTrait implementation in scope. ## 0.12.0 (2024-04-21) diff --git a/docs/modules/ROOT/pages/api/erc721.adoc b/docs/modules/ROOT/pages/api/erc721.adoc index afe5e4b63..903aac215 100644 --- a/docs/modules/ROOT/pages/api/erc721.adoc +++ b/docs/modules/ROOT/pages/api/erc721.adoc @@ -192,6 +192,17 @@ ERC721 component implementing <> and <>. Requirements: -- `operator` cannot be the caller. +- `operator` is not the zero address. [.contract-item] [[ERC721Component-get_approved]] @@ -379,49 +416,49 @@ If the URI is not set for `token_id`, the return value will be an empty `ByteArr [[ERC721Component-balanceOf]] ==== `[.contract-item-name]#++balanceOf++#++(self: @ContractState, account: ContractAddress) -> u256++` [.item-kind]#external# -See <>. +See <>. [.contract-item] [[ERC721Component-ownerOf]] ==== `[.contract-item-name]#++ownerOf++#++(self: @ContractState, tokenId: u256) -> ContractAddress++` [.item-kind]#external# -See <>. +See <>. [.contract-item] [[ERC721Component-safeTransferFrom]] ==== `[.contract-item-name]#++safeTransferFrom++#++(ref self: ContractState, from: ContractAddress, to: ContractAddress, tokenId: u256, data: Span)++` [.item-kind]#external# -See <>. +See <>. [.contract-item] [[ERC721Component-transferFrom]] ==== `[.contract-item-name]#++transferFrom++#++(ref self: ContractState, from: ContractAddress, to: ContractAddress, tokenId: u256)++` [.item-kind]#external# -See <>. +See <>. [.contract-item] [[ERC721Component-setApprovalForAll]] ==== `[.contract-item-name]#++setApprovalForAll++#++(ref self: ContractState, operator: ContractAddress, approved: bool)++` [.item-kind]#external# -See <>. +See <>. [.contract-item] [[ERC721Component-getApproved]] ==== `[.contract-item-name]#++getApproved++#++(self: @ContractState, tokenId: u256) -> ContractAddress++` [.item-kind]#external# -See <>. +See <>. [.contract-item] [[ERC721Component-isApprovedForAll]] ==== `[.contract-item-name]#++isApprovedForAll++#++(self: @ContractState, owner: ContractAddress, operator: ContractAddress) -> bool++` [.item-kind]#external# -See <>. +See <>. [.contract-item] [[ERC721Component-tokenURI]] ==== `[.contract-item-name]#++tokenURI++#++(self: @ContractState, tokenId: u256) -> ByteArray++` [.item-kind]#external# -See <>. +See <>. ==== Internal functions @@ -437,7 +474,12 @@ This should be used inside the contract's constructor. ==== `[.contract-item-name]#++_owner_of++#++(self: @ContractState, token_id: felt252) -> ContractAddress++` [.item-kind]#internal# Internal function that returns the owner address of `token_id`. -This function will panic if the token does not exist. + +[.contract-item] +[[ERC721Component-_require_owned]] +==== `[.contract-item-name]#++_require_owned++#++(self: @ContractState, token_id: felt252) -> ContractAddress++` [.item-kind]#internal# + +Version of xref:#ERC721Component-_owner_of[_owner_of] that panics if owner is the zero address. [.contract-item] [[ERC721Component-_exists]] @@ -448,61 +490,70 @@ Internal function that returns whether `token_id` exists. Tokens start existing when they are minted (<>), and stop existing when they are burned (<>). [.contract-item] -[[ERC721Component-_is_approved_or_owner]] -==== `[.contract-item-name]#++_is_approved_or_owner++#++(ref self: ContractState, spender: ContractAddress, token_id: u256) -> bool++` [.item-kind]#internal# +[[ERC721Component-_approve]] +==== `[.contract-item-name]#++_approve++#++(ref self: ContractState, to: ContractAddress, token_id: u256, auth: ContractAddress)++` [.item-kind]#internal# -Internal function that returns whether `spender` is allowed to manage `token_id`. +Approve `to` to operate on `token_id` -Requirements: +The `auth` argument is optional. If the value passed is non-zero, then this function will check that `auth` is +either the owner of the token, or approved to operate on all tokens held by this owner. -- `token_id` exists. +Emits an <> event. [.contract-item] -[[ERC721Component-_approve]] -==== `[.contract-item-name]#++_approve++#++(ref self: ContractState, to: ContractAddress, token_id: u256)++` [.item-kind]#internal# +[[ERC721Component-_approve_with_optional_event]] +==== `[.contract-item-name]#++_approve_with_optional_event++#++(ref self: ContractState, to: ContractAddress, token_id: u256, auth: ContractAddress, emit_event: bool)++` [.item-kind]#internal# -Internal function that changes or reaffirms the approved address for an NFT. +Variant of xref:#ERC721Component-_approve[_approve] with an optional flag to enable or disable the `Approval` event. +The event is not emitted in the context of transfers. -Emits an <> event. +WARNING: If `auth` is zero and `emit_event` is false, this function will not check that the token exists. Requirements: -- `token_id` exists. -- `to` is not the current token owner. +- if `auth` is non-zero, it must be either the owner of the token or approved to +operate on all of its tokens. + +May emit an <> event. [.contract-item] [[ERC721Component-_set_approval_for_all]] ==== `[.contract-item-name]#++_set_approval_for_all++#++(ref self: ContractState, owner: ContractAddress, operator: ContractAddress, approved: bool)++` [.item-kind]#internal# -Internal function that enables or disables approval for `operator` to manage all of the -`owner` assets. - -Emits an <> event. +Enables or disables approval for `operator` to manage +all of the `owner` assets. Requirements: -- `operator` cannot be the caller. +- `operator` is not the zero address. + +Emits an <> event. [.contract-item] [[ERC721Component-_mint]] ==== `[.contract-item-name]#++_mint++#++(ref self: ContractState, to: ContractAddress, token_id: u256)++` [.item-kind]#internal# -Internal function that mints `token_id` and transfers it to `to`. +Mints `token_id` and transfers it to `to`. +Internal function without access restriction. -Emits an <> event. +WARNING: This method may lead to the loss of tokens if `to` is not aware of the ERC721 protocol. Requirements: - `to` is not the zero address. -- `token_id` does not already exist. +- `token_id` does not exist. + +Emits a <> event. [.contract-item] [[ERC721Component-_transfer]] ==== `[.contract-item-name]#++_transfer++#++(ref self: ContractState, from: ContractAddress, to: ContractAddress, token_id: u256)++` [.item-kind]#internal# -Internal function that transfers `token_id` from `from` to `to`. +Transfers `token_id` from `from` to `to`. -Emits an <> event. +Internal function without access restriction. + +WARNING: This method may lead to the loss of tokens if `to` is not aware of the ERC721 protocol. Requirements: @@ -510,52 +561,58 @@ Requirements: - `from` is the token owner. - `token_id` exists. +Emits a <> event. + [.contract-item] [[ERC721Component-_burn]] ==== `[.contract-item-name]#++_burn++#++(ref self: ContractState, token_id: u256)++` [.item-kind]#internal# -Internal function that destroys `token_id`. -The approval is cleared when the token is burned. -This internal function does not check if the sender is authorized to operate on the token. +Destroys `token_id`. The approval is cleared when the token is burned. -Emits an <> event. +This internal function does not check if the caller is authorized +to operate on the token. Requirements: -`token_id` exists. +- `token_id` exists. + +Emits a <> event. [.contract-item] [[ERC721Component-_safe_mint]] ==== `[.contract-item-name]#++_safe_mint++#++(ref self: ContractState, to: ContractAddress, token_id: u256, data: Span)++` [.item-kind]#internal# -Internal function that mints `token_id` and transfers it to `to`. -If `to` is not an account contract, `to` must support <>; otherwise, the transaction will fail. +Mints `token_id` if `to` is either an account or `IERC721Receiver`. -Emits an <> event. +`data` is additional data, it has no specified format and is forwarded in `IERC721Receiver::on_erc721_received` to `to`. + +WARNING: This method makes an external call to the recipient contract, which can lead to reentrancy vulnerabilities. Requirements: -- `token_id` does not already exist. -- `to` is either an account contract or supports the <> interface. +- `token_id` does not exist. +- `to` is either an account contract or supports the `IERC721Receiver` interface. + +Emits a <> event. [.contract-item] [[ERC721Component-_safe_transfer]] ==== `[.contract-item-name]#++_safe_transfer++#++(ref self: ContractState, from: ContractAddress, to: ContractAddress, token_id: u256, data: Span)++` [.item-kind]#internal# -Internal function that transfers `token_id` token from `from` to `to`, checking first that contract recipients are aware of the ERC721 protocol to prevent tokens from being forever locked. - -`data` is additional data, it has no specified format and it is sent in call to `to`. +Transfers ownership of `token_id` from `from` if `to` is either an account or `IERC721Receiver`. -This internal function does not include permissions but can be useful for instances like implementing alternative mechanisms to perform signature-based token transfers. +`data` is additional data, it has no specified format and is forwarded in `IERC721Receiver::on_erc721_received` to `to`. -Emits an <> event. +WARNING: This method makes an external call to the recipient contract, which can lead to reentrancy vulnerabilities. Requirements: - `to` cannot be the zero address. - `from` must be the token owner. - `token_id` exists. -- `to` either is an account contract or supports the <> interface. +- `to` is either an account contract or supports the `IERC721Receiver` interface. + +Emits a <> event. [.contract-item] [[ERC721Component-_set_base_uri]] @@ -569,6 +626,49 @@ Internal function that sets the `base_uri`. Base URI for computing <>. +If set, the resulting URI for each token will be the concatenation of the base URI and the token ID. +Returns an empty `ByteArray` if not set. + +[.contract-item] +[[ERC721Component-_is_authorized]] +==== `[.contract-item-name]#++_is_authorized++#++(self: @ContractState, owner: ContractAddress, spender: ContractAddress, token_id: u256) -> bool++` [.item-kind]#internal# + +Returns whether `spender` is allowed to manage ``owner``'s tokens, or `token_id` in +particular (ignoring whether it is owned by `owner`). + +WARNING: This function assumes that `owner` is the actual owner of `token_id` and does not verify this +assumption. + +[.contract-item] +[[ERC721Component-_check_authorized]] +==== `[.contract-item-name]#++_check_authorized++#++(self: @ContractState, owner: ContractAddress, spender: ContractAddress, token_id: u256) -> bool++` [.item-kind]#internal# + +Checks if `spender` can operate on `token_id`, assuming the provided `owner` is the actual owner. + +Requirements: + +- `owner` cannot be the zero address. +- `spender` cannot be the zero address. +- `spender` must be the owner of `token_id` or be approved to operate on it. + +WARNING: This function assumes that `owner` is the actual owner of `token_id` and does not verify this +assumption. + +[.contract-item] +[[ERC721Component-_update]] +==== `[.contract-item-name]#++_update++#++(ref self: ContractState, to: ContractAddress, token_id: u256, auth: ContractAddress)++` [.item-kind]#internal# + +Transfers `token_id` from its current owner to `to`, or alternatively mints (or burns) if the current owner +(or `to`) is the zero address. Returns the owner of the `token_id` before the update. + +The `auth` argument is optional. If the value passed is non-zero, then this function will check that +`auth` is either the owner of the token, or approved to operate on the token (by the owner). + +Emits a <> event. + +NOTE: This function can be extended using the `ERC721HooksTrait`, to add +functionality before and/or after the transfer, mint, or burn. + ==== Events [.contract-item] diff --git a/docs/modules/ROOT/pages/erc721.adoc b/docs/modules/ROOT/pages/erc721.adoc index 43ca9beea..eab3314eb 100644 --- a/docs/modules/ROOT/pages/erc721.adoc +++ b/docs/modules/ROOT/pages/erc721.adoc @@ -95,7 +95,7 @@ Here's an example of a basic contract: #[starknet::contract] mod MyNFT { use openzeppelin::introspection::src5::SRC5Component; - use openzeppelin::token::erc721::ERC721Component; + use openzeppelin::token::erc721::{ERC721Component, ERC721HooksEmptyImpl}; use starknet::ContractAddress; component!(path: ERC721Component, storage: erc721, event: ERC721Event); diff --git a/src/presets/erc721.cairo b/src/presets/erc721.cairo index 8d57b1f65..18205513d 100644 --- a/src/presets/erc721.cairo +++ b/src/presets/erc721.cairo @@ -12,7 +12,7 @@ mod ERC721Upgradeable { use openzeppelin::access::ownable::OwnableComponent; use openzeppelin::introspection::src5::SRC5Component; - use openzeppelin::token::erc721::ERC721Component; + use openzeppelin::token::erc721::{ERC721Component, ERC721HooksEmptyImpl}; use openzeppelin::upgrades::UpgradeableComponent; use openzeppelin::upgrades::interface::IUpgradeable; use starknet::{ContractAddress, ClassHash}; @@ -100,7 +100,6 @@ mod ERC721Upgradeable { break; } let id = *token_ids.pop_front().unwrap(); - self.erc721._mint(recipient, id); } } diff --git a/src/tests/mocks/erc721_mocks.cairo b/src/tests/mocks/erc721_mocks.cairo index 6a183bdcc..a1862a768 100644 --- a/src/tests/mocks/erc721_mocks.cairo +++ b/src/tests/mocks/erc721_mocks.cairo @@ -1,7 +1,7 @@ #[starknet::contract] mod DualCaseERC721Mock { use openzeppelin::introspection::src5::SRC5Component; - use openzeppelin::token::erc721::ERC721Component; + use openzeppelin::token::erc721::{ERC721Component, ERC721HooksEmptyImpl}; use starknet::ContractAddress; component!(path: ERC721Component, storage: erc721, event: ERC721Event); @@ -57,7 +57,7 @@ mod DualCaseERC721Mock { #[starknet::contract] mod SnakeERC721Mock { use openzeppelin::introspection::src5::SRC5Component; - use openzeppelin::token::erc721::ERC721Component; + use openzeppelin::token::erc721::{ERC721Component, ERC721HooksEmptyImpl}; use starknet::ContractAddress; component!(path: ERC721Component, storage: erc721, event: ERC721Event); @@ -109,7 +109,7 @@ mod SnakeERC721Mock { mod CamelERC721Mock { use openzeppelin::introspection::src5::SRC5Component; use openzeppelin::token::erc721::ERC721Component::{ERC721Impl, ERC721MetadataImpl}; - use openzeppelin::token::erc721::ERC721Component; + use openzeppelin::token::erc721::{ERC721Component, ERC721HooksEmptyImpl}; use starknet::ContractAddress; component!(path: ERC721Component, storage: erc721, event: ERC721Event); diff --git a/src/tests/presets/test_erc721.cairo b/src/tests/presets/test_erc721.cairo index 2edef2fcf..28109390d 100644 --- a/src/tests/presets/test_erc721.cairo +++ b/src/tests/presets/test_erc721.cairo @@ -269,14 +269,6 @@ fn test_approve_from_unauthorized() { dispatcher.approve(SPENDER(), TOKEN_1); } -#[test] -#[should_panic(expected: ('ERC721: approval to owner', 'ENTRYPOINT_FAILED'))] -fn test_approve_to_owner() { - let dispatcher = setup_dispatcher(); - - dispatcher.approve(OWNER(), TOKEN_1); -} - #[test] #[should_panic(expected: ('ERC721: invalid token ID', 'ENTRYPOINT_FAILED'))] fn test_approve_nonexistent() { @@ -308,20 +300,6 @@ fn test_set_approval_for_all() { assert!(is_not_approved_for_all); } -#[test] -#[should_panic(expected: ('ERC721: self approval', 'ENTRYPOINT_FAILED'))] -fn test_set_approval_for_all_owner_equal_operator_true() { - let dispatcher = setup_dispatcher(); - dispatcher.set_approval_for_all(OWNER(), true); -} - -#[test] -#[should_panic(expected: ('ERC721: self approval', 'ENTRYPOINT_FAILED'))] -fn test_set_approval_for_all_owner_equal_operator_false() { - let dispatcher = setup_dispatcher(); - dispatcher.set_approval_for_all(OWNER(), false); -} - // // transfer_from & transferFrom // diff --git a/src/tests/token/test_erc721.cairo b/src/tests/token/test_erc721.cairo index 4b3590f53..5ee9ce4cb 100644 --- a/src/tests/token/test_erc721.cairo +++ b/src/tests/token/test_erc721.cairo @@ -10,8 +10,8 @@ use openzeppelin::tests::mocks::erc721_receiver_mocks::{ }; use openzeppelin::tests::mocks::non_implementing_mock::NonImplementingMock; use openzeppelin::tests::utils::constants::{ - DATA, ZERO, OWNER, RECIPIENT, SPENDER, OPERATOR, OTHER, NAME, SYMBOL, TOKEN_ID, PUBKEY, - BASE_URI, BASE_URI_2 + DATA, ZERO, OWNER, CALLER, RECIPIENT, SPENDER, OPERATOR, OTHER, NAME, SYMBOL, TOKEN_ID, + TOKEN_ID_2, PUBKEY, BASE_URI, BASE_URI_2 }; use openzeppelin::tests::utils; use openzeppelin::token::erc721::ERC721Component::{ @@ -20,6 +20,7 @@ use openzeppelin::token::erc721::ERC721Component::{ use openzeppelin::token::erc721::ERC721Component::{Approval, ApprovalForAll, Transfer}; use openzeppelin::token::erc721::ERC721Component::{ERC721Impl, ERC721MetadataImpl, InternalImpl}; use openzeppelin::token::erc721::ERC721Component; +use openzeppelin::token::erc721::interface::IERC721; use openzeppelin::token::erc721; use openzeppelin::utils::serde::SerializedAppend; use starknet::ContractAddress; @@ -71,7 +72,7 @@ fn setup_camel_account() -> ContractAddress { // #[test] -fn test_initialize() { +fn test_initializer() { let mut state = COMPONENT_STATE(); let mock_state = CONTRACT_STATE(); @@ -156,7 +157,7 @@ fn test_get_approved() { let token_id = TOKEN_ID; assert_eq!(state.get_approved(token_id), ZERO()); - state._approve(spender, token_id); + state._approve(spender, token_id, ZERO()); assert_eq!(state.get_approved(token_id), spender); } @@ -167,34 +168,6 @@ fn test_get_approved_nonexistent() { state.get_approved(u256_from_felt252(7)); } -#[test] -fn test__exists() { - let mut state = COMPONENT_STATE(); - let token_id = TOKEN_ID; - - let not_exists = !state._exists(token_id); - assert!(not_exists); - - let mut owner = state.ERC721_owners.read(token_id); - assert!(owner.is_zero()); - - state._mint(RECIPIENT(), token_id); - - let exists = state._exists(token_id); - assert!(exists); - - owner = state.ERC721_owners.read(token_id); - assert_eq!(owner, RECIPIENT()); - - state._burn(token_id); - - let not_exists = !state._exists(token_id); - assert!(not_exists); - - owner = state.ERC721_owners.read(token_id); - assert!(owner.is_zero()); -} - // // approve & _approve // @@ -237,25 +210,33 @@ fn test_approve_from_unauthorized() { } #[test] -#[should_panic(expected: ('ERC721: approval to owner',))] -fn test_approve_to_owner() { +#[should_panic(expected: ('ERC721: invalid token ID',))] +fn test_approve_nonexistent() { + let mut state = COMPONENT_STATE(); + state.approve(SPENDER(), TOKEN_ID); +} + +#[test] +fn test__approve() { let mut state = setup(); + state._approve(SPENDER(), TOKEN_ID, ZERO()); + assert_only_event_approval(ZERO(), OWNER(), SPENDER(), TOKEN_ID); - testing::set_caller_address(OWNER()); - state.approve(OWNER(), TOKEN_ID); + let approved = state.get_approved(TOKEN_ID); + assert_eq!(approved, SPENDER()); } #[test] #[should_panic(expected: ('ERC721: invalid token ID',))] -fn test_approve_nonexistent() { +fn test__approve_nonexistent() { let mut state = COMPONENT_STATE(); - state.approve(SPENDER(), TOKEN_ID); + state._approve(SPENDER(), TOKEN_ID, ZERO()); } #[test] -fn test__approve() { +fn test__approve_auth_is_owner() { let mut state = setup(); - state._approve(SPENDER(), TOKEN_ID); + state._approve(SPENDER(), TOKEN_ID, OWNER()); assert_only_event_approval(ZERO(), OWNER(), SPENDER(), TOKEN_ID); let approved = state.get_approved(TOKEN_ID); @@ -263,17 +244,25 @@ fn test__approve() { } #[test] -#[should_panic(expected: ('ERC721: approval to owner',))] -fn test__approve_to_owner() { +fn test__approve_auth_is_approved_for_all() { let mut state = setup(); - state._approve(OWNER(), TOKEN_ID); + let auth = CALLER(); + testing::set_caller_address(OWNER()); + state.set_approval_for_all(auth, true); + utils::drop_event(ZERO()); + + state._approve(SPENDER(), TOKEN_ID, auth); + assert_only_event_approval(ZERO(), OWNER(), SPENDER(), TOKEN_ID); + + let approved = state.get_approved(TOKEN_ID); + assert_eq!(approved, SPENDER()); } #[test] -#[should_panic(expected: ('ERC721: invalid token ID',))] -fn test__approve_nonexistent() { - let mut state = COMPONENT_STATE(); - state._approve(SPENDER(), TOKEN_ID); +#[should_panic(expected: ('ERC721: unauthorized caller',))] +fn test__approve_auth_not_authorized() { + let mut state = setup(); + state._approve(SPENDER(), TOKEN_ID, CALLER()); } // @@ -302,19 +291,10 @@ fn test_set_approval_for_all() { } #[test] -#[should_panic(expected: ('ERC721: self approval',))] -fn test_set_approval_for_all_owner_equal_operator_true() { +#[should_panic(expected: ('ERC721: invalid operator',))] +fn test_set_approval_for_all_invalid_operator() { let mut state = COMPONENT_STATE(); - testing::set_caller_address(OWNER()); - state.set_approval_for_all(OWNER(), true); -} - -#[test] -#[should_panic(expected: ('ERC721: self approval',))] -fn test_set_approval_for_all_owner_equal_operator_false() { - let mut state = COMPONENT_STATE(); - testing::set_caller_address(OWNER()); - state.set_approval_for_all(OWNER(), false); + state.set_approval_for_all(ZERO(), true); } #[test] @@ -338,17 +318,10 @@ fn test__set_approval_for_all() { } #[test] -#[should_panic(expected: ('ERC721: self approval',))] -fn test__set_approval_for_all_owner_equal_operator_true() { +#[should_panic(expected: ('ERC721: invalid operator',))] +fn test__set_approval_for_all_invalid_operator() { let mut state = COMPONENT_STATE(); - state._set_approval_for_all(OWNER(), OWNER(), true); -} - -#[test] -#[should_panic(expected: ('ERC721: self approval',))] -fn test__set_approval_for_all_owner_equal_operator_false() { - let mut state = COMPONENT_STATE(); - state._set_approval_for_all(OWNER(), OWNER(), false); + state._set_approval_for_all(OWNER(), ZERO(), true); } // @@ -362,7 +335,7 @@ fn test_transfer_from_owner() { let owner = OWNER(); let recipient = RECIPIENT(); // set approval to check reset - state._approve(OTHER(), token_id); + state._approve(OTHER(), token_id, ZERO()); utils::drop_event(ZERO()); assert_state_before_transfer(owner, recipient, token_id); @@ -384,7 +357,7 @@ fn test_transferFrom_owner() { let owner = OWNER(); let recipient = RECIPIENT(); // set approval to check reset - state._approve(OTHER(), token_id); + state._approve(OTHER(), token_id, ZERO()); utils::drop_event(ZERO()); assert_state_before_transfer(owner, recipient, token_id); @@ -403,6 +376,7 @@ fn test_transferFrom_owner() { #[should_panic(expected: ('ERC721: invalid token ID',))] fn test_transfer_from_nonexistent() { let mut state = COMPONENT_STATE(); + testing::set_caller_address(OWNER()); state.transfer_from(ZERO(), RECIPIENT(), TOKEN_ID); } @@ -410,6 +384,7 @@ fn test_transfer_from_nonexistent() { #[should_panic(expected: ('ERC721: invalid token ID',))] fn test_transferFrom_nonexistent() { let mut state = COMPONENT_STATE(); + testing::set_caller_address(OWNER()); state.transferFrom(ZERO(), RECIPIENT(), TOKEN_ID); } @@ -762,6 +737,7 @@ fn test_safeTransferFrom_to_non_receiver() { #[should_panic(expected: ('ERC721: invalid token ID',))] fn test_safe_transfer_from_nonexistent() { let mut state = COMPONENT_STATE(); + testing::set_caller_address(OWNER()); state.safe_transfer_from(ZERO(), RECIPIENT(), TOKEN_ID, DATA(true)); } @@ -769,6 +745,7 @@ fn test_safe_transfer_from_nonexistent() { #[should_panic(expected: ('ERC721: invalid token ID',))] fn test_safeTransferFrom_nonexistent() { let mut state = COMPONENT_STATE(); + testing::set_caller_address(OWNER()); state.safeTransferFrom(ZERO(), RECIPIENT(), TOKEN_ID, DATA(true)); } @@ -1078,7 +1055,7 @@ fn test__transfer_to_zero() { } #[test] -#[should_panic(expected: ('ERC721: wrong sender',))] +#[should_panic(expected: ('ERC721: invalid sender',))] fn test__transfer_from_invalid_owner() { let mut state = setup(); state._transfer(RECIPIENT(), OWNER(), TOKEN_ID); @@ -1229,7 +1206,7 @@ fn test__safe_mint_already_exist() { fn test__burn() { let mut state = setup(); - state._approve(OTHER(), TOKEN_ID); + state._approve(OTHER(), TOKEN_ID, ZERO()); utils::drop_event(ZERO()); assert_eq!(state.owner_of(TOKEN_ID), OWNER()); @@ -1284,6 +1261,305 @@ fn test__set_base_uri() { assert_eq!(base_uri_2, BASE_URI_2()); } +// +// Internals +// + +#[test] +fn test__owner_of() { + let mut state = setup(); + let owner = state._owner_of(TOKEN_ID); + assert_eq!(owner, OWNER()); +} + +#[test] +fn test__require_owned() { + let mut state = setup(); + let owner = state._require_owned(TOKEN_ID); + assert_eq!(owner, OWNER()); +} + +#[test] +#[should_panic(expected: ('ERC721: invalid token ID',))] +fn test__require_owned_non_existent() { + let mut state = setup(); + state._require_owned(0x123); +} + +#[test] +fn test__exists() { + let mut state = COMPONENT_STATE(); + let token_id = TOKEN_ID; + + let not_exists = !state._exists(token_id); + assert!(not_exists); + + let mut owner = state.ERC721_owners.read(token_id); + assert!(owner.is_zero()); + + state._mint(RECIPIENT(), token_id); + + let exists = state._exists(token_id); + assert!(exists); + + owner = state.ERC721_owners.read(token_id); + assert_eq!(owner, RECIPIENT()); + + state._burn(token_id); + + let not_exists = !state._exists(token_id); + assert!(not_exists); + + owner = state.ERC721_owners.read(token_id); + assert!(owner.is_zero()); +} + +#[test] +fn test__approve_with_optional_event_emitting() { + let mut state = setup(); + state._approve_with_optional_event(SPENDER(), TOKEN_ID, ZERO(), true); + assert_only_event_approval(ZERO(), OWNER(), SPENDER(), TOKEN_ID); + + let approved = state.get_approved(TOKEN_ID); + assert_eq!(approved, SPENDER()); +} + +#[test] +fn test__approve_with_optional_event_not_emitting() { + let mut state = setup(); + state._approve_with_optional_event(SPENDER(), TOKEN_ID, ZERO(), false); + utils::assert_no_events_left(ZERO()); + + let approved = state.get_approved(TOKEN_ID); + assert_eq!(approved, SPENDER()); +} + +#[test] +#[should_panic(expected: ('ERC721: invalid token ID',))] +fn test__approve_with_optional_event_nonexistent_emitting() { + let mut state = COMPONENT_STATE(); + state._approve_with_optional_event(SPENDER(), TOKEN_ID, ZERO(), true); +} + +#[test] +fn test__approve_with_optional_event_nonexistent_not_emitting() { + let mut state = setup(); + state._approve_with_optional_event(SPENDER(), TOKEN_ID, ZERO(), false); + utils::assert_no_events_left(ZERO()); + + let approved = state.get_approved(TOKEN_ID); + assert_eq!(approved, SPENDER()); +} + +#[test] +fn test__approve_with_optional_event_auth_is_owner() { + let mut state = setup(); + state._approve_with_optional_event(SPENDER(), TOKEN_ID, OWNER(), false); + utils::assert_no_events_left(ZERO()); + + let approved = state.get_approved(TOKEN_ID); + assert_eq!(approved, SPENDER()); +} + +#[test] +fn test__approve_with_optional_event_auth_is_approved_for_all() { + let mut state = setup(); + let auth = CALLER(); + testing::set_caller_address(OWNER()); + state.set_approval_for_all(auth, true); + utils::drop_event(ZERO()); + + state._approve_with_optional_event(SPENDER(), TOKEN_ID, auth, false); + utils::assert_no_events_left(ZERO()); + + let approved = state.get_approved(TOKEN_ID); + assert_eq!(approved, SPENDER()); +} + +#[test] +#[should_panic(expected: ('ERC721: unauthorized caller',))] +fn test__approve_with_optional_event_auth_not_authorized() { + let mut state = setup(); + state._approve_with_optional_event(SPENDER(), TOKEN_ID, CALLER(), false); +} + +#[test] +fn test__is_authorized_owner() { + let mut state = setup(); + let authorized = state._is_authorized(OWNER(), OWNER(), TOKEN_ID); + assert!(authorized); +} + +#[test] +fn test__is_authorized_approved_for_all() { + let mut state = setup(); + + testing::set_caller_address(OWNER()); + state.set_approval_for_all(SPENDER(), true); + utils::drop_event(ZERO()); + + let authorized = state._is_authorized(OWNER(), SPENDER(), TOKEN_ID); + assert!(authorized); +} + +#[test] +fn test__is_authorized_approved() { + let mut state = setup(); + + testing::set_caller_address(OWNER()); + state.approve(SPENDER(), TOKEN_ID); + utils::drop_event(ZERO()); + + let authorized = state._is_authorized(OWNER(), SPENDER(), TOKEN_ID); + assert!(authorized); +} + +#[test] +fn test__is_authorized_not_authorized() { + let mut state = setup(); + let not_authorized = !state._is_authorized(OWNER(), CALLER(), TOKEN_ID); + assert!(not_authorized); +} + +#[test] +fn test__is_authorized_zero_address() { + let mut state = setup(); + let not_authorized = !state._is_authorized(OWNER(), ZERO(), TOKEN_ID); + assert!(not_authorized); +} + +#[test] +fn test__check_authorized_owner() { + let mut state = setup(); + state._check_authorized(OWNER(), OWNER(), TOKEN_ID); +} + +#[test] +fn test__check_authorized_approved_for_all() { + let mut state = setup(); + + testing::set_caller_address(OWNER()); + state.set_approval_for_all(SPENDER(), true); + utils::drop_event(ZERO()); + + state._check_authorized(OWNER(), SPENDER(), TOKEN_ID); +} + +#[test] +fn test__check_authorized_approved() { + let mut state = setup(); + + testing::set_caller_address(OWNER()); + state.approve(SPENDER(), TOKEN_ID); + utils::drop_event(ZERO()); + + state._check_authorized(OWNER(), SPENDER(), TOKEN_ID); +} + +#[test] +#[should_panic(expected: ('ERC721: invalid token ID',))] +fn test__check_authorized_owner_is_zero() { + let mut state = setup(); + state._check_authorized(ZERO(), OWNER(), TOKEN_ID); +} + +#[test] +#[should_panic(expected: ('ERC721: unauthorized caller',))] +fn test__check_authorized_not_authorized() { + let mut state = setup(); + state._check_authorized(OWNER(), CALLER(), TOKEN_ID); +} + +#[test] +#[should_panic(expected: ('ERC721: unauthorized caller',))] +fn test__check_authorized_zero_address() { + let mut state = setup(); + state._check_authorized(OWNER(), ZERO(), TOKEN_ID); +} + +#[test] +fn test_update_mint() { + let mut state = setup(); + + state._update(RECIPIENT(), TOKEN_ID_2, ZERO()); + assert_only_event_transfer(ZERO(), ZERO(), RECIPIENT(), TOKEN_ID_2); + + let owner = state.owner_of(TOKEN_ID_2); + assert_eq!(owner, RECIPIENT()); + + let balance = state.balance_of(RECIPIENT()); + assert_eq!(balance, 1); +} + +#[test] +fn test_update_burn() { + let mut state = setup(); + + state._update(ZERO(), TOKEN_ID, ZERO()); + assert_only_event_transfer(ZERO(), OWNER(), ZERO(), TOKEN_ID); + + let owner = state._owner_of(TOKEN_ID); + assert_eq!(owner, ZERO()); + + let balance = state.balance_of(OWNER()); + assert_eq!(balance, 0); +} + +#[test] +fn test_update_transfer() { + let mut state = setup(); + + state._update(RECIPIENT(), TOKEN_ID, ZERO()); + assert_only_event_transfer(ZERO(), OWNER(), RECIPIENT(), TOKEN_ID); + assert_state_after_transfer(OWNER(), RECIPIENT(), TOKEN_ID); +} + +#[test] +fn test_update_auth_owner() { + let mut state = setup(); + state._update(RECIPIENT(), TOKEN_ID, OWNER()); + assert_only_event_transfer(ZERO(), OWNER(), RECIPIENT(), TOKEN_ID); + assert_state_after_transfer(OWNER(), RECIPIENT(), TOKEN_ID); +} + +#[test] +fn test_update_auth_approved_for_all() { + let mut state = setup(); + testing::set_caller_address(OWNER()); + state.set_approval_for_all(OPERATOR(), true); + utils::drop_event(ZERO()); + + state._update(RECIPIENT(), TOKEN_ID, OPERATOR()); + assert_only_event_transfer(ZERO(), OWNER(), RECIPIENT(), TOKEN_ID); + assert_state_after_transfer(OWNER(), RECIPIENT(), TOKEN_ID); +} + +#[test] +fn test_update_auth_approved() { + let mut state = setup(); + testing::set_caller_address(OWNER()); + state.approve(OPERATOR(), TOKEN_ID); + utils::drop_event(ZERO()); + + state._update(RECIPIENT(), TOKEN_ID, OPERATOR()); + assert_only_event_transfer(ZERO(), OWNER(), RECIPIENT(), TOKEN_ID); + assert_state_after_transfer(OWNER(), RECIPIENT(), TOKEN_ID); +} + +#[test] +#[should_panic(expected: ('ERC721: unauthorized caller',))] +fn test_update_auth_not_approved() { + let mut state = setup(); + state._update(RECIPIENT(), TOKEN_ID, CALLER()); +} + +#[test] +#[should_panic(expected: ('ERC721: invalid token ID',))] +fn test_update_mint_auth_not_zero() { + let mut state = setup(); + state._update(RECIPIENT(), TOKEN_ID_2, CALLER()); +} + // // Helpers // diff --git a/src/token/erc721.cairo b/src/token/erc721.cairo index 823b4c561..56f9eb8a2 100644 --- a/src/token/erc721.cairo +++ b/src/token/erc721.cairo @@ -5,4 +5,7 @@ mod erc721_receiver; mod interface; use erc721::ERC721Component; +use erc721::ERC721HooksEmptyImpl; use erc721_receiver::ERC721ReceiverComponent; +use interface::ERC721ABIDispatcher; +use interface::ERC721ABIDispatcherTrait; diff --git a/src/token/erc721/erc721.cairo b/src/token/erc721/erc721.cairo index 807d7bb64..1029f84bd 100644 --- a/src/token/erc721/erc721.cairo +++ b/src/token/erc721/erc721.cairo @@ -1,6 +1,8 @@ // SPDX-License-Identifier: MIT // OpenZeppelin Contracts for Cairo v0.12.0 (token/erc721/erc721.cairo) +use starknet::ContractAddress; + /// # ERC721 Component /// /// The ERC721 component provides implementations for both the IERC721 interface @@ -74,16 +76,35 @@ mod ERC721Component { mod Errors { const INVALID_TOKEN_ID: felt252 = 'ERC721: invalid token ID'; const INVALID_ACCOUNT: felt252 = 'ERC721: invalid account'; + const INVALID_OPERATOR: felt252 = 'ERC721: invalid operator'; const UNAUTHORIZED: felt252 = 'ERC721: unauthorized caller'; - const APPROVAL_TO_OWNER: felt252 = 'ERC721: approval to owner'; - const SELF_APPROVAL: felt252 = 'ERC721: self approval'; const INVALID_RECEIVER: felt252 = 'ERC721: invalid receiver'; const ALREADY_MINTED: felt252 = 'ERC721: token already minted'; - const WRONG_SENDER: felt252 = 'ERC721: wrong sender'; + const INVALID_SENDER: felt252 = 'ERC721: invalid sender'; const SAFE_MINT_FAILED: felt252 = 'ERC721: safe mint failed'; const SAFE_TRANSFER_FAILED: felt252 = 'ERC721: safe transfer failed'; } + // + // Hooks + // + + trait ERC721HooksTrait { + fn before_update( + ref self: ComponentState, + to: ContractAddress, + token_id: u256, + auth: ContractAddress + ); + + fn after_update( + ref self: ComponentState, + to: ContractAddress, + token_id: u256, + auth: ContractAddress + ); + } + // // External // @@ -93,6 +114,7 @@ mod ERC721Component { TContractState, +HasComponent, +SRC5Component::HasComponent, + +ERC721HooksTrait, +Drop > of interface::IERC721> { /// Returns the number of NFTs owned by `account`. @@ -107,13 +129,15 @@ mod ERC721Component { /// /// - `token_id` exists. fn owner_of(self: @ComponentState, token_id: u256) -> ContractAddress { - self._owner_of(token_id) + self._require_owned(token_id) } /// Transfers ownership of `token_id` from `from` if `to` is either an account or `IERC721Receiver`. /// /// `data` is additional data, it has no specified format and it is sent in call to `to`. /// + /// WARNING: This method makes an external call to the recipient contract, which can lead to reentrancy vulnerabilities. + /// /// Requirements: /// /// - Caller is either approved or the `token_id` owner. @@ -130,14 +154,16 @@ mod ERC721Component { token_id: u256, data: Span ) { + ERC721::transfer_from(ref self, from, to, token_id); assert( - self._is_approved_or_owner(get_caller_address(), token_id), Errors::UNAUTHORIZED + _check_on_erc721_received(from, to, token_id, data), Errors::SAFE_TRANSFER_FAILED ); - self._safe_transfer(from, to, token_id, data); } /// Transfers ownership of `token_id` from `from` to `to`. /// + /// WARNING: This method may lead to the loss of tokens if `to` is not aware of the ERC721 protocol. + /// /// Requirements: /// /// - Caller is either approved or the `token_id` owner. @@ -152,10 +178,13 @@ mod ERC721Component { to: ContractAddress, token_id: u256 ) { - assert( - self._is_approved_or_owner(get_caller_address(), token_id), Errors::UNAUTHORIZED - ); - self._transfer(from, to, token_id); + assert(!to.is_zero(), Errors::INVALID_RECEIVER); + + // Setting an "auth" arguments enables the `_is_authorized` check which verifies that the token exists + // (from != 0). Therefore, it is not needed to verify that the return value is not 0 here. + let previous_owner = self._update(to, token_id, get_caller_address()); + + assert(from == previous_owner, Errors::INVALID_SENDER); } /// Change or reaffirm the approved address for an NFT. @@ -163,19 +192,11 @@ mod ERC721Component { /// Requirements: /// /// - The caller is either an approved operator or the `token_id` owner. - /// - `to` cannot be the token owner. /// - `token_id` exists. /// /// Emits an `Approval` event. fn approve(ref self: ComponentState, to: ContractAddress, token_id: u256) { - let owner = self._owner_of(token_id); - - let caller = get_caller_address(); - assert( - owner == caller || ERC721::is_approved_for_all(@self, owner, caller), - Errors::UNAUTHORIZED - ); - self._approve(to, token_id); + self._approve(to, token_id, get_caller_address()); } /// Enable or disable approval for `operator` to manage all of the @@ -183,7 +204,7 @@ mod ERC721Component { /// /// Requirements: /// - /// - `operator` cannot be the caller. + /// - `operator` is not the zero address. /// /// Emits an `Approval` event. fn set_approval_for_all( @@ -198,7 +219,7 @@ mod ERC721Component { /// /// - `token_id` exists. fn get_approved(self: @ComponentState, token_id: u256) -> ContractAddress { - assert(self._exists(token_id), Errors::INVALID_TOKEN_ID); + self._require_owned(token_id); self.ERC721_token_approvals.read(token_id) } @@ -215,6 +236,7 @@ mod ERC721Component { TContractState, +HasComponent, +SRC5Component::HasComponent, + +ERC721HooksTrait, +Drop > of interface::IERC721Metadata> { /// Returns the NFT name. @@ -234,7 +256,7 @@ mod ERC721Component { /// /// - `token_id` exists. fn token_uri(self: @ComponentState, token_id: u256) -> ByteArray { - assert(self._exists(token_id), Errors::INVALID_TOKEN_ID); + self._require_owned(token_id); let base_uri = self._base_uri(); if base_uri.len() == 0 { return ""; @@ -250,6 +272,7 @@ mod ERC721Component { TContractState, +HasComponent, +SRC5Component::HasComponent, + +ERC721HooksTrait, +Drop > of interface::IERC721CamelOnly> { fn balanceOf(self: @ComponentState, account: ContractAddress) -> u256 { @@ -302,6 +325,7 @@ mod ERC721Component { TContractState, +HasComponent, +SRC5Component::HasComponent, + +ERC721HooksTrait, +Drop > of interface::IERC721MetadataCamelOnly> { fn tokenURI(self: @ComponentState, tokenId: u256) -> ByteArray { @@ -318,6 +342,7 @@ mod ERC721Component { TContractState, +HasComponent, impl SRC5: SRC5Component::HasComponent, + impl Hooks: ERC721HooksTrait, +Drop > of InternalTrait { /// Initializes the contract by setting the token name, symbol, and base URI. @@ -337,55 +362,76 @@ mod ERC721Component { src5_component.register_interface(interface::IERC721_METADATA_ID); } + /// Returns the owner address of `token_id`. + fn _owner_of(self: @ComponentState, token_id: u256) -> ContractAddress { + self.ERC721_owners.read(token_id) + } + /// Returns the owner address of `token_id`. /// /// Requirements: /// /// - `token_id` exists. - fn _owner_of(self: @ComponentState, token_id: u256) -> ContractAddress { - let owner = self.ERC721_owners.read(token_id); - match owner.is_zero() { - bool::False(()) => owner, - bool::True(()) => panic_with_felt252(Errors::INVALID_TOKEN_ID) - } + fn _require_owned( + self: @ComponentState, token_id: u256 + ) -> ContractAddress { + let owner = self._owner_of(token_id); + assert(!owner.is_zero(), Errors::INVALID_TOKEN_ID); + owner } /// Returns whether `token_id` exists. fn _exists(self: @ComponentState, token_id: u256) -> bool { - !self.ERC721_owners.read(token_id).is_zero() + !self._owner_of(token_id).is_zero() } - /// Returns whether `spender` is allowed to manage `token_id`. + /// Approve `to` to operate on `token_id` /// - /// Requirements: + /// The `auth` argument is optional. If the value passed is non-zero, then this function will check that `auth` is + /// either the owner of the token, or approved to operate on all tokens held by this owner. /// - /// - `token_id` exists. - fn _is_approved_or_owner( - self: @ComponentState, spender: ContractAddress, token_id: u256 - ) -> bool { - let owner = self._owner_of(token_id); - let is_approved_for_all = ERC721::is_approved_for_all(self, owner, spender); - owner == spender - || is_approved_for_all - || spender == ERC721::get_approved(self, token_id) + /// Emits an `Approval` event. + fn _approve( + ref self: ComponentState, + to: ContractAddress, + token_id: u256, + auth: ContractAddress + ) { + self._approve_with_optional_event(to, token_id, auth, true); } - /// Changes or reaffirms the approved address for an NFT. + /// Variant of `_approve` with an optional flag to enable or disable the `Approval` event. The event is not + /// emitted in the context of transfers. /// - /// Internal function without access restriction. + /// WARNING: If `auth` is zero and `emit_event` is false, this function will not check that the token exists. /// /// Requirements: /// - /// - `token_id` exists. - /// - `to` is not the current token owner. + /// - If `auth` is non-zero, it must be either the owner of the token or approved to + /// operate on all of its tokens. /// - /// Emits an `Approval` event. - fn _approve(ref self: ComponentState, to: ContractAddress, token_id: u256) { - let owner = self._owner_of(token_id); - assert(owner != to, Errors::APPROVAL_TO_OWNER); + /// May emit an `Approval` event. + fn _approve_with_optional_event( + ref self: ComponentState, + to: ContractAddress, + token_id: u256, + auth: ContractAddress, + emit_event: bool + ) { + if emit_event || !auth.is_zero() { + let owner = self._require_owned(token_id); + + if !auth.is_zero() && owner != auth { + let is_approved_for_all = ERC721::is_approved_for_all(@self, owner, auth); + assert(is_approved_for_all, Errors::UNAUTHORIZED); + } + + if emit_event { + self.emit(Approval { owner, approved: to, token_id }); + } + } self.ERC721_token_approvals.write(token_id, to); - self.emit(Approval { owner, approved: to, token_id }); } /// Enables or disables approval for `operator` to manage @@ -393,7 +439,7 @@ mod ERC721Component { /// /// Requirements: /// - /// - `operator` cannot be the caller. + /// - `operator` is not the zero address. /// /// Emits an `Approval` event. fn _set_approval_for_all( @@ -402,7 +448,7 @@ mod ERC721Component { operator: ContractAddress, approved: bool ) { - assert(owner != operator, Errors::SELF_APPROVAL); + assert(!operator.is_zero(), Errors::INVALID_OPERATOR); self.ERC721_operator_approvals.write((owner, operator), approved); self.emit(ApprovalForAll { owner, operator, approved }); } @@ -410,6 +456,8 @@ mod ERC721Component { /// Mints `token_id` and transfers it to `to`. /// Internal function without access restriction. /// + /// WARNING: This method may lead to the loss of tokens if `to` is not aware of the ERC721 protocol. + /// /// Requirements: /// /// - `to` is not the zero address. @@ -418,18 +466,18 @@ mod ERC721Component { /// Emits a `Transfer` event. fn _mint(ref self: ComponentState, to: ContractAddress, token_id: u256) { assert(!to.is_zero(), Errors::INVALID_RECEIVER); - assert(!self._exists(token_id), Errors::ALREADY_MINTED); - self.ERC721_balances.write(to, self.ERC721_balances.read(to) + 1); - self.ERC721_owners.write(token_id, to); + let previous_owner = self._update(to, token_id, Zeroable::zero()); - self.emit(Transfer { from: Zeroable::zero(), to, token_id }); + assert(previous_owner.is_zero(), Errors::ALREADY_MINTED); } /// Transfers `token_id` from `from` to `to`. /// /// Internal function without access restriction. /// + /// WARNING: This method may lead to the loss of tokens if `to` is not aware of the ERC721 protocol. + /// /// Requirements: /// /// - `to` is not the zero address. @@ -444,17 +492,11 @@ mod ERC721Component { token_id: u256 ) { assert(!to.is_zero(), Errors::INVALID_RECEIVER); - let owner = self._owner_of(token_id); - assert(from == owner, Errors::WRONG_SENDER); - // Implicit clear approvals, no need to emit an event - self.ERC721_token_approvals.write(token_id, Zeroable::zero()); + let previous_owner = self._update(to, token_id, Zeroable::zero()); - self.ERC721_balances.write(from, self.ERC721_balances.read(from) - 1); - self.ERC721_balances.write(to, self.ERC721_balances.read(to) + 1); - self.ERC721_owners.write(token_id, to); - - self.emit(Transfer { from, to, token_id }); + assert(!previous_owner.is_zero(), Errors::INVALID_TOKEN_ID); + assert(from == previous_owner, Errors::INVALID_SENDER); } /// Destroys `token_id`. The approval is cleared when the token is burned. @@ -468,21 +510,16 @@ mod ERC721Component { /// /// Emits a `Transfer` event. fn _burn(ref self: ComponentState, token_id: u256) { - let owner = self._owner_of(token_id); - - // Implicit clear approvals, no need to emit an event - self.ERC721_token_approvals.write(token_id, Zeroable::zero()); - - self.ERC721_balances.write(owner, self.ERC721_balances.read(owner) - 1); - self.ERC721_owners.write(token_id, Zeroable::zero()); - - self.emit(Transfer { from: owner, to: Zeroable::zero(), token_id }); + let previous_owner = self._update(Zeroable::zero(), token_id, Zeroable::zero()); + assert(!previous_owner.is_zero(), Errors::INVALID_TOKEN_ID); } /// Mints `token_id` if `to` is either an account or `IERC721Receiver`. /// /// `data` is additional data, it has no specified format and it is sent in call to `to`. /// + /// WARNING: This method makes an external call to the recipient contract, which can lead to reentrancy vulnerabilities. + /// /// Requirements: /// /// - `token_id` does not exist. @@ -506,6 +543,8 @@ mod ERC721Component { /// /// `data` is additional data, it has no specified format and it is sent in call to `to`. /// + /// WARNING: This method makes an external call to the recipient contract, which can lead to reentrancy vulnerabilities. + /// /// Requirements: /// /// - `to` cannot be the zero address. @@ -539,6 +578,89 @@ mod ERC721Component { fn _base_uri(self: @ComponentState) -> ByteArray { self.ERC721_base_uri.read() } + + /// Returns whether `spender` is allowed to manage `owner`'s tokens, or `token_id` in + /// particular (ignoring whether it is owned by `owner`). + /// + /// WARNING: This function assumes that `owner` is the actual owner of `token_id` and does not verify this + /// assumption. + fn _is_authorized( + self: @ComponentState, + owner: ContractAddress, + spender: ContractAddress, + token_id: u256 + ) -> bool { + let is_approved_for_all = ERC721::is_approved_for_all(self, owner, spender); + + !spender.is_zero() + && (owner == spender + || is_approved_for_all + || spender == ERC721::get_approved(self, token_id)) + } + + /// Checks if `spender` can operate on `token_id`, assuming the provided `owner` is the actual owner. + /// + /// Requirements: + /// + /// - `owner` cannot be the zero address. + /// - `spender` cannot be the zero address. + /// - `spender` must be the owner of `token_id` or be approved to operate on it. + /// + /// WARNING: This function assumes that `owner` is the actual owner of `token_id` and does not verify this + /// assumption. + fn _check_authorized( + self: @ComponentState, + owner: ContractAddress, + spender: ContractAddress, + token_id: u256 + ) { + // Non-existent token + assert(!owner.is_zero(), Errors::INVALID_TOKEN_ID); + assert(self._is_authorized(owner, spender, token_id), Errors::UNAUTHORIZED); + } + + /// Transfers `token_id` from its current owner to `to`, or alternatively mints (or burns) if the current owner + /// (or `to`) is the zero address. Returns the owner of the `token_id` before the update. + /// + /// The `auth` argument is optional. If the value passed is non-zero, then this function will check that + /// `auth` is either the owner of the token, or approved to operate on the token (by the owner). + /// + /// Emits a `Transfer` event. + /// + /// NOTE: This function can be extended using the `ERC721HooksTrait`, to add + /// functionality before and/or after the transfer, mint, or burn. + /// + fn _update( + ref self: ComponentState, + to: ContractAddress, + token_id: u256, + auth: ContractAddress + ) -> ContractAddress { + Hooks::before_update(ref self, to, token_id, auth); + + let from = self._owner_of(token_id); + + // Perform (optional) operator check + if !auth.is_zero() { + self._check_authorized(from, auth, token_id); + } + if !from.is_zero() { + let zero_address = Zeroable::zero(); + self._approve_with_optional_event(zero_address, token_id, zero_address, false); + + self.ERC721_balances.write(from, self.ERC721_balances.read(from) - 1); + } + if !to.is_zero() { + self.ERC721_balances.write(to, self.ERC721_balances.read(to) + 1); + } + + self.ERC721_owners.write(token_id, to); + self.emit(Transfer { from, to, token_id }); + + Hooks::after_update(ref self, to, token_id, auth); + + from + } } /// Checks if `to` either is an account contract or has registered support @@ -563,6 +685,7 @@ mod ERC721Component { TContractState, +HasComponent, impl SRC5: SRC5Component::HasComponent, + +ERC721HooksTrait, +Drop > of interface::ERC721ABI> { // IERC721 @@ -584,7 +707,6 @@ mod ERC721Component { ERC721::safe_transfer_from(ref self, from, to, token_id, data); } - fn transfer_from( ref self: ComponentState, from: ContractAddress, @@ -685,3 +807,20 @@ mod ERC721Component { } } } + +/// An empty implementation of the ERC721 hooks to be used in basic ERC721 preset contracts. +impl ERC721HooksEmptyImpl of ERC721Component::ERC721HooksTrait { + fn before_update( + ref self: ERC721Component::ComponentState, + to: ContractAddress, + token_id: u256, + auth: ContractAddress + ) {} + + fn after_update( + ref self: ERC721Component::ComponentState, + to: ContractAddress, + token_id: u256, + auth: ContractAddress + ) {} +}