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

Add an internal _setApprovalForAll function (721 & 1155) #2834

Merged
merged 5 commits into from
Oct 6, 2021

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Aug 23, 2021

Fixes #2832

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@Amxx Amxx force-pushed the feature/_setApprovalForAll branch from 89fe594 to 7a1a8ae Compare August 23, 2021 13:23
@Amxx Amxx mentioned this pull request Aug 24, 2021
1 task
address operator,
bool approved
) internal virtual {
// TODO: add sanity checks?
Copy link
Contributor

Choose a reason for hiding this comment

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

What checks did you have in mind?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

possibly require(owner != operator) ... but I was not really sure

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with adding that require statement. We should move the one in the external function into the internal function.

Note: This PR is also pending a changelog entry.

@Amxx Amxx marked this pull request as ready for review October 6, 2021 07:06
frangio
frangio previously approved these changes Oct 6, 2021
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

LGTM!

@frangio frangio enabled auto-merge (squash) October 6, 2021 13:38
@frangio frangio merged commit caba6b9 into OpenZeppelin:master Oct 6, 2021
@Amxx Amxx deleted the feature/_setApprovalForAll branch October 6, 2021 15:20
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.

Add method to modify _operatorApprovals from derived contracts
2 participants