Investigate if we can change the time parameter in withdraw function

[andreivladbrg]

Ref: #4 (comment)

[IaroslavMazur]

I have removed the amount parameter from withdraw function and added a time one in both streamedAmountOf and withdraw functions. @IaroslavMazur, please lmk if you have suggestions for a better name.

I've given this some consideration, brainstormed it with ChatGPT - and the best suggestion we've come up with is, probably, the following: to rename streamedAmountOf(uint256 streamId, uint40 time) into streamedAmountUntil(uint256 streamId, uint40 time), with time possibly becoming timeLimit or withdrawalTime.

While this leads to the 2 streamedAmount...() functions now having different names, the new names is very suggestive of the function's specificity (which isn't the case right now, with the time argument being ambiguous).

---

Side note: wouldn't withdrawalTime, actually, be a better name for the Stream's lastTimeUpdate field, as well? 🤔

[IaroslavMazur]

Now, to address the unresolved conversation from the associated PR, let me duplicate the conversation here - and follow up on it in the next comment:

---

Iaroslav:

What if we keep the original amount parameter - and only introduce the new time logic inside the body of _withdraw(), by calculating the new value for the lastTimeUpdate of the respective Stream based on the passed amount (i.e. _streams[streamId].lastTimeUpdate += amount / _streams[streamId].ratePerSecond)?

This would keep the ABI as simple as possible for both the contract users (for whom it'd be more natural to be thinking in terms of the amount they'd like to withdraw vs the time up to which they'd like to withdraw) and us (e.g. streamedAmountOf() won't need to require the extra time parameter either).

---

Andrei:

i.e. _streams[streamId].lastTimeUpdate += amount / _streams[streamId].ratePerSecond

Interesting idea, didn't think about it before. However, there is the possibility of math issues when dividing the amount by rps and it may cause lost funds(just a small amount) for recipients, (disclaimer: I am not sure).

Let's consider the example from the README's issues chapter, 10 assets per day, and keep in mind that the amounts are 18 decimals numbers, i.e. 100 = 100e18:

When passing a number like 100.123e18 and it will get devided by rps = 0.000115740740740740e18 the result should be: 865062.720000005536401.. but in solidity the result would be just 865062. Wouldn't it lead to lost funds? What do you think? Can you research this more and provide some tests?

e.g. streamedAmountOf() won't need to require the extra time parameter eithe

There are two streamedAmountOf functions, one which has the time parameter one that has not:

/// @inheritdoc ISablierV2OpenEnded 
function streamedAmountOf(uint256 streamId) external view notCanceled(streamId) returns (uint128 streamedAmount) { 
    streamedAmount = _streamedAmountOf(streamId, uint40(block.timestamp)); 
} 
 
/// @inheritdoc ISablierV2OpenEnded 
function streamedAmountOf( 
    uint256 streamId, 
    uint40 time 
) 
    external 
    view 
    notCanceled(streamId) 
    returns (uint128 streamedAmount) 
{ 
    streamedAmount = _streamedAmountOf(streamId, time); 
} 

So, technically the time isn't required

[IaroslavMazur]

Implemented the proposed changes in #17

[smol-ninja]

Closing this discussion as well. Ref: #17