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

feat: added an ERC721 NFT Contract #214

Closed
wants to merge 15 commits into from
7 changes: 7 additions & 0 deletions Scarb.lock
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,13 @@ version = "0.1.0"
name = "erc20"
version = "0.1.0"

[[package]]
name = "erc721"
version = "0.1.0"
dependencies = [
"snforge_std",
]

[[package]]
name = "errors"
version = "0.1.0"
Expand Down
1 change: 1 addition & 0 deletions listings/applications/erc721/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
target
18 changes: 18 additions & 0 deletions listings/applications/erc721/Scarb.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
[package]
name = "erc721"
version = "0.1.0"
edition = "2023_11"

# See more keys and their definitions at https://docs.swmansion.com/scarb/docs/reference/manifest.html

[dependencies]
starknet.workspace = true

[dev-dependencies]
snforge_std.workspace = true

[scripts]
test.workspace = true

[[target.starknet-contract]]
build-external-contracts = ["erc20::token::erc20"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remember to address all review comments, see #214 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please remember to address all review comments, see #214 (comment)

Alright thank you.

209 changes: 209 additions & 0 deletions listings/applications/erc721/src/erc721.cairo
Original file line number Diff line number Diff line change
@@ -0,0 +1,209 @@
use starknet::ContractAddress;

#[starknet::interface]
pub trait IERC721<TContractState> {
Copy link
Contributor

@0xNeshi 0xNeshi Jun 28, 2024

Choose a reason for hiding this comment

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

The IERC721 interface in the PR does not match the conventional one:
- IERC721 does not contain functions to get get_name, get_symbol and get_token_uri, those are part of IERC721Metadata. See the expected IERC721 interface definition here.
- The IERC721Metadata interface is missing; also, it's function names are fn name, fn symbol and fn token_uri (all without the get_ part).
- You can use OpenZeppelin's ERC721 implementation as inspiration for this PR, it will make it much easier to implement, see https://github.com/OpenZeppelin/cairo-contracts/blob/main/src/token/erc721.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, a simplified version of OpenZeppelin is really enough for this!

fn balance_of(self: @TContractState, account: ContractAddress) -> u256;
fn owner_of(self: @TContractState, token_id: u256) -> ContractAddress;
fn get_approved(self: @TContractState, token_id: u256) -> ContractAddress;
fn is_approved_for_all(
self: @TContractState, owner: ContractAddress, operator: ContractAddress
) -> bool;
fn approve(self: @TContractState, to: ContractAddress, token_id: u256);
fn set_approval_for_all(self: @TContractState, operator: ContractAddress, approved: bool);
fn transfer_from(
self: @TContractState, from: ContractAddress, to: ContractAddress, token_id: u256
);
fn mint(ref self: TContractState, to: ContractAddress, token_id: u256);
}


#[starknet::interface]
pub trait IERC721Metadata<TContractState> {
raizo07 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not resolve comments that have not been addressed.
This trait should be implemented, as per the standard.

fn name(self: @TContractState) -> felt252;
raizo07 marked this conversation as resolved.
Show resolved Hide resolved
fn symbol(self: @TContractState) -> felt252;
fn token_uri(self: @TContractState, token_id: u256) -> felt252;
Copy link
Contributor

Choose a reason for hiding this comment

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

felt252 will not be able to hold a full token URI, which will quite possibly be longer than 31 characters; that's another reason to use ByteArray instead.

}

#[starknet::contract]
mod ERC721 {
use starknet::ContractAddress;
use starknet::get_caller_address;
use core::num::traits::zero::Zero;

#[storage]
struct Storage {
name: felt252,
Copy link
Contributor

Choose a reason for hiding this comment

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

name and symbol should have the type of ByteArray

symbol: felt252,
owners: LegacyMap::<u256, ContractAddress>,
balances: LegacyMap::<ContractAddress, u256>,
token_approvals: LegacyMap::<u256, ContractAddress>,
operator_approvals: LegacyMap::<(ContractAddress, ContractAddress), bool>,
token_uri: LegacyMap::<u256, felt252>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Use ByteArray instead of felt252

}

#[event]
#[derive(Drop, starknet::Event)]
enum Event {
Approval: Approval,
Transfer: Transfer,
ApprovalForAll: ApprovalForAll
}

#[derive(Drop, starknet::Event)]
struct Approval {
owner: ContractAddress,
to: ContractAddress,
token_id: u256
}

#[derive(Drop, starknet::Event)]
struct Transfer {
from: ContractAddress,
to: ContractAddress,
token_id: u256
}

#[derive(Drop, starknet::Event)]
struct ApprovalForAll {
owner: ContractAddress,
operator: ContractAddress,
approved: bool
}

mod Errors {
// Error messages for each function
pub const ADDRESS_ZERO: felt252 = 'ERC721: invalid owner';
pub const INVALID_TOKEN_ID: felt252 = 'ERC721: invalid token ID';
pub const APPROVAL_TO_CURRENT_OWNER: felt252 = 'ERC721: approval to owner';
pub const NOT_TOKEN_OWNER: felt252 = 'ERC721: not token owner';
pub const APPROVE_TO_CALLER: felt252 = 'ERC721: approve to caller';
pub const CALLER_NOT_OWNER_NOR_APPROVED: felt252 = 'ERC721: caller is not approved';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub const CALLER_NOT_OWNER_NOR_APPROVED: felt252 = 'ERC721: caller is not approved';
pub const CALLER_NOT_APPROVED: felt252 = 'ERC721: caller is not approved';

pub const CALLER_IS_NOT_OWNER: felt252 = 'ERC721: caller is not owner';
pub const TRANSFER_TO_ZERO_ADDRESS: felt252 = 'ERC721: to the zero address';
raizo07 marked this conversation as resolved.
Show resolved Hide resolved
pub const TOKEN_ALREADY_MINTED: felt252 = 'ERC721: token already minted';
}

#[constructor]
fn constructor(ref self: ContractState, _name: felt252, _symbol: felt252) {
self.name.write(_name);
self.symbol.write(_symbol);
}

#[generate_trait]
impl IERC721Impl of IERC721Trait {
fn balance_of(self: @ContractState, account: ContractAddress) -> u256 {
assert(account.is_non_zero(), Errors::ADDRESS_ZERO);
self.balances.read(account)
}

fn owner_of(self: @ContractState, token_id: u256) -> ContractAddress {
let owner = self.owners.read(token_id);
assert(owner.is_non_zero(), Errors::INVALID_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.

The error message should state the owner is invalid

owner
}

fn get_approved(self: @ContractState, token_id: u256) -> ContractAddress {
assert(self._exists(token_id), Errors::INVALID_TOKEN_ID);
self.token_approvals.read(token_id)
}

fn is_approved_for_all(
self: @ContractState, owner: ContractAddress, operator: ContractAddress
) -> bool {
self.operator_approvals.read((owner, operator))
}

fn approve(ref self: ContractState, to: ContractAddress, token_id: u256) {
let owner = self.owner_of(token_id);
assert(to != owner, Errors::APPROVAL_TO_CURRENT_OWNER);
assert(
raizo07 marked this conversation as resolved.
Show resolved Hide resolved
get_caller_address() == owner
|| self.is_approved_for_all(owner, get_caller_address()),
Errors::NOT_TOKEN_OWNER
);
self.token_approvals.write(token_id, to);
self.emit(Approval { owner: self.owner_of(token_id), to: to, token_id: token_id });
}

fn set_approval_for_all(
ref self: ContractState, operator: ContractAddress, approved: bool
) {
let owner = get_caller_address();
assert(owner != operator, Errors::APPROVE_TO_CALLER);
raizo07 marked this conversation as resolved.
Show resolved Hide resolved
self.operator_approvals.write((owner, operator), approved);
self.emit(ApprovalForAll { owner: owner, operator: operator, approved: approved });
}

fn transfer_from(
ref self: ContractState, from: ContractAddress, to: ContractAddress, token_id: u256
) {
assert(
self._is_approved_or_owner(get_caller_address(), token_id),
Errors::CALLER_NOT_OWNER_NOR_APPROVED
);
self._transfer(from, to, token_id);
}
}

#[generate_trait]
impl ERC721HelperImpl of ERC721HelperTrait {
fn _exists(self: @ContractState, token_id: u256) -> bool {
self.owner_of(token_id).is_non_zero()
}

fn _is_approved_or_owner(
self: @ContractState, spender: ContractAddress, token_id: u256
) -> bool {
let owner = self.owners.read(token_id);
spender == owner
|| self.is_approved_for_all(owner, spender)
|| self.get_approved(token_id) == spender
}

fn _set_token_uri(ref self: ContractState, token_id: u256, token_uri: felt252) {
raizo07 marked this conversation as resolved.
Show resolved Hide resolved
assert(self._exists(token_id), Errors::INVALID_TOKEN_ID);
self.token_uri.write(token_id, token_uri)
}

fn _transfer(
ref self: ContractState, from: ContractAddress, to: ContractAddress, token_id: u256
) {
assert(from == self.owner_of(token_id), Errors::CALLER_IS_NOT_OWNER);
assert(to.is_non_zero(), Errors::TRANSFER_TO_ZERO_ADDRESS);

self.token_approvals.write(token_id, Zero::zero());

self.balances.write(from, self.balances.read(from) - 1.into());
self.balances.write(to, self.balances.read(to) + 1.into());

self.owners.write(token_id, to);

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

fn _mint(ref self: ContractState, to: ContractAddress, token_id: u256) {
assert(to.is_non_zero(), 'TO_IS_ZERO_ADDRESS');
assert(!self.owner_of(token_id).is_non_zero(), Errors::TOKEN_ALREADY_MINTED);

let receiver_balance = self.balances.read(to);
self.balances.write(to, receiver_balance + 1.into());

self.owners.write(token_id, to);

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

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

self.token_approvals.write(token_id, Zero::zero());

let owner_balance = self.balances.read(owner);
self.balances.write(owner, owner_balance - 1.into());

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

self.emit(Transfer { from: owner, to: Zero::zero(), token_id: token_id });
}
}
}
raizo07 marked this conversation as resolved.
Show resolved Hide resolved
raizo07 marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions listings/applications/erc721/src/lib.cairo
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
mod erc721;
Copy link
Contributor

@0xNeshi 0xNeshi Jul 22, 2024

Choose a reason for hiding this comment

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

Add the missing mod tests declaration, check out the official Cairo docs on how to write tests and have them actually picked up by the compiler. Then the bash scripts/cairo_programs_verifier.sh will work as expected (see #214 (comment)).

Loading