ERC777 internal _burn

[MickdeGraaf]

Fixes #1908

Currently the _burn function is private which makes it hard to build tokens that deflate.
This PR makes the _burn function internal

[stale]

Hi all!
This Pull Request has not had any recent activity, is it still relevant? If so, what is blocking it? Is there anything we can do to help move it forward?
Thanks!

[frangio]

Thanks for contributting @MickdeGraaf!

My only concern with this PR is that it allows passing an arbitrary operator. This would be consistent with the current _mint function, but I feel that may have been an oversight. In my mind it would make much more sense to automatically set either this (or msg.sender?) as the operator in the internal functions. Otherwise the events emitted could be "lying". I realize this may be too much of a detail but I want us to think about it a bit before merging.

cc @nventuro

[MickdeGraaf]

@frangio Being able to set the operator manually allows for much more flexibility when extending this contract.

On another note: There are a lot of functions in ERC777 which are private but could be internal or don't have any way to be set from deriving contracts.

Setting an operator is now only possible from an external function. In my use case I want to set an operator from a contract deriving from ERC777. In the ERC20 contract things like setting allowances are possible via internal functions.

[frangio]

We deliberately restricted the internal API and kept many things private or external to give us some time to figure out the implications before committing to opening it up. If we were to design ERC20 today I'm not sure we would allow so many arbitrary things to be done internally. I'm curious to hear about your opinion and experience.

In this specific case, what is the functionality that you would like to implement? And is it authorizeOperator that you'd like to call internally?

[stale]

Hi all!
This Pull Request has not had any recent activity, is it still relevant? If so, what is blocking it? Is there anything we can do to help move it forward?
Thanks!

[frangio]

Hi @MickdeGraaf, I don't want to delay this for much longer but it would be very helpful for us to understand what you were trying to do so that we can design the best solution possible for the use case.

Hope you can share a few more details. Thanks!

[MicahZoltu]

IIUC, the fear is that the token itself is acting as an operator, but is not registered as an operator?

The canonical example to work with is a wrapper token (e.g., W-ETH) which has a deposit and withdraw function. The deposit function will call _mint(...) and the withrdaw function will call _burn. The caller of these functions will be the token owner or an operator on behalf of the owner, so I think it would be incorrect to set an operator in those cases.

One can certainly imagine a situation where the token contract calls _burn when initiated by an external contract that isn't the holder, in which case they should use operatorBurn. However, I don't think we should make this token behave incorrectly for wrappers (a fairly common situation with ERC-777) just because there is fear of someone writing a token that behaves contrary to spec.

[frangio]

The caller of these functions will be the token owner or an operator on behalf of the owner, so I think it would be incorrect to set an operator in those cases.

Who should be the operator in the events emitted in those cases?

[MicahZoltu]

For deposit/withdraw I think the event should be the same as a user transferring their own assets (which I believe means the operator is set to 0). It is the user who is taking an action (depositing or withdrawing underlying asset) which involves a mint/burn, so neither should be treated as an operator interaction.

[frangio]

@nventuro and I have been debating this and we have decided to merge this to be consistent with the existence of _mint. However, we have an intuition that the operator should always automatically be msg.sender and will probably consider providing this behavior in the future, as usual either in a major version or otherwise keeping backwards compatibility. The EIP is rather underspecified with regard to operators, so we think merging this PR is okay.

[frangio]

Thanks everyone!

[MicahZoltu]

What is the process for getting a new NPM package published to https://www.npmjs.com/package/@openzeppelin/contracts with this change?

[MicahZoltu]

Assuming the release is on some cadence other than "now", can anyone recommend a workaround for authoring a wrapper contract so I can move forward with development while waiting?

My thought is to add this as a default operator so I can call operatorBurn, but I cannot figure out how to set default operators. The ERC777 constructor takes an address[] memory defaultOperators as a cosntructor parameter, but you cannot provide dynamic memory arrays as a literal in Solidity so I'm not actually sure if it is possible to populate that constructor argument.

// Invalid type for argument in modifier invocation. Invalid implicit conversion from address[1] memory to address[] memory requested.
constructor() ERC777("My Token", "TOK", [address(this)]) public { ... }

// Explicit type conversion not allowed from "address[1] memory" to "address[] memory".
constructor() ERC777("My Token", "TOK", address[]([address(this)])) public { ... }

[nventuro]

Hm, I'm actually not sure how you could handle that one. I'd ask on the solidity-dev gitter channel.

[MicahZoltu]

I asked in Solidity Gitter (which I believe is the appropriate room for such questions) but haven't received any advice.

Is it possible that the current OZ contracts have a bug in that it is impossible to construct an ERC777 with a default operator? If so I can open a bug, I assumed you all had tested that scenario and I just didn't know what I was doing. :)

[nventuro]

You can pass a default operator, the issue you're having is that you're computing the address if said operator in the constructor itself, and Solidity is not letting you do it due to limitations of the type system. If you knew the address of the operator beforehand, you'd simply pass it as an argument to the constructor.

I did some experimenting on Remix and this seems to work:

contract ERC777 {
    address[] _operators;
    
    constructor(address[] memory operators) public {
        _operators = operators;
    }
}

contract MyContract is ERC777 {
    constructor() ERC777(toArray(address(this))) public {
        
    }
    
    function toArray(address addr) private pure returns (address[] memory) {
        address[] memory array = new address[](1);
        array[0] = addr;
        return array;
    }
}

[MicahZoltu]

I'll give that a try once back at a computer, thanks!

Though, I think I would have the same problem with a constant address as the error is with passing a dynamic array to the constructor. For example I believe (not at computer to check) that ERC777("","",[0xadd12e55...]) would yield the same error.

[nventuro]

Yes, I'd expect the same. I can't think of any solution other than the helper function that I shared above, this probably warrants discussion with the Solidity team.

[MicahZoltu]

Was this intentionally not included in the 2.4.0 release from 5 hours ago or did it somehow get missed?

[frangio]

The release was announced today but it actually happened a couple days ago. There's several things in master that didn't make it though because we had branched off 2.4 a few months ago and it had been in beta since then.

Sorry this didn't make it! 2.5 is gonna happen relatively soon anyway.

[nventuro]

That made me notice we forgot to add a changelog entry for this, I addressed that in a20408a