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

Feature: ERC721 NFT Contract #250

Open
wants to merge 24 commits into
base: dev
Choose a base branch
from
Open

Conversation

0xNeshi
Copy link
Contributor

@0xNeshi 0xNeshi commented Oct 17, 2024

Issue(s): Close #197

Description

Implement a simple ERC721 contract inspired by OZ's implementation.

Note: should replace non-maintained PR #214

Checklist

@julio4
Copy link
Contributor

julio4 commented Nov 20, 2024

@0xNeshi The repository has been upgraded to a new site generator and the Markdown format to include code listings is slightly different. The CONTRIBUTING guidelines has been updated to reflect these changes.
Could you rebase this PR onto dev. If there's any issues or if you want me to do these changes let me know.

@julio4 julio4 changed the base branch from main to dev November 20, 2024 03:33
Copy link
Contributor

@julio4 julio4 left a comment

Choose a reason for hiding this comment

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

LGTM, a few small changes to follow the IERC721 as much as possible!

{{#include ../../listings/applications/721/src/erc721.cairo}}
```

There are other implementations, such as the [Open Zeppelin](https://docs.openzeppelin.com/contracts-cairo/0.7.0/erc721) one.
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -0,0 +1,19 @@
# ERC721 Token

Contracts that follow the [ERC721 Standard](https://eips.ethereum.org/EIPS/eip-721) are called ERC721 tokens. They are used to represent non-fungible assets.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a small comment to recommend reader to actually click and read the EIP to learn about all the erc721 interface specifications

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to also add the optional ERC721Metadata an ERC721Enumerable interfaces

Comment on lines +5 to +26
pub trait IERC721<TContractState> {
fn balance_of(self: @TContractState, owner: ContractAddress) -> u256;
fn owner_of(self: @TContractState, token_id: u256) -> ContractAddress;
fn safe_transfer_from(
ref self: TContractState,
from: ContractAddress,
to: ContractAddress,
token_id: u256,
data: Span<felt252>
);
fn transfer_from(
ref self: TContractState, from: ContractAddress, to: ContractAddress, token_id: u256
);
fn approve(ref self: TContractState, approved: ContractAddress, token_id: u256);
fn set_approval_for_all(ref self: TContractState, operator: ContractAddress, approved: bool);
fn get_approved(self: @TContractState, token_id: u256) -> ContractAddress;
fn is_approved_for_all(
self: @TContractState, owner: ContractAddress, operator: ContractAddress
) -> bool;
fn mint(ref self: TContractState, to: ContractAddress, token_id: u256);
fn burn(ref self: TContractState, token_id: u256);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

mint and burn are not part of the official ERC interface, add them in a separate interface to separate the official interface and your implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Add a small comment to explain why we don't have safeTransferFrom(address _from, address _to, uint256 _tokenId) (and that the same behavior can be obtained by calling safe_transfer_from(_from, _to, _tokenId, _data) with empty _data)

Comment on lines +135 to +154
fn mint(ref self: ContractState, to: ContractAddress, token_id: u256) {
assert(!to.is_zero(), Errors::INVALID_RECEIVER);
assert(self.owners.read(token_id).is_zero(), Errors::ALREADY_MINTED);

self.balances.write(to, self.balances.read(to) + 1);
self.owners.write(token_id, to);

self.emit(Transfer { from: Zero::zero(), to, token_id });
}

fn burn(ref self: ContractState, token_id: u256) {
let owner = self._require_owned(token_id);

self.balances.write(owner, self.balances.read(owner) - 1);

self.owners.write(token_id, Zero::zero());
self.approvals.write(token_id, Zero::zero());

self.emit(Transfer { from: owner, to: Zero::zero(), token_id });
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Move these in the internal impl as _mint and _burn, and implement mint and burn in a separate interface impl block that call the internal functions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: ERC721 NFT Contract
2 participants