Should we give up on backward compatibility for the NFTDescriptor contract?

[andreivladbrg]

In the latest PR #1108, we realized that we had an incorrect string returned by the tokenURI function. This was a consequence of the package tethering and the singleton. The issue consisted of having too many "Sablier" and "Lockup" words included in the string.

One of the reasons for making this mistake was that we wanted to maintain backward compatibility, i.e. allowing the latest version of the NFTDescriptor contract to be compatible with all previous releases, even though the actual SVG was not changed (just small things). But in reality, we have never updated the previous versions of the contract with the latest, and those small changes require a non-zero effort from our side. As we release more versions, the difficulty of keeping track of backward compatibility will likely rise exponentially. For example, these lines:

v2-core/src/LockupNFTDescriptor.sol

Lines 73 to 83 in 9ef51a3

	// Retrieve the underlying token contract's address.
	if (vars.lockupModel.equal("Sablier Lockup")) {
	// For Lockup contract versions v2.0.0 and later, use the `getUnderlyingToken` function.
	vars.token = address(vars.lockup.getUnderlyingToken(streamId));
	}
	// For Lockup contract versions earlier than v2.0.0, use the `getAsset` function.
	else {
	(, bytes memory returnData) =
	address(lockup).staticcall(abi.encodeWithSignature("getAsset(uint256)", streamId));
	vars.token = abi.decode(returnData, (address));
	}

are required just to maintain compatibility with the previous wording we used for token (asset). If, in the future, we decide to rename the token to "asset" again (hypothetically—we can’t know for sure in advance), we will need to handle all such cases. This is just one example; there are more places.

Another argument is that the NFTDescriptor is at its size limit, so adding a lot of logic just for backward compatibility will reduce the potential for design improvements, as there is no more space left.

TLDR:

1. Non-zero effort to maintain backward compatibility.
2. We are not actually updating the contracts.
3. Limited flexibility to add new design features due to size constraints.

Having said that, should we give up on backward compatibility? Curious to hear what you think, @sablier-labs/solidity.

[smol-ninja]

Thank you for the discussion. I support giving up on backward compatibility based on the 2nd and 3rd points. As for the 1st point, I agree the maintenance cost isn't negligible, but this applies to most things, and in this case, the effort required is minimal (as outlined in solution below). I'd argue that the issue arises due to the testing approach used in test_WhenTokenURIDecoded.

If size were not a limiting factor, backward compatibility can offer an advantage. If we update the SVG art in the future and earlier lockup versions still hold significant TVL, we might want to refresh their NFTs look.

---

Re my proposed solution, (originally shared on Slack), if we decide to retain backward compatibility in v2.0.0:

The issue lies with this text, "This NFT represents a payment stream in a Sablier Lockup", which generates the following descriptions:

• v2.0.0: This NFT represents a payment stream in a Sablier Lockup Sablier Lockup
• Previous versions: This NFT represents a payment stream in a Sablier Lockup Sablier Lockup Linear

This redundancy occurs because mapSymbol returns strings prefixed with Sablier Lockup, which is already included in the generateDescription function's return value.

To resolve this, we can adjust the text in this line to “This NFT represents a payment stream in a”. With this change, mapSymbol can continue returning "Sablier Lockup", "Sablier Lockup Linear" etc., resulting in the following right descriptions:

• v2.0.0: This NFT represents a payment stream in a Sablier Lockup
• Previous versions: This NFT represents a payment stream in a Sablier Lockup Linear

Alternatively, we could modify the text to "This NFT represents a payment stream in a Sablier". In this case, mapSymbol should return "Lockup", "Lockup Linear" etc., which would also resolve the issue.

This change is necessary regardless, be avoid repetition of "Sablier Lockup Sablier Lockup" in v2.0.0.

---

Re test_WhenTokenURIDecoded, the current process for updating it involves hardcoding the expectedDecodedTokenURI based on the Foundry log (I suppose). If the tokenURI returns an incorrect string, we end up copying that invalid string to expectedDecodedTokenURI causing the test to pass incorrectly.

A better approach would be to independently generate the expectedDecodedTokenURI and compare it against the returned value. Alternatively, we could compare the JSON object in test_WhenTokenURIDecoded, this would enable us to clearly specify the some of the texts, though this would require further research to implement.

[PaulRBerg]

UPDATE: As discussed on Slack, feel free to drop backward compatibility.

---

Thanks @andreivladbrg.

I am in favor of maintaining backward-compatibility for now, in V2.0.0. Post this release, we can consider letting go of backward-compatibility.

The fact that we haven't updated the NFT descriptors in the previous releases is simply an error. We should do that, at least on the chains with large TVL. I will create a task on Notion about this, and I will ask @maxdesalle to submit the transactions in Safe.

Edit: it'd be better to update the NFT descriptor after the V2.0.0 release so that the string is generated correctly, and to minimize the workload for all of us.

Re my proposed solution

I'd prefer the text to say this (see my comment here):

This NFT represents a stream in a Sablier Lockup contract

And:

This NFT represents a stream in a Sablier Flow contract

A better approach would be to independently generate the expectedDecodedTokenURI

I agree with this @smol-ninja, could you please create a GitHub issue?

[andreivladbrg]

As discussed on slack, we will no longer support backwards compatibility.