Test the code identity between the Lockup contracts

[PaulRBerg]

Problem

Because Solidity does not have anything equivalent to Rust traits, we are forced to duplicate any logic that touches upon different structs despite the logic being completely identical.

• _cancel
• _renounce
• _withdraw

Technically speaking, there is a way out, but it's ugly and not worth it. We could implement a setter for every struct field (e.g., setIsCancelable, setWasCanceled, etc.), but:

1. Doing so would double the number of lines of code written for implementing the cancel and withdraw functionality
2. The gas cost would increase (there would be more internal calls, some of which may not be inlined)
3. There would still be a lot of duplicated code.

Solution

Introduce a new class of tests, i.e. differential tests, that perform the following checks:

1. Read the LockupLinear and the LockupDynamic contracts (and potentially LockupTranched in the future)
2. Compare the implementations of the _cancel and the _withdraw functions. They should be 100% identical, down to the English comments.
   • The awk utility might be helpful here

The tests would use Bash scripting and the ffi cheat code to read the file contents.

Alternatively, if implementing this in Forge is too complicated, we can write a CI check. There should be GitHub Actions for performing diffs between files.

References

• https://book.getfoundry.sh/forge/differential-ffi-testing
• https://twitter.com/PaulRBerg/status/1531652974161838082
• perf: dry "isCold" and "isWarm" #664 (comment)

[andreivladbrg]

Given how our test structure is designed (shared dir) is the code identity test really needed?

We are running the same tests for these functions:cancel, renounce and withdraw in both contracts, so if there were to be difference between the implementations, the tests would fail for that contract (ofc assuming we have a 100% coverage for each function and we are testing what each line does).

I believe this test would be helpful only if we want to check if the comments are identical, thus we should lower the priority.

Regarding the test method, using forge seems to be an overkill. Adding a Bash script in the shell directory, which should be run during CI, would be more appropriate.

[PaulRBerg]

Yes, performing these code identity checks would still be helpful even if we run the same tests for these contracts.

1. Code identity is different from functional logic. The same test can pass for two different implementations. E.g., 1 + 3 = 2 +2. So it's not just about the comments.
2. Fallibilism leads us to assume that we have made errors in our test suites (which are large and complicated). These code identity checks would be comparatively smaller, providing us additional confidence that everything works as expected.

Regarding the test method, using forge seems to be an overkill. Adding a Bash script in the shell directory, which should be run during CI, would be more appropriate

Fair enough

[andreivladbrg]

@PaulRBerg since we implemented #813 should we close this issue?

[PaulRBerg]

Not sure. Isn't there duplicated code still?

If there is, we should keep this issue open (though revise the description in light of the new spec).

[andreivladbrg]

Just these lines, but I don't think they count

Linear:

v2-core/src/SablierV2LockupLinear.sol

Lines 238 to 244 in aabd935

	// Safe Interactions: query the protocol fee. This is safe because it's a known Sablier contract that does
	// not call other unknown contracts.
	UD60x18 protocolFee = comptroller.protocolFees(params.asset);
	// Checks: check the fees and calculate the fee amounts.
	Lockup.CreateAmounts memory createAmounts =
	Helpers.checkAndCalculateFees(params.totalAmount, protocolFee, params.broker.fee, MAX_FEE);

Dynamic:

v2-core/src/SablierV2LockupDynamic.sol

Lines 319 to 325 in aabd935

	// Safe Interactions: query the protocol fee. This is safe because it's a known Sablier contract that does
	// not call other unknown contracts.
	UD60x18 protocolFee = comptroller.protocolFees(params.asset);
	// Checks: check the fees and calculate the fee amounts.
	Lockup.CreateAmounts memory createAmounts =
	Helpers.checkAndCalculateFees(params.totalAmount, protocolFee, params.broker.fee, MAX_FEE);

Everything else is completely different

[PaulRBerg]

Yeah, they don't count, especially since we might end up removing protocol fees.