sablier-labs / v2-core

⏳ Core smart contracts of the Sablier V2 token distribution protocol
https://sablier.com
Other
308 stars 44 forks source link

Rename "range" data types to "timestamps" #869

Closed razgraf closed 5 months ago

razgraf commented 6 months ago

Given we now have generic createWithTimestamps methods instead of the old variations (durations, deltas, ranges, milestones), shouldn't we rename the dedicated LockupLinear parameter for start / cliff / end from range to timestamps in the create method (and the DataType itself)?

CC: @sablier-labs/solidity

PaulRBerg commented 6 months ago

I was instinctually on board with this idea, but the following rationale made me change my mind:

Nonetheless, it's a valid point, and I suggest we sleep on it some more. We should also ask the auditors for feedback.

smol-ninja commented 6 months ago

I am in favour of this idea (I have also been thinking about this for a while and one more refactor I will start a separate discussion for). The Range struct contains information on timestamps. Even in the case of LD and LT, the range indicates unix timestamps for the segment/tranche start and end time.

From Cambridge dictionary:

Range is the amount, number, or type of something between an upper and a lower limit.

So in my opinion, it makes more sense to use range in the context of durations than the actual timestamps.

My comments/questions on @PaulRBerg's points:

"timestamps" = start time + all segments/ tranches

Did you mean "timestamps" = start time + all segments/tranches timestamps? If so, that's more like a "current timestamp". I am not sure how it can be confused with the Timestamp struct.

"range" = start time + end time

I didn't understand this as well. Can you please explain again? Did you mean range = start unix time + end unix time? If yes then how is it different from the above context?

This would introduce inconsistency: LL would have getTimestamps and LD and LT getRange

We should also rename the range to timestamp in the context of Lockup Linear as well. In LL, the range struct contains three timestamps: start time, cliff time and end time. "Range" keyword can be used to determine stream range such as end time - start time, end time - cliff time and so on.

And it would be a breaking change WRT V2.1, and we wish to minimize breaking changes

That is something I agree with as well.