Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use try...catch in cancelMultiple and withdrawMultiple to handle invalid stream IDs #917

Open
smol-ninja opened this issue May 8, 2024 · 7 comments
Labels
effort: medium Default level of effort. priority: 2 We will do our best to deal with this. type: feature New feature or request. work: complicated Sense-analyze-respond. The relationship between cause and effect requires analysis or expertise.

Comments

@smol-ninja
Copy link
Member

As discussed here, batch functions such as cancelMultiple and withdrawMultiple should be allowed to continue execution if one of the stream IDs revert.

A sample implementation would look like the following:

event InvalidStreamIDInBatch(uint256 id, string memory reason);

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 InvalidStreamIDInBatch(streamIds[i], reason);
        }
    }
}
@smol-ninja smol-ninja added type: feature New feature or request. priority: 2 We will do our best to deal with this. work: complicated Sense-analyze-respond. The relationship between cause and effect requires analysis or expertise. effort: medium Default level of effort. labels May 8, 2024
@PaulRBerg
Copy link
Member

LGTM except for the error name.

I would create two errors, one for withdrawals, and another for cancellations.

I'd also say something like InvalidWithdrawalInBatch

@PaulRBerg
Copy link
Member

PaulRBerg commented May 17, 2024

@smol-ninja could you please create a milestone for the next Lockup release, and include this issue in that milestone?

And let's name it according to the package tethering approach.

@smol-ninja
Copy link
Member Author

Will do.

@smol-ninja smol-ninja modified the milestone: V2.1 May 17, 2024
@smol-ninja
Copy link
Member Author

smol-ninja commented May 17, 2024

Did you mean board or milestone @PaulRBerg?

We used board to track v2.2, which I have created one here: https://github.com/orgs/sablier-labs/projects/19/views/2. I am calling it Lockup 1.2.1 but if it ends up being a big release (depending on the issues and features we add to it), we can rename it to Lockup 2.0.0.

Note the description: Tracking bugs, new ideas and feature requests for Sablier Lockup 1.2.1 which will succeed 1.2.0 (also known as V2.2)

@PaulRBerg
Copy link
Member

Sorry, I meant a board.

The name sounds good.

@PaulRBerg
Copy link
Member

This feature would mitigate L-07. WithdrawMultiple can be DOS'ed by a random user from the CodeHawk report, wouldn't it?

@smol-ninja
Copy link
Member Author

Yes.

@smol-ninja smol-ninja self-assigned this Sep 2, 2024
@smol-ninja smol-ninja removed their assignment Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: medium Default level of effort. priority: 2 We will do our best to deal with this. type: feature New feature or request. work: complicated Sense-analyze-respond. The relationship between cause and effect requires analysis or expertise.
Projects
None yet
Development

No branches or pull requests

2 participants