Add a different deposit function with the Broker parameter

[andreivladbrg]

Initially posted in the PR: #77

Since create and deposit are distinct operations, the most logical place to implement the Broker logic is in deposit because that is where the ERC20 transfer occurs. Therefore, I suggest adding another function called depositWithBroker instead of modifying the current deposit function.

The motivation is as follows: Unlike the lockup contracts, where the create function (where the token transfer happens) is called once for a unique streamId, here deposit might be called multiple times (the whole idea with OE is to have top-ups) for a unique ID. Therefore, to improve the DX—and since the broker param is most often set as address(0) and 0%—let’s have a default deposit function (current version) and add another with the Broker parameter. This way, we mentain the business improvements, by allowing other UIs to profit from our contract, without consuming extra gas for each regular top made through our app.

    function depositWithBroker(uint256 streamId, uint128 totalAmount, Broker calldata broker) external {
        if (broker.fee.gt(MAX_BROKER_FEE)) {
            revert Errors.SablierV2OpenEnded_BrokerFeeTooHigh(broker.fee, MAX_BROKER_FEE);
        }
        uint128 brokerFeeAmount;
        uint128 brokerTransferAmount;
        uint128 depositAmount;

        if (broker.fee.gt(ud(0))) {
            unchecked {
                brokerFeeAmount = ud(totalAmount).mul(broker.fee).intoUint128();
                depositAmount = totalAmount - brokerFeeAmount;
            }
            brokerTransferAmount = _calculateTransferAmount(streamId, brokerFeeAmount);
        }

        deposit(streamId, depositAmount);

        // Make here the broker transfer to keep the CEI pattern.
        if (brokerTransferAmount > 0) {
            _streams[streamId].asset.safeTransferFrom({ from: msg.sender, to: broker.account, value: brokerTransferAmount });
        }
    }

---

Pros

1. More gas efficient for the default version
2. DX improved for integrators who don't care about the Broker (most of them)
3. A more minimalist API where "andDeposit" functions don't need to implement Broker param (e.g. createAndDeposit or restartAndDeposit)`

Cons

1. The contract ABI is increased by one or two functions

---

IMO, the pros outweigh the cons.

@sablier-labs/solidity @razgraf wdyt?

[smol-ninja]

Interesting point. Now I am thinking why should we even include a broker in the Open-ended? Perhaps we skip it in v1 and consider it for a future release if needed.

My rationale is that, as you mentioned, deposits will be made multiple times in a stream. People can bypass broker fees by interacting directly with the Sablier UI or etherscan. The time to learn this is very low compared to the time to learn to create a stream.

This logic doesn't apply to lockup streams, as creating a stream is more challenging than depositing into an open-ended stream. So what if we don't include it and find a new approach in the future - an approach that can prevent bypassing broker fees?

[andreivladbrg]

People can bypass broker fees by interacting directly with the Sablier UI or etherscan.

I am not sure I understand this. What do you mean by "bypass broker fees".

Broker fees are good feature for UI integrators (of our contracts) who want to take a fee on their platform. The end user, who interacts through our app, doesn't even need to know what a broker is.

This logic doesn't apply to lockup streams, as creating a stream is more challenging than depositing into an open-ended stream. So what if we don't include it and find a new approach in the future - an approach that can prevent bypassing broker fees?

again, I am not sure I understand this point either, can you please reformulate?

[smol-ninja]

I am sorry. Let me try again.

By "bypass broker fees", I referred to avoiding the broker commissions.

Since stream creators are not going to be noob users, if there is a broker fee, senders will learn about it and figure out a way to skip it by either directly calling deposit() (instead of depositWithBroker()) with etherscan or through Sablier UI.

You can argue that they can do the same in lockup as well but calling deposit() on OE is easier than creating a lockup stream which requires struct preparation. Thus, broker commissions on createWithDurations and createWithTimestamps are justifiable and can be charged.

I know that broker fees are good for UI integrators, but in the case of OE, it will be quite difficult for them to take a commission compared to Lockup. Here, the broker value should also be extracted on create function but thats not possible.

So I am trying to understand if we are really helping brokers by integrating broker fee only in the deposit function.

[andreivladbrg]

Now I am thinking why should we even include a broker in the Open-ended?

Before responding, I want to clarify that you've changed your mind about Broker fees, which were mentioned by you in your initial review here: #7, and now you consider them not useful for OE contracts, have I understood you correctly?

[smol-ninja]

Haha yes. It was in December. Back then, I had less understanding of streams than I have now. 😅

FWIW I have also worked on Broker PR where I had put efforts into implementing it two times but I am still willing to drop the broker fee if we all consider it not useful for OE. Its fine for me as I think we are still brainstorming some of the OE features.

