Conversation
lemunozm
left a comment
lemunozm
left a comment
There was a problem hiding this comment.
Some NITs, but I need to tell you this refactor is awesome!! Much more legible and organized. Really awesome 🙌🏻
| if (c.vaultRegistry != address(0)) { | ||
| _checkWard(c.vaultRegistry, c.messageDispatcher, "vaultRegistry <- messageDispatcher"); | ||
| } | ||
| if (c.hubHandler != address(0)) { | ||
| _checkWard(c.hubHandler, c.messageDispatcher, "hubHandler <- messageDispatcher"); | ||
| } |
There was a problem hiding this comment.
In which case, vaultRegister or hubHandler will be 0? I think never can happen, and if happens, it should be an error. Same for other checks in this file
There was a problem hiding this comment.
You are right. Will remove the if != address(0) guards since _checkWard` already handles zero addresses gracefully. I would keep the guards for adapter checks though since these cannot be assumed to be deployed on all networks.
There was a problem hiding this comment.
Maybe for the adapters we could read the EnvConfig, and perform the exact check in each chain. Just an idea we don't need to implement now
wischli
added
type:feature
lemunozm
left a comment
lemunozm
left a comment
There was a problem hiding this comment.
All changes look good! Just seems the NETWORK env var is not accesible 🤔
Good catch! Should be fixed in 9ef021e |
lemunozm
left a comment
lemunozm
left a comment
There was a problem hiding this comment.
LGTM!
Once approved, only the weekly-validation.yml CI workflow should be merged into main
Just one point. How do you want to merge this PR based on the above?
Happy to discuss this in the sync beforehand. My idea was to reduce this PR just to the |
|
Ok, fine to me! I would expect other actions there apart from validations (for example, the VerifyFactoryContracts script). So maybe we could just name it I'll move the factory verifier there once all of this gets merged |
wischli
force-pushed
the
tests_live-validators
branch
from
f393e5f to
fc7bf5d
Compare
github-actions
bot
added
scope:infra
|
Auto-labeled: |
|
Coverage after merging tests_live-validators into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
2 participants
Merge strategy
This PR targets
mainfor review purposes. Once approved, only theweekly-validation.ymlCI workflow should be merged intomain. The validator and investment flow test code should be pushed to the separatelive-validatorsbranch, which is pinned to the latest deployed version and runs on a weekly schedule. This keepsmainfree of fork test code that is tightly coupled to the currently deployed contracts. Which was an issue we discussed in the past.Product requirements
ForkTestLiveValidation.sol(1805 lines) with modular validator architecture: One validator per concern, indexer-driven, zero hardcoded addressesVaultRegistryrouting corruption from theunlinkVaultbug (ref)Design notes
BaseValidatorsubclasses, each querying the GraphQL indexer for its data. Adding a validator = one file + one array entry inLiveValidationForkTest.InvestmentFlowExecutorhandles local async, cross-chain async, and cross-chain sync deposit flows with shared redeem logic.weekly-validation.ymlonmainchecks out thelive-validatorsbranch on a Monday schedule via GCP-authed fork tests across 9 networks.Notes