Periphery contracts

[smol-ninja]

Minimum Viable Features (MVF):

• cancelMultiple
• createMultiple
• createMultipleAndDepositMultiple
• withdrawMultiple

Refer #7

[andreivladbrg]

Later edit: see my next message

---

I think these should not be implemented in this repo, but in v2-periphery

We can leave them as backlog for the time being. The periphery is a public repo and we want to keep these new contracts design private.

[andreivladbrg]

We will implement the periphery contracts for OE in this repo, since we also agreed on sablier-labs/v2-core#808

[andreivladbrg]

Based on the rationale from the withdrawMultiple function discussion in the lockup repo (whether to move it to the batch contract or not), should we apply the same logic here? That is, should we implement all these functions in the SablierV2OpenEnded contract instead of adding a new batch contract, given its size is far from being exceeded?

An advantage of this approach is that for functions like depositMultiple, you wouldn't need to perform two ERC-20 transfers (user -> batch contract -> streaming contract) but only one (user -> streaming contract).

[andreivladbrg]

@smol-ninja @PaulRBerg tagging you here to make a decision on the above comment

[smol-ninja]

I agree with your point. None of these functions would have additional logic other than just batching calls to core functions. So I am in favour of having them as a part of OpenEnded contract.

[PaulRBerg]

Not able to review at the moment, unfortunately. Will review when I do a full review of this repo later.

[andreivladbrg]

For createMultiple function, should we have an array of assets, or should we apply the same logic from BatchLockup where we have batch of params (sender/recipient/amount) but only one asset? I would go with opt.2

@smol-ninja

[smol-ninja]

I would go with option 2 as well. Very unlikely that someone would want to create streams with different assets.

[razgraf]

I may have misunderstood things but, while this conversation refers to only 4 methods, #55 hints at completely removing the idea of a Batch Periphery.

While I haven't looked enough at the open-ended code, my comments are the following (from the perspective of a client-side integrator):

• Having dedicated periphery contracts means not exposing people to bare-bones implementations, but to more fleshed out functionality
• Having smaller ABIs and a more compact list of methods makes searching, filtering and sharing easier
  • Having a very large ABI or end-contract will increase the number of similar looking methods one must look at

At the same time, the periphery is meant to be hot-swapped in case we need to (1) fix something or (2) add functionality, while the core contracts (being more state-oriented) can keep working and accruing streams.

---

I would however like to hear again all the advantages of merging these two, if that was indeed what this conversation was describing.

[andreivladbrg]

From my perspective, all started when I made the proposal of moving the withdrawMultiple from lockup contract within the BatchLockup contract: sablier-labs/v2-core#891

Both @PaulRBerg @smol-ninja have said that it wouldn't be needed since the contracts haven't reached the maximum contract size limit. Then, with my comment I suggest to apply the same rational that has been presented to me into that discussion here in this repo (if we don't reach the maximum contract size limit we don't need to add a separate BatchOpenEnded contract.

---

I would however like to hear again all the advantages of merging these two, if that was indeed what this conversation was describing.

The main advantage IMO is that we don't need to perform multiple ERC20 transfers for:

• depositMultiple:

flowchart LR
    creator("Creator") -->|createMultiple| batchContract("Batch Contract")
    batchContract --> |transferFrom once| creator
    batchContract -->|calls create multiple times| openEndedContract("Open Ended Contract")
    openEndedContract --> |transferFrom multiple time| batchContract

    classDef default fill:#f9f,stroke:#333,stroke-width:2px;
    class creator,batchContract,openEndedContract default;

Loading

• withdrawMultiple:

flowchart LR
    caller("Caller") -->|withdrawMultiple| batchContract("Batch Contract")
    batchContract -->|calls create multiple times| openEndedContract("Open Ended Contract")
    openEndedContract --> |transfer multiple times to| recipient("Recipient")
 
    classDef default fill:#f9f,stroke:#333,stroke-width:2px;
    class creator,batchContract,openEndedContract default;

Loading

---

Having dedicated periphery contracts means not exposing people to bare-bones implementations, but to more fleshed out functionality

At the same time, having multiple contracts may make integration more challenging, given the complexity of dealing with several contracts, along with higher maintenance costs on our side.

---

IMO, there are good arguments for both options. Wdyt?

[smol-ninja]

@andreivladbrg I think there should be some correction in your diagram.

• In withdrawMultiple, the tokens will directly be transferred to the stream recipients so there is no transfer callback from the OE contract to the batch contract.
• The only actions where the transfer of tokens is between batch and OE is during the deposit.
• Even the refund can have a direct transfer from OE to the sender.

Correct me here if I am wrong.

---

Having said that, since it only affects deposits, I agree with the your point that the cost of maintaining the codebase increases if we combine batch and core contracts, even if the contract size is well below the limit.

In the original discussions, we only anticipated 4 batch functions but now it seems like the list has grown to several of them.

In Lockup contracts, we only use batch for create-multiple streams. In OE, we can use for several actions as we have discussed. We can experiment with different batch functions, frequently release newer versions based on user requirements without affecting core logic. And this has changed my mind about merging periphery and core in OE.

[andreivladbrg]

In withdrawMultiple, the tokens will directly be transferred to the stream recipients so there is no transfer callback from the OE contract to the batch contract.

yeah, correct

Even the refund can have a direct transfer from OE to the sender

cancelMultiple and refundMultiple MUST remain in the OE contract, as we check for the msg.sender == sender

the only functions that can be in a separate contract are: createMultiple, createMultipleAndDepositMultiple, depositMultiple and withdrawMultiple

[PaulRBerg]

I've caught up with all comments here. My thoughts:

This appears to be one of those nasty situations in which there is no clearly superior solution.

I suggesting going with a separate Batch contract - not because I think it's clearly better than a merging approach, but because making decisions quickly is important.

P.S. we should be cognizant of decision fatigue.

[PaulRBerg]

Actually, here's a good reason in favor of having a Batch contract.

Gas costs are less important in OpenEnded than they are in Lockup, because we will have more L2 users than in Lockup.

People use L1 with Lockup streams due to the large value deposited upfront. However, with OpenEnded, the median dollar value of a deposit should be significantly smaller.

[smol-ninja]

I would also like to take your input on #88 before I can agree with your reason for having a separate batch contract.

[PaulRBerg]

Done

[andreivladbrg]

Should we close this, since we agreed on having a batch function in the core contract? @smol-ninja @PaulRBerg

[smol-ninja]

Yes.