Originally posted by **smol-ninja** January 26, 2024
A note to the reader: **The following doesn't affect the deployed version of Sablier contracts**.
## Context
The recent [PR: Make withdraw function callable by any account](https://github.com/sablier-labs/v2-core/issues/731) enables anyone to call `withdraw()` on any stream. Each withdrawal attempts to call `onLockupStreamWithdrawn()` on both sender and recipient if they are contracts.
## Problem
We use the same function name `onLockupStreamWithdrawn()` in both `ISablierV2Recipient` and `ISablierV2Sender`. This leads to duplicate calls on this function when `withdraw()` is triggered by a third party and the sender and recipient are the same contract addresses.
[SablierV2Lockup.sol#L278C1-L297C10](https://github.com/sablier-labs/v2-core/blob/staging/src/abstracts/SablierV2Lockup.sol#L278C1-L297C10)
```solidity
if (msg.sender != recipient && recipient.code.length > 0) {
try ISablierV2Recipient(recipient).onLockupStreamWithdrawn({.. /* snip */ ..}) { } catch { }
}
if (msg.sender != sender && sender.code.length > 0) {
try ISablierV2Sender(sender).onLockupStreamWithdrawn({.. /* snip */ ..}) { } catch { }
}
```
## Solution
Add a a check whether sender and recipient are same.
```solidity
if (msg.sender != recipient && recipient.code.length > 0) {
try ISablierV2Recipient(recipient).onLockupStreamWithdrawn({.. /* snip */ ..}) { } catch { }
}
if (sender != recipient && msg.sender != sender && sender.code.length > 0) {
try ISablierV2Sender(sender).onLockupStreamWithdrawn({.. /* snip */ ..}) { } catch { }
}
```
I am facing with a few problems while writing test cases for this.
when sender == recipient, how to write a test to not expect a call? A test which should detect the above issue.
New concrete tests are needed for hook calls. Incorporating them into withdraw would be difficult because the basis of the withdraw tree structure is based on whether the withdrawal address is stream recipient or not. In the new test, we can have a structure like this:
@smol-ninja all this is not a matter of internal knowledge at Sablier. You should ask your questions in the Foundry community forums (GitHub discussions, issues, and their Telegram chat groups).
how to write a test to not expect a call?
don't think this is possible but again, let's ask the Foundry team
only one hook call should be expected
By passing the count parameter to vm.expectCall, see:
for 1. PRB already answered, i.e. count can be used
regarding 2. i think it would be better to have these branches instead, as they are more concise, clearer and shorter:
given sender == recipient
when caller unknown
only one hook call should be expected
when the caller is the sender and the recipient
no hook call should be expected
given sender != recipient
when caller unknown
both hook calls should be expected
when the caller is the sender
recipient hook calls should be expected
when the caller is the recipient
sender hook calls should be expected
all this is not a matter of internal knowledge at Sablier. You should ask your questions in the Foundry community forums
@PaulRBerg are you sure about this statement? this might only be ok for the question "how to write a test to not expect a call?" but, the rest, what does Foundry have to do with how we write our tree structure for the withdraw function?
As discussed in https://github.com/sablier-labs/v2-core/discussions/804
I am facing with a few problems while writing test cases for this.
when sender == recipient, how to write a test to not expect a call? A test which should detect the above issue.
New concrete tests are needed for hook calls. Incorporating them into
withdrawwould be difficult because the basis of the withdraw tree structure is based on whether the withdrawal address is stream recipient or not. In the new test, we can have a structure like this:Any thoughts @andreivladbrg?
@smol-ninja all this is not a matter of internal knowledge at Sablier. You should ask your questions in the Foundry community forums (GitHub discussions, issues, and their Telegram chat groups).
don't think this is possible but again, let's ask the Foundry team
By passing the
countparameter tovm.expectCall, see:Thank you for your reply @PaulRBerg.
Totally, I agree. I will ask there.
Oh yes!
for 1. PRB already answered, i.e.
countcan be usedregarding 2. i think it would be better to have these branches instead, as they are more concise, clearer and shorter:
@PaulRBerg are you sure about this statement? this might only be ok for the question "how to write a test to not expect a call?" but, the rest, what does Foundry have to do with how we write our tree structure for the
withdrawfunction?Yes. It's always a good idea to talk to the community and correct errors in our understanding. That's the default approach we should take.
https://twitter.com/PaulRBerg/status/1335282362968633344
I wasn't referring to that - I was referring to his questions about how to expect calls
My wording was bad, though - I shouldn't have said "all this". Sorry for that.
I like this more.
No problem. I understood your point.