Use the `IERC721Metadata` interface instead of `ISablierV2Lockup` in the NFT descriptor

[PaulRBerg]

We're passing the ISablierV2Lockup as one of the parameters of the tokenURI function in ISablierV2NFTDescriptor, but this doesn't seem like a good choice:

https://github.com/sablierhq/v2-core/blob/876456a685722b80504e4b36396819003fa0dfb7/src/interfaces/ISablierV2NftDescriptor.sol#L14

Future Sablier contracts may not inherit from ISablierV2Lockup but may still be ERC-721 NFTs. It'd be a shame if we couldn't use the same ISablierV2NFTDescriptor interface for these future contracts.

Thus, I suggest we use the IERC721Metadata instead, so that future contracts remain compatible with the descriptor interface.

[andreivladbrg]

I don't think this is a good idea. We need the getters from the ISablierV2Lockup interface to make the NFT unique. This was the whole idea of having NFTs on the chain - to create the token URI starting from the actual startTime, stopTime, and streamedAmount.

[PaulRBerg]

I would have expected a bit of appreciation for the problem situation that I have unsurfaced here, regardless of the validity of your claim (which is not valid, but even if it were, you should have acknowledged the compatibility problem).

We will be able to do this in the actual implementation (when we get to write it up in April):

function tokenURI(IERC721Metadata sablierContract, uint256 streamId) external view override returns (string memory uri) {
    ISablierV2Lockup(address(sablierContract)).getStartTime(streamId);
    // rest of the function
}

What matters is that the interface is the same so that it can be reused across all future Sablier V2 contracts. The common denominator between these V2 contracts will be IERC721Metadata.

[andreivladbrg]

I would have expected a bit of appreciation for the problem situation that I have unsurfaced here, regardless of the validity of your claim (which is not valid, but even if it were, you should have acknowledged the compatibility problem).

I'm not sure what you mean by this. Let's keep the discussion about the issue: ISablierV2Lockup vs IERC721Metadata

In that case why don't we use an address for the parameter?

ISablierV2Lockup(address(sablierContract))

Why would you prefer to overcomplicate things when we have all we need in the ISablierV2Lockup, not to add the ugly syntax instead of a nicer:

• sablierContract.getStartTime(streamId);
• sablierContract.symbol();

Plus you need to have two imports. ISablierV2Lockup it's just better to use.

[PaulRBerg]

I'm not sure what you mean by this. Let's keep the discussion about the issue:

This is very much on the topic. Rephrasing:

Even if your contention had been correct (which is not the case), you still should have acknowledged that we have a problem. Namely, using ISablierV2Lockup wouldn't work long-term because we would inevitably have to modify and rectify the ISablierV2NFTDescriptor interface when we ship a new contract.

In that case why don't we use an address for the parameter?

We could, but I think it provides better context and readability with the IERC721Metadata interface. It clearly communicates that the token URI is for an ERC-721 NFT.

Why would you prefer to overcomplicate things when have all you need in the ISablierV2Lockup, not to add the ugly syntax instead of a nicer:

See the first paragraph in this comment, plus the first comment. You did not understand the problem situation. Read carefully what I said again.

[PaulRBerg]

You can still have the "beautiful" syntax:

ISablierV2Lockup lockup = ISablierV2Lockup(address(sablierContract));
lockup.getStartTime(streamId);
lockup.symbol();

[andreivladbrg]

wouldn't work long-term because we would inevitably have to modify and rectify the ISablierV2NFTDescriptor interface when we ship a new contract.

If you mean for a different product, such as payroll, it would require a new implementation specific to that product. However, we had discussed the idea of creating a common interface for both products.

We could, but I think it provides better context and readability with the IERC721Metadata interface. It clearly communicates that the token URI is for an ERC-721 NFT.

Using an ISablierV2Lockup would be even clearer.

You can still have the "beautiful" syntax:

Why to add one more step, plus multiple imports? We can't use the getters without importing the interface.

[PaulRBerg]

If you mean for a different product

Yes, this is what I've been referring to all along by "Future Sablier contracts".

such as payroll

Let's refer to this as EVM-based open-ended streaming from now on.

we had discussed the idea of creating a common interface for both products

I don't remember this but even if we did, I am re-opening the discussion now.

Using an ISablierV2Lockup would be even clearer.

Maybe, but again, you'd have to create multiple interfaces in the future.

