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
1 change: 1 addition & 0 deletions listings/applications/erc721/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
target
19 changes: 19 additions & 0 deletions listings/applications/erc721/Scarb.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
[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]
erc20 = { path = "../erc20" }
raizo07 marked this conversation as resolved.
Show resolved Hide resolved
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.

210 changes: 210 additions & 0 deletions listings/applications/erc721/src/erc721.cairo
Original file line number Diff line number Diff line change
@@ -0,0 +1,210 @@
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 get_name(self: @TContractState) -> felt252;
fn get_symbol(self: @TContractState) -> felt252;
fn get_token_uri(self: @TContractState, token_id: u256) -> felt252;
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::contract]
mod ERC721 {

raizo07 marked this conversation as resolved.
Show resolved Hide resolved
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
}

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

#[generate_trait]
impl IERC721Impl of IERC721Trait {
// Returns the name of the token collection
fn get_name(self: @ContractState) -> felt252 {
self.name.read()
}

// Returns the symbol of the token collection
fn get_symbol(self: @ContractState) -> felt252 {
self.symbol.read()
}

// Returns the metadata URI for a given token ID
fn get_token_uri(self: @ContractState, token_id: u256) -> felt252 {
assert(self._exists(token_id), 'ERC721: 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.

self.token_uri.read(token_id)
}

// Returns the number of tokens owned by a specific address
fn balance_of(self: @ContractState, account: ContractAddress) -> u256 {
assert(account.is_non_zero(), 'ERC721: address zero');
self.balances.read(account)
}

// Returns the owner of the specified token ID
fn owner_of(self: @ContractState, token_id: u256) -> ContractAddress {
let owner = self.owners.read(token_id);
assert(owner.is_non_zero(), 'ERC721: invalid token ID');
owner
}

// Returns the approved address for a specific token ID
fn get_approved(self: @ContractState, token_id: u256) -> ContractAddress {
assert(self._exists(token_id), 'ERC721: invalid token ID');
self.token_approvals.read(token_id)
}

// Checks if an operator is approved to manage all of the assets of an owner
fn is_approved_for_all(self: @ContractState, owner: ContractAddress, operator: ContractAddress) -> bool {
self.operator_approvals.read((owner, operator))
}

// Approves another address to transfer the given token ID
fn approve(ref self: ContractState, to: ContractAddress, token_id: u256) {
let owner = self.owner_of(token_id);
assert(to != owner, 'Approval to current owner');
assert(get_caller_address() == owner || self.is_approved_for_all(owner, get_caller_address()), '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 }
);
}

// Sets or unsets the approval of a given operator
fn set_approval_for_all(ref self: ContractState, operator: ContractAddress, approved: bool) {
let owner = get_caller_address();
assert(owner != operator, 'ERC721: approve to caller');
self.operator_approvals.write((owner, operator), approved);
self.emit(
ApprovalForAll{ owner: owner, operator: operator, approved: approved }
);
}

// Transfers a specific token ID to another address
fn transfer_from(ref self: ContractState, from: ContractAddress, to: ContractAddress, token_id: u256) {
assert(self._is_approved_or_owner(get_caller_address(), token_id), 'neither owner nor approved');
self._transfer(from, to, token_id);
}
}

#[generate_trait]
impl ERC721HelperImpl of ERC721HelperTrait {
// Checks if a specific token ID exists
fn _exists(self: @ContractState, token_id: u256) -> bool {
self.owner_of(token_id).is_non_zero()
}

// Checks if a spender is the owner or an approved operator of a specific token ID
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
}

// Sets the metadata URI for a specific token ID
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), 'ERC721: invalid token ID');
self.token_uri.write(token_id, token_uri)
}

// Transfers a specific token ID from one address to another
fn _transfer(ref self: ContractState, from: ContractAddress, to: ContractAddress, token_id: u256) {
assert(from == self.owner_of(token_id), 'ERC721: Caller is not owner');
assert(to.is_non_zero(), 'ERC721: transfer to 0 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 }
);
}

