Replies: 8 comments 38 replies
-
This would indeed be a very significant improvement in terms of user experience, even though the liquidity would still remain fragmented across chains and different protocol version, to be honest. With regards to security, isn't the current compartmentalization beneficial? If a vulnerability is found in LockupDynamic, our other contracts wouldn't be impacted by it, for example. |
Beta Was this translation helpful? Give feedback.
-
Great idea. I'd also love to see a singleton contract handling all types of lockup streams for the same reason you have mentioned above. So I decided to run some tests and see if its possible. With I started with simply copying all code from Lockup Linear and Lockup Tranched into Lockup Dynamic (with some changes to the functions name as the overlap) and here is the build size:
This is the absolute worse case. I also ran the benchmark to check the gas usage. As an example, Here are a few ideas to DRY'ify the code:
|
Beta Was this translation helpful? Give feedback.
-
Thanks for opening this. I really like the idea. To make a comparison, I’ve listed some pros and cons (feel free to add or mention more, but my two cents is that the pros far outweigh the cons): Pros:
Cons:
Thoughts on the actual implementationFirst, I will list the key differences between models (they share a lot of common logic):
If the gas implications are not a concern, we could internally remove the concept of a "cliff" and achieve the same result in a dynamic setup using two segments. For a linear stream, we can achieve the same result with a single segment. Doing so would dry-fy the logic between linear and dynamic models. Comparing the benchmarks, the difference would be around ~40-50k gas in If we agree with the change above, to fix the gas problem, we can redefining what a segment is (further dry-fy the tranched contract as well). i.e. we could remove the exponent field from the struct and store the exponents separately (just for dynamic functionality), similar to how we currently handle POC code
struct Segment {
uint128 amount;
uint40 milestone;
}
struct Stream {
// --snip--
Segment[] segments;
}
// only storage we will have
mapping(uint256 streamId => Stream) internal _streams; // containing the segments
mapping(uint256 streamId => UD2x18[] exponent) internal _exponents; // only for dynamic
function calculateStreamedAmount(uint256 streamId) internal returns (uint128) {
if(isLinear(streamId)) {
return streamedAmountInCurrentSegment(streamId);
} else if (isDynamic(streamId)) {
return sumElapsedSegmentAmounts(streamId) + streamedAmountInCurrentSegment(streamId);
} else if (isTranched(streamId)) {
return sumElapsedSegmentAmounts(streamId);
}
}
/// the function will take into consideration the exponent only if it is a LD stream.
function streamedAmountInCurrentSegment(uint256 streamId) internal returns (uint128) {
// -- snip --
UD60x18 elapsedTime = ud(block.timestamp - start);
UD60x18 elapsedTimePercentage = elapsedTime.div(end - start);
if (isDynamic(streamId)) {
SD59x18 elapsedTimePercentageSD = elapsedTimePercentage.intoSD59x18();
SD59x18 exponent = _exponents[streamId][count].intoSD59x18();
elapsedTimePercentage = elapsedTimePercentageSD.pow(exponent).intoUD60x18();
}
UD60x18 streamedAmount = elapsedTimePercentage.mul(segmentAmount);
return streamedAmount.intoUint128();
} Some invariants depending on the model:
But to achieve this, we would need, as you, @smol-ninja and @PaulRBerg, pointed out, an additional enum to identify the type of model the stream is using. Very curious about what you have to say @sablier-labs/solidity. |
Beta Was this translation helpful? Give feedback.
This comment has been hidden.
This comment has been hidden.
-
I finally got it. And moved LL, LD and LT into one contract without changing the lockup designs. You can review the changes in #1069. The singleton contract has a size of 23,853 Bytes with 1000 runs (yes thats 1000 runs), and that is 723 B less than maximum. The major changes that contributed to bytecode reduction are follows:
Pros:
Cons:
@andreivladbrg I'd appreciate if you can review this. I think this is a very good approach since there is not much change to the current design of the lockups. And we should move in this direction. It has all the Pros that you mentioned above without any of those Cons. tagging @sablier-labs/solidity |
Beta Was this translation helpful? Give feedback.
-
To avoid re-reading everything, can you say if reading @smol-ninja's last message would be sufficient to have all the context I need on this? |
Beta Was this translation helpful? Give feedback.
-
I favor @smol-ninja's proposal. @andreivladbrg thank you for your feedback. For the sake of simplicity, I suggest keeping the LL, LT, and LD stream models as separate concepts. Without this separation, we would introduce a major refactor in both the UI code and the subgraphs. In the latter, special logic would be required for differentiating linear streams from other non-linear ones.
This is a false dichotomy. Merging the Lockups into one contract doesn't make the code more or less maintainable. |
Beta Was this translation helpful? Give feedback.
-
One small caveat with this merger (and I preface this by saying I haven't yet analyzed it in detail yet) is that it may break some assumptions in the client apps / services. We've been able to work our way back from a contract address or an alias to a stream type until now. ... it may require us to pipe (for example) two requests rather than a single one (since we have to now find out the type first). One simple example is a multicall. We either run:
|
Beta Was this translation helpful? Give feedback.
-
Problem
As @maxdesalle pointed out on Slack, one of the things that holds back NFT adoption is the Balkanization of the NFT collections: different releases, different chains, and different Lockup flavors.
The same issue has recently arisen in a conversation with a user — they were unsure which distribution shape in the UI corresponded to which Lockup model.
Solution
Merge all Lockups in one contract with the goal of providing a superior user experience.
I imagine that the maximum contract size of ~24kB following prevents us from accomplishing this design.
But what if we lower the optimizer runs, and we subsume the
LockupLinear
logic inLockupDynamic
, i.e. we let the LD logic to handle the linear streams?RFC
cc @sablier-labs/engineers for feedback.
Beta Was this translation helpful? Give feedback.
All reactions