feat: added an ERC721 NFT Contract #214

raizo07 · 2024-06-04T22:23:32Z

Issue(s): #197

Description

Implemented a simple ERC721 Non-Fungible Token (NFT) contract

Checklist

Contract Tests: Added tests to cover the changes

julio4

You need to add a md page in src, detailing each methods of the interface as well.

raizo07 · 2024-06-05T07:39:03Z

You need to add a md page in src, detailing each methods of the interface as well.

Alright I'll submit a PR for that along side the test today.

0xNeshi · 2024-06-07T08:28:53Z

@raizo07 need any help with this?

raizo07 · 2024-06-07T08:34:03Z

@raizo07 need any help with this?

I'm on it already, when I'm done I'll make it a draft PR so you can help review before the main review if that's okay with you.

julio4

Some changes!

listings/applications/ERC721/ERC721.cairo

…tead of u256

raizo07 · 2024-06-18T03:34:47Z

@julio4 Kindly review this.

raizo07 · 2024-06-20T12:08:24Z

Hello @julio4 @misicnenad Any chance of you reviewing this soon?

0xNeshi

A couple of things:

You forgot to address Jules change request related to mod errors.
You are missing Scarb.toml file.
Expanding on the previous point, you should use a valid Cairo project structure, e.g. see listings/templates/scarb.
You can easily create the correct Cairo project structure using the scarb new <project-name> command (see The Cairo Book for more details).
The IERC721 interface in the PR does not match the conventional one:
- different function names
- should use u256 instead of felts and u128, as Jules said at the end of his original comment
- IERC721 does not contain functions to get name, symbol and token_uri (those are part of IERC721Metadata).
To confirm you implemented everything correctly, you could run bash scripts/cairo_programs_verifier.sh from the root directory - it will pinpoint any potential issues in your code (see project's Contributing guidelines).

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.

listings/applications/ERC721/ERC721.cairo

listings/applications/ERC721/test_erc721.rs

julio4 · 2024-06-21T11:09:04Z

A couple of things:

1. You forgot to address [Jules change request related to `mod errors`](https://github.com/NethermindEth/StarknetByExample/pull/214/files#r1637947087).

2. You are missing [Scarb.toml](https://book.cairo-lang.org/ch13-00-introduction-to-starknet-smart-contracts.html?highlight=scarb.toml#scarb) file.

3. Expanding on the previous point, you should use a valid Cairo project structure, e.g. see [listings/templates/scarb](https://github.com/NethermindEth/StarknetByExample/tree/main/listings/templates/scarb).
   You can easily create the correct Cairo project structure using the `scarb new <project-name>` command (see [The Cairo Book](https://book.cairo-lang.org/ch01-02-hello-world.html#creating-a-project-with-scarb) for more details).

4. The `IERC721` interface in the PR does not match the conventional one:
   
   * different function names
   * should use `u256` instead of felts and `u128`, as [Jules said at the end of his original comment](https://github.com/NethermindEth/StarknetByExample/pull/214#discussion_r1640884954)
   * `IERC721` does not contain functions to get `name`, `symbol` and `token_uri` (those are part of `IERC721Metadata`).

5. To confirm you implemented everything correctly, you could run `bash scripts/cairo_programs_verifier.sh` from the root directory - it will pinpoint any potential issues in your code (see project's [Contributing guidelines](https://github.com/NethermindEth/StarknetByExample/blob/main/CONTRIBUTING.md#verification-script)).

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.

Yes, this are all valid concerns outlined by @misicnenad

raizo07 · 2024-06-26T13:49:07Z

Hello @julio4 @misicnenad can you review?

raizo07 · 2024-06-28T09:29:49Z

Hello @julio4 @misicnenad can you review?

Have you been able to take a look at this? @misicnenad

0xNeshi

The PR is much better.

You may have missed some of the change requests though. Please make sure to address all of them, as it makes for much faster PR reviews.

listings/applications/erc721/Scarb.toml

listings/applications/erc721/src/tests.cairo

listings/applications/erc721/src/erc721.cairo

0xNeshi · 2024-06-28T10:07:41Z

listings/applications/erc721/src/erc721.cairo

+
+        // 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');


You should address Jules's change request related to mod errors

0xNeshi · 2024-06-28T10:10:03Z

listings/applications/erc721/src/erc721.cairo

+use starknet::ContractAddress;
+
+#[starknet::interface]
+pub trait IERC721<TContractState> {


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.

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

0xNeshi · 2024-06-28T10:11:53Z

listings/applications/erc721/src/tests.cairo

@@ -0,0 +1,90 @@
+#[cfg(test)]
+mod tests {
+    use super::*;


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).

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

command as requested and it passed as can be seen from the attached image.

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.

0xNeshi · 2024-06-28T10:16:02Z

listings/applications/erc721/src/tests.cairo

+        contract._mint(to, token_id);
+
+        assert_eq!(contract.owner_of(token_id), to);
+        assert_eq!(contract.balance_of(to), 1.into());


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:

testing events in plain Cairo: https://starknet-by-example.voyager.online/getting-started/testing/contract-testing.html#testing-events

testing events with Foundry: https://foundry-rs.github.io/starknet-foundry/testing/testing-events.html

julio4 · 2024-07-01T06:41:04Z

listings/applications/erc721/src/erc721.cairo

+use starknet::ContractAddress;
+
+#[starknet::interface]
+pub trait IERC721<TContractState> {


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

0xNeshi · 2024-07-05T07:16:53Z

listings/applications/erc721/src/tests.cairo

+
+    let new_owner = contract.owner_of(nft_id);
+    assert(new_owner == user, 'wrong new OWNER');
+}


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

listings/applications/erc721/src/tests.cairo

listings/applications/erc721/src/erc721.cairo

raizo07 · 2024-07-22T09:14:57Z

Hello @julio4 I've implemented all the necessary changes. Kindly review.

0xNeshi · 2024-07-22T10:37:36Z

listings/applications/erc721/src/test_ERC721.cairo

+#[test]
+#[available_gas(2000000)]
+fn test_erc721_burn() {


Implement the test

0xNeshi · 2024-07-22T10:39:21Z

listings/applications/erc721/src/erc721.cairo

@@ -19,9 +19,9 @@ pub trait IERC721<TContractState> {

 #[starknet::interface]
 pub trait IERC721Metadata<TContractState> {


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

0xNeshi · 2024-07-22T10:40:41Z

listings/applications/erc721/src/erc721.cairo

@@ -116,20 +117,23 @@ mod ERC721 {
        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);
+
+            // Split the combined assert into two separate asserts


Remove this comment

0xNeshi · 2024-07-22T10:41:48Z

listings/applications/erc721/src/erc721.cairo

        fn set_approval_for_all(
            ref self: ContractState, operator: ContractAddress, approved: bool
        ) {
            let owner = get_caller_address();
-            assert(owner != operator, Errors::APPROVE_TO_CALLER);
+            // assert(owner != operator, Errors::APPROVE_TO_CALLER);


Instead of commenting the unnecessary code, just remove it

0xNeshi · 2024-07-22T10:47:57Z

listings/applications/erc721/src/tests.cairo

@@ -0,0 +1,90 @@
+#[cfg(test)]
+mod tests {
+    use super::*;


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.

0xNeshi

I will give you some very frank constructive criticism:

It is frustrating to have to repeat oneself when reviewing PRs, as it is a drain on the reviewer's time.
For the last couple of PRs you've been given the same comments repeatedly:
- Implement the IERC721Metadata trait (see feat: added an ERC721 NFT Contract #214 (comment) and feat: added an ERC721 NFT Contract #214 (comment))
- Implement event emission checks in your tests (see feat: added an ERC721 NFT Contract #214 (comment))
- Set up the tests correctly for Cairo, not for Rust (related feat: added an ERC721 NFT Contract #214 (comment))
Only once you have addressed all of the change requests should you ping the reviewers to review your code.
Related to invalid tests: read the official language documentation before you attempt to submit a PR in said language. The reviewers are giving you respect by investing their time to check your code and leave feedback - the least you could do is read the docs to be able to submit valid code for review.

0xNeshi · 2024-07-22T10:48:13Z

listings/applications/erc721/src/test_ERC721.cairo

@@ -0,0 +1,217 @@
+use starknet::{ContractAddress, contract_address_const, get_caller_address};


Revert the file name to tests.cairo, see #214 (comment).
This will make all of the issues with this test file apparent to you.

0xNeshi · 2024-07-22T10:48:57Z

listings/applications/erc721/src/erc721.cairo

+
+    #[storage]
+    struct Storage {
+        name: felt252,


name and symbol should have the type of ByteArray

0xNeshi · 2024-07-22T10:49:17Z

listings/applications/erc721/src/erc721.cairo

+        balances: LegacyMap::<ContractAddress, u256>,
+        token_approvals: LegacyMap::<u256, ContractAddress>,
+        operator_approvals: LegacyMap::<(ContractAddress, ContractAddress), bool>,
+        token_uri: LegacyMap::<u256, felt252>,


Use ByteArray instead of felt252

0xNeshi · 2024-07-22T10:55:47Z

listings/applications/erc721/src/lib.cairo

@@ -0,0 +1 @@
+mod erc721;


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)).

0xNeshi · 2024-07-22T10:58:12Z

listings/applications/erc721/Scarb.toml

+test.workspace = true
+
+[[target.starknet-contract]]
+build-external-contracts = ["erc20::token::erc20"]


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

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

Alright thank you.

julio4 · 2024-08-05T04:37:49Z

@raizo07 I might have to close this PR, I understand that you put lots of effort into it and hopefully learned a lot in the process. However it is still below the quality we are expected. I hope you can understand and feel free to contribute again when you have a more solid understanding of Cairo and Starknet.

raizo07 · 2024-08-05T04:39:29Z

Hello @julio4 I'm currently resolving all the issues @misicnenad mentioned.

julio4 · 2024-08-08T04:15:19Z

Hello @julio4 I'm currently resolving all the issues @misicnenad mentioned.

I'll keep it open for a while but please take into account comments by @misicnenad, which I agree with.

feat: added an ERC721 NFT Contract

1ef0fe4

julio4 requested changes Jun 5, 2024

View reviewed changes

feat:test for ERC721

0c25f82

raizo07 added 2 commits June 10, 2024 18:04

Merge branch 'NethermindEth:main' into feat/ERC721

488abd9

code fix

bddc71c

julio4 requested changes Jun 13, 2024

View reviewed changes

listings/applications/ERC721/ERC721.cairo Outdated Show resolved Hide resolved

listings/applications/ERC721/ERC721.cairo Outdated Show resolved Hide resolved

listings/applications/ERC721/ERC721.cairo Outdated Show resolved Hide resolved

updated the IERC721 interface at the top of the fileand used u128 ins…

f30fb44

…tead of u256

0xNeshi suggested changes Jun 20, 2024

View reviewed changes

listings/applications/ERC721/ERC721.cairo Outdated Show resolved Hide resolved

listings/applications/ERC721/ERC721.cairo Outdated Show resolved Hide resolved

listings/applications/ERC721/test_erc721.rs Outdated Show resolved Hide resolved

julio4 mentioned this pull request Jun 21, 2024

Feature: Merkle Tree #223

Closed

raizo07 added 2 commits June 21, 2024 12:05

fix u256

7f8b973

fix: project structure, add Scarb.toml

3aaae72

This comment was marked as outdated.

Sign in to view

0xNeshi suggested changes Jun 28, 2024

View reviewed changes

Merge branch 'NethermindEth:main' into feat/ERC721

1dd2772

julio4 requested changes Jul 1, 2024

View reviewed changes

fix revieved changes

2b0a306

raizo07 marked this pull request as draft July 2, 2024 05:57

add mod errors

1da1af8

raizo07 marked this pull request as ready for review July 4, 2024 15:14

0xNeshi suggested changes Jul 5, 2024

View reviewed changes

fix reviewwd changes

8815769

raizo07 marked this pull request as draft July 14, 2024 11:16

raizo07 added 3 commits July 15, 2024 18:19

fixed placeholder names

b5ae02b

proper naming of test file

8afbe04

scarb fmt

aa78eb5

raizo07 marked this pull request as ready for review July 16, 2024 10:19

raizo07 requested review from 0xNeshi and julio4 July 16, 2024 10:19

0xNeshi reviewed Jul 22, 2024

View reviewed changes

0xNeshi suggested changes Jul 22, 2024

View reviewed changes

Merge branch 'NethermindEth:main' into feat/ERC721

ccbc6f5

raizo07 marked this pull request as draft August 8, 2024 04:13

0xNeshi mentioned this pull request Oct 17, 2024

Feature: ERC721 NFT Contract #250

Open

3 tasks

julio4 closed this Oct 29, 2024

		@@ -19,9 +19,9 @@ pub trait IERC721<TContractState> {

		#[starknet::interface]
		pub trait IERC721Metadata<TContractState> {

		@@ -0,0 +1,217 @@
		use starknet::{ContractAddress, contract_address_const, get_caller_address};

feat: added an ERC721 NFT Contract #214

feat: added an ERC721 NFT Contract #214

Conversation

raizo07 commented Jun 4, 2024

Description

Checklist

julio4 left a comment

Choose a reason for hiding this comment

raizo07 commented Jun 5, 2024

0xNeshi commented Jun 7, 2024

raizo07 commented Jun 7, 2024

julio4 left a comment • edited Loading

Choose a reason for hiding this comment

raizo07 commented Jun 18, 2024

raizo07 commented Jun 20, 2024

0xNeshi left a comment • edited Loading

Choose a reason for hiding this comment

julio4 commented Jun 21, 2024

raizo07 commented Jun 26, 2024 • edited Loading

raizo07 commented Jun 28, 2024

This comment was marked as outdated.

0xNeshi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xNeshi Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xNeshi Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xNeshi Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

0xNeshi Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raizo07 commented Jul 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xNeshi Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

0xNeshi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xNeshi Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julio4 commented Aug 5, 2024

raizo07 commented Aug 5, 2024

julio4 commented Aug 8, 2024

julio4 left a comment •

edited

Loading

0xNeshi left a comment •

edited

Loading

raizo07 commented Jun 26, 2024 •

edited

Loading

0xNeshi left a comment •

edited

Loading

0xNeshi Jun 28, 2024 •

edited

Loading

0xNeshi Jun 28, 2024 •

edited

Loading

0xNeshi Jul 22, 2024 •

edited

Loading

0xNeshi Jun 28, 2024 •

edited

Loading

0xNeshi Jul 22, 2024 •

edited

Loading

0xNeshi left a comment •

edited

Loading

0xNeshi Jul 22, 2024 •

edited

Loading