MerkleLockup for LockupTranched

[andreivladbrg]

Closes #273

Simillar to Lockup Dynamic, but we store in the MerkleLockupLT contract an array of structs with percetanges and durations (how Razvan suggested here).

 struct TrancheWithPercentage {
     UD2x18 amountPercentage; // 1e18 = 100%
     uint40 duration;
 }
 MerkleLockupLT.TrancheWithPercentage[] internal _tranchesWithPercentages;

I used UD2x18 instead of UD60x18 to be more gas efficient. The sum of these percentages must be equal to 100%

There is no change in the claim function parameters between linear and tranched. Thus, the Merkle tree would be the same.

[smol-ninja]

Reviewed src. Made a PR to arrange imports alphabetically.

[smol-ninja]

I have reviewed the PR. Left my comments below.

I think we should find a new name for tranches percentages. tranchesWithPercentages does not look apt to me.

[smol-ninja]

Also, MerkleLockupLT fork tests are failing at assertEq(vars.actualStream, vars.expectedStream);. The tranche amounts mismatch.

[andreivladbrg]

I think we should find a new name for tranches percentages. tranchesWithPercentages does not look apt to me

I agree that the name is not the best, but I couldn't find something better. Do you have other suggestions?

Also, should we not use plural?

[andreivladbrg]

@smol-ninja addressed all the feedback, lmk if it looks good.

for some reason the ci passes, still not sure why

[smol-ninja]

Every time there is a new commit in v2-core#staging, the bun lockfile needs to be updated to fetch the latest commit. Without which, ci fails. Not sure why this behaviour though.

[smol-ninja]

Looks good to me except the following comments:

Also, should we not use plural?

I thought about it. My view is that using a plural for the variable name makes more sense. Struct TrancheWithPercentage represents single tranche while tranchesWithPercentages represents a set of tranche. So I am good with using plural in this context. However I have the following suggestions:

1. Because it leads to longer names, how about we use synonym of percentage which is Pct - see Cambridge definition? That will change tranchesWithPercentages to tranchesWithPct. I know it might not be super clear compared to the original name, but curious to know what your think about it.

For example,

• actualTranchesWithPercentages becomes actualTranchesWithPct.
• MerkleLockupLT.TrancheWithPercentage[] tranchesWithPercentages becomes MerkleLockupLT.TrancheWithPct[] tranchesWithPct.
• SablierV2MerkleLockupFactory_PercentageSumNotEqualOneHundred becomes SablierV2MerkleLockupFactory_TotalPctNotEqual100

2. And instead of amountsSum and PercentagesSum, how about totalAmount and totalPercentage / totalPct? But If double s doesn't look weird to you, feel free to ignore this and such comments.

Please review this commit: 6ad73ed

[smol-ninja]

LGTM except minor refactors in createMerkleLockupLT.t.sol and createMerkleLockupLT.tree.

[andreivladbrg]

My view is that using a plural for the variable name makes more sense. Struct TrancheWithPercentage represents single tranche while tranchesWithPercentages represents a set of tranche.

My reasoning was the same.

Because it leads to longer names, how about we use synonym of percentage which is Pct - see Cambridge definition? That will change tranchesWithPercentages to tranchesWithPct. I know it might not be super clear compared to the original name, but curious to know what your think about it.

Interesting idea, but the name is not that long. Plus, the core structs have pretty much the same size (e.g., SegmentWithDuration).

Also, I discussed it with Paul, and he said that it is clearer with the long form (Pct might be confusing to some individuals who are not familiar with the term).

Please review this commit: 6ad73ed

It looks good, thanks

[andreivladbrg]

LGTM except minor refactors in createMerkleLockupLT.t.sol and createMerkleLockupLT.tree.

forced push a commit, lmk if everything is alright, if yes, can you approve the PR?

[andreivladbrg]

thanks for the review @smol-ninja

i've pushed a commit now, lmk if it looks good

[smol-ninja]

LGTM except my last reply.