Should we have a common interface and errors directories after package tethering?

[andreivladbrg]

In the PR #979 a question was raised that remained open ended.

After merging core and periphery into a single repository, we now have a shared architecture between them (abstracts, interfaces, types).

The question and proposal from the PR: Should we merge the interfaces and the errors into a single dir in the root of src?

current
├── core
│   └──everything the same
└── periphery
    └── everything the same

idea
├── core
│   └──everything the same but without errors and interfaces
├── errors
│   └── both core and periphery errors
├── interfaces
│   └── both core and periphery interfaces
└── periphery
    └── everything the same but without errors and interfaces

I consider that it is better to keep them separate for an easier repo structure to manage (current version). What do you think? Do you have any suggestions for structuring the src dir? @sablier-labs/solidity

[smol-ninja]

A few questions:

1. Do you propose to merge the directories but keep separate files for, let's say, Errors? If so, what would be the name of the Error files? Or do you propose to merge the files as well i.e. single Error.sol to include both core and periphery errors?

2. In case of interfaces, will there be two repos: core and periphery inside the interfaces folder? Can you please post a detailed directory structure as its not very clear from the OP?

3. If you propose to merge the files, then why not merge other directories as well? I can't think of an argument to only merge the interfaces and Errors but not the others like the following:

src
├── abstracts
│   └── both core and periphery abstract contracts OR in separate core and periphery folders
├── interfaces
│   └── both core and periphery interfaces
├── libraries
│   └── Errors from both core and periphery, Helpers from core
└── types
    └── Single Datatypes to include both core and periphery

[andreivladbrg]

First, I want to mention that I am not in favor of this change, so I don't propose it but I am addressing this idea, it was just a question left unanswered.

Also, what made me think of this question was that if you look in the out abi it had to create a speacial libraries/errors dir because it Errors.json would have duplicated:

v2-core/shell/prepare-artifacts.sh

Lines 46 to 47 in 9c8ff1f

	core_libraries=./artifacts/core/libraries
	cp out-optimized/Errors.sol/Errors.json $core_libraries

v2-core/shell/prepare-artifacts.sh

Lines 67 to 68 in 9c8ff1f

	periphery_libraries=./artifacts/periphery/libraries
	cp out-optimized/libraries/Errors.sol/Errors.json $periphery_libraries

• Do you propose to merge the directories but keep separate files for, let's say, Errors? If so, what would be the name of the Error files? Or do you propose to merge the files as well i.e. single Error.sol to include both core and periphery errors?

Same file with a single library declared - each error has the name of the contract on which is thrown

2. In case of interfaces, will there be two repos: core and periphery inside the interfaces folder?

all interfaces inside interfaces dir without adding core and periphery folders:

src/interfaces/
├── IAdminable.sol
├── ILockupNFTDescriptor.sol
├── ISablierBatchLockup.sol
├── ISablierLockup.sol
├── ISablierLockupDynamic.sol
├── ISablierLockupLinear.sol
├── ISablierLockupRecipient.sol
├── ISablierLockupTranched.sol
├── ISablierMerkleBase.sol
├── ISablierMerkleFactory.sol
├── ISablierMerkleInstant.sol
├── ISablierMerkleLL.sol
└── ISablierMerkleLT.sol

3. If you propose to merge the files, then why not merge other directories as well? I can't think of an argument to only merge the interfaces and Errors but not the others like the following:

I've mentioned above the reason for Errors, and in case of interfaces, I guess my rational was that them (interfaces) are not implementing any logic but declaring the API of the Lockup protocol, so, one could look only at this dir to have an initial understanding.

[smol-ninja]

Thank you for the explanation.

I want to mention that I am not in favor of this change, so I don't propose it but I am addressing this idea.

Sorry for misunderstanding your intention earlier. I also do not support this change because I believe it would make it more difficult to navigate the repository. The Core and Periphery are two different sets of contracts, and the Periphery can work with various versions of the Core. Additionally, there are no shared error messages between the two. Combining them would raise the deployment costs because the Errors.sol file would be larger.

You have already addressed the issue of duplicate Errors.sol in the out folder by changing the path for the periphery. I think that’s a good solution since the out folder is just for deployments and not a working directory.

Regarding the merging of interfaces

You pointed out that interfaces define the contracts' API. Then, it makes sense to keep them separate because the lockup and periphery do not share any API.

I believe that those who are interested in studying the core contracts will only want to look at the core interfaces and vice versa.

[PaulRBerg]

As per @smol-ninja's feedback above, I think that we should not merge Core and Periphery.

Closing as per unanimous agreement.