// Mints a new token with a specific ID to a specified address
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(), 'ERC721: 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 }
);
}

// Burns a specific token ID, removing it from existence
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)).

90 changes: 90 additions & 0 deletions listings/applications/erc721/src/tests.cairo
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
#[cfg(test)]
mod tests {
use super::*;
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 tests are written in Rust, not in Cairo.

Once you rewrite them, you should run bash scripts/cairo_programs_verifier.sh from the root directory - it will pinpoint any potential issues in your code and verify all of the tests pass (see project's Contributing guidelines).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tests are written in Rust, not in Cairo.

Once you rewrite them, you should run bash scripts/cairo_programs_verifier.sh from the root directory - it will pinpoint any potential issues in your code and verify all of the tests pass (see project's Contributing guidelines).

I ran the bash scripts/cairo_programs_verifier.sh
Screenshot 2024-07-16 at 11 39 27
command as requested and it passed as can be seen from the attached image.

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.

That is because you renamed the file from tests.cairo to test_ERC721.cairo, but didn't declare the corresponding module in lib.cairo.
Please read how to properly write tests in Cairo using the official documentation and apply that.

use starknet::testing::ContractState;
use starknet::ContractAddress;
use starknet::get_caller_address;

#[test]
fn test_constructor() {
let contract_state = ContractState::default();
let contract = ERC721Contract::constructor(&contract_state, "MyToken".into(), "MTK".into());

assert_eq!(contract.get_name(), "MyToken".into());
assert_eq!(contract.get_symbol(), "MTK".into());
}

#[test]
fn test_mint() {
let contract_state = ContractState::default();
let mut contract = ERC721Contract::constructor(&contract_state, "MyToken".into(), "MTK".into());

let to = ContractAddress::from(1);
let token_id = 1.into();

contract._mint(to, token_id);

assert_eq!(contract.owner_of(token_id), to);
assert_eq!(contract.balance_of(to), 1.into());
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.

All of the tests that test functions that emit events should assert those events have been emitted as well.

Below are the links on how to do that depending on the testing library you use:

}

#[test]
fn test_transfer() {
let contract_state = ContractState::default();
let mut contract = ERC721Contract::constructor(&contract_state, "MyToken".into(), "MTK".into());

let from = ContractAddress::from(1);
let to = ContractAddress::from(2);
let token_id = 1.into();

contract._mint(from, token_id);
contract._transfer(from, to, token_id);

assert_eq!(contract.owner_of(token_id), to);
assert_eq!(contract.balance_of(to), 1.into());
assert_eq!(contract.balance_of(from), 0.into());
}

#[test]
fn test_approve() {
let contract_state = ContractState::default();
let mut contract = ERC721Contract::constructor(&contract_state, "MyToken".into(), "MTK".into());

let owner = ContractAddress::from(1);
let approved = ContractAddress::from(2);
let token_id = 1.into();

contract._mint(owner, token_id);
contract.approve(approved, token_id);

assert_eq!(contract.get_approved(token_id), approved);
}

#[test]
fn test_set_approval_for_all() {
let contract_state = ContractState::default();
let mut contract = ERC721Contract::constructor(&contract_state, "MyToken".into(), "MTK".into());

let owner = get_caller_address();
let operator = ContractAddress::from(2);

contract.set_approval_for_all(operator, true);

assert!(contract.is_approved_for_all(owner, operator));
}

#[test]
fn test_burn() {
let contract_state = ContractState::default();
let mut contract = ERC721Contract::constructor(&contract_state, "MyToken".into(), "MTK".into());

let owner = ContractAddress::from(1);
let token_id = 1.into();

contract._mint(owner, token_id);
contract._burn(token_id);

assert_eq!(contract.owner_of(token_id).is_zero(), true);
assert_eq!(contract.balance_of(owner), 0.into());
}
}
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.

Still missing the newline character, please resolve the conversation only once the request is addressed.