Why to add one more step, plus multiple imports?

Because you'd have to create multiple interfaces in the future (e.g. SablierV2OpenEndedNFTDescriptor), and rename ISablierV2NFTDescriptor to ISablierV2LockupNFTDescriptor. Let's keep just one, the one that we currently have: ISablierV2NFTDescriptor.

Importing the same interfaces multiple times is easier than creating multiple interfaces and maintaining them.

[PaulRBerg]

@andreivladbrg all clear here? If yes, can you accept #366?

[andreivladbrg]

all along by "Future Sablier contracts".

Airstream is also going to be a future contract, but it doesn't require any new descriptor.

I don't remember this

The idea was to create a common interface for both products called ISablierV2.

you'd have to create multiple interfaces in the future.

If you suggest to install the lockup repo(which is not a small one) in the open-ended repo just to import the ISablierV2NFTDescriptor I don't think it's a good idea. We would have to create a new interface in that repo:

interface ISablierV2NftDescriptor {
   function tokenURI(ISablierV2OpenEnded openEnded, uint256 streamId) external view returns (string memory uri);
}

and rename ISablierV2NFTDescriptor to ISablierV2LockupNFTDescriptor

We don't have to do that. We can keep the ISablierV2NFTDescriptor name with different parameter because "ISablierV2" can refer to both lockup and open-ended interfaces.

Let's keep just one, the one that we currently have: ISablierV2NFTDescriptor.

As said above we will.

can you accept https://github.com/sablierhq/v2-core/pull/366?

I can accept it but my position is that using the ISablierV2Lockup instead of IERC721Metadata would be better.

[PaulRBerg]

Airstream is also going to be a future contract

(i) airstreams are a form of lockup stream, and (ii) they will be kept in the periphery, so not relevant to this discussion.

The idea was to create a common interface for both products called ISablierV2.

Yes, and we will eventually do that. But that is orthogonal to the problem situation I have unsurfaced here; it looks like you keep missing my point.

If you suggest to install the lockup repo(which is not a small one) in the open-ended repo just to import the ISablierV2NFTDescriptor I don't think it's a good idea

That open-ended streaming contract (if it ever gets implemented) will be implemented in this repo. The name of this repo is v2-core, not v2-lockup.

We can keep the ISablierV2NFTDescriptor name with different parameter because "ISablierV2" can refer to both lockup and open-ended interfaces.

This is bad software design. You never have two symbols that refer to different implementations.

using the ISablierV2Lockup instead of IERC721Metadata would be better.

It simply can't be that way, no matter how you stretch this idea. Having two different implementation share the same interface name is woefully bad software architecture.

[PaulRBerg]

By the same token, why wouldn't we name both SablierV2LockupLinear and SablierV2LockupPro to just SablierV2LockupStreaming (and potentially move them to separate repos), just because we can?

[andreivladbrg]

That open-ended streaming contract (if it ever gets implemented) will be implemented in this repo. The name of this repo is v2-core, not v2-lockup.

Ok this is why we were disagreeing, we started from different perspectives.

I am not sure why we would do that as our last commit will be the auditors one? Plus it will be a public repo and anyone would be able to see the ongoing work.

It simply can't be that way

Agree to disagree.

[PaulRBerg]

I am not sure why we would do that as our last commit will be the auditors one?

Do you think we will stop developing this repo once we ship V2.0?

The frozen commit will be relevant only in the context of V2.0, so that we can prove that the production bytecodes match the frozen commit bytecodes.

Plus it will be a public repo and anyone would be able to see the ongoing work.

What's wrong with that?

And also - private fork? 💁‍♂️

Agree to disagree.

I'm sorry but this is not a matter of taste or opinion. With your suggestion, we would have this:

├── ISablierV2NFTDescriptor.sol
└── ISablierV2NFTDescriptor.sol

That is, two interfaces that live under the same roof, named the same, but which rely upon different protocol interfaces (ISablierV2Lockup and ISablierV2OpenEnded) in their implementations of tokenURI.

Do you now understand why this is non-sense?

I will not give up, and I will not merge #366, until you do.

[andreivladbrg]

I feel like we deviated from the subject.

Let's continue the discussion about this privately.

[andreivladbrg]

Implemented here https://github.com/sablierhq/v2-core/pull/366

[PaulRBerg]

We stayed completely on topic in all comments here.