Will we lose ability to ignore reverts if we switch to batch for the cancel/withdraw multiple functionality?

[PaulRBerg]

The idea to jettison the cancelMultiple and withdrawMultiple functions has been thrown around as a potential solution for lowering the contract size should this become necessary in the future.

However, I now wonder if this is really feasible. By switching to batch, I reckon that we would lose the ability to ignore reverts in cancelMultiple and withdrawMultiple, which would be bad.

Is that so, @sablier-labs/solidity?

[smol-ninja]

Just a note, we only ignore reverts in withdrawMultiple. cancelMultiple would still revert if any of the stream ID reverts.

Now regarding withdrawMultiple, I think I have raised it in the past:

1. Since its a public function; and
2. It makes call to public withdraw function

How about moving it to SablierBatchLockup? This way, we can still keep it without affecting the Lockup's contract size.

[andreivladbrg]

re reverting or not when using batch, we could also add a boolean parameter to the function, allowing the user to decide whether to revert when calling it:

function batch(bytes[] calldata calls, bool revertOnFail) external payable returns (bytes[] memory results);

wdyt?

[PaulRBerg]

Interesting idea, though that should be implemented as a separate function to keep the UX clean and avoid accidents where users wanted to revert (or not revert) and the function did the opposite.

[andreivladbrg]

Interesting idea, though that should be implemented as a separate function to keep the UX clean and avoid accidents where users wanted to revert (or not revert) and the function did the opposite.

sure.
my point was more about the fact that we can achieve the "non-reverting" behavior with batch as well, so i wouldn’t justify keeping the Multiple function just for this purpose

[smol-ninja]

was I right that by using batch, we would lose the ability to ignore reverts? Can you confirm that, please?

Yes.

though that should be implemented as a separate function

Instead of implementing it as a separate function, I'd vouch for keeping withdrawMultiple instead. But if we modify the current function to include revertOnFail as Andrei suggested then size increase should not be that much.

Motivation behind moving it to SablierBatchLockup was to keep the contract size low without losing on the non reverting functionality. But as you pointed, it would come at the cost of increased gas.

keep the UX clean

How does it make UX worse? I think its a good design. Instead of revertOnFail, we can have continueOnFail so that by default, batch reverts on fail.

[PaulRBerg]

Fair points about keeping a single batch function.

Before continuing the discussion here, I'd like us to first reach a resolution here: #1148.