test: adding codebase/deployed tests scenarios [PoC]

[wei3erHase]

The objective of this PoC strategy, is to enable both testing scenarios while developing and debugging the spells. Benefitting from contract inheritance, the testing codebase would be exactly the same for one or another, without requiring to multiply codebase (which is a pain to maintain).

When working on a spell, a developer may need to make some changes to debug what's happening on chain, and the current workflow for him is to change the deployed address to address(0) to declare that the test suite should read (and deploy) the codebase spell, instead of reading the forked one.

Forgetting to delete this address, could result in a frustrating DX experience, while developer is making changes to the codebase, but no changes in tests appear. Instead, with the current PoC, the deployer could separate which test to perform, doing either make test match=Deployed or make test match=Codebase, or enable the CI to run different scopes at different steps.

Also, this strategy, could benefit from running certain tests only when it makes sense to run, instead of hacking that test to pass even when it's run on the Codebase, for example, testBytecode in this PoC is set to run only on the Deployed scope (notice: it has 23 tests, while Codebase has 22).

Of course, this needs to be addressed after optimizing the tests in such a way that they don't last minutes, but afterwards, the benefits for the developer or the CI process could overweight the waiting period of running these "duplicated" tests.

[amusingaxl]

Hey, thanks for the contribution.

A couple of observations here:

Forgetting to delete this address, could result in a frustrating DX experience, while developer is making changes to the codebase, but no changes in tests appear.

The spell crafter is expected to follow the Crafter's Checklist. In that document, there is an explicit set of steps that would prevent this issue.

For an outsider, it might be not that obvious that such a checklist exists, but the spell teams make sure to go through them when onboarding a new team member or a new spell team.

In the recent past, we briefly discussed the possibility of migrating the spell-related checklist to this very repo, so it becomes self-contained, but we didn't reach a conclusion back then. Maybe we should revisit that.

doing either make test match=Deployed or make test match=Codebase

That would be yet another thing crafters and reviewers would have to keep in mind: "am I running the tests for a new spell or for an existing one?". This adds to the already huge pile of cognitive overhead involved in the process.

Personally, I would try to avoid that as much as possible unless there is a significant gain in terms of security.

The tests that don't make much sense during the development cycle, such as testByteCodeMatches, are usually cheap to run, so there is no problem of running them all the time. We could potentially leverage the skip(bool) cheatcode to detect if it should be skipped for explicitness' sake, but I wouldn't try to extract the different execution paths into different contracts, specially using inheritance, which is more often than not a code smell.

[wei3erHase]

ey @amusingaxl thanks again for the review and detailed answer! ❤️
yess i'm aware of the checklists of steps to be followed on the spell creation process, thanks for pointing them, am slowly onboarding myself in the whole workflow :)

That would be yet another thing crafters and reviewers would have to keep in mind: "am I running the tests for a new spell or for an existing one?". This adds to the already huge pile of cognitive overhead involved in the process.

this could be programatically skipped using #379, with a whenDeployed modifier if the spell address is 0x0 for example

the cognitive overhead reduction would be visible on the test logs:

• before deployment all "Deployed" tests should look like [SKIP] codebase should [PASS]
• after deployment all "Deployed" and "Codebase" tests should [PASS]

are usually cheap to run, so there is no problem of running them all the time. We could potentially leverage the skip(bool) cheatcode to detect if it should be skipped for explicitness' sake

I agree that explicitness in this test would be a step forward in security, given that it adds a layer of extra complexity to PASS when the spell is not deployed, and if for some reason, after deployment the test suite is being run on the Codebase (and not the Deployed scope), the reviewer will see a ✅ , on a very critical test, that could potentially jeopardize the whole protocol (if for example, the DssExecLib differs in the deployed spell than in the codebase one).

under the same criteria, the rest of the tests could benefit from being redundantly called, despite the fact that if bytecode matches, the contracts should behave exactly the same, I give you the point there that, this is a pretty safe assumption to make

[amusingaxl]

this could be programatically skipped using #379, with a whenDeployed modifier if the spell address is 0x0 for example

That is not a bad idea! We could implement that if we decide go with the explicit skip.

after deployment the test suite is being run on the Codebase (and not the Deployed scope), the reviewer will see a ✅ , on a very critical test, that could potentially jeopardize the whole protocol

I understand where you stand on this, but please notice that the deployment of the spell is handled by the deploy.sh script, which automatically updates the config.sol file with the information about the deployed spell.

There are also some items in the reviewer check list for the Deployed Stage that would catch such problem.