What happens with debt when a stream is canceled?

[razgraf]

Say I have a stream of 1000$/month, I keep it open for 3 months but only fund it with 1000$. At the end of the period, I have 2000$ in debt.

What happens when I cancel/stop the payment?

• should the sender be allowed to cancel only if they pay the debt? (might be linked to Voiding streams with negative debt for improved accounting #57, as they could ask the recipient to void it first if they don't want to pay)
• should I be able to cancel/stop/pause it without paying the debt?

Follow-up questions: based on the answers for the above, should we have utilities like "payDebtAndCancelStream" to be able do do everything in one tx?

CC: @smol-ninja @andreivladbrg

[smol-ninja]

What happens when I cancel/stop the payment?

In the current design, the sender will be able to cancel. The consequence of not allowing them to cancel when there is a debt is that they can just ignore it. There is an assumption that there will a mutual understanding between the sender and the recipient. One party can, of course, rug another party by not fulfilling his debt obligations.

Follow-up questions: based on the answers for the above, should we have utilities like "payDebtAndCancelStream" to be able to do everything in one tx?

Yeah, we can. I like the idea.

[razgraf]

Would be great if you guys could offer a summary of pause and void here so I can mark that answer as correct for future reference 🙏 Or link to an existing discussion that's related if we have any. Thank you!

[smol-ninja]

You can refer to the "Table" in this discussion.

To summarize:

• pause() will pause the stream by setting ratePerSecond to zero. It will also store debt accumulated until then. It can only be called by the sender.
• void() will also pause the stream but it will set debt to zero. It can only be called by the recipient.

cc @andreivladbrg.

[andreivladbrg]

Follow-up questions: based on the answers for the above, should we have utilities like "payDebtAndCancelStream" to be able to do everything in one tx?

great idea @razgraf 🫡

#59

[smol-ninja]

@razgraf wanted to hear your opinion on this approach? Should it continue to remain the same or should it not reset debt to 0?

[razgraf]

Resetting debt to zero is fine.

By default, we'll suggest the user to call payDebtAndCancel when they cancel as this is the "fair" assumed behavior (you want to pay what is owed). If they choose the forced cancel option, we can handle things on the UI side and highlight when a stream has been stopped with outstanding debt.

Do you guys think this would be a good way to implement the cancel flow?

[andreivladbrg]

Do you guys think this would be a good way to implement the cancel flow?

Personally, I find the current flow satisfactory. However, @smol-ninja has suggested adding a new debt variable: #64.

Since we will have payDebtAndCancel, I think that would suffice, and adding a new variable wouldn't be needed.

[razgraf]

That's an interesting approach too 🤔

• would simplify the list of methods
• would hint at the importance of debt being paid at time of cancel (any cancel)
• would however make separation of concerns harder, as it would make "cancel" have more than one purpose

I know @PaulRBerg is usually promoting simplicity in the responsibility of methods. Because of that I'm leaning towards having two separate "core" method (payDebt, cancel) and an aggregated "peripheral" one (payDebtAndCancel).

At the same time... cannot understate the beauty of a self-explanatory API (one method = "hey, when you cancel, always consider how the debt may affect your agreements").

Sorry for potentially not being helpful here :D

[andreivladbrg]

Because of that I'm leaning towards having two separate "core" method (payDebt, cancel) and an aggregated "peripheral" one (payDebtAndCancel).

It may not be possible to have payDebtAndCancel in a separate contract, as the sender needs to be the caller in OE contract

[razgraf]

Ah yes the sender for cancel, you're right.

[smol-ninja]

the same time... cannot understate the beauty of a self-explanatory API

I like the approach of designing core APIs such that each of them does just one job. i.e. _adjustRatePerSecond() only adjusts the rate per second without transferring any asset, _cancel() only cancels the stream without making any transfers.

It can make them more composable, easier to implement a new API without re-deploying the core contract. Now to achieve complex interactions such as cancel and refund or restart and deposit, there could be 3 ways (or more):

• Separate periphery contracts: It comes with problem that @andreivladbrg has pointed out. This can be taken care of with either delegatecall() but because it is a separate contract, it increases the complexity compared to the advantage it offers.

• Implement periphery related functions in the core contracts: this is how we are doing it right now but it limits the number of batch functions we can write. We will still have to deploy a new contract if there is a requirement for a new API.

• Allow UI to execute a combination of functions directly on the core contract

And I think the 3rd point gives us a really good food for thought. I have shared it before and it was discarded for the following reason:

I would definitely like to have named parameters and structs instead of encoded bytes for the methods as it makes debugging easier.
Bytes are probably more efficient and scalable (that batch method acts a bit like execute(bytes) in safes) but DX-wise, not sure we need it right now.

But I would like to pick it again because I think I agreed easily with what @razgraf pointed out earlier.

function batch(bytes[] calldata calls, bool revertOnFail) external payable {
    for (uint256 i = 0; i < calls.length; i++) {
        (bool success, bytes memory result) = address(this).delegatecall(calls[i]);
        if (!success && revertOnFail) {
            _getRevertMsg(result);
        }
    }
}

I would definitely like to have named parameters and structs instead of encoded bytes for the methods as it makes debugging easier.

Can you please explain why would you have to deal with raw bytes directly? In the UI, isn't it possible to use the function named parameters to construct the calls array?

Bytes are probably more efficient and scalable (that batch method acts a bit like execute(bytes) in safes) but DX-wise, not sure we need it right now.

The above function does not make it efficient at all. And yes it acts similarly to execute(bytes) but it is a much simpler version. If you look at it, it just calls all the functions mentioned in the call parameter to itself.

The above function is so useful that it wouldn't even require us to deploy periphery contract (until they offer some benefits) and we can absolutely expose any API through the user interface.

I would like to hear criticism against this approach again. Tagging @sablier-labs/solidity for comments.

FWIW Uniswap also has a similar function.

[razgraf]

one can easily construct input data for the batch function without requiring an external script

True, but my point is: they still have to run encodeFunctionData({abi, method, variables, ...}) or similar to convert that to bytes, don't they?

fragmentation

Sorry my bad, I misread your solution as having those key important functions be implemented in a periphery contract still. You meant have everything in the core: simple methods, key compounded methods and the batch, right?

---

Note: I'm not against the idea of dropping the periphery if in the end it just makes sense to, I'm just trying to poke holes so we don't miss something (including DX).

[smol-ninja]

True, but my point is: they still have to run encodeFunctionData({abi, method, variables, ...}) or similar to convert that to bytes, don't they?

I agree with you. They will.

Sorry my bad, I misread your solution as having those key important functions be implemented in a periphery contract still. You meant have everything in the core: simple methods, key compounded methods and the batch, right?

Yes thats what I meant.

Note: I'm not against the idea of dropping the periphery if in the end it just makes sense to, I'm just trying to poke holes so we don't miss something (including DX).

Yes I agree, its important to have this discussion before we make any conclusion.

@andreivladbrg do you anticipate any security concern using batch function other that what we have already discussed?

[andreivladbrg]

@andreivladbrg do you anticipate any security concern using batch function other that what we have already discussed?

nope, there isn't anything else i can think of atm

[razgraf]

@smol-ninja would you mind moving the idea of this batch function into a dedicated discussion?

If you think it would improve our contract design, let's give it a shot - we'll implement it, and after the first sprint evaluate the DX behind it (after we bake it into the app too). If it's clearly annoying, we can simply go back to a periphery design or go over the aggregated-core-methods idea again (having everything in the core).

[smol-ninja]

Agree. Started here: #88

[PaulRBerg]

To avoid having to read all comments here, could you guys confirm that this discussion has effectively been deprecated in favor of @smol-ninja's proposal to carry forward the debt?

If yes, we should close this as outdated.

[smol-ninja]

Yes we can.