search

sablier-labs / v2-core

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

Test the code identity between the Lockup contracts #771

Closed PaulRBerg closed 8 months ago

PaulRBerg commented 9 months ago

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.

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 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

andreivladbrg commented 9 months ago

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 commented 9 months ago

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 commented 8 months ago

@PaulRBerg since we implemented https://github.com/sablier-labs/v2-core/pull/813 should we close this issue?

PaulRBerg commented 8 months ago

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 commented 8 months ago

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

Linear:

https://github.com/sablier-labs/v2-core/blob/aabd9356b3120aaecc3f4f95df14bd1957135085/src/SablierV2LockupLinear.sol#L238-L244

Dynamic:

https://github.com/sablier-labs/v2-core/blob/aabd9356b3120aaecc3f4f95df14bd1957135085/src/SablierV2LockupDynamic.sol#L319-L325

Everything else is completely different

PaulRBerg commented 8 months ago

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