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

ERC-721 extension #33

Open
wants to merge 2 commits into
base: erc-721
Choose a base branch
from
Open

ERC-721 extension #33

wants to merge 2 commits into from

Conversation

damianmarti
Copy link
Member

An ERC-721 extension to demonstrate how to integrate an ERC-721 NFT.

localhost_3000_erc721

@damianmarti damianmarti changed the base branch from main to erc-721 October 18, 2024 13:51
README.md Outdated

This repository holds all the BG curated extensions for [create-eth](https://github.com/scaffold-eth/create-eth), so you can extend the functionality of your Scaffold-ETH project.
This extension introduces an ERC-721 token contract and demonstrates how to use it, including getting the total supply and holder balance, listing all NFTs and NFTs from an address, and how to transfer.
Copy link
Contributor

@Pabl0cks Pabl0cks Oct 22, 2024

Choose a reason for hiding this comment

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

A couple of nitpicks:

  • Something looks wrong when you read this for first time: listing all NFTs and NFTs from an address
    Maybe something like listing all NFTs from the collection and NFTs from the connected address (or from the contract)

  • Maybe also change how to transfer for how to transfer a token or how to transfer NFTs.

README.md Outdated

## Usage
The ERC-721 Token Standard introduces a standard for Non-Fungible Tokens ([EIP-721](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol)), in other words, each Token is unique.
Copy link
Contributor

Choose a reason for hiding this comment

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

each Token is unique I think token can be uncapitalized there

README.md Outdated

The ERC-721 token contract is implemented using the [ERC-721 token implementation](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol) from OpenZeppelin.

The ERC-721 token implementation uses the [ERC-721 Enumerable extension](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/extensions/ERC721Enumerable.sol) from OpenZeppelin to list all tokens and all the tokens owned by an address. You can remove this if you plan to use an indexer, like a Subgraph or Ponder (extensions available).
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could add the link to the extension list from se website: (extensions available)

README.md Outdated

The ERC-721 token contract is implemented using the [ERC-721 token implementation](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol) from OpenZeppelin.

The ERC-721 token implementation uses the [ERC-721 Enumerable extension](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/extensions/ERC721Enumerable.sol) from OpenZeppelin to list all tokens and all the tokens owned by an address. You can remove this if you plan to use an indexer, like a Subgraph or Ponder (extensions available).
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add again from the contract or from the collection to the sentence from OpenZeppelin to list all tokens

README.md Outdated

The ERC-721 token implementation uses the [ERC-721 Enumerable extension](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/extensions/ERC721Enumerable.sol) from OpenZeppelin to list all tokens and all the tokens owned by an address. You can remove this if you plan to use an indexer, like a Subgraph or Ponder (extensions available).

## Installation

You can install any of the extensions in this repository by running the following command:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can get rid of this sentence, looks generic and i think the command is enough to understand


import { useEffect, useState } from "react";
import { NFTCard } from "./NFTCard";
import { useAccount } from "wagmi";
Copy link
Contributor

Choose a reason for hiding this comment

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

We can delete that

}

export const AllNfts = () => {
const [myNfts, setMyNfts] = useState<Collectible[]>([]);
Copy link
Contributor

Choose a reason for hiding this comment

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

allNfts, setAllNfts

});

useEffect(() => {
const updateMyNfts = async (): Promise<void> => {
Copy link
Contributor

Choose a reason for hiding this comment

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

updateAllNfts

name: metadata.name,
});
} catch (e) {
notification.error("Error fetching your NTFs");
Copy link
Contributor

Choose a reason for hiding this comment

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

"Error fetching NFTs"


setLoading(true);
const collectibleUpdate: Collectible[] = [];
const totalBalance = parseInt(totalSupply.toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

totalSupply or totalNfts instead of totalBalance?

return (
<>
<div className="flex justify-center items-center space-x-2 flex-col sm:flex-row">
<p className="y-2 mr-2 font-bold text-2xl">Total Supply:</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

my-2 ?

name: metadata.name,
});
} catch (e) {
notification.error("Error fetching your NTFs");
Copy link
Contributor

Choose a reason for hiding this comment

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

"your NFTs"

return (
<>
<div className="flex justify-center items-center space-x-2 flex-col sm:flex-row">
<p className="y-2 mr-2 font-bold text-2xl">Your Balance:</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

my-2?

await writeSE2TokenAsync({ functionName: "mintItem", args: [toAddress] });
setToAddress("");
} catch (e) {
console.error("Error while transfering token", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

"Error while minting token" or "Error while minting token to another address"

<div>
<p>
This extension introduces an ERC-721 token contract and demonstrates how to use it, including getting the
total supply and holder balance, listing all NFTs and NFTs from an address, and how to transfer.
Copy link
Contributor

Choose a reason for hiding this comment

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

listing all NFTs from the contract / from the collection

>
EIP-721
</a>
), in other words, each Token is unique.
Copy link
Contributor

Choose a reason for hiding this comment

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

token

ERC-721 Enumerable extension
</a>{" "}
from OpenZeppelin to list all tokens and all the tokens owned by an address. You can remove this if you
plan to use an indexer, like a Subgraph or Ponder (extensions available).
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -0,0 +1,19 @@
export const extraContents = `## 🚀 Setup ERC-721 NFT Extension

This extension introduces an ERC-721 token contract and demonstrates how to use it, including getting the total supply and holder balance, listing all NFTs and NFTs from an address, and how to transfer.
Copy link
Contributor

Choose a reason for hiding this comment

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

all NFTs from the contract / all NFTs from the collection


This extension introduces an ERC-721 token contract and demonstrates how to use it, including getting the total supply and holder balance, listing all NFTs and NFTs from an address, and how to transfer.

The ERC-721 Token Standard introduces a standard for Non-Fungible Tokens ([EIP-721](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol)), in other words, each Token is unique.
Copy link
Contributor

Choose a reason for hiding this comment

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

token

Copy link
Contributor

@Pabl0cks Pabl0cks left a comment

Choose a reason for hiding this comment

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

gj Damu! Extension is working nicely, just added some comments and nitpicks, almost all of them are just copys, messages or variable names.

<div>
<p>Below you can mint an NFT for yourself or to another address.</p>
<p>
You can see your balance and your NFTs and after that you can see the total supply and all the NFTs
Copy link
Contributor

Choose a reason for hiding this comment

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

"You can see your balance and your NFTs, and below you can see the total supply and all the NFTs"

Copy link
Contributor

Choose a reason for hiding this comment

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

My proposal is not the best probably, maybe you can just add some commas to yours 🙏

@damianmarti
Copy link
Member Author

gj Damu! Extension is working nicely, just added some comments and nitpicks, almost all of them are just copys, messages or variable names.

Thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants