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.

Sign up for GitHub

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

Jump to bottom

Feature: ERC721 NFT Contract #250

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

Feature: ERC721 NFT Contract #250

wants to merge 38 commits into from

Conversation

0xNeshi
Copy link
Contributor

@0xNeshi 0xNeshi commented Oct 17, 2024 •
edited
Loading

Issue(s): Close #197

Description

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

Note: should replace non-maintained PR #214

Checklist

0xNeshi added 24 commits October 16, 2024 17:58
@0xNeshi
Add ERC721 initial impl
9703d23
@0xNeshi
Rename token.cairo->erc721.cairo
41bd832
@0xNeshi
Simplify contract
cdc5a9a
@0xNeshi
Implement mint and burn
286e8c4
@0xNeshi
refactor erc721
0b1bb65
@0xNeshi
rename back contract->erc721
eda9fc9
@0xNeshi
Set up tests
446f735
@0xNeshi
Add all getter tests
7a3afc7
@0xNeshi
Return contract_address from deploy
fbb07af
@0xNeshi
Add approve tests
15fa3ae
@0xNeshi
Add transfer_from tests
dceff79
@0xNeshi
Add safe_transfer_from tests
f5ed044
@0xNeshi
Add mint tests
9e7ab1d
@0xNeshi
Add burn tests
4cb576f
@0xNeshi
Add internal tests
282fe6b
@0xNeshi
Move interfaces into interfaces.cairo + fix build errors
e5e84e9
@0xNeshi
fix tests
6bf7d66
@0xNeshi
Fix approvals update in transfer_from
69bc00a
@0xNeshi
Remove redundant build-external-contracts section from Scarb.toml
09b01b7
@0xNeshi
Move snforge_std dep to dev deps
9e79fb3
@0xNeshi
Set edition to workspace version
20b34b1
@0xNeshi
Revert edition to specific version + remove [lib]
aa3c4a2
@0xNeshi
Merge remote-tracking branch 'origin/main' into erc721
fef0cd3
@0xNeshi
Update edition to point to workspace
0650e1e
@julio4 julio4 mentioned this pull request Nov 4, 2024
Open
@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
julio4
julio4 reviewed Nov 20, 2024
View reviewed changes
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!

src/applications/erc721.md Outdated
{{#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.

Update to https://docs.openzeppelin.com/contracts-cairo/0.19.0/erc721

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to 0.20.0 as the most recent one, see https://docs.openzeppelin.com/contracts-cairo/0.20.0/erc721

Updated the link for erc20 too, see 4e9607c

src/applications/erc721.md Outdated
@@ -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 Author

Choose a reason for hiding this comment

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

b92ae56

listings/applications/erc721/src/interfaces.cairo
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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only the interfaces, no implementation?

listings/applications/erc721/src/interfaces.cairo Outdated Show resolved Hide resolved
listings/applications/erc721/src/erc721.cairo Outdated Show resolved Hide resolved
@0xNeshi
Copy link
Contributor Author

0xNeshi commented Nov 25, 2024

Thanks for the review!
Will address all change requests as I get some spare time this week

@0xNeshi 0xNeshi requested a review from julio4 December 7, 2024 15:11
@0xNeshi
Copy link
Contributor Author

0xNeshi commented Dec 7, 2024 •
edited
Loading

@julio4 addressed all the comments and redeployed the contract (updated the link in the PR description).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julio4 julio4 Awaiting requested review from julio4

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature: ERC721 NFT Contract
2 participants
@0xNeshi @julio4