search

sablier-labs / v2-core

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

Feedback from PRB on V2.2 #883

Closed PaulRBerg closed 2 months ago

PaulRBerg commented 2 months ago

I spent a few days reviewing the staging branch - good job @sablier-labs/solidity!

Closes #870, #871, #878, #879, and #880.

andreivladbrg commented 2 months ago

Thanks for the review PRB!

Haven't finished reviewing yet, but I want to point out one thing to get things move faster, I guess this was not intentional @PaulRBerg:

https://github.com/sablier-labs/v2-core/blob/bda5c856a292827950ce65adb35cc401fba637f3/src/SablierV2LockupTranched.sol#L47-L49

Can you confirm, please, that cliffs mapping should not be Tranched contract?

Doing some changes locally, so I will push commit once I finish.

PaulRBerg commented 2 months ago

Thanks for the counter-review, guys.

Can you confirm, please, that cliffs mapping should not be Tranched contract?

Ofc not. I have accidentally copy-pasted it there. Good catch.

PaulRBerg commented 2 months ago

Thanks for the kind words, @andreivladbrg.

I have addressed most of your feedback with my latest commit.

~Are we good to merge now?~ edit: nevermind, I am due to review two more PRs.

The precompile tests are failing, but that's because of https://github.com/sablier-labs/v2-core/issues/886.

PaulRBerg commented 2 months ago

Ok guys, I reviewed everything.

Whenever you're ready, we can merge this.

P.S. let's squash before merging, please.

PaulRBerg commented 2 months ago

@andreivladbrg @smol-ninja can you approve so we can merge?

PaulRBerg commented 2 months ago

I've also bumped PRBMath (see context here: https://github.com/PaulRBerg/prb-math/pull/225).