search

sablier-labs / v2-core

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

Fix fork tests for NFT descriptor #893

Closed PaulRBerg closed 2 months ago

PaulRBerg commented 2 months ago

As pointed out here, the following tests are incorrect:

The tokenURIBefore should NOT equal the tokenURIAfter because the latest NFT descriptor contains the note about transferability, whereas the previous NFTDescriptor (part of the V2.1 release) used by V2.0 doesn't contain such a note.

What we have to do:

  1. Explain why the current test passes.
  2. Rewrite the test to check for an inequality instead of an equality.
  3. Decode the URI and look for the note about transferability (checking if the string contains the substring we're interested in).
PaulRBerg commented 2 months ago

Any updates on this, @smol-ninja? It's likely that this will get flagged during the audit.

smol-ninja commented 2 months ago

The tests are passing because the fuzzed streams are transferrable (since most streams are transferrable, the probability of getting a stream with non-transferability is very low).

If isTransferable is true, the description field of uri will be the "same" for v2.2, v2.1 and v2.0.

The description of v2.2 SVG only differs from v2.0 and v2.1 when isTransferable is false and that's when the test would also fail.

However, I was thinking since the goal is to check the "compatibility" of the new NFTDescriptor with previous deployments, the only necessary check should be that the call to tokenURI doesn't revert. So we should only check that it passes without to need for equality/inequality at all (we are doing that separately in tokenURI.t.sol).

What do you think about this?

PaulRBerg commented 2 months ago

Great sleuthing! You're right.

the only necessary check should be that the call to tokenURI doesn't revert

Yeah, that makes sense.

There's no expectNotRevert so we can resort to adding an explanatory comment above the final call that explains this rationale.

smol-ninja commented 2 months ago

We have expectCall. I ran some tests and the expectCall fails if the next call reverts.

PaulRBerg commented 2 months ago

Awesome, let's go with expectCall then