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

ERC721 properties #22

Merged
merged 24 commits into from
Apr 4, 2023
Merged

ERC721 properties #22

merged 24 commits into from
Apr 4, 2023

Conversation

tuturu-tech
Copy link
Contributor

  • Added 19 ERC721 properties that test for mintable, burnable, and transferrable tokens. The properties are split into external and internal to support both testing approaches.
  • Added tests for all external and internal ERC721 properties.
  • Added a README to the ERC721 folder that contains guidelines on how to run the tests as well as a list of properties that are tested.
  • Added an ERC721 section to the main README, based on the ERC20 section.
  • Added an ERC721 properties table to PROPERTIES.md.
  • Added examples to the tests folder, although they are not tested yet

The README assumes Echidna 2.1.0 is used since it uses echidna instead of echidna-test. Properties are pulled from crytic/slither#953. PR relates to #5

tuturu-tech and others added 18 commits March 28, 2023 19:02
@tuturu-tech tuturu-tech marked this pull request as ready for review April 3, 2023 11:21
ggrieco-tob
ggrieco-tob requested changes Apr 3, 2023
View reviewed changes
PROPERTIES.md Outdated Show resolved Hide resolved
@tuturu-tech tuturu-tech requested a review from ggrieco-tob April 4, 2023 08:16
ggrieco-tob
ggrieco-tob reviewed Apr 4, 2023
View reviewed changes
contracts/ERC721/external/properties/ERC721ExternalBurnableProperties.sol Outdated
assertWithMsg(false, "ownerOf didn't revert for burned token");
}

// todo burned token cannot be minted again
Copy link
Member

Choose a reason for hiding this comment

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

Remove and add a comment in corresponding issue

ggrieco-tob
ggrieco-tob reviewed Apr 4, 2023
View reviewed changes
contracts/ERC721/internal/properties/ERC721BasicProperties.sol Outdated
assertWithMsg(ownerOf(tokenId) == msg.sender, "safeTransferFrom does not revert if receiver does not implement ERC721.onERC721Received");
}

// todo test_ERC721_setApprovalForAllWorksAsExpected
Copy link
Member

Choose a reason for hiding this comment

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

Remove and add comment to corresponding issue about a new property

@ggrieco-tob ggrieco-tob merged commit 2bfa2d0 into main Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ggrieco-tob ggrieco-tob ggrieco-tob approved these changes

@glarregay-tob glarregay-tob Awaiting requested review from glarregay-tob glarregay-tob is a code owner

@bsamuels453 bsamuels453 Awaiting requested review from bsamuels453 bsamuels453 is a code owner

@montyly montyly Awaiting requested review from montyly montyly is a code owner

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

Successfully merging this pull request may close these issues.
2 participants
@tuturu-tech @ggrieco-tob