Place SablierBatchLockup and core contracts into src directory

[smol-ninja]

^{Originally discussed in #1068}

• Place SablierBatchLockup into core directory
• Move files from core directory upstream and delete core directory
• Refactor tests

[andreivladbrg]

for simplicity, i would recommend doing something:

function createWithTimestampsLLMultiple(Params[] memory params) public returns (uint256 memory streamIds) {
   streamIds = new uint256[](params.length);
   for (uint256 i = 0; i < params.length; ++i) {
       streamIds[i] = createWithTimestampsLL(params[i]);
}

also, to have less logic, i.e. smaller bytecode size, wdyt?

[PaulRBerg]

Interesting idea — though I'd name this like so:

• createLLWithTimestamps
• createMultipleLLWithTimestamps

[smol-ninja]

Looks great @andreivladbrg

[andreivladbrg]

From a private discussion with @smol-ninja on Slack

What if we completely remove the SablierBatchLockup contract entirely and try to fit all the logic into the SablierLockup contract if there won’t be a contract size issue (since now we have moved logic into public libraries):

This way, it will be better for two reasons:

1. Completely singleton architecture for lockup product
2. More gas efficient, as there will be only one ERC20 transfer instead of two

@PaulRBerg cc

@razgraf will there be a problem in terms of backwards compatibility?

[PaulRBerg]

In favor.

[smol-ninja]

In favour.

if there won’t be a contract size issue (since now we have moved logic into public libraries)

fyi the contract contract is 24,284 at 1000 runs which is 292 less than the allowed (this is using public libraries).

[andreivladbrg]

1000 runs which is 292 less than the allowed (this is using public libraries

We can lower it to 800,700; it won't affect the gas too much, right?

[smol-ninja]

I think so. But I don't know how much size would create functions add. New selectors add significant to the bytecode size. And this will be adding 6 new selectors.

We will have try it to see how it plays out.

[razgraf]

There are 2 advantages of the Batch contract I would be looking to retain if we go with this refactor:

1. All the popular batch methods (createWith...) - even if batch is amazing for future proofing, you know my stance on the UX of having function signatures in block explorers
2. A single entry-point for the allowance - since Batch talks to all 3 variations of Lockup, the user has to only approve a token once towards Batch, with it taking care of the routing towards Lockup

If number 1 is not a problem due to size and number 2 becomes a non-problem when we implement #1064, then I'm in favor of this refactor as well. Will think of any other quirks as we go.

[andreivladbrg]

I hope I didn't misunderstand your point.

1. All the popular batch methods (createWith...) - even if batch is amazing for future proofing, you know my stance on the UX of having function signatures in block explorers

My point is to keep the same UX of having the function signatures in etherscan, but not in a separate contract; instead, in the core contract itself (if contract size is not a problem):

// Same contract
function createLL(Params memory params) public returns (uint256 streamId);
function createLLMultiple(Params[] memory params) public returns (uint256[] memory streamId);

A single entry-point for the allowance

there will be one entry point for allowance, SablierLockup

[smol-ninja]

I think @razgraf's point is valid about having the same entry point for all lockups since Batch can talk to all the variations of Lockup, which in @andreivladbrg's proposal would not be the case. For each lockup deployment, the user would have to approve it every time.

On second thought today, I find including createMultiple functions in core contracts a "good to have" over necessity. My argument against it is that by having create multiple functions in the core, it gives us less room to add new functionalities in the future. And by avoiding create multiple functions (or any multiple functions) in the core, it will keep it lean and contain core functionalities of the protocol. If one wants to use core directly for multiple functions, they can always use batch.

In the future, if we plan to add new functionalities without exceeding the contract size, I am afraid we might then decide to remove create multiple functions again.

[andreivladbrg]

proposal would not be the case. For each lockup deployment, the user would have to approve it every time.

we deploy a new version of BatchLockup contract pretty much on each iteration, so this point i don't think it stands out

In the future, if we plan to add new functionalities without exceeding the contract size

re, the size, yes, that's a valid point, but let's first see what is the size increase, and then decide.

I am afraid we might then decide to remove create multiple functions again.

i believe we can't know this for sure in advance

[smol-ninja]

I believe we can't know this for sure in advance

Agree with you on this. My point was that if it comes to us removing multiple functions again from the core due to size constraints in the future, then we should avoid adding them now, even if the contract size is within limits. It's very likely that we will add new functionalities or new code to the Lockup contract next year, and as a consequence, we can reach a point where we decide to refactor again by removing create multiple functions.

But as you said, let's see the size increase because of create functions first.

[andreivladbrg]

Just tested, and adding all the create multiple functions in the SablierLockup would increase the contract size too much and would require us to lower the optimizer runs below 100, which obviously doesn’t do justice. So we shall stick to the initial proposal, i.e. just move the batch contract into src/core

Functions placed in SablierLockup

function createWithDurationsLDMultiple(
    Lockup.CreateWithDurations[] calldata params,
    LockupDynamic.SegmentWithDuration[][] calldata segmentsWithDuration
)
    external
    returns (uint256 streamId)
{
    if (params.length != segmentsWithDuration.length) {
        revert Errors.SablierLockup_CreateMultipleInvalidArrayLengths(params.length, segmentsWithDuration.length);
    }

    for (uint256 i = 0; i < params.length; ++i) {
        streamId = createWithDurationsLD(params[i], segmentsWithDuration[i]);
    }
}

function createWithDurationsLLMultiple(
    Lockup.CreateWithDurations[] calldata params,
    LockupLinear.Durations[] calldata durations
)
    external
    returns (uint256 streamId)
{
    if (params.length != durations.length) {
        revert Errors.SablierLockup_CreateMultipleInvalidArrayLengths(params.length, durations.length);
    }

    for (uint256 i = 0; i < params.length; ++i) {
        streamId = createWithDurationsLL(params[i], durations[i]);
    }
}

function createWithDurationsLTMultiple(
    Lockup.CreateWithDurations[] calldata params,
    LockupTranched.TrancheWithDuration[][] calldata tranchesWithDuration
)
    external
    returns (uint256 streamId)
{
    if (params.length != tranchesWithDuration.length) {
        revert Errors.SablierLockup_CreateMultipleInvalidArrayLengths(params.length, tranchesWithDuration.length);
    }

    for (uint256 i = 0; i < params.length; ++i) {
        streamId = createWithDurationsLT(params[i], tranchesWithDuration[i]);
    }
}

function createWithTimestampsLDMultiple(
    Lockup.CreateWithTimestamps[] calldata params,
    LockupDynamic.Segment[][] calldata segments
)
    external
    returns (uint256 streamId)
{
    if (params.length != segments.length) {
        revert Errors.SablierLockup_CreateMultipleInvalidArrayLengths(params.length, segments.length);
    }

    for (uint256 i = 0; i < segments.length; ++i) {
        streamId = createWithTimestampsLD(params[i], segments[i]);
    }
}

function createWithTimestampsLLMultiple(
    Lockup.CreateWithTimestamps[] calldata params,
    uint40[] calldata cliffTime
)
    external
    returns (uint256 streamId)
{
    if (params.length != cliffTime.length) {
        revert Errors.SablierLockup_CreateMultipleInvalidArrayLengths(params.length, cliffTime.length);
    }

    for (uint256 i = 0; i < cliffTime.length; ++i) {
        streamId = createWithTimestampsLL(params[i], cliffTime[i]);
    }
}

function createWithTimestampsLTMultiple(
    Lockup.CreateWithTimestamps[] calldata params,
    LockupTranched.Tranche[][] calldata tranches
)
    external
    returns (uint256 streamId)
{
    if (params.length != tranches.length) {
        revert Errors.SablierLockup_CreateMultipleInvalidArrayLengths(params.length, tranches.length);
    }

    for (uint256 i = 0; i < tranches.length; ++i) {
        streamId = createWithTimestampsLT(params[i], tranches[i]);
    }
}