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

add minter to project.cairo #108

Closed
wants to merge 3 commits into from
Closed

add minter to project.cairo #108

wants to merge 3 commits into from

Conversation

od-hunter
Copy link

@od-hunter od-hunter commented Jul 1, 2024 •
edited
Loading

Closes #72

@julienbrs
Copy link
Collaborator

Can you please write tests to show everything is working fine @od-hunter?

@od-hunter
Copy link
Author

I am running into this error when I try to run the test using scarb test test_project_mint
image

@julienbrs

@od-hunter
Copy link
Author

od-hunter commented Jul 4, 2024 •
edited
Loading

Can you please write tests to show everything is working fine @od-hunter?

I’ve finished the tests sir.

tekkac
tekkac requested changes Jul 5, 2024
View reviewed changes
Copy link
Contributor

@tekkac tekkac left a comment

Choose a reason for hiding this comment

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

Needed changes

  • remove ERC721 and use OpenZeppelin ERC721.
  • remove has_minted_nft from storage and check erc721 balance during mint instead.

src/contracts/erc721.cairo
use starknet::ContractAddress;

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

Choose a reason for hiding this comment

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

erc721 should come from openzeppelin

Copy link
Author

Choose a reason for hiding this comment

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

  • The reason for using a custom ERC721 contract is because of the functionality required of the mint function, which openzeppelin ERC721 does not have:
image
  • The issue also stated that any ERC721 contract can be used:
image

@tekkac

src/contracts/project.cairo
@@ -1,8 +1,33 @@
use starknet::ContractAddress;

#[starknet::interface]
Copy link
Contributor

Choose a reason for hiding this comment

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

interface is duplicated here
also the interface is not the standard EIP one

@tekkac
Copy link
Contributor

tekkac commented Jul 29, 2024

Thanks for your contribution.
Closing this as the issue is no longer revelant for cpv3.

@tekkac tekkac closed this Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tekkac tekkac tekkac requested changes

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

Successfully merging this pull request may close these issues.

Minter of ERC721
3 participants
@od-hunter @julienbrs @tekkac