From 8f0f98f77206e6f45178aee958e6b08356fd586c Mon Sep 17 00:00:00 2001 From: Andrew Richardson Date: Fri, 1 Mar 2024 17:20:33 -0500 Subject: [PATCH 1/2] Update to latest OpenZeppelin base contracts Signed-off-by: Andrew Richardson --- samples/solidity/contracts/ERC20NoData.sol | 2 +- samples/solidity/contracts/ERC20WithData.sol | 2 +- samples/solidity/contracts/ERC721NoData.sol | 16 ++++++--------- samples/solidity/contracts/ERC721WithData.sol | 20 ++++++------------- samples/solidity/package-lock.json | 8 ++++---- samples/solidity/package.json | 2 +- samples/solidity/test/ERC20NoData.ts | 14 ++++++------- samples/solidity/test/ERC20WithData.ts | 6 +++--- samples/solidity/test/ERC721NoData.ts | 16 ++++++++------- samples/solidity/test/ERC721WithData.ts | 4 ++-- 10 files changed, 40 insertions(+), 50 deletions(-) diff --git a/samples/solidity/contracts/ERC20NoData.sol b/samples/solidity/contracts/ERC20NoData.sol index 35156da..4f7fa95 100644 --- a/samples/solidity/contracts/ERC20NoData.sol +++ b/samples/solidity/contracts/ERC20NoData.sol @@ -18,7 +18,7 @@ import '@openzeppelin/contracts/access/Ownable.sol'; * This is a sample only and NOT a reference implementation. */ contract ERC20NoData is Context, Ownable, ERC20, ERC20Burnable { - constructor(string memory name, string memory symbol) ERC20(name, symbol) {} + constructor(string memory name, string memory symbol) ERC20(name, symbol) Ownable(msg.sender) {} function mint(address to, uint256 amount) public onlyOwner { _mint(to, amount); diff --git a/samples/solidity/contracts/ERC20WithData.sol b/samples/solidity/contracts/ERC20WithData.sol index a641807..071ee9a 100644 --- a/samples/solidity/contracts/ERC20WithData.sol +++ b/samples/solidity/contracts/ERC20WithData.sol @@ -25,7 +25,7 @@ import './IERC20WithData.sol'; * This is a sample only and NOT a reference implementation. */ contract ERC20WithData is Context, Ownable, ERC165, ERC20, IERC20WithData { - constructor(string memory name, string memory symbol) ERC20(name, symbol) {} + constructor(string memory name, string memory symbol) ERC20(name, symbol) Ownable(msg.sender) {} function supportsInterface( bytes4 interfaceId diff --git a/samples/solidity/contracts/ERC721NoData.sol b/samples/solidity/contracts/ERC721NoData.sol index 5e2d7a6..2b9a554 100644 --- a/samples/solidity/contracts/ERC721NoData.sol +++ b/samples/solidity/contracts/ERC721NoData.sol @@ -6,7 +6,6 @@ import '@openzeppelin/contracts/token/ERC721/ERC721.sol'; import '@openzeppelin/contracts/token/ERC721/extensions/ERC721Burnable.sol'; import '@openzeppelin/contracts/utils/Context.sol'; import '@openzeppelin/contracts/access/Ownable.sol'; -import '@openzeppelin/contracts/utils/Counters.sol'; /** * Example ERC721 token with mint and burn. @@ -19,18 +18,15 @@ import '@openzeppelin/contracts/utils/Counters.sol'; * This is a sample only and NOT a reference implementation. */ contract ERC721NoData is Context, Ownable, ERC721, ERC721Burnable { - using Counters for Counters.Counter; + uint256 private _nextTokenId = 1; - Counters.Counter private _tokenIdCounter; - - constructor(string memory name, string memory symbol) ERC721(name, symbol) { - // Start counting at 1 - _tokenIdCounter.increment(); - } + constructor( + string memory name, + string memory symbol + ) ERC721(name, symbol) Ownable(msg.sender) {} function safeMint(address to) public onlyOwner { - uint256 tokenId = _tokenIdCounter.current(); - _tokenIdCounter.increment(); + uint256 tokenId = _nextTokenId++; _safeMint(to, tokenId); } diff --git a/samples/solidity/contracts/ERC721WithData.sol b/samples/solidity/contracts/ERC721WithData.sol index 51c0e73..70e71d0 100644 --- a/samples/solidity/contracts/ERC721WithData.sol +++ b/samples/solidity/contracts/ERC721WithData.sol @@ -5,7 +5,6 @@ pragma solidity ^0.8.0; import '@openzeppelin/contracts/access/Ownable.sol'; import '@openzeppelin/contracts/utils/Context.sol'; import '@openzeppelin/contracts/utils/Strings.sol'; -import '@openzeppelin/contracts/utils/Counters.sol'; import '@openzeppelin/contracts/token/ERC721/ERC721.sol'; import './IERC721WithData.sol'; @@ -25,9 +24,7 @@ import './IERC721WithData.sol'; * This is a sample only and NOT a reference implementation. */ contract ERC721WithData is Context, Ownable, ERC721, IERC721WithData { - using Counters for Counters.Counter; - - Counters.Counter private _tokenIdCounter; + uint256 private _nextTokenId = 1; string private _baseTokenURI; // Optional mapping for token URIs @@ -37,10 +34,8 @@ contract ERC721WithData is Context, Ownable, ERC721, IERC721WithData { string memory name, string memory symbol, string memory baseTokenURI - ) ERC721(name, symbol) { + ) ERC721(name, symbol) Ownable(msg.sender) { _baseTokenURI = baseTokenURI; - // Start counting at 1 - _tokenIdCounter.increment(); } function supportsInterface( @@ -52,8 +47,7 @@ contract ERC721WithData is Context, Ownable, ERC721, IERC721WithData { } function mintWithData(address to, bytes calldata data) public virtual onlyOwner { - uint256 tokenId = _tokenIdCounter.current(); - _tokenIdCounter.increment(); + uint256 tokenId = _nextTokenId++; _safeMint(to, tokenId, data); _setTokenURI(tokenId, string(abi.encodePacked(_baseURI(), Strings.toString(tokenId)))); } @@ -63,8 +57,7 @@ contract ERC721WithData is Context, Ownable, ERC721, IERC721WithData { bytes calldata data, string memory tokenURI_ ) public virtual onlyOwner { - uint256 tokenId = _tokenIdCounter.current(); - _tokenIdCounter.increment(); + uint256 tokenId = _nextTokenId++; _safeMint(to, tokenId, data); // If there is no tokenURI passed, concatenate the tokenID to the base URI @@ -112,14 +105,13 @@ contract ERC721WithData is Context, Ownable, ERC721, IERC721WithData { } function tokenURI(uint256 tokenId) public view virtual override returns (string memory) { - require(_exists(tokenId), 'ERC721WithData: Token does not exist'); - + _requireOwned(tokenId); string memory uri = _tokenURIs[tokenId]; return uri; } function _setTokenURI(uint256 tokenId, string memory _tokenURI) internal { - require(_exists(tokenId), 'ERC721WithData: Token does not exist'); + _requireOwned(tokenId); _tokenURIs[tokenId] = _tokenURI; } diff --git a/samples/solidity/package-lock.json b/samples/solidity/package-lock.json index 3916ab8..36c124d 100644 --- a/samples/solidity/package-lock.json +++ b/samples/solidity/package-lock.json @@ -9,7 +9,7 @@ "version": "1.0.0", "license": "ISC", "dependencies": { - "@openzeppelin/contracts": "^4.7.3" + "@openzeppelin/contracts": "^5.0.2" }, "devDependencies": { "@nomicfoundation/hardhat-toolbox": "^4.0.0", @@ -1494,9 +1494,9 @@ } }, "node_modules/@openzeppelin/contracts": { - "version": "4.9.6", - "resolved": "https://registry.npmjs.org/@openzeppelin/contracts/-/contracts-4.9.6.tgz", - "integrity": "sha512-xSmezSupL+y9VkHZJGDoCBpmnB2ogM13ccaYDWqJTfS3dbuHkgjuwDFUmaFauBCboQMGB/S5UqUl2y54X99BmA==" + "version": "5.0.2", + "resolved": "https://registry.npmjs.org/@openzeppelin/contracts/-/contracts-5.0.2.tgz", + "integrity": "sha512-ytPc6eLGcHHnapAZ9S+5qsdomhjo6QBHTDRRBFfTxXIpsicMhVPouPgmUPebZZZGX7vt9USA+Z+0M0dSVtSUEA==" }, "node_modules/@scure/base": { "version": "1.1.5", diff --git a/samples/solidity/package.json b/samples/solidity/package.json index 110da01..ffc7667 100644 --- a/samples/solidity/package.json +++ b/samples/solidity/package.json @@ -18,6 +18,6 @@ "typescript": "^5.3.3" }, "dependencies": { - "@openzeppelin/contracts": "^4.7.3" + "@openzeppelin/contracts": "^5.0.2" } } diff --git a/samples/solidity/test/ERC20NoData.ts b/samples/solidity/test/ERC20NoData.ts index 4f2923c..53b5da6 100644 --- a/samples/solidity/test/ERC20NoData.ts +++ b/samples/solidity/test/ERC20NoData.ts @@ -1,6 +1,6 @@ import { SignerWithAddress } from '@nomicfoundation/hardhat-ethers/signers'; -import { expect } from "chai"; -import { ethers } from "hardhat"; +import { expect } from 'chai'; +import { ethers } from 'hardhat'; import { ERC20NoData } from '../typechain-types'; describe('ERC20NoData - Unit Tests', async function () { @@ -43,9 +43,9 @@ describe('ERC20NoData - Unit Tests', async function () { it('Mint - Non-deployer of contract should not be able to mint tokens', async function () { expect(await deployedERC20NoData.balanceOf(signerB.address)).to.equal(0); // Signer B mint to Signer B (Not allowed) - await expect(deployedERC20NoData.connect(signerB).mint(signerB.address, 20)).to.be.revertedWith( - 'Ownable: caller is not the owner', - ); + await expect( + deployedERC20NoData.connect(signerB).mint(signerB.address, 20), + ).to.be.revertedWithCustomError(deployedERC20NoData, 'OwnableUnauthorizedAccount'); expect(await deployedERC20NoData.balanceOf(signerB.address)).to.equal(0); }); @@ -162,11 +162,11 @@ describe('ERC20NoData - Unit Tests', async function () { // Signer B attempts to burn tokens from Signer A wallet (not allowed) await expect( deployedERC20NoData.connect(signerB).burnFrom(deployerSignerA.address, 10), - ).to.be.revertedWith('ERC20: insufficient allowance'); + ).to.be.revertedWithCustomError(deployedERC20NoData, 'ERC20InsufficientAllowance'); // Signer A attempts to burn tokens from Signer B wallet (not allowed) await expect( deployedERC20NoData.connect(deployerSignerA).burnFrom(signerB.address, 10), - ).to.be.revertedWith('ERC20: insufficient allowance'); + ).to.be.revertedWithCustomError(deployedERC20NoData, 'ERC20InsufficientAllowance'); expect(await deployedERC20NoData.balanceOf(deployerSignerA.address)).to.equal(20); expect(await deployedERC20NoData.balanceOf(signerB.address)).to.equal(20); diff --git a/samples/solidity/test/ERC20WithData.ts b/samples/solidity/test/ERC20WithData.ts index 54a3359..bbbcf0b 100644 --- a/samples/solidity/test/ERC20WithData.ts +++ b/samples/solidity/test/ERC20WithData.ts @@ -47,7 +47,7 @@ describe('ERC20WithData - Unit Tests', async function () { // Signer B mint to Signer B (Not allowed) await expect( deployedERC20WithData.connect(signerB).mintWithData(signerB.address, 20, '0x00'), - ).to.be.revertedWith('Ownable: caller is not the owner'); + ).to.be.revertedWithCustomError(deployedERC20WithData, 'OwnableUnauthorizedAccount'); expect(await deployedERC20WithData.balanceOf(signerB.address)).to.equal(0); }); @@ -57,7 +57,7 @@ describe('ERC20WithData - Unit Tests', async function () { // Signer B mint to Signer B (Not allowed) await expect( deployedERC20WithData.connect(signerB).mintWithData(signerB.address, 20, '0x00'), - ).to.be.revertedWith('Ownable: caller is not the owner'); + ).to.be.revertedWithCustomError(deployedERC20WithData, 'OwnableUnauthorizedAccount'); expect(await deployedERC20WithData.balanceOf(signerB.address)).to.equal(0); }); @@ -142,7 +142,7 @@ describe('ERC20WithData - Unit Tests', async function () { deployedERC20WithData .connect(deployerSignerA) .transferWithData(signerB.address, signerC.address, 11, '0x00'), - ).to.be.revertedWith('ERC20: insufficient allowance'); + ).to.be.revertedWithCustomError(deployedERC20WithData, 'ERC20InsufficientAllowance'); expect(await deployedERC20WithData.balanceOf(deployerSignerA.address)).to.equal(0); expect(await deployedERC20WithData.balanceOf(signerB.address)).to.equal(20); diff --git a/samples/solidity/test/ERC721NoData.ts b/samples/solidity/test/ERC721NoData.ts index 5f6f2be..a3b1e82 100644 --- a/samples/solidity/test/ERC721NoData.ts +++ b/samples/solidity/test/ERC721NoData.ts @@ -36,7 +36,7 @@ describe('ERC721NoData - Unit Tests', async function () { // Signer B mint to Signer B (Not allowed) await expect( deployedERC721NoData.connect(signerB).safeMint(signerB.address), - ).to.be.revertedWith('Ownable: caller is not the owner'); + ).to.be.revertedWithCustomError(deployedERC721NoData, 'OwnableUnauthorizedAccount'); expect(await deployedERC721NoData.balanceOf(signerB.address)).to.equal(0); }); @@ -56,7 +56,7 @@ describe('ERC721NoData - Unit Tests', async function () { // Signer B mint token to Signer B (Not allowed) await expect( deployedERC721NoData.connect(signerB).safeMint(signerB.address), - ).to.be.revertedWith('Ownable: caller is not the owner'); + ).to.be.revertedWithCustomError(deployedERC721NoData, 'OwnableUnauthorizedAccount'); expect(await deployedERC721NoData.balanceOf(signerB.address)).to.equal(0); }); @@ -136,7 +136,7 @@ describe('ERC721NoData - Unit Tests', async function () { deployedERC721NoData .connect(deployerSignerA) ['safeTransferFrom(address,address,uint256)'](signerB.address, signerC.address, 1), - ).to.be.revertedWith('ERC721: caller is not token owner or approved'); + ).to.be.revertedWithCustomError(deployedERC721NoData, 'ERC721InsufficientApproval'); expect(await deployedERC721NoData.balanceOf(deployerSignerA.address)).to.equal(0); expect(await deployedERC721NoData.balanceOf(signerB.address)).to.equal(2); @@ -213,12 +213,14 @@ describe('ERC721NoData - Unit Tests', async function () { .to.emit(deployedERC721NoData, 'Transfer') .withArgs(ZERO_ADDRESS, signerC.address, 3); // Signer B attempts to burn token from Signer A wallet (not allowed) - await expect(deployedERC721NoData.connect(signerB).burn(1)).to.be.revertedWith( - 'ERC721: caller is not token owner or approved', + await expect(deployedERC721NoData.connect(signerB).burn(1)).to.be.revertedWithCustomError( + deployedERC721NoData, + 'ERC721InsufficientApproval', ); // Signer C attempts to burn token from Signer B wallet (not allowed) - await expect(deployedERC721NoData.connect(signerC).burn(2)).to.be.revertedWith( - 'ERC721: caller is not token owner or approved', + await expect(deployedERC721NoData.connect(signerC).burn(2)).to.be.revertedWithCustomError( + deployedERC721NoData, + 'ERC721InsufficientApproval', ); expect(await deployedERC721NoData.balanceOf(deployerSignerA.address)).to.equal(1); diff --git a/samples/solidity/test/ERC721WithData.ts b/samples/solidity/test/ERC721WithData.ts index 78e1ec1..49f0502 100644 --- a/samples/solidity/test/ERC721WithData.ts +++ b/samples/solidity/test/ERC721WithData.ts @@ -58,7 +58,7 @@ describe('ERC721WithData - Unit Tests', async function () { // Signer B mint token to Signer B (Not allowed) await expect( deployedERC721WithData.connect(signerB).mintWithData(signerB.address, '0x00'), - ).to.be.revertedWith('Ownable: caller is not the owner'); + ).to.be.revertedWithCustomError(deployedERC721WithData, 'OwnableUnauthorizedAccount'); expect(await deployedERC721WithData.balanceOf(signerB.address)).to.equal(0); }); @@ -137,7 +137,7 @@ describe('ERC721WithData - Unit Tests', async function () { deployedERC721WithData .connect(deployerSignerA) .transferWithData(signerB.address, signerC.address, 1, '0x00'), - ).to.be.revertedWith('ERC721: caller is not token owner or approved'); + ).to.be.revertedWithCustomError(deployedERC721WithData, 'ERC721InsufficientApproval'); expect(await deployedERC721WithData.balanceOf(deployerSignerA.address)).to.equal(0); expect(await deployedERC721WithData.balanceOf(signerB.address)).to.equal(2); From cb34958f81c430a52c9ba5fd2e79c2c59f80c00c Mon Sep 17 00:00:00 2001 From: Andrew Richardson Date: Mon, 4 Mar 2024 08:34:50 -0500 Subject: [PATCH 2/2] ERC721WithData should extend ERC721URIStorage This makes the contracts more consistent with OpenZeppelin examples. No signatures are changed, but there is one slight change in behavior: if you specify a "base URI", then individual token URIs are always understood to be a suffix for that base URI. The previous contract would ignore the base URI when a token URI was specified. Signed-off-by: Andrew Richardson --- samples/solidity/contracts/ERC721WithData.sol | 27 +++----------- samples/solidity/test/ERC721WithData.ts | 35 +------------------ 2 files changed, 5 insertions(+), 57 deletions(-) diff --git a/samples/solidity/contracts/ERC721WithData.sol b/samples/solidity/contracts/ERC721WithData.sol index 70e71d0..855a93f 100644 --- a/samples/solidity/contracts/ERC721WithData.sol +++ b/samples/solidity/contracts/ERC721WithData.sol @@ -5,7 +5,7 @@ pragma solidity ^0.8.0; import '@openzeppelin/contracts/access/Ownable.sol'; import '@openzeppelin/contracts/utils/Context.sol'; import '@openzeppelin/contracts/utils/Strings.sol'; -import '@openzeppelin/contracts/token/ERC721/ERC721.sol'; +import '@openzeppelin/contracts/token/ERC721/extensions/ERC721URIStorage.sol'; import './IERC721WithData.sol'; /** @@ -23,13 +23,10 @@ import './IERC721WithData.sol'; * * This is a sample only and NOT a reference implementation. */ -contract ERC721WithData is Context, Ownable, ERC721, IERC721WithData { +contract ERC721WithData is Context, Ownable, ERC721URIStorage, IERC721WithData { uint256 private _nextTokenId = 1; string private _baseTokenURI; - // Optional mapping for token URIs - mapping(uint256 => string) private _tokenURIs; - constructor( string memory name, string memory symbol, @@ -40,7 +37,7 @@ contract ERC721WithData is Context, Ownable, ERC721, IERC721WithData { function supportsInterface( bytes4 interfaceId - ) public view virtual override(ERC721, IERC165) returns (bool) { + ) public view virtual override(ERC721URIStorage, IERC165) returns (bool) { return interfaceId == type(IERC721WithData).interfaceId || super.supportsInterface(interfaceId); @@ -96,23 +93,7 @@ contract ERC721WithData is Context, Ownable, ERC721, IERC721WithData { } function _baseURI() internal view virtual override returns (string memory) { - bytes memory tempURITest = bytes(_baseTokenURI); - if (tempURITest.length == 0) { - return 'firefly://token/'; - } else { - return _baseTokenURI; - } - } - - function tokenURI(uint256 tokenId) public view virtual override returns (string memory) { - _requireOwned(tokenId); - string memory uri = _tokenURIs[tokenId]; - return uri; - } - - function _setTokenURI(uint256 tokenId, string memory _tokenURI) internal { - _requireOwned(tokenId); - _tokenURIs[tokenId] = _tokenURI; + return _baseTokenURI; } function baseTokenUri() public view virtual override returns (string memory) { diff --git a/samples/solidity/test/ERC721WithData.ts b/samples/solidity/test/ERC721WithData.ts index 49f0502..ecde9aa 100644 --- a/samples/solidity/test/ERC721WithData.ts +++ b/samples/solidity/test/ERC721WithData.ts @@ -35,7 +35,7 @@ describe('ERC721WithData - Unit Tests', async function () { it('Create - Should create a new ERC721 instance with default state', async function () { expect(await deployedERC721WithData.name()).to.equal(contractName); expect(await deployedERC721WithData.symbol()).to.equal(contractSymbol); - expect(await deployedERC721WithData.baseTokenUri()).to.equal('firefly://token/'); + expect(await deployedERC721WithData.baseTokenUri()).to.equal(''); }); it('Mint - Should mint successfully with a custom URI', async function () { @@ -73,7 +73,6 @@ describe('ERC721WithData - Unit Tests', async function () { .to.emit(deployedERC721WithData, 'Transfer') .withArgs(ZERO_ADDRESS, deployerSignerA.address, 1); expect(await deployedERC721WithData.balanceOf(deployerSignerA.address)).to.equal(1); - expect(await deployedERC721WithData.tokenURI(1)).to.equal('firefly://token/1'); // Signer A transfer token to Signer B await expect( deployedERC721WithData @@ -248,36 +247,4 @@ describe('ERC721WithData - Unit Tests', async function () { expect(await deployedERC721WithData.balanceOf(signerB.address)).to.equal(1); expect(await deployedERC721WithData.balanceOf(signerC.address)).to.equal(1); }); - - it("URI - Minted token URIs should be 'firefly://token/'", async function () { - expect(await deployedERC721WithData.balanceOf(deployerSignerA.address)).to.equal(0); - expect(await deployedERC721WithData.balanceOf(signerB.address)).to.equal(0); - expect(await deployedERC721WithData.balanceOf(signerC.address)).to.equal(0); - // Signer A mints token to itself - await expect( - deployedERC721WithData.connect(deployerSignerA).mintWithData(deployerSignerA.address, '0x00'), - ) - .to.emit(deployedERC721WithData, 'Transfer') - .withArgs(ZERO_ADDRESS, deployerSignerA.address, 1); - // Signer A mints token to Signer B - await expect( - deployedERC721WithData.connect(deployerSignerA).mintWithData(signerB.address, '0x00'), - ) - .to.emit(deployedERC721WithData, 'Transfer') - .withArgs(ZERO_ADDRESS, signerB.address, 2); - // Signer A mints token to Signer C - await expect( - deployedERC721WithData.connect(deployerSignerA).mintWithData(signerC.address, '0x00'), - ) - .to.emit(deployedERC721WithData, 'Transfer') - .withArgs(ZERO_ADDRESS, signerC.address, 3); - - expect(await deployedERC721WithData.tokenURI(1)).to.equal('firefly://token/1'); - expect(await deployedERC721WithData.tokenURI(2)).to.equal('firefly://token/2'); - expect(await deployedERC721WithData.tokenURI(3)).to.equal('firefly://token/3'); - - expect(await deployedERC721WithData.balanceOf(deployerSignerA.address)).to.equal(1); - expect(await deployedERC721WithData.balanceOf(signerB.address)).to.equal(1); - expect(await deployedERC721WithData.balanceOf(signerC.address)).to.equal(1); - }); });