Immediately transfer the streamed portion when creating streams

[PaulRBerg]

Idea

There are several cases when streams are created and a portion of the deposit amount is immediately withdrawable:

• Unlock-Linear and Unlock-Cliff streams
• Streams with the start time in the past

What if we modified the create stream function to transfer any immediately withdrawable amount to the recipient?

Cost/ Benefit Analysis

Pros:

• Improved UX for recipients and senders who'd like their recipients to have access to the unlocked amounts from day 1
• More gas efficiency for recipients

Cons:

• Less TVL in Sablier
• Less potential to earn revenues (e.g., by lending the deposits).
• Less gas efficiency for senders

Validation

We have received an explicit feature request for this from CerebrumDAO:

RFC

I'm on the fence. I posted this because I thought it was an interesting idea.

cc @sablier-labs/solidity and @sablier-labs/frontend

[smol-ninja]

Interesting idea. I can see why it is useful for the users. We can do it without making any changes to our core contracts.

Since the Sablier UI interacts only with the periphery to create all the streams, we can add the following additional logic to it:

Click to expand code

// Modify {BatchLockup.CreateWithDurationsLL} to include instant unlocked amount
struct CreateWithDurationsLL {
    // --- snip ---
    uint128 totalAmount;
    // --- snip ---
    LockupLinear.Durations durations;
    Broker broker;
    uint128 instantUnlockAmount;
}

// Modify the create functions to handle instant unlocked amount.
function createWithDurationsLL(
    ISablierV2LockupLinear lockupLinear,
    IERC20 asset,
    BatchLockup.CreateWithDurationsLL[] calldata batch
) returns (uint256[] memory streamIds) {
    // --- snip ---

    // Perform the ERC-20 transfer and approve {SablierV2LockupLinear} to spend the amount of assets.
    _handleTransfer(address(lockupLinear), asset, transferAmount);

    // --- snip ---
    for (i = 0; i < batchSize; ++i) {
        // Transfer the instant unlocked assets to the recipient.
        if (batch[i].instantUnlockAmount > 0) {
            asset.safeTransfer({ to: batch[i].recipient, value: batch[i].instantUnlockAmount });
        }
    
        // Create the stream.
        streamIds[i] = lockupLinear.createWithDurations(
            LockupLinear.CreateWithDurations({
                // --- snip ---
                totalAmount: batch[i].totalAmount - batch[i].instantUnlockAmount, // deduct instant unlocked amount from total amount
                asset: asset,
                // --- snip ---
            })
        );
    }
}

Also, instead of transferring assets directly from msg.sender to the recipient, it would be better to transfer instantly unlocked amount from the periphery contract to the recipient. The benefit is that for the recipients as well as the senders, Sablier continues to act as the facilitator.

flowchart LR
    sender[Sender]
    recipient[Recipient]
    periphery[(Periphery Contract)]
    core[(Core Contract)]

    sender -- "total" --> periphery
    periphery -- "instant unlocked" --> recipient
    periphery --  "(total - instantUnlocked)" --> core

Loading

Additionally, this also makes it more gas efficient to create instantly unlocked streams without even going through the core logic at all i.e. when totalAmount equals instantUnlockAmount. One big problem with that is no NFT. But do we need NFT when there is no locked tokens at all?

I am in favour of this.

[andreivladbrg]

One big problem with that is no NFT. But do we need NFT when there is no locked tokens at all?

I don’t understand this point. You can’t create a stream with a deposit amount of 0.

I see a problem with the proposed design (handled in periphery). Isn’t it problematic to have a deposit amount (also in the NFT) less than the actual amount that the sender wanted to deposit?

---

