Refactoring the batch functions

[PaulRBerg]

Idea: what if renamed cancelMultiple to batchCancel, and withdrawMultiple to batchWithdraw? That would be more in line with the "batch" terminology.

Or, what if we refactored the cancel and the withdraw functions to take arrays as inputs? 🤔

@sablier-labs/solidity

[andreivladbrg]

personally, i have no strong opinion on either of these two options; both are good IMO. however, i think having the function name first and then the word "batch" would be better because it makes alphabetical ordering easier. cancelBatch vs batchCancel

Or, what if we refactored the cancel and the withdraw functions to take arrays as inputs? 🤔

interesting proposal.
two things come to mind: 1. what are the gas implications? 2. does it result in worse DX for managing streams?

uint256 myId = 123;
uint128 myAmount = 1e18;
address myTo = address(0xBEEF);
lockup.withdraw(myId, myAmount, myTo);

// vs
uint256[] memory myId = new uint256()[1];
myId[0] = 123;
uint128[] memory myAmount = new uint128()[1];
myAmount[0] = 1e18;
address[] memory myTo = new address()[1];
myTo[0] = address(0xBEEF);
lockup.withdraw(myId, myAmount, myTo);

[smol-ninja]

what if we refactored the cancel and the withdraw functions to take arrays as inputs?

I like it for cancel and renounce. But for withdraw, I agree with Andrei, it would make it look ugly to withdraw from a single stream. But what if we use an array of tuple(streamID, amount, to) instead of three lists?

Another idea is that instead of changing withdraw, how about we modify withdrawMax to accept an array of inputs? We use withdraw in other functions as well such as withdrawMaxAndTransfer so changing it would mean changing them as well. I suppose most people like to withdraw the maximum amount so withdrawMax should be fine. And to withdraw custom amounts from multiple streams, they can always use batch.

To summarize, I propose the following:

1. cancel: accepts an array of stream IDs
2. renounce: accepts an array of stream IDs
3. withdrawMax: accepts an array of tuple(stream IDs, to)
4. Remove cancelMultiple, renounceMultiple and withdrawMultiple and save some on contract size.

what are the gas implications

Wouldn't the gas be same for 1 stream?

Thoughts?

[andreivladbrg]

Wouldn't the gas be same for 1 stream?

no, having arrays as input params is more expensive than single params (i don't know for how much, might be small diff tho). also, because the function implements a loop (whether one iteration is made or not)

cancel: accepts an array of stream IDs
renounce: accepts an array of stream IDs
withdrawMax: accepts an array of tuple(stream IDs, to)
Remove cancelMultiple, renounceMultiple and withdrawMultiple and save some on contract size.

hmm, i have mixed feelings about this, as we would have inconsistencies between function names: 1. the withdraw function has no array inputs, but we have withdrawMultiple 2. cancel and renounce have only array inputs

Remove cancelMultiple, renounceMultiple and withdrawMultiple and save some on contract size.

IMO, we could remove all "multiple" functions, because as far as i can see in flow, batch works well. but first, let's wait and see if we face a situation where we need to add more features and run into problems with the size.

[smol-ninja]

having arrays as input params is more expensive than single params

I think you may be right. The calldata length would increase because it will now includes array length. But its quite small: 16 gas per non-zero byte.

I tested the following code in remix (it contains a loop as well).

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;

contract One {
    // 795 gas
    function withdraw(uint , address ) public pure returns (bool) {
        return true;
    }
}

contract Two {
    struct Withdraw {
        uint streamId;
        address to;
    }

    // 783 gas
    function withdraw(Withdraw[] calldata ids) public pure returns (bool) {
        for (uint i; i < ids.length; ++i) {
            ids[i];
        }
        return true;
    }
}

Nonetheless, its won't be much difference for a single stream.

but we have withdrawMultiple

No, we wont have withdrawMultiple. withdrawMax will take an array of inputs.

but first, let's wait and see if we face a situation where we need to add more features and run into problems with the size

fair enough.

[PaulRBerg]

Great feedback from both of you!

Good point that we can delete the *Multiple functions because they can be replicated via batch, and as a result, also lower the contract size. As Andrei said, we can keep them in mind as extra weight that can be jettisoned whenever we need to lower the contract size.

Interesting proposal from @smol-ninja, but I'm not convinced that the UX costs are worth the refactor. A function with an array is uglier than a function with a single parameter, and I speculate that in the majority of cases, single params are what integrators use, e.g. in EarnM's OTC box contract, this is true.

It would also be inconsistent to have an array in cancel,renounce, and withdrawMax, but not in withdraw. But we should also not implement an array in withdraw to avoid having to do that in withdrawMaxAndTransfer.

So let's just keep the system as is and consider jettisoning the *Multiple functions, if need be.

Closing this discussion for now.