Give free will to the recipient to burn the NFT

[PaulRBerg]

As per the discussions in https://github.com/sablierhq/v2-core/discussions/129, we should refactor the contracts to give free will to the recipient to burn the NFT whenever they want. That is, we should stop deleting the NFT in the _cancel and the _withdraw functions, and move the burn logic in a separate external function that can be called by the recipient if and when they so wish to burn the NFT (but only after the stream had been deleted from the _streams mapping).

As I see it, there are two benefits of doing this:

1. It would enhance the DX of integrating Sablier, because integrators wouldn't have to handle the "NFT burned" case anymore.
2. It would make it slightly easier to index historical streams (since there would remain an on-chain reference to the stream, via the NFT), and also a bit of historical value. Anyone wants to make a bet on the floor price of future famous Sablier V2 NFTs?

[andreivladbrg]

How should we name the function?

• burn
• burnAndDelete

[PaulRBerg]

burn, definitely. Why would we name it burnAndDelete?

As I said, this function would only burn the NFT. The stream struct would still be deleted from the mapping in the _cancel and the _withdraw functions.

[andreivladbrg]

That's not what we agreed on. For safety reason we should delete the storage from both places(Sablier and ERC721 contracts) in the same function:

function burnAndDelete(streamId) external {
     // some checking logic
     delete _streams[streamId];
     _burn(streamId);
}

[andreivladbrg]

The protocol invariant: if the stream exists, the NFT exists, and if the stream doesn't exist, then the NFT doesn't exist either.

[PaulRBerg]

That "invariant" was written in the context of my not accepting to move the burn functionality in a separate function. It is completely unrelated to this.

Could you explain why is it safer to keep the delete functionality in the same function where the burn is performed?

[PaulRBerg]

I would even flip your argument and say that deleting the stream in the regular way (in _cancel and _withdraw) would be safer, because no additional interactions would be permitted against that stream. For instance, what happens if you call _cancel twice?

You will have to add another boolean in the struct (which will cost 20k gas) called isCanceled so that additional calls to _cancel are not permitted - if they were permitted, that would be a bug the size of the statue of Liberty, since users could "cancel" multiple times and drain the contract.

I suggest we stick with my proposal above, which is to only separate the burn logic, and keep the delete logic as is.

[andreivladbrg]

You will have to add another boolean in the struct (which will cost 20k gas) called isCanceled so that additional calls to _cancel are not permitted - if they were permitted, that would be a bug the size of the statue of Liberty, since users could "cancel" multiple times and drain the contract.

1. Another boolean would not cost 20k gas more because the last block from the struct is not full (256 bits).
2. We don't even need another value, you just call renounce to make it non-cancelable.

There is even the possibility to call withdraw after the cancellation, to solve this issue, we would have to update the withdrawnAmount to be equal to depositAmount.

[PaulRBerg]

Another boolean would not cost 20k gas more because the last block from the struct is not full (256 bits).

Fair.

We don't even need another value, you just call renounce to make it non-cancelable.

Yes, you could do this, but it would be a weird way of solving the problem. It wouldn't be intuitive at all. Generally speaking, it's a bad practice to use one function for multiple purposes (unless that function is purposefully made generic, but this isn't the case with renounce, which has a very specific job).

we would have to update the withdrawnAmount to be equal to depositAmount.

Could you further develop this idea, please?

[andreivladbrg]

I am in a thought process, in the end you might right with deleting the stream being the best choice.