Check if OpenZeppelin can be removed from peer dependencies

[PaulRBerg]

Requiring users to install OpenZeppelin v4.9.2 may clash with the user's installation of OpenZeppelin.

We technically only depend upon the IERC20 interface, which should be the same across all OpenZeppelin V4/ V5 releases? 🧐

[andreivladbrg]

We don't only depend upon the IERC20 interface, we also use OpenZeppelin for many other dependencies:

https://github.com/sablier-labs/v2-core/blob/b5d02b7f97bb81cde884b3585064f98d95989e53/src/abstracts/SablierV2Lockup.sol#L4-L5

https://github.com/sablier-labs/v2-core/blob/b5d02b7f97bb81cde884b3585064f98d95989e53/src/SablierV2LockupLinear.sol#L5-L6

https://github.com/sablier-labs/v2-core/blob/b5d02b7f97bb81cde884b3585064f98d95989e53/src/SablierV2NFTDescriptor.sol#L5-L8

[PaulRBerg]

Sorry, I was unclear in my original comment.

What I meant is that IERC20 is the only OpenZeppelin interface that has to be provided by Sablier package users (when creating streams).

ERC721 and the rest are irrelevant because package users never interact with them directly. For instance, they could use their own ERC-721 interface as long as there is a common denominator between that and our ERC-721. However, IERC20 has to be exactly the same as OpenZeppelin's implementation.

For a better understanding of peer dependencies, read this StackOverflow post.

[PaulRBerg]

Related tweet.

[waynehoover]

Is this related to this error I'm getting when trying to compile: Error: Content for lib/v2-core/lib/openzeppelin-contracts/contracts/token/ERC20/IERC20.sol not available

[PaulRBerg]

@waynehoover I don't think so. This GitHub issue is about Node.js dependencies, whereas you seem to be using submodules and remappings (which are a pain to work with, TBH, we are considering dropping them for good).

You may just have to run foundryup - they fixed this bug yesterday https://github.com/foundry-rs/foundry/issues/5447.

[PaulRBerg]

On a second read, your errors seems different from what I have seen before.

@waynehoover could you share a link to your code?

might be worth sharing this with the Foundry team so they can have a look (and open a GitHub issue in Foundry).

[PaulRBerg]

Francisco Giordano (OpenZeppelin) created a proposal in Solidity's forum to make interfaces structural:

https://forum.soliditylang.org/t/making-interfaces-structural/1790

[PaulRBerg]

@andreivladbrg can you take a look at this task, please?

[andreivladbrg]

@andreivladbrg can you take a look at this task, please?

The simplest and quickest solution that comes to mind is to change the 1. create functions parameter 2. update the Stream struct from IERC20 to address. Would you like to make the change?

IMO it would improve the DX for integrators because they would not have to double import the interface:

import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { IERC20 as SablierIERC20 } from "@sablier/v2-core/types/tokens/IERC20.sol";

[PaulRBerg]

That is a solution, yes, but this GitHub issues is specifically about investigating whether OpenZeppelin can be removed from the Node.js peer dependencies. Can you look into this, please?

[andreivladbrg]

Yes it can, why couldn't it be removed if we have them listed under dependencies?

We could also add "^": @openzeppelin/contracts": "^4.9.2",

Also, you didn't answer my question from above, would you like to make the change?

BTW, I tried to replicate the scenario from your tweet but I couldn't, I even pushed a commit in the allo-v2 fork to remove the SablierIERC20 import. Now it seems to work which is odd.

[PaulRBerg]

@andreivladbrg let's discuss offline about Node.js peer dependencies.

you didn't answer my question from above, would you like to make the change?

no.

as I've said; that change is not about this GitHub issue. Let's discuss offline.

[PaulRBerg]

As discussed on the phone, we are able to remove OpenZeppelin from peer dependencies because it doesn't matter what specific version of IERC20 the user is consuming in their Node.js environment. All of OpenZeppelin's implementations are compatible with Sablier.