Dry fy shared logic and add util Modifiers contract

[andreivladbrg]

Changes

Closes #1028 and #1045

---

Motivation

There are two things that this PR should solve:

1. Making refactoring the tests a much easier process
2. New readers will spend much less time understanding the test structure (which is currently very convoluted, as we’ll see below)

About the refactor process

I will start with what this PR began with, which is adding the Modifiers utility contract. Currently, in the stating
branch, we have two possible places where we declare our modifiers:

1. In the test files themselves:

v2-core/test/core/integration/concrete/lockup/burn/burn.t.sol

Line 38 in a322ba6

modifier givenNotNull() {

2. In the shared files between integration tests (1. concrete, 2. fuzz):

v2-core/test/core/integration/shared/lockup/createWithTimestamps.t.sol

Line 13 in a322ba6

modifier whenAssetContract() {

Some of the modifiers contained logic that assumed some setup beforehand, but now, since the Modifier contract is the
parent (in terms of inheritance), they can't have that logic anymore.

Also, after moving all the modifiers to the new contract mentioned above, I ended up with shared files that contained
only two lines of code, e.g. this one:

v2-core/test/core/integration/shared/lockup/streamedAmountOf.t.sol

Lines 7 to 11 in a322ba6

	uint256 internal defaultStreamId;
	function setUp() public virtual override {
	defaultStreamId = createDefaultStream();
	}

Which made me question whether we really need these shared files. The purpose of these files is to have a unique setup
or specific helper logic between <function_name>_Concrete_Test and <function_name>_Fuzz_Test (which we’ll call 'Type of test'). However, this was not the case for the vast majority of them, as they only contained duplicated logic throughout
the test files. Faced with this problem, I asked myself where the best place to put this duplicated logic would be, and
the answer is Integration_Test. But once I started doing this, I realized there was another component in this
refactor: Lockup_Integration_Shared_Test, which complicates things further from an inheritance tree standpoint as the only purpose of it is to declare some virtual functions, which are implemented later (LL/LD/LT).

There are a lot of pieces in this puzzle that don’t need to be divided into so many parts, as it makes 1) making changes
to the code and 2) understanding the code for new readers more fragile.

At this point, I really had to understand our inheritance tree of a lockup function, and I came up with this diagram:

Click to see diagram

flowchart TD
    FunctionName_LockupModel_Integration_TypeTest --> LockupModel_Integration_TypeTest
    FunctionName_LockupModel_Integration_TypeTest --> FunctionName_Integration_TypeTest
    LockupModel_Integration_TypeTest --> Integration_Test
    LockupModel_Integration_TypeTest --> LockupModel_Integration_Shared_Test
    FunctionName_Integration_TypeTest --> Integration_Test
    FunctionName_Integration_TypeTest --> FunctionName_Integration_Shared_Test
    LockupModel_Integration_Shared_Test --> Lockup_Integration_Shared_Test
    FunctionName_Integration_Shared_Test --> Lockup_Integration_Shared_Test
    Lockup_Integration_Shared_Test --> Base_Test
    Integration_Test --> Base_Test
    Base_Test --> Utils

Loading

After seeing this, we can conclude some obvious things:

1. Lockup_Integration_Shared_Test and Integration_Test share the same depth, indicating that they can be abstracted into a single test.
2. Both LockupModel_Integration_TypeTest and FunctionName_Integration_TypeTest depend on Lockup_Integration_Shared_Test and Integration_Test, making the organization difficult to follow.

This structure adds unneeded complexity, it's like trying to reach your left ear with your right hand.

By abstracting the Lockup_Integration_Shared_Test's logic into Integration_Test, we can remove LockupModel_Integration_TypeTestcompletely, as its only purpose is to call the setup functions from the left side (LockupModel_Integration_Shared_Test) and the right side (Integration_Test) of the diagram. The resulting inheritance diagram would look like this:

Click to see diagram

