Feedback from PRB on V2.2 #883

PaulRBerg · 2024-04-05T16:01:46Z

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

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

chore: improve wording comments chore: misc improvements and renames docs: capitalize ID refactor: improve variable names refactor: separate max segment and tranche counts refactor: use for loop instead of while

perf: optimize "_update" by testing "_isTransferable" last test: apply "inheritdoc" test: define "withdrawAmount" once test: improve function names test: improve writing in comments test: set the license to "UNLICENSED"

docs: improve writing in NatSpec refactor: rename "sablierAddress" to "sablierStringified"

docs: improve writing in custom errors' NatSpec

docs: improve writing in NatSpec test: apply "seconds" keyword

chore: improve writing in comments test: bring back test in "createWithDurations" test: close #880 test: fuzz cliff time as zero test: improve BTs and associated modifiers

docs: _create in NatSpec

smol-ninja

Fantastic review 👏. Those are really good changes. I like using the term distribution instead of streaming.

I've added two new commits (e88ef6e and bda5c85) to address some minor changes.

src/SablierV2LockupDynamic.sol

test/integration/shared/lockup-linear/createWithTimestamps.t.sol

test/mocks/erc20/ERC20Mock.sol

andreivladbrg · 2024-04-06T12:30:31Z

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:

v2-core/src/SablierV2LockupTranched.sol

Lines 47 to 49 in bda5c85

    
           /// @dev Stream tranches mapped by stream IDs. This complements the `_streams` mapping in {SablierV2Lockup}. 
        
           mapping(uint256 id => uint40 cliff) internal _cliffs; 
        
           mapping(uint256 id => LockupTranched.Tranche[] tranches) internal _tranches;

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

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

chore: fix segments reference fix: mistaken "cliffs" array

PaulRBerg · 2024-04-07T10:51:02Z

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.

test/utils/Precompiles.t.sol

andreivladbrg

Great findings and suggestions PRB. Short form for _create is better, capitalizing the ID improves readability and also like more and prefer the naming of the functions in the Helpers library now. I remember using the same terminology when the checks were initially moved to a separate library.

script/Base.s.sol

script/GenerateSVG.s.sol

src/SablierV2LockupDynamic.sol

src/SablierV2LockupTranched.sol

src/interfaces/ISablierV2LockupLinear.sol

src/types/DataTypes.sol

docs: improve wording in comments perf: use "unchecked" for tranche calculations refactor: rename variables

PaulRBerg · 2024-04-08T15:12:26Z

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 #886.

src/SablierV2LockupTranched.sol

chore: update "filter_paths" docs: improve writing in comments refactor: include "streamId" in "SablierV2Lockup_WithdrawToZeroAddress" refactor: alphabetical order

PaulRBerg · 2024-04-08T18:00:14Z

Ok guys, I reviewed everything.

Whenever you're ready, we can merge this.

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

test: replace "maxUint40" with "maxOfThree"

slither.config.json

PaulRBerg · 2024-04-09T15:35:46Z

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

PaulRBerg · 2024-04-09T15:36:41Z

I've also bumped PRBMath (see context here: PaulRBerg/prb-math#225).

* docs: improve wording in NatSpec chore: improve wording comments chore: misc improvements and renames docs: capitalize ID refactor: improve variable names refactor: separate max segment and tranche counts refactor: use for loop instead of while * build: bump OpenZeppelin to v5.0.2 * docs: improve writing in NatSpec perf: optimize "_update" by testing "_isTransferable" last test: apply "inheritdoc" test: define "withdrawAmount" once test: improve function names test: improve writing in comments test: set the license to "UNLICENSED" * refactor: rename "streamingModel" to "sablierModel" docs: improve writing in NatSpec refactor: rename "sablierAddress" to "sablierStringified" * refactor: rename internal create function * chore: singular Check, Effect, Interaction docs: improve writing in custom errors' NatSpec * perf: close #878 docs: improve writing in NatSpec test: apply "seconds" keyword * refactor: reorder checks chore: improve writing in comments test: bring back test in "createWithDurations" test: close #880 test: fuzz cliff time as zero test: improve BTs and associated modifiers * test: remove unused import * test: include map symbol test for lockup tranched docs: _create in NatSpec * revert: bring back GPL V3 license in mocks chore: fix segments reference fix: mistaken "cliffs" array * refactor: update precompiles * test: reuse "createWithTimestamps" modifiers * test: revert max counts to 500 * test: use lockup object * style: correct bad formated natspec * refactor: rename "currentTime" to "blockTimestamp" docs: improve wording in comments perf: use "unchecked" for tranche calculations refactor: rename variables * docs: improve writing in NatSpec * refactor: order functions and fields alphabetically * refactor: rename variable * docs: capitalize IDs chore: update "filter_paths" docs: improve writing in comments refactor: include "streamId" in "SablierV2Lockup_WithdrawToZeroAddress" refactor: alphabetical order * build: bump sphinx * test: remove ERC721Mock and use forge mock * test: update precompiles * chore: say "block timestamp" in comments * refactor: update gas snapshot test: replace "maxUint40" with "maxOfThree" * chore: add trailing slash * build: bump PRBMath --------- Co-authored-by: smol-ninja <[email protected]> Co-authored-by: andreivladbrg <[email protected]>

PaulRBerg added 7 commits April 4, 2024 18:56

docs: improve wording in NatSpec

ac1c4de

chore: improve wording comments chore: misc improvements and renames docs: capitalize ID refactor: improve variable names refactor: separate max segment and tranche counts refactor: use for loop instead of while

build: bump OpenZeppelin to v5.0.2

55532b1

docs: improve writing in NatSpec

1d693b9

