Include periphery in this repo

[andreivladbrg]

This PR addresses the first point in package tethering issue #808

The idea here is to create multiple PRs, to make the review process easier (some of PRs will have a failing CI, because the fixes are done in the upcoming ones).

Note

This PR is meant to be merged into staging after #980 and #981 (and more to come) are merged into it (Merge order: last created –> second last –> … –> first created). The review would be easier if it is done in the order these PRs were created: first 979, second 980, third 981 and so on..

Each PR is going to be left as "draft" until the upcoming one is merged into it.
last created (not draft), second last(draft) … first created(draft)

Changes

• Add core dir in src
• Move all production contracts into core
• Add periphery dir in src
• Move all production periphery contracts here
• Update script import paths

---

Question: I have thought about merging the Errors libraries into a single one and adding a dedicated errors directory in the src root. Also, I was considering whether we should move all interfaces (both core and periphery) to the root

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

But, I consider that it is better to separate them for an easier repo structure to manage (current version), wdyt, do you have a better suggestion on how to structure the src?

[andreivladbrg]

@PaulRBerg and @smol-ninja could you please provide your input on the question raised in the PR comment?

[PaulRBerg]

This is not an easy choice. I need time to review this PR properly before sharing my input, and I don't yet know when I will be able to take a look at it. I'm sorry.

[smol-ninja]

Looks good. Just a note that when we merge these changes into the staging branch, we should make sure to add all the authors of v2-periphery as co-authors of the commit.

[PaulRBerg]

I would rename the PR title to "Merge the Periphery contracts"

[smol-ninja]

do you have a better suggestion on how to structure the src?

@andreivladbrg I would suggest to keep core and periphery separate for these PRs and then start a separate discussion about the new structure that you have in your mind. Combining all the interfaces into a separate folder is a good topic to discuss.

[andreivladbrg]

I would rename the PR title to "Merge the Periphery contracts"

the idea is to merge each PR into this branch, and then merge it to staging, as stated in the OP comment (see notes):

so, "Include periphery in this repo" is better, since it will have all relevant files (src, tests, scripts, etc..)

[smol-ninja]

I don't think this would ever work. We have discussed this previously that deploying the entire protocol in a single tx would cause gas to exceed block limit.

As a solution what we can do is add two new options --core and --periphery to select which one to deploy.

But lets create another issue for that.

[andreivladbrg]

We have discussed this previously that deploying the entire protocol in a single tx would cause gas to exceed block limit.

the problem before was not about gas, but about different optimizer runs between core/periphery which lead to a NFTDescriptor and Dynamic contracts being too big, now since we use the same foundry configuration it would work (tested):

v2-core/foundry.toml

Line 18 in a34e528

optimizer_runs = 1000

https://github.com/sablier-labs/v2-periphery/blob/2d338a7ff6452b20a25e779f5a960f047a5d16af/foundry.toml#L20

But lets create another issue for that

don't think we need one as per above reason

[smol-ninja]

Oh. I tested it yesterday and it showed gas to be over 30M. How much gas requirement it shows to you?

[smol-ninja]

c00cf23

• I have moved generic functions to Base test contract, such as expectMultipleCallsToTransferFrom as they relate to ERC20 transfers.
• I've also created another section called CALL EXPECTS - LOCKUP (please have a look at the base file). expectMultipleCallsToCreateWithDurationsLD etc. also relate to the core function even though they are used in periphery.
• Periphery_Test only contains MERKLE-LOCKUP section which imo can also be moved to Base. I don't think it makes Base less readable.

---

We should use super.setUp() instead of <PARENT_CONTRACT>.setUp(). Why? So that, we can remove the setup functions which has no other logic than just calling the parent setup function.

// This would not be required if we use super.setUp().
function setUp() public virtual override {
    Base_Test.setUp();
}

You can argue that <PARENT_CONTRACT>.setUp() increases clarity. However, setup functions are always right below the inheritance statements. So its easy to look at the parent contract.

contract CHILD is PARENT {
function setUp() public virtual override {
    PARENT.setup();
}

PARENT.setup() is useful when we are inheriting multiple contracts which is not case here.

@andreivladbrg thoughts?

[andreivladbrg]

@andreivladbrg thoughts?

no strong opinions on setUp, but you are right about: "<PARENT_CONTRACT>.setUp() increases clarity"
do as you wish

the commit looks good

[smol-ninja]

do as you wish

Then I would refactor it to use super.setUp in the next PR so that we can remove those redundant setup functions. They take up 7 lines.

I have decided to keep <CONTRACT>.setUp().

[PaulRBerg]

I have decided to keep <CONTRACT>.setUp().

gud, me like it