What should we do with the NFT stream after it gets canceled or fully withdrawn?

[andreivladbrg]

Now whenever a stream is cancelled or fully withdrawn, it is deleted only from the Sablier contracts storage.
https://github.com/sablierhq/v2-core/blob/c04f0ffb71678e1c82a5cb6515fa54d68dc9e994/src/SablierV2Linear.sol#L185
https://github.com/sablierhq/v2-core/blob/c04f0ffb71678e1c82a5cb6515fa54d68dc9e994/src/SablierV2Linear.sol#L298-L300

Since we inherit the ERC721 contract in the SablierV2Linear and SablierV2Pro, we also have other storages, that can be deleted via _burn function, besides our _streams mapping.

In my opinion, we should only use delete _streams[streamId] together with the _burn function, and never just one of them.

I think there are three good options:

1. Remove deletion from the _cancel and _withdraw functions, and add an external function that burns and deletes the NFT, it can be called by the recipient only after the stream is finished. This will give free will wether he wants to keep the NFT or not.
2. Delete and burn the stream always when the stream gets cancelled or fully withdrawn. This will save gas.
3. Remove deletion only from the _withdraw function, add an external function, and also call _burn from the ERC721 contract in the _cancel function.

At the moment, I don't know how the SVG NFT will work, it might depend on the Stream struct, in this case, the option to choose is option 1.

What do you think? Do you have other suggestions?

[PaulRBerg]

Thanks for opening this discussion. I agree with you that we should make it a protocol invariant that if the stream exists, the NFT exists, and if the stream doesn't exist, then the NFT doesn't exist either. In other words:

A Sablier V2 stream exists if and only if the NFT exists.

So I went with option 2 and implemented it in this commit:

29732fb

There was no need for a subroutine that performs the cancelation and the deletion. It's fine to duplicate this logic a bit, since it's only applied in two functions (_cancel and _withdraw). The subroutine would probably end up costing more gas.

[PaulRBerg]

I'm circling back on this topic, as per the latest discussions I had with @razgraf in #150.

We should re-consider whether keeping the NFT alive wouldn't actually be beneficial for third-party integrations. By not having to check for the possibility that the NFT gets burned, we might simplify the DX of integrating Sablier V2.

[PaulRBerg]

In light of #164, @razgraf do you think it would still be inconvenient to burn the NFT?

The thing is, even with the NFT alive, the integrator cannot call any function on Sablier V2 anymore. All they can do is pass around the NFT, which is worthless by that time.

[razgraf]

What do other protocols do in a similar situation? Does Uniswap burn the position NFT after withdrawing funds from it? Maybe they have a good explanation for why/why-not that would help make things clearer.

[PaulRBerg]

@andreivladbrg should know the answer to this question.

[andreivladbrg]

Uniswap has a different function for burning the NFT, i.e. my 1. option.

[PaulRBerg]

Your 1st option is slowly becoming the winner ..

[PaulRBerg]

Going to lock this discussion since we ended up separating the burn function in #201.