search

sablier-labs / v2-core

⏳ Core smart contracts of the Sablier V2 token distribution protocol
https://sablier.com
Other
290 stars 38 forks source link

Write backward compatibility tests for NFTDescriptor #881

Closed PaulRBerg closed 2 months ago

PaulRBerg commented 2 months ago

As discussed here, we will deploy a new NFTDescriptor contract for V2.2.

We will then use our admin account and call setNFTDescriptor on all V2.0 and V2.1 deployments.

It would be helpful to write a few fork tests to prove that this is a safe operation.

smol-ninja commented 2 months ago

While working on this, I came to realize the new NFTDescriptor is not compatible with v2.0 but only with v2.1. This is due to the fact the new NFTDescriptor includes isTransferable in the svg description.

https://github.com/sablier-labs/v2-core/blob/76e29fee33befaeb54a82473c28cf182e6f8855d/src/SablierV2NFTDescriptor.sol#L96

v2.0 doesn't have isTransferable.

PaulRBerg commented 2 months ago

Houston, we have a problem. Is the solution to keep using the previous NFT descriptor for V2.0?

smol-ninja commented 2 months ago

Houston, we have a problem. Is the solution to keep using the previous NFT descriptor for V2.0?

Yes! Or remove isTransferable from the NFT description but the reason behind adding it is strong enough to keep it.

Additionally, we can use a low-level call to fetch isTransferable so in the case of V2.0, it would be just blank in the description. I am a bit inclined towards this solution but wouldn't it confuse users?

smol-ninja commented 2 months ago

@PaulRBerg any comment on this?

PaulRBerg commented 2 months ago

we can use a low-level call to fetch

This is da wei!

it would be just blank in the description

It doesn't have to be blank. All V2.0 streams are transferable, so we can say that.