Why do we need to revert a batch execution if one of the execution reverts?

[smol-ninja]

The batch functions such as withdraw-multiple and cancel-multiple revert if the execution of one of the stream IDs reverts. In such cases, the reverted transaction would consume a lot of gas because of the nature of the batch functions.

Thus, I would like to suggest that instead of reverting the entire transaction, it should proceed with the valid stream IDs and ignore the invalid ones.

function cancelMultiple(uint256[] calldata streamIds) external override {
    for (uint256 i = 0; i < streamIds.length; ++i) {
        try cancel(streamIds[i]) {
        } catch Error(string memory reason){
            emit Log(reason);
        }
    }
}

For invalid IDs, the catch block should catch and emit the reverting error. This would help the user identify the invalid IDs by looking at the explorer.

This offers three advantages to the users:

1. The transaction cost would not be wasted if some of the IDs are invalid.
2. Since it would execute the valid ones, the user would not have to re-execute in case an invalid ID has been accidentally inserted.
3. It can also be very useful in case of withdrawMultiple. Anyone can run a keeper job that would keep calling withdrawMultiple on all stream IDs without fail.

[andreivladbrg]

Great idea Shub, I like this. Curious about @PaulRBerg opinion.

The only downside I can think of now, is that it will cost more gas (if all stream IDs are correct). Also, we it would be a good idea to also return the streams IDs that have been canceled/withdrawn successfully.
And for functions like createAndDepositMultiple it would require to use depositMultiple and it will cost again more gas, as we've discussed in the PR.

[smol-ninja]

1. Why would it consume more gas if all stream IDs are correct?

2. Yes I agree that the function can return valid stream IDs and emit invalid ones. But returning the valid ones would require maintaining an array and that would consume additional gas.
   Another thought: since the array has to be dynamically-sized at compile time, .push() method won't be available. So how do you plan to store and return valid IDs?

3. For createAndDepositMultiple, please see my comment. It wouldn't cost more gas this way.

[PaulRBerg]

Interesting idea, @smol-ninja.

I remember we have previously discussed this in V2 Core, but I can't remember exactly where. Anyway, the rationale for reverting was the following:

Fail fast, fail hard.

However, I'm not as convinced that we should prioritize that principle over the practical benefits of letting the withdrawals pass through, especially since withdrawals can be public now.

So let's do it as you suggested @smol-ninja, and let's also open an issue in V2 Core not just here.

[andreivladbrg]

I remember we have previously discussed this in V2 Core, but I can't remember exactly where. Anyway, the rationale for reverting was the following:

yeah, same

[PaulRBerg]

@smol-ninja are you sure that having just one catch arm is enough to catch all possible errors?

Looking at Solidity by Example, my sense is that we need another arm for bytes memory reason.

[smol-ninja]

No, we don't need to catch anything else.

• catch Error(string memory reason) is executed on errors caused by revert(...). This is of our interest.
• catch (bytes memory reason) is to catch other errors. If we do not catch them, then the entire withdrawMultiple function will revert. One such example is revert to assert. And in such cases, we do want the entire tx to revert, so we don't need to catch this.

However, there is another problem with try..catch. It can only be used on functions from external contracts or to create a new contract. That means we cannot use it in the same contract (all our batch functions reside in the same core contract currently).

Source: https://docs.soliditylang.org/en/latest/control-structures.html

[PaulRBerg]

Got it.

executed on errors caused by revert(...)

Would that catch both revert reason strings and custom errors?

we do want the entire tx to revert, so we don't need to catch this.

Yes. If we implement it like that, adding this as an explanatory comment would be informative.

there is another problem with try..catch. It can only be used on functions from external contracts or to create a new contract

Oh. Then, do we have to switch to low-level calls?

[smol-ninja]

If you mean require(...) by custom errors, then yes. It would catch require(...) and revert(...) both.

Yes. If we implement it like that, adding this as an explanatory comment would be informative.

Agree.

Oh. Then, do we have to switch to low-level calls?

Interesting question. Can do but I wouldn't prefer low-level calls for batch functions. We are also still making a decision on whether there should be a separate periphery contract or not. So it also depends on that. If we end up setting up a separate periphery contract, then we can safely implement try...catch in the periphery.

[PaulRBerg]

It would catch require(...) and revert(...) both

Great!

Can do but I wouldn't prefer low-level calls for batch functions.

Got it

So it also depends on that. If we end up setting up a separate periphery contract, then we can safely implement try...catch in the periphery.

Sounds good

[smol-ninja]

Since we no longer have actionMultiple functions in Flow anymore, we can close this discussion. For Lockup, I have created the following issue to experiment with this.

• Use try...catch in cancelMultiple and withdrawMultiple to handle invalid stream IDs v2-core#917