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 ERC721APausable as equivalent to ERC721Pausable #205

Merged
merged 7 commits into from
Mar 28, 2022
Merged

Add ERC721APausable as equivalent to ERC721Pausable #205

merged 7 commits into from
Mar 28, 2022

Conversation

johnnyshankman
Copy link
Contributor

@johnnyshankman johnnyshankman commented Mar 27, 2022 •
edited
Loading

This PR

Adds contracts/extensions/ERC721APausable.sol as an option for giving developers an easy way to swap out their OpenZeppelin ERC721Pausable inherited contract implementations with something very similar in usage.

Reference

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/extensions/ERC721Pausable.sol

Notes

Can be used alongside ERC721ABurnable without issues!

Usage

See Mock class.

ahbanavi
ahbanavi suggested changes Mar 27, 2022
View reviewed changes
Copy link
Contributor

@ahbanavi ahbanavi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM,
IMO we should use if statement with custom error instead of require. See #73
And don't forget to add docs too.

contracts/extensions/ERC721APausable.sol Outdated Show resolved Hide resolved
contracts/extensions/ERC721APausable.sol Outdated Show resolved Hide resolved
@johnnyshankman
Copy link
Contributor Author

@ahbanavi cheers, making those updates this morning.

@johnnyshankman
Copy link
Contributor Author

Adding unit tests now!

@johnnyshankman
Copy link
Contributor Author

johnnyshankman commented Mar 27, 2022 •
edited
Loading

@Vectorized fixed my formatting issues with the linter and picked up a few other fixes along the way. @ahbanavi wrote up those unit tests and added in two extra tests for checking that it composes with ERC721ABurnable correctly and pauses burns.

± |pr/205 → github-desktop-johnnyshankman/patch-1 U:1 ✗| → npm run lint

> erc721a@3.1.0 lint
> npm run lint:js && npm run lint:sol


> erc721a@3.1.0 lint:js
> eslint --ignore-path .gitignore . --fix


> erc721a@3.1.0 lint:sol
> prettier --write "contracts/**/*.sol"

contracts/ERC721A.sol 198ms
contracts/extensions/ERC721ABurnable.sol 6ms
contracts/extensions/ERC721AOwnersExplicit.sol 11ms
contracts/extensions/ERC721APausable.sol 4ms
contracts/mocks/ERC721ABurnableMock.sol 4ms
contracts/mocks/ERC721ABurnableOwnersExplicitMock.sol 4ms
contracts/mocks/ERC721ABurnableStartTokenIdMock.sol 3ms
contracts/mocks/ERC721AGasReporterMock.sol 3ms
contracts/mocks/ERC721AMock.sol 7ms
contracts/mocks/ERC721AOwnersExplicitMock.sol 3ms
contracts/mocks/ERC721AOwnersExplicitStartTokenIdMock.sol 2ms
contracts/mocks/ERC721APausableMock.sol 4ms
contracts/mocks/ERC721AStartTokenIdMock.sol 2ms
contracts/mocks/ERC721ReceiverMock.sol 4ms
contracts/mocks/StartTokenIdHelper.sol 2ms

@johnnyshankman
Revert "Little easter egg"
1260958 
This reverts commit 710cf78.
@johnnyshankman
Copy link
Contributor Author

@Vectorized thank you for all the feedback! Tests and actions are passing now. @ahbanavi lmk if there's any other updates you'd like for me to make! 😄 pleasure teamworking with y'all so far 🤝

@Vectorized Vectorized merged commit 3f47769 into chiru-labs:main Mar 28, 2022
@johnnyshankman johnnyshankman deleted the patch-1 branch March 28, 2022 01:52
ahbanavi added a commit to ahbanavi/ERC721A that referenced this pull request Mar 28, 2022
@ahbanavi
Revert "Add ERC721APausable as equivalent to ERC721Pausable (chiru-la…
d8bdfb1 
…bs#205)"

This reverts commit 3f47769.
@johnnyshankman
Copy link
Contributor Author

@ahbanavi why was this reverted?

@ahbanavi
Copy link
Contributor

ahbanavi commented Mar 28, 2022 •
edited
Loading

@ahbanavi why was this reverted?

I just having some experiments on my fork, no worries.

holic
holic reviewed Apr 11, 2022
View reviewed changes
contracts/extensions/ERC721APausable.sol
uint256 quantity
) internal virtual override {
super._beforeTokenTransfers(from, to, startTokenId, quantity);
if (paused()) revert ContractPaused();
Copy link

Choose a reason for hiding this comment

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

ooc what's the reason for this check/revert being last instead of first?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good question! I believe OZ’s implementation also has it after the super call. But I see where you are coming from.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@holic holic holic left review comments

@Vectorized Vectorized Vectorized left review comments

@ahbanavi ahbanavi ahbanavi 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.
4 participants
@johnnyshankman @ahbanavi @holic @Vectorized