Skip to content
This repository has been archived by the owner on Sep 9, 2024. It is now read-only.

Added reverts to ERC721 view functions #129

Merged
merged 7 commits into from
Sep 1, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/tokens/ERC721.huff
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@
#define macro BALANCE_OF() = takes (0) returns (0) {
NON_PAYABLE() // []
0x04 calldataload // [account]
// revert if account is zero address
dup1 continue jumpi
ZERO_ADDRESS(0x00)
continue:
[BALANCE_LOCATION] LOAD_ELEMENT_FROM_KEYS(0x00) // [balance]
0x00 mstore // []
0x20 0x00 return // []
MathisGD marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -114,6 +118,10 @@
#define macro OWNER_OF() = takes (0) returns (0) {
0x04 calldataload // [tokenId]
[OWNER_LOCATION] LOAD_ELEMENT_FROM_KEYS(0x00) // [owner]
// revert if owner is zero address/not minted
dup1 continue jumpi
NOT_MINTED(0x00)
continue:
0x00 mstore // []
0x20 0x00 return // []
MathisGD marked this conversation as resolved.
Show resolved Hide resolved
}
Expand Down
23 changes: 12 additions & 11 deletions test/tokens/ERC721.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,10 @@ contract ERC721Test is Test {
// We can't burn a token we don't have
assertEq(token.balanceOf(address(this)), 0);
assertEq(token.getApproved(1337), address(0));
assertEq(token.ownerOf(1337), address(0));

// Commenting out as this will revert due to not having ownership
// assertEq(token.ownerOf(1337), address(0));
manasbir marked this conversation as resolved.
Show resolved Hide resolved

vm.expectRevert(bytes("NOT_MINTED"));
token.burn(1337);

Expand All @@ -136,7 +139,9 @@ contract ERC721Test is Test {
token.burn(1337);
assertEq(token.balanceOf(address(this)), 0);
assertEq(token.getApproved(1337), address(0));
assertEq(token.ownerOf(1337), address(0));

// Commenting out as this will revert due to not having ownership
// assertEq(token.ownerOf(1337), address(0));
manasbir marked this conversation as resolved.
Show resolved Hide resolved

vm.expectRevert(bytes("NOT_MINTED"));
token.burn(1337);
Expand Down Expand Up @@ -459,8 +464,8 @@ contract ERC721Test is Test {
}

function testBalanceOfZeroAddress() public {
vm.expectRevert(bytes("ZERO_ADDRESS"));
uint256 bal = token.balanceOf(address(0));
assertEq(0, bal);
}

// function testFailOwnerOfUnminted() public view {
Expand All @@ -484,12 +489,8 @@ contract ERC721Test is Test {

assertEq(token.balanceOf(to), 0);

// vm.expectRevert("NOT_MINTED");
// token.ownerOf(id);

// vm.expectRevert("NOT_MINTED");
address owner = token.ownerOf(id);
assertEq(owner, address(0));
vm.expectRevert(bytes("NOT_MINTED"));
token.ownerOf(id);
}

function testApprove(address to, uint256 id) public {
Expand All @@ -512,7 +513,7 @@ contract ERC721Test is Test {
assertEq(token.balanceOf(address(this)), 0);
assertEq(token.getApproved(id), address(0));

// vm.expectRevert("NOT_MINTED");
vm.expectRevert(bytes("NOT_MINTED"));
address owner = token.ownerOf(id);
assertEq(owner, address(0));
}
Expand Down Expand Up @@ -819,7 +820,7 @@ contract ERC721Test is Test {
}

function testOwnerOfUnminted(uint256 id) public {
vm.expectRevert(bytes("NOT_MINTED"));
address owner = token.ownerOf(id);
assertEq(owner, address(0));
}
}