fix: align DOS Testnet deployment with current chain - #7
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the DeployDOSDomainPolicyTestnet script to load contract addresses and configurations dynamically from environment variables instead of using hardcoded constants. It also updates the expected deployer address across the testnet deployment script, PowerShell invocation script, and unit tests. Feedback is provided to add a zero-address validation check for the voucher signer in the preflight function to prevent the script from reverting after broadcasting has already started.
| function preflight(address broadcaster, DeploymentConfig memory config) internal view { | ||
| if (block.chainid != EXPECTED_CHAIN_ID) { | ||
| revert UnexpectedChain(block.chainid, EXPECTED_CHAIN_ID); | ||
| } | ||
| if (broadcaster != EXPECTED_OWNER) { | ||
| revert UnexpectedOwner(broadcaster, EXPECTED_OWNER); | ||
| } | ||
| if (DOS_REGISTRY.code.length == 0) { | ||
| revert MissingContractCode(DOS_REGISTRY); | ||
| if (address(config.registry).code.length == 0) { | ||
| revert MissingContractCode(address(config.registry)); | ||
| } | ||
| if (PRICE_ORACLE.code.length == 0) { | ||
| revert MissingContractCode(PRICE_ORACLE); | ||
| if (address(config.priceOracle).code.length == 0) { | ||
| revert MissingContractCode(address(config.priceOracle)); | ||
| } | ||
| if (LEGACY_DOS_REGISTRAR.code.length == 0) { | ||
| revert MissingContractCode(LEGACY_DOS_REGISTRAR); | ||
| if (config.legacyRegistrar.code.length == 0) { | ||
| revert MissingContractCode(config.legacyRegistrar); | ||
| } | ||
| if (!PermissionedRegistry(DOS_REGISTRY).hasRootRoles(REGISTRAR_ROLES, LEGACY_DOS_REGISTRAR)) { | ||
| revert LegacyRegistrarAlreadyRetired(LEGACY_DOS_REGISTRAR); | ||
| if (!config.registry.hasRootRoles(REGISTRAR_ROLES, config.legacyRegistrar)) { | ||
| revert LegacyRegistrarAlreadyRetired(config.legacyRegistrar); | ||
| } | ||
| } |
There was a problem hiding this comment.
In preflight, config.voucherSigner is not validated to ensure it is not address(0). If DOS_DOMAIN_VOUCHER_SIGNER is misconfigured or set to the zero address, the script will start broadcasting and then revert during the deployment of DosDomainPolicy (which checks for address(0) in its constructor). Adding a check for config.voucherSigner != address(0) in preflight prevents starting a broadcast with an invalid configuration.
function preflight(address broadcaster, DeploymentConfig memory config) internal view {
if (block.chainid != EXPECTED_CHAIN_ID) {
revert UnexpectedChain(block.chainid, EXPECTED_CHAIN_ID);
}
if (broadcaster != EXPECTED_OWNER) {
revert UnexpectedOwner(broadcaster, EXPECTED_OWNER);
}
if (config.voucherSigner == address(0)) {
revert("ZeroAddress");
}
if (address(config.registry).code.length == 0) {
revert MissingContractCode(address(config.registry));
}
if (address(config.priceOracle).code.length == 0) {
revert MissingContractCode(address(config.priceOracle));
}
if (config.legacyRegistrar.code.length == 0) {
revert MissingContractCode(config.legacyRegistrar);
}
if (!config.registry.hasRootRoles(REGISTRAR_ROLES, config.legacyRegistrar)) {
revert LegacyRegistrarAlreadyRetired(config.legacyRegistrar);
}
}
Summary\n- Align the Testnet baseline deployment preflight with the current 3939 genesis and funded canonical owner.\n- Read policy dependency addresses from runtime configuration and protect policy deployment with RPC/genesis, code, role-admin, balance, and simulation guards.\n- Update Testnet deployment fixtures and stabilize the Devnet sync E2E timestamp assertion.\n\n## Verification\n- forge build\n- forge test --match-path test/unit/testnet/DeployDOSDomainPolicyTestnet.t.sol\n- forge test --match-path test/unit/testnet/WrappedDOS.t.sol\n- forge test --match-path test/unit/registrar/DosDomainPolicy.t.sol\n- policy invoker required-env and noncanonical-signer fail-closed checks\n- GitHub CI: all prior jobs passed except Devnet sync E2E had a one-second timestamp race; this commit corrects that assertion and CI is being rerun.\n\nMainnet is unchanged.