[andreivladbrg]

Ok, thank you. In that case, I would say this is more of a business decision than a technical one. In this discussion, I was strictly talking about how the implementation should look, rather than brainstorming whether we should remove the broker or not

Since stream creators are not going to be noob users, if there is a broker fee, senders will learn about it and figure out a way to skip it by either directly calling deposit() (instead of depositWithBroker()) with etherscan or through Sablier UI.
You can argue that they can do the same in lockup as well but calling deposit() on OE is easier than creating a lockup stream which requires struct preparation. Thus, broker commissions on createWithDurations and createWithTimestamps are justifiable and can be charged.

IMO it’s not about how "noob" the users are :)), nor is it about how difficult it is to make an action on the blockchain. It’s simply about whether you want the comfort of using a nice UI, which may or may not include a fee.

I know that broker fees are good for UI integrators, but in the case of OE, it will be quite difficult for them to take a commission compared to Lockup. Here, the broker value should also be extracted on create function but thats not possible.

That’s a discussion that the integrators should have; we just offer the platform to make achieving profits easier

---

Since is a business decision, I'll let @PaulRBerg & @razgraf decide as I see pros and cons in both.

[PaulRBerg]

I'm skeptical that broker fees will be helpful for integrators. I'm not aware of any integration that charges broker fees at the moment. It's also likely that an integrator would prefer to devise their own revenue generating mechanism to account for their compliance requirements.

That said, as I've argued here, we can use the broker fees for charging fees in our UI. I think that this is the greatest benefit conferred by broker fees - that we can charge UI fees without asking the user to sign two txs, i.e. one for paying the broker fee and another for creating the stream.

Additionally, having a broker fee system would allow us to shill it to investors.

Thus, I'm in favor of implementing broker fees in OpenEnded. But I'm ambivalent about whether we implement a separate function or not. I will defer to your judgement here @andreivladbrg @smol-ninja.

P.S. depositViaBroker would be a better name.

[smol-ninja]

Given that we are going forward with Broker fee, I'd like to agree with @andreivladbrg then to have a separate deposit function, depositViaBroker, to save on gas for user.

@andreivladbrg I will update my PR.

[andreivladbrg]

Thank you!

[smol-ninja]

@andreivladbrg one more question, lets say we keep functions like createAndDeposit, depositMultiple, restartStreamAndDeposit, in your original idea, did you consider implementing similar functions with broker transfers or no (just one function in the code i.e. depositViaBroker)?

[smol-ninja]

I am not entirely sure how it should be done. I am thinking of doing a gas comparison to see how expensive it is to have a broker inside the deposit function itself. When a broker is 0, the only gas overheads are three gt comparisons which may not be as significant as we thought in the OP.

Because if include createAndDepositViaBroker then why not restartStreamAndDepositViaBroker and payDebtViaBrokerAndPause as well? What are the arguments against them? After the stream has been created through a broker, he would want to take commission on every deposit, made in any form, through him.

[PaulRBerg]

I agree with @smol-ninja. I am now in favor of baking the broker fee in all functions.

The fact that it would serve us an advertisement is a great benefit.

[andreivladbrg]

I don't agree with this, having the batch function would allow us to implement restartStreamAndDepositViaBroker & payDebtViaBrokerAndPause, and any other combination, if we have the default functions depositViaBroker createAndDepositViaBroker, it is all we need to achieve everything.

---

to @smol-ninja
IMO the biggest argument against the idea of including the Broker inside the default deposit function is not the the gas costs but the improvements in the API for contract integrators. Unlike the Lockup contracts where we have the structs params here would require to have an import in plus, just to pass a null value.

What are the arguments against them

the argument against them is that the ABI is bigger and it is contradictory with the decision made to have a minimalist API. we should not have other functions other than the ones that will really be used. the broker param is 99% of the time address(0), and as discussed here: #88 (reply in thread) we will have a batch function

---

to @PaulRBerg

The fact that it would serve us an advertisement is a great benefit.

Having depositViaBroker and createAndDepositViaBroker will offer us the same level of advertisement benefit. Having all functions with "ViaBroker" is not needed as we will achieve the same flow with batch function.
Also, as you said here:

I'm skeptical that broker fees will be helpful for integrators. I'm not aware of any integration that charges broker fees at the moment. It's also likely that an integrator would prefer to devise their own revenue generating mechanism to account for their compliance requirements

[PaulRBerg]

Good points @andreivladbrg. I have changed my mind and I favor your approach now.

Unlike the Lockup contracts where we have the structs params here would require to have an import in plus, just to pass a null value.

yeah that's bad UX

[smol-ninja]

Thank you both for your comments. This settles the debate then. We will have depositViaBroker and createAndDepositViaBroker only.