I think the easiest way to address this is to implement a createAndWithdraw function (we don't need to re-implement any logic). This way, not every creation of a stream is affected by higher gas costs, and we can keep all information about the actions made on chain.

[smol-ninja]

I don’t understand this point. You can’t create a stream with a deposit amount of 0.

If totalAmount equals instantUnlockAmount then no stream would be created because the deposit amount is zero. So the instant unlocked amount would be transferred to the recipient without any stream. No stream equals no NFT.

Isn’t it problematic to have a deposit amount (also in the NFT) less than the actual amount that the sender wanted to deposit?

The deposit amount is the amount that the sender wants to stream. I don't see a problem if it's not equal to the total amount deposited into the periphery. But I am curious to know what problem you are talking about. Is it that the total amount deposited mismatches the amount in the lockup stream? If so, how's that a problem?

createAndWithdraw

Since atleast 1 second has to be passed after the stream has been created (instant unlock through core means 1 sec cliff), wouldn't it withdraw 0 amount if create and withdraw are called in the same tx?

[andreivladbrg]

If totalAmount equals instantUnlockAmount

then, it is not streaming anymore, it is just an ERC20 transfer. (you can't create a stream with a zero amount/an end time in the past)

The deposit amount is the amount that the sender wants to stream... Is it that the total amount deposited mismatches the amount in the lockup stream? If so, how's that a problem?

imagine a tool built on top of sablier which does data analysis about the vested tokens. he would need to have an extra layer of complexity (is transfer made before stream created).

one more problem, if there is an instant amount unlocked, it means there is a start time in the past, which means that you will need to create the stream with the start time equal block.timestamp, which is going to be one more incorrect information. (vesting period started actually more in the past)

Since atleast 1 second has to be passed after the stream has been created (instant unlock through core means 1 sec cliff), wouldn't it withdraw 0 amount if create and withdraw are called in the same tx?

what do you mean? why does it need to pass at least 1 second? as mentioned above, it must have a start time in the past so that there is an actual unlock amount, which ultimately means the withdrawable amount is greater than 0 at block.timestamp.

/// @notice Only `createWithTimestamps` function can be used to actually have an unlock amount.
function createAndWithdraw(Params memory params) public returns (uint256 streamId) {
    streamId = lockup.createWithTimestamps(params); 
    lockup.withdrawMax(streamId);
}

this is much simpler, you don't need to implement additional logic

[smol-ninja]

You are right and I am in agreement with you now. createAndWithdraw is indeed a better and simpler solution than what I proposed. Oh by the way, how about putting it in periphery instead of core? It seems like a periphery feature.

function createWithDurationsLD(
    ISablierV2LockupDynamic lockupDynamic,
    IERC20 asset,
    BatchLockup.CreateWithDurationsLD[] calldata batch,
    bool instantWithdraw
)
    external
    override
    returns (uint256[] memory streamIds)
{
    // -- everything same as now -- //
    if (instantWithdraw) {
        lockupDynamic.withdrawMax(streamIds[i]);
    }
}

We might also want to "not revert" in withdrawMax when amount is zero to avoid DoS during stream creation (or have a check in UI to avoid calling this function when there is no instant withdraw).

[andreivladbrg]

Oh by the way, how about putting it in periphery instead of core? It seems like a periphery feature

you are right, it would be better in the periphery

[PaulRBerg]

Thanks for your feedback @smol-ninja @andreivladbrg.

It seems to me that you have overcomplicated the implementation?

I'm sorry for not being clear in the OP, but my proposal was simply to modify the internals of the _create function to call _withdraw if there's a non-zero withdrawable amount.

Doing so would encapsulate and standardize this behavior across all Sablier streams. There would NOT be two types of stream creations: (i) some which transfer immediately vested funds and (ii) some which don't. There would only be creations that transfer all immediately vested funds. Additionally, this design wouldn't complicate the lookup cost for external integrators building analytics tools because:

• There would be only one type of deposit amount (as compared to a periphery solution)
• The standard withdrawal mechanism would be employed

[PaulRBerg]

cc @smol-ninja @andreivladbrg, it looks like you didn't follow up here?

[smol-ninja]

When this discussion was started, there was no batch function. Does having the batch function now change your mind about it? An alternate solution could be to provide this feature in the UI as the following:

1. sender provides intent through a checkbox indicating whether they would like to immediately transfer the streamed amount to the recipient.
2. If selected, the UI calls create + withdraw through batch.
3. If not, the UI only calls create.

Having _withdraw inside _create would also require moving hook calls inside _withdraw. Consider an example where the recipient is some sort of stream manager contract. When a stream is created with the stream manager as the recipient, the transaction will mint an NFT to the stream manager as well as transfer the streamed portion to it. The manager contract will not be able to update its state because there is no hook call made to it. This can be a breaking change for integrators which assume that create does not withdraw tokens.

Using batch, at the least, the stream manager can implement a hook to be called upon withdrawal.

[andreivladbrg]

Nice idea, I like it! Using batch is indeed an elegant way to achieve this without modifying the core contracts. I tested it, and it works.

Test create and withdraw

// placed in: tests/integration/concrete/lockup-linear/create-with-timestamps-ll/createWithTimestampsLL.t.sol
function test_CreateAndWithdraw() external {
    bytes[] memory batchData = new bytes[](2);
    uint256 nextId = lockup.nextStreamId();
    _defaultParams.unlockAmounts.start = 1e18;
    _defaultParams.createWithTimestamps.timestamps.start = getBlockTimestamp() - 1;
    bytes memory createData = abi.encodeCall(
        lockup.createWithTimestampsLL,
        (_defaultParams.createWithTimestamps, _defaultParams.unlockAmounts, _defaultParams.cliffTime)
    );
    bytes memory withdrawData = abi.encodeCall(lockup.withdrawMax, (nextId, users.recipient));

    batchData[0] = createData;
    batchData[1] = withdrawData;

    expectCallToTransferFrom({
        token: dai,
        from: users.sender,
        to: address(lockup),
        value: defaults.DEPOSIT_AMOUNT()
    });
    expectCallToTransfer({ token: dai, to: users.recipient, value: 1e18 });

    lockup.batch(batchData);

    assertEq(lockup.getWithdrawnAmount(nextId), 1e18);
}

[PaulRBerg]

The existence of batch does change my mind about this. It's a powerful feature, isn't it?

Also, good criticism for my proposal to move _withdraw inside _create.

My thoughts now:

• I'm loath to introduce an additional setting in the stream creation form. A simple UI with few configurations has lots of benefits (as evidenced by the success of tools like Google and ChatGPT).
• Since we haven't received any requests for this feature in the meantime, this is not a high-priority feature
• Should it become a high-priority feature in the future, we can consider implementing via batch in the UI, but only under an Advanced Settings panel or something so that the UX remains unencumbered for the vast majority of users.