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 method to modify _operatorApprovals from derived contracts #2832

Closed
chrisdotn opened this issue Aug 19, 2021 · 1 comment · Fixed by #2834
Closed

Add method to modify _operatorApprovals from derived contracts #2832

chrisdotn opened this issue Aug 19, 2021 · 1 comment · Fixed by #2834

Comments

@chrisdotn
Copy link

🧐 Motivation
It is impossible to extend ERC-1155 in a way that the derived contract is accessing the _operatorApprovals, because the mapping is private and there is no internal method to modify it. Thus, only the ERC1155 contract itself can set or unset approvals. This makes extensions like an ERC1155Permit impossible as they need a way to modify the approval mapping. The proposal is to add an internal function, which can be used in contracts extending ERC1155 to modify _operatorApprovals.

📝 Details
Contracts extending ERC1155 need a way to modify _operatorApprovals. There are two options:

  1. Change _operatorApprovals visibility from private to internal
  2. Add an internal function to modify the mapping.

The proposal is for option two as it aligns well with ERC20, which has a similar setup: private _allowances, which can be modfied in derived contracts with _approve(owner, spender, amount).

The function to be added to ERC1155 is this:

function _setApprovalForAll(address owner, address operator, bool approved) 
   internal virtual {
  require(owner != operator, "ERC1155: setting approval status for self");
  require(owner != address(0), "ERC1155: approve from the zero address");
  require(operator != address(0), "ERC1155: approve to the zero address");

  _operatorApprovals[owner][operator] = approved;
  emit ApprovalForAll(owner, operator, approved);
}

The main difference to the existing public setApprovalForAll(address operator, bool approved) is that it allows to add approvals for other owners than msg.sender, which is necessary if we want to create an ERC1155Permit that would set approvals based on signatures.

@frangio
Copy link
Contributor

frangio commented Aug 20, 2021

I agree with this, adding the internal function. We also need an internal _setApprovalForAll for ERC721. Would be ok to include both in the same PR.

Does anyone want to submit a PR for this?

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 a pull request may close this issue.

2 participants