Suggestion to refactor the LockupDynamic create methods

[razgraf]

We'll start with a comparison of the current create methods in our contracts

Lockup Linear (Durations)	Lockup Linear (Range)
v2-core/src/interfaces/ISablierV2LockupLinear.sol Lines 64 to 72 in b23b065 function createWithDurations( address sender, address recipient, uint128 totalAmount, IERC20 asset, bool cancelable, LockupLinear.Durations calldata durations, Broker calldata broker ) external returns (uint256 streamId);	v2-core/src/interfaces/ISablierV2LockupLinear.sol Lines 102 to 110 in b23b065 function createWithRange( address sender, address recipient, uint128 totalAmount, IERC20 asset, bool cancelable, LockupLinear.Range calldata range, Broker calldata broker ) external returns (uint256 streamId);

Lockup Dynamic (Deltas)	Lockup Dynamic (Milestones)
v2-core/src/interfaces/ISablierV2LockupPro.sol Lines 74 to 83 in b23b065 function createWithDeltas( address sender, address recipient, uint128 totalAmount, LockupPro.Segment[] memory segments, IERC20 asset, bool cancelable, uint40[] memory deltas, Broker calldata broker ) external returns (uint256 streamId);	v2-core/src/interfaces/ISablierV2LockupPro.sol Lines 115 to 124 in b23b065 function createWithMilestones( address sender, address recipient, uint128 totalAmount, LockupPro.Segment[] memory segments, IERC20 asset, bool cancelable, uint40 startTime, Broker calldata broker ) external returns (uint256 streamId);

Problem no. 1 - LockupDynamic.Segment

Context

The same segment is being used in both create variants. The milestones flavor uses the segment properly, by accessing all variables, including milestones. The deltas flavor won't use milestones at all. Instead, it will make use of an additional method argument called deltas[] where it will source its durations from.

Problem

By reusing the segment struct we're causing a few headaches to any developer reading the interface:

• milestones isn't being used at all in the delta variant (we ask for a redundant value)
• the assumption of an array with segment definitions is broken, as now we're back to multiple different data sources defining a segment (which now have to be synced)

I propose we implement two separate structs, one including a milestone, the other a duration.
As an alternative (although I hope we can have distinct structs), we can discuss the possibility of having a single struct that either re-uses a time dimension (e.g. lifetime) but means different things based on the variant chosen -- which is still bad UX but might save up on gas.
As a second alternative, we revert back to the separate arrays, although I think that was more gas/storage intensive.

Problem no. 2 - Order of arguments

The linear methods follow an order: sender, recipient, total, asset, cancelable, [...special...], broker
The dynamic methods have: sender, recipient, total, [...special...], asset, cancelable, [...special...], broker

Wouldn't it be best to group these similar arguments together, by moving asset and cancelable after the total amount?

Problem no. 3 - Deltas vs Durations and Milestones vs Ranges

For consistency, why don't we use durations and ranges for both contracts? At least with milestones I can understand that we're naming the method after the specific milestone field of the segment struct, that's driving the temporal logic here. Instead of range, which would work too.

But deltas vs. durations feels a bit redundant. Was this made so we have the names completely split between contracts?

[PaulRBerg]

Great feedback, thanks.

LockupDynamic.Segment

we're causing a few headaches to any developer reading the interface

The interface actually documents the requirement that the milestones should be empty:

v2-core/src/interfaces/ISablierV2LockupPro.sol

Line 56 in b23b065

/// - The segment milestones should be empty, as they will be overridden.

now we're back to multiple different data sources defining a segment (which now have to be synced)

I don't quite follow this particular point. There are no multiple data sources for the deltas - there's just the deltas array. Nothing has to be synced.

I propose we implement two separate structs, one including a milestone, the other a duration.

Sounds good. I will open an issue later today.

we can discuss the possibility of having a single struct that either re-uses a time dimension (e.g. lifetime)

Big footgun.

we revert back to the separate arrays, although I think that was more gas/storage intensive.

Definitely not. Besides increased gas costs, the DX would be much worse.

Order of Arguments

Wouldn't it be best to group these similar arguments together, by moving asset and cancelable after the total amount?

The rationale was to group the parameters by relatedness. The segments are closely related to the totalAmount because of the invariant "deposit amount == Σ segment amounts". Also, we didn't look at this from the point of view of grouping similar functions across the linear and the dynamic contract; only within each contract.

All in all, I'm happy to group the parameters as you suggested. Will open an issue.

Side note - for maximum clarity in future discussions, can you use the term "parameters"/ "params" instead of "arguments" in the context of the contract repositories? @andreivladbrg and I are now using the word "parameter" to refer to function parameter definitions, and the word "argument" to refer to the values the said params take. Apparently, this is the formal way of using these terms.

Deltas vs Durations and Milestones vs Ranges

For consistency, why don't we use durations and ranges for both contracts

Consistency is not always desirable. It was a conscious decision to name the functions differently, because the dynamic segments are qualitatively different from the linear segments. Different function names communicate this notion better.

Instead of range, which would work too.