flowchart TD
    FunctionName_LockupModel_Integration_TypeTest --> LockupModel_Integration_Shared_Test
    FunctionName_LockupModel_Integration_TypeTest --> FunctionName_Integration_TypeTest
    LockupModel_Integration_Shared_Test --> Integration_Test
    FunctionName_Integration_TypeTest --> Integration_Test
    Integration_Test --> Base_Test
    Base_Test --> Utils

Loading

As you can see, it is much simpler now to follow.

Click to see test tree structure comparison

Before PR

test/core/integration/shared/
├── lockup
│   ├── Lockup.t.sol
│   ├── cancel.t.sol
│   ├── cancelMultiple.t.sol
│   ├── createWithDurations.t.sol
│   ├── createWithTimestamps.t.sol
│   ├── getWithdrawnAmount.t.sol
│   ├── streamedAmountOf.t.sol
│   ├── withdraw.t.sol
│   ├── withdrawMax.t.sol
│   ├── withdrawMaxAndTransfer.t.sol
│   ├── withdrawMultiple.t.sol
│   └── withdrawableAmountOf.t.sol
├── lockup-dynamic
│   └── LockupDynamic.t.sol
├── lockup-linear
│   └── LockupLinear.t.sol
├── lockup-tranched
│   └── LockupTranched.t.sol
└── nft-descriptor
    └── NFTDescriptor.t.sol

After PR

test/core/integration/shared/
├── lockup
│   ├── cancelMultiple.t.sol
│   └── withdrawMultiple.t.sol
├── lockup-dynamic
│   └── LockupDynamic.t.sol
├── lockup-linear
│   └── LockupLinear.t.sol
├── lockup-tranched
│   └── LockupTranched.t.sol
└── nft-descriptor
    └── NFTDescriptor.t.sol

---

Conclusion

This PR will bring improvements for both the Sablier team and external contributors, enhancing collaboration and code quality.

[smol-ninja]

@andreivladbrg can you please update this PR? I would like to review it now.

[andreivladbrg]

@smol-ninja it is updated now, you can start the review 👌

[smol-ninja]

Good work @andreivladbrg. I have reviewed the core tests. I will review periphery after you have replied to my comments below. Two things I want to mention:

1. Even after closing all the tabs, the browser was working slow because of large number of changes. This is a good argument in favour of why we should split large PRs like this into multiple PRs so that each PR has changes in not more than 50 files. An idea could be to have one PR for core test, another PR for periphery test etc.

2. Most of the comments below apply throughout the code. So if we decide to resolve one comment by making proposed changes, they will also apply to other parts of the diff where I have not commented.

[andreivladbrg]

Even after closing all the tabs, the browser was working slow because of large number of changes. This is a good argument in favour of why we should split large PRs like this into multiple PRs so that each PR has changes in not more than 50 files. An idea could be to have one PR for core test, another PR for periphery test etc.

I’m so sorry to hear the review process was difficult for you. I agree that breaking down work into multiple PRs can make reviews smoother (remember the package tethering PRs?). In this case, however, the process was challenging for me as well. Unlike the package tethering, where I had a clear understanding of the necessary changes from the outset, this time, every adjustment seemed to cause something else to break.
In the future, I’ll do my best to make the review process easier. This experience was tough for both of us and highlights how exhaustive the test structure really was (and how needed this change was).

---

@smol-ninja I reintroduced the defaults and users variables in the Modifiers contract, which should address most of your feedback. Could you please take another look at my latest commit 91f4d50 ?

[PaulRBerg]

Also: Loom videos for explaining stuff in large PRs would be helpful.

[smol-ninja]

I re-reviewed the core tests again and discovered a few more areas where changes may be required. I also found redundant codes which I removed in f28239b. The commit includes some changes related to coding pattern. Let me know if you have any question about it.

I will be reviewing periphery now.

[smol-ninja]

Here is periphery review.

[smol-ninja]

[andreivladbrg]

It was journey, thank you again Shub🚀