perf: optimize "_update" by testing "_isTransferable" last test: apply "inheritdoc" test: define "withdrawAmount" once test: improve function names test: improve writing in comments test: set the license to "UNLICENSED"

refactor: rename "streamingModel" to "sablierModel"

fb34afa

docs: improve writing in NatSpec refactor: rename "sablierAddress" to "sablierStringified"

refactor: rename internal create function

91f1b7e

chore: singular Check, Effect, Interaction

0119dc7

docs: improve writing in custom errors' NatSpec

perf: close #878

839607c

docs: improve writing in NatSpec test: apply "seconds" keyword

PaulRBerg requested review from smol-ninja and andreivladbrg April 5, 2024 16:01

refactor: reorder checks

b510c03

chore: improve writing in comments test: bring back test in "createWithDurations" test: close #880 test: fuzz cliff time as zero test: improve BTs and associated modifiers

PaulRBerg force-pushed the prb-feedback branch from b3529b2 to b510c03 Compare April 5, 2024 16:02

test: remove unused import

e88ef6e

smol-ninja force-pushed the prb-feedback branch from f946df9 to e88ef6e Compare April 6, 2024 09:58

test: include map symbol test for lockup tranched

bda5c85

docs: _create in NatSpec

smol-ninja requested changes Apr 6, 2024

View reviewed changes

src/SablierV2LockupDynamic.sol Outdated Show resolved Hide resolved

test/integration/shared/lockup-linear/createWithTimestamps.t.sol Outdated Show resolved Hide resolved

test/mocks/erc20/ERC20Mock.sol Outdated Show resolved Hide resolved

PaulRBerg mentioned this pull request Apr 7, 2024

Order modifiers alphabetically #884

Closed

revert: bring back GPL V3 license in mocks

5a500f8

chore: fix segments reference fix: mistaken "cliffs" array

refactor: update precompiles

f6909be

smol-ninja reviewed Apr 7, 2024

View reviewed changes

test/utils/Precompiles.t.sol Show resolved Hide resolved

PaulRBerg and others added 3 commits April 7, 2024 15:21

test: reuse "createWithTimestamps" modifiers

70b490f

test: revert max counts to 500

34bf2d4

test: use lockup object

7132b80

smol-ninja approved these changes Apr 7, 2024

View reviewed changes

smol-ninja mentioned this pull request Apr 7, 2024

Order modifiers alphabetically #885

Merged

andreivladbrg requested changes Apr 8, 2024

View reviewed changes

andreivladbrg and others added 2 commits April 8, 2024 13:39

style: correct bad formated natspec

9371cee

refactor: rename "currentTime" to "blockTimestamp"

2ae73ab

docs: improve wording in comments perf: use "unchecked" for tranche calculations refactor: rename variables

PaulRBerg added 3 commits April 8, 2024 17:12

docs: improve writing in NatSpec

49c7bea

refactor: order functions and fields alphabetically

005f43d

refactor: rename variable

fff43d5

smol-ninja reviewed Apr 8, 2024

View reviewed changes

src/SablierV2LockupTranched.sol Outdated Show resolved Hide resolved

docs: capitalize IDs

676fae1

chore: update "filter_paths" docs: improve writing in comments refactor: include "streamId" in "SablierV2Lockup_WithdrawToZeroAddress" refactor: alphabetical order

smol-ninja and others added 5 commits April 9, 2024 10:15

build: bump sphinx

351ae01

test: remove ERC721Mock and use forge mock

188e244

test: update precompiles

59cb669

chore: say "block timestamp" in comments

c805841

refactor: update gas snapshot

13b6c09

test: replace "maxUint40" with "maxOfThree"

andreivladbrg reviewed Apr 9, 2024

View reviewed changes

slither.config.json Outdated Show resolved Hide resolved

chore: add trailing slash

089564f

build: bump PRBMath

12c6b32

smol-ninja approved these changes Apr 9, 2024

View reviewed changes

andreivladbrg approved these changes Apr 9, 2024

View reviewed changes

andreivladbrg merged commit e9fcd6d into staging Apr 9, 2024
9 checks passed

PaulRBerg deleted the prb-feedback branch April 10, 2024 14:14

PaulRBerg mentioned this pull request May 16, 2024

PRB feedback on benchmarks #930

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feedback from PRB on V2.2 #883

Feedback from PRB on V2.2 #883

PaulRBerg commented Apr 5, 2024 •

edited

Loading

smol-ninja left a comment

andreivladbrg commented Apr 6, 2024 •

edited

Loading

PaulRBerg commented Apr 7, 2024

andreivladbrg left a comment

PaulRBerg commented Apr 8, 2024 •

edited

Loading

PaulRBerg commented Apr 8, 2024 •

edited

Loading

PaulRBerg commented Apr 9, 2024

PaulRBerg commented Apr 9, 2024

Feedback from PRB on V2.2 #883

Feedback from PRB on V2.2 #883

Conversation

PaulRBerg commented Apr 5, 2024 • edited Loading

smol-ninja left a comment

Choose a reason for hiding this comment

andreivladbrg commented Apr 6, 2024 • edited Loading

PaulRBerg commented Apr 7, 2024

andreivladbrg left a comment

Choose a reason for hiding this comment

PaulRBerg commented Apr 8, 2024 • edited Loading

PaulRBerg commented Apr 8, 2024 • edited Loading

PaulRBerg commented Apr 9, 2024

PaulRBerg commented Apr 9, 2024

PaulRBerg commented Apr 5, 2024 •

edited

Loading

andreivladbrg commented Apr 6, 2024 •

edited

Loading

PaulRBerg commented Apr 8, 2024 •

edited

Loading

PaulRBerg commented Apr 8, 2024 •

edited

Loading