Make "batch" function payable

[PaulRBerg]

https://github.com/sablier-labs/company-discussions/discussions/72#discussioncomment-11292236

• Mark all functions as payable.
• Do NOT emit the fee in any event because the withdraw function can be called in multiple ways, e.g, directly via withdraw, via withdrawMultiple, or via batch, which would make the accounting difficult to handle.

We will keep track of ETH transfers through other means, e.g., Etherscan, subgraphs, etc.

[smol-ninja]

Lets include this change alongside making other withdraw functions payable (withdrawMultiple) as a part of #1087 (cc @andreivladbrg).

[andreivladbrg]

I was trying to implement this and am facing some issues. I’m afraid we won’t be able to achieve this via batch.

If we declare batch as payable and actually send a msg.value greater than 0, all functions within the calls array would also need to be declared as payable; otherwise, it would revert. Also, if we declare the batch function as payable (and all the other functions within calls are also payable), the fee paid by the caller would be msg.value * length of the array, which we don’t want.

This leads to the conclusion that we can’t declare it as payable, and we can’t use the withdraw function within the batch function.

[smol-ninja]

In the for loop inside batch, we can compare the selectors and if it matches with withdraw and withdraw related functions, then only we would forward msg.value to the delegate call.

What do you think of this?

[andreivladbrg]

don't think will work as the delegatecall does not have "value" option

[smol-ninja]

delegatecall forwards msg.value. See here without specifying value option as long as the batch is payable.

One thing to test is that if a function is not marked as payable, can it still access msg.value? If it does not, then its good for us. So that only functions with payable modifier will be able to access msg.value.

[andreivladbrg]

we can't do this: address(this).delegatecall{ value: msg.value }(calls[i]);

[razgraf]

Curious, why does Uniswap's multicall work? What's different in their implementation?

Note: as you can see they don't specify a value in delegatecall

[PaulRBerg]

Uniswap has a different problem situation, which is not related to our problem situation.

Our problem is that we want to charge a flat, per-transaction ETH fee regardless of how many withdrawals are made in a transaction. Since the withdraw function cannot (at the moment) differentiate whether it was called directly or via withdrawMultiple or via batch, we would need to introduce some programmatic gymnastics to account for these possibilities in order to emit a correct msg.value. Here's an example:

• User withdraws from 5 streams
• The event CollectFee would get emitted 5 times, with the same msg.value logged each time
• That would be wrong because the msg.value was charged only once

[andreivladbrg]

Curious, why does Uniswap's multicall work? What's different in their implementation?

they have declared all the functions where Multicall is inherited as payable, we've had a call and that's what we decided as well (all functions in Lockup function will be payable - not only withdraw)

e.g.

https://github.com/Uniswap/v3-periphery/blob/0682387198a24c7cd63566a2c58398533860a5d1/contracts/SwapRouter.sol#L115-L117

https://github.com/Uniswap/v3-periphery/blob/0682387198a24c7cd63566a2c58398533860a5d1/contracts/SwapRouter.sol#L132-L134

https://github.com/Uniswap/v3-periphery/blob/0682387198a24c7cd63566a2c58398533860a5d1/contracts/SwapRouter.sol#L203-L205

https://github.com/Uniswap/v3-periphery/blob/0682387198a24c7cd63566a2c58398533860a5d1/contracts/SwapRouter.sol#L224-L226

etc..