No, I don't think range would work for the dynamic contract. A range is something predefined that has a start and an end. Whereas a segment milestone is just a single point in time. Thus, "Milestone" is a great fit for the dynamic segments, and by naming the create function createWithMilestones, we give a hint to the developer who calls this function that it's their job to pass the milestones (rather than the protocol's).

Was this made so we have the names completely split between contracts?

Yes. Also, I think that the "delta" is a better fit in the context of the dynamic contract, because it better indicates the differential temporality of the segments. Whereas a "duration" sounds (to me, at least) like something more rigid and set-in-stone.

[PaulRBerg]

Afterthought: seemingly in spite of what I said above, I can also see some benefits in using the same create function names between the linear and the dynamic contract, e.g. easier to remember the API of Sablier V2. Striking a balance between good DX and fit-for-purpose nomenclature is hard.

I suggest we address the first two problems first, and then we can reconsider the final names of the functions.

[PaulRBerg]

@razgraf what option do you like more:

1. LockupPro.SegmentWithDelta[] memory segments
2. LockupPro.SegmentWithDelta[] memory segmentsWithDeltas

My worry with the 1st is that it may easily mistaken for the standard segments (with milestones), but the 2nd option is a bit verbose.

WDYT?

[razgraf]

Hmm ok, but seeing as SegmentWithDelta isn't an extension but a different flavor, I'd still split the two. I feel this is similar to when we had create and createWithDurations, but now we have specified purposes with no "base" short version.

Regarding the name of the variable itself, segments is KISS. The type appears in the ABI so it's clear enough which segment we're referring to.

[PaulRBerg]

isn't an extension but a different flavor, I'd still split the two

They may be different flavors, but one of them (SegmentWithMilestone) is the "real deal" - i.e. what is found in contract storage and used in the long term by the contract.

I feel this is similar to when we had create and createWithDurations

It's different this time. As I said, what you call SegmentWithMilestone is the canonical segment - the SegmentWithDelta will turn into a SegmentWithMilestone even after #328 gets implemented. Whereas there's no such thing as createWithDurations turning into create. They are just two different functions, which do different things.

A list of some of the places where SegmentWithMilestone is needed:

• createWithMilestones
• getSegments
• getStream
• streamedAmountOf

Whereas SegmentWithDelta is an in-memory array that is needed only in the createWithDeltas function, and even then, it's used as a stepping stone for SegmentWithMilestone.

Therefore, I suggest we stick with just Segment.

Regarding the name of the variable itself, segments is KISS.

I agree. I will use just segments.

[razgraf]

Got it. The other more elegant way would've been to have a separate Segment entity, as a union between the theoretical SegmentWithDelta and SegmentWithMilestone. But that's probably getting into the more "expensive" area for gas consumption.

[andreivladbrg]

I am with @PaulRBerg on this one, we should keep the Segment name for the milestones one.

Regarding the options, I think the second one LockupPro.SegmentWithDelta[] memory segmentsWithDeltas would be better because in the createWithDeltas we call the internal function _createWithMilestones which has already a segments param that is a LockupPro.Segment type.

[PaulRBerg]

to have a separate Segment entity, as a union between the theoretical SegmentWithDelta and SegmentWithMilestone

Union types (or anything resembling them) do not exist in Solidity, but in any case, wouldn't this get back to square one? The very purpose of creating the new struct is to not have redundant inputs in the createWithDeltas function. If I understand your point well, this join type would contain both deltas and milestones.

But that's probably getting into the more "expensive" area for gas consumption.

Also this, yeah.

I am with @PaulRBerg on this one, we should keep the Segment name for the milestones one.

Cool, thanks for chiming in, @andreivladbrg.

I think the second one LockupPro.SegmentWithDelta[] memory segmentsWithDeltas would be better because in the createWithDeltas we call the internal function _createWithMilestones

Nah, I disagree. I side with @razgraf on this point - we shouldn't optimize the public smart contract API based on internal logic. Using just segments is fine because the type exists in the ABI.

[PaulRBerg]

The 1st and the 2nd suggestions made by @razgraf have been implemented by me in #334. I have also opted for naming the SegmentWithDelta[] array as just segments instead of segmentsWithDeltas.

However, I didn't implement the 3rd suggestion, the one about the name of the create functions. After further consideration, I continue to be against using the same name across the two lockup streaming contracts.

1. There's no good replacement for createWithMilestones in the pro contract. createWithRange doesn't communicate well enough the fact that the function expects multiple segments, which can have different milestone values.
2. (Related to point 1) Keeping just createWithDurations the same feels off to me.
3. createWithDeltas sounds like a better choice due to the argument I have given in my comment above - which is that "deltas" seems to be more suggestive of there being a multiplicity of values.

[PaulRBerg]

@razgraf if you agree with the above, can we lock this discussion?

[razgraf]

We're basically using two concepts: create-with-duration(s) and create-with-dates, but splitting the API based on the target contract. I don't necessarily agree due to the underlying concepts being pretty much the same (of course, duration vs. delta with a higher level of correlation than range vs. milestone) but I can live with it.

[PaulRBerg]

using two concepts: create-with-duration(s) and create-with-dates

Yes, qualitatively speaking, there are only two concepts. But, quantitatively, there are four concepts, because with the Dynamic contract, there can be many more durations and dates.

but I can live with it.

Ok, thanks. We can also ask for a third-party take on this subject, i.e., our auditors.

[PaulRBerg]

Going to close this since we settled upon the current API.