Repository navigation
Conversation
github-actions
Bot
added
audit:needed
|
Auto-labeled: |
wischli
removed
audit:needed
|
Amazing! Just as NOTE, we'll need to move later the To be care: we're adding |
Yeah I assumed it was intended to be missing for a less complicated repo restructure.
Why omit the connections commit? It represents the live condition and without it, the fork validation correctly logs issues for the ETH<>Pharos lane to a mismatch in live to local configuration of the adapter setup. I rather regard this as an oversight we missed to apply to live branch. |
Agree it should be reachable from this branch, but not in this PR. The ideal workflow should be:
|
lemunozm
left a comment
lemunozm
left a comment
There was a problem hiding this comment.
I'm re-reviewing this, and not sure if we're mixing architectures with tests and validators.
Your current set up extends the spell test with validators. It runs the validators from the test.
My last thought about the validators were that they could be fully split from tests. I think one thing is "test the spell itself" and another thing is "validate the environment after the spell".
In the Validator example the idea was the following:
// test/integration/spell/example-spell/ExampleTest.t.sol
contract ExampleTest {
string chainName = "ethereum";
BaseValidator[] pre;
BaseValidator[] cache;
BaseValidator[] post;
constructor() {
// Add pre validators:
pre.push(new Validate_PreExample());
// Add cache validators:
cache.push(new Validate_CacheExample());
// Add post validators:
post.push(new Validate_PostExample());
}
function testExample() external {
ValidationExecutor executor = new ValidationExecutor(chainName, "example");
executor.runPreValidation(pre, false);
executor.runCacheValidation(cache);
// Here goes the spell
executor.runPostValidation(post, testContractsFromConfig(Env.load(chainName)));
// You can also use testContractsFromConfig(fullDeployer)
}
}In this case, and in my head (open to listen your thoughts), I would leave the V2Cleanings.t.sol as they were, just focus on tests. And I would add a new ValidatorTest like the above, just calling the spell execution.
Wondering also if you found some blocker with this schema
|
@lemunozm You're right. I rushed in order to prio quick validatio. The current shape conflates the two and that's the right thing to pull apart. Three things shaped the current PR that I don't think your sketch covers as-stated, so I'd want to fold them in rather than just split the test:
IMO, the rough plan building on your sketch would be:
Future spells then become a spell test file (focused, like today) + validator test file (thin orchestrator using the new framework methods + spell-specific pre/post validators). For demonstration/reference purposes, I would migrate V2Cleanings to the new pattern here. WDYT? |
|
Thanks for showing this points! True we can iterate forward this scheme. I agree with your proposal. Just regarding point 1. I think we can impl Regarding point 2. Also unsure right now how to fit this, so let's iterate with your thoughts. Regarding point 3. I think you just need to pass the current contracts already exists ( |
github-actions
Bot
added
audit:needed
|
Thanks @lemunozm, all implemented. V2Cleanings is migrated to the new pattern and green on eth/base/arb (focused spell test + validator regression test),
With the per-error diff in, One deviation from what I sketched: returning For point 2, it ended up as a
Agree. Added a Also added an author guide for the pattern to the validation README. The 009 Plume adapter spell could be the second consumer ( WDYT on the executor-storage baseline shape? |
| ├── V2CleaningsValidatorTest.t.sol # extends SpellRegressionTest | ||
| └── validators/ | ||
| └── Validate_Example.sol # Example: PRE query, cache, POST read | ||
| └── Validate_V2Cleanings.sol # Pre (soft) / Cache / Post (hard) validators |
There was a problem hiding this comment.
NIT. Maybe we don't need to maintain the current set up here. Initially this was to show how to organize things, but does not need a real representation. we can just write:
└── <other>/
There was a problem hiding this comment.
Yeah, definitely can be removed now as it was just a showcase that I want to ref to Claude when doing the next spell post spell fork assertion.
| # Spell Validation Framework | ||
|
|
||
| Validates Centrifuge protocol state before and after executing governance spells. Queries the GraphQL indexer to ensure no blocking operations exist (PRE) and that state is correctly preserved (POST). | ||
| Validates Centrifuge protocol state before and after executing governance spells. Queries the GraphQL indexer to ensure no blocking operations exist (PRE) and that state is correctly preserved (POST), and provides a spell-agnostic **environment regression** harness (`SpellRegressionTest`) that tolerates pre-existing live errors and fails only on regressions a spell introduces. |
There was a problem hiding this comment.
I think the changes in this README and base validators should be done in main instead. Only validators that really use the current ABI should be in live, WDYT?
In my mind everything under utils should be generic enough to be in main, and everything under v2-cleanings should be in live.
Wondering if EDIT: My bad, it's ABI independentFlowRegression should also be in a subfolder like v2-cleanings. Looks like this is an actual validator in some way. At least it's attached to some ABI, IIUC.
There was a problem hiding this comment.
Agree with the split in principle, generic framework in main, ABI-bound validators in live.
However, none of it exists on main today, test/integration/spell/utils and test/integration/fork are live-only. So this is less "move this PR's README and base validator delta over" and more "port the whole framework", i.e. BaseValidator, ValidationExecutor, TestContracts, InvestmentFlowExecutor, example-spell and the README.
Two ABI couplings to solve in that port. SpellRegressionTest's default _structuralValidators() imports the 8 fork validators, which by your own criterion stay in live, so on main that default would need to become abstract with live providing the set. And TestContracts plus InvestmentFlowExecutor compile against whatever src they sit on, on main they would track the dev ABI while spells always run against the deployed one, so we would pay for keeping them compiling on both branches.
I'm not sure what the preferred order is TBH
- Either we merge this PR without any V2Cleanings content against
live, then rebaseliveagainstmainand cherry-pick relevant validators fromlivetomain - Or we push validation infra to
mainand the actual fork validators toliveafter rebasing tomain.
The second one is cleaner probably but requires more work.
There was a problem hiding this comment.
However, none of it exists on main today, test/integration/spell/utils and test/integration/fork are live-only
AFAICT, test/integration/spell/utils already exists in main. At least the validator infrastructure part.
No strong opinion about the preferred order at all! What is simple for you. As far as we get things correctly placed I'm fine with both solutions 😄
Everything from your last comment looks good to me! Just I don't really follow what do you refer with this, can you expand? |
Sorry, should have expanded on that. "Executor-storage baseline" is how the structural validator baseline survives the spell cast. The regression diff needs the pre-cast errors available post-cast. The natural API (capture returns exec = new ValidationExecutor(network, "v2cleanings");
exec.captureErrorBaseline(validators); // pre-cast: runs validators, stores their error keys in exec storage
spell.cast();
exec.runValidationDiffPost(freshValidators); // post-cast: re-runs, diffs against the stored keysPost-cast errors whose key is in the baseline are PRE-EXISTING (tolerated), new keys are REGRESSION (test fails), baseline keys that disappeared are IMPROVED. Nothing nested ever crosses an ABI boundary and nothing is stored to the disk. The spell-specific cache validators still use the file-backed The question was just whether you are fine with that shape vs e.g. file-caching the baseline too. Happy to adjust if you prefer the cache route, but IMO storage is the simpler one since it needs no key management and no cleanup. |
|
Ok, I think now understand! Yeah, I think the storage is good. The downside will be that we need to handle it in the infrastructure, right? But I think this is ok. It's just another utility for validator implementators. Maybe something to consider. What if for the spell we want to test it in an anvil fork? In that scenario spell.cast() will be in a separated environment and we'll need anyways to use the file-catching the stuff. We had similar issues in the v3.1 migration with anvil, because the tx are called from outside |
|
Coverage after merging live_refactor-fork-validation into live will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Good catch, that's the right scenario to poke at 😅 For the in-process forge test we have today, there's no infra to handle. The executor is created, writes its baseline, and gets diffed all in one It's true your anvil case is different and you're right there. If Though IMO moving to that model is more than the baseline. |
|
Seems fair to me! |
|
Closing because outdated |
2 participants
Please note all staged
V2Cleanings*.solwill be removed before merging this PR such that only these commits remain:Product requirements
SpellForkTestpre/post validation framework toV2CleaningsSpellTestso the spell cast is verified against the live-state baseline.V2CleaningsSpellas named constants with source references so the spell script and test can use them without re-declaring addresses locally.Design notes
ValidationExecutorgains arunValidationCountErrorsmethod: runs validators without reverting and returns the total error count.SpellForkTestuses this to snapshot the pre-cast error count and assert it did not increase post-cast — tolerating pre-existing live errors and only failing on regressions the spell introduces.SpellForkTestis a new abstract base (intest/integration/spell/utils/) that spell fork tests inherit instead ofTest. It calls_castSpell(child-defined) between a pre/post validator snapshot and an investment-flow diff._customPostAssertionsis an optional hook for spell-specific checks.V2CleaningsSpellTestis migrated fromTesttoSpellForkTest: the old flat_testCaseis replaced by_castSpell+_customPostAssertions; balance snapshots (_preTreasuryUsdc, etc.) are captured in instance variables during_castSpelland consumed in_customPostAssertions.How to validate the V2 Cleanings Spell
You can check the V2Cleanings Fork CI checks in the Github Runner.
But to save you time, here are the important details.
[PRE-EXISTING]means the failure was there both before AND after the spell ran. It is not gone post-cast; the spell simply didn't introduce or fix it.Here's an excerpt:
Ethereum
The vault warnings also happen pre-validation, so no new issue. They are tracked here.
Base
Arbitrum