[robertocarlous]

Closes #244

Summary

• Adds dincli system verify-deployment: a read-only command that reads foundry/deployments/<network>.json and checks six categories against the live chain, reporting [OK] / [DRIFT] / [ERROR] / [INFO] per item. Exits 0 on all-OK, 1 on any DRIFT or ERROR.
• Extends DeployPlatform.s.sol and UpgradePlatform.s.sol to write implementation addresses, extcodehashes, deployer, and optional git commit hash into the JSON at deploy time — giving the drift check a source of truth to compare against.

Check categories

#	Check	DRIFT condition
1	EIP-1967 implementation slot	Slot value ≠ dinXxxImpl
2	EIP-1967 admin slot	Slot value ≠ proxyAdminXxx
3	extcodehash(impl)	≠ dinXxxImplCodehash in JSON
4	ProxyAdmin.owner()	≠ recorded owner
5	DinCoordinator wiring	dinToken / dinValidatorStake / dinModelRegistry mismatch
6	Slasher authorizations	Expected address not authorized (only with --slashers)

Changes

dincli/cli/system.py

• New verify-deployment command with --file, --no-color (auto-detected from isatty()), and --slashers flags.
• Uses ctx.obj.w3 and ctx.obj.network directly — no wallet needed.

foundry/script/DeployPlatform.s.sol

• Refactored _writeDeployments to accept the Deployment struct + deployer.
• Now writes dinXxxImpl (implementation address via Upgrades.getImplementationAddress), dinXxxImplCodehash (via assembly extcodehash), owner (deployer/ProxyAdmin owner), and deployedAtCommit (GIT_COMMIT_HASH env var, empty string if unset).

foundry/script/UpgradePlatform.s.sol

• After upgradeProxy(), patches the relevant impl address, implCodehash, and commit back into the JSON via vm.writeJson.

tests/test_verify_deployment.py

• 16 unit tests using a DummyWeb3 that mocks eth.get_storage_at, eth.get_code, and eth.contract, following the pattern of tests/test_dintoken.py.
• Covers: all-OK exit 0, DRIFT on impl/admin/codehash/owner mismatch, slasher OK and DRIFT paths, file-not-found and malformed-JSON error paths.

Documentation/public/guides/verify-deployment.md — new usage guide.

Developer/BACK_LOG.md — BL-33 flipped to ✅ Fixed.

Test plan

• python3.12 -m pytest tests/test_verify_deployment.py — 16/16 pass
• python3.12 -m pytest tests/test_dintoken.py tests/test_ipfs_config.py — all pass (no regressions)
• forge build in foundry/ — exits 0 (DeployPlatform and UpgradePlatform compile cleanly)
• Note: conflicts expected with PR test(adversarial): P3Adversarial.t.sol — task_240926_17 Part 2 #218 (dincli/cli/system.py) and PR feat(stake): cross-model S5 escalation (#193) #236 (foundry/script/DeployPlatform.s.sol) — both open. The system.py change is additive (new command only, no edits to existing commands); the DeployPlatform change is isolated to _writeDeployments. Will rebase after those land.

Closes #244

[robertocarlous]

Cc: @umeradl

[Santiagocetran]

Thanks, Robbert. Reviewed commit 04ae2dd against issue #244 and its approved amendments. I found three points to address before approval.

1. P2: Use the actual contract getters for wiring checks.

The checker calls dinValidatorStake() and dinModelRegistry() on DinCoordinator, but the contract exposes dinValidatorStakeContract() and no dinModelRegistry() getter. This can report an error for a correctly configured deployment. The mocked tests provide those same nonexistent getters, hiding the mismatch.

Please use getters and relationships present in the actual contracts, and add regression coverage that matches the real ABI.

2. P2: Allow normal CLI invocation without a wallet.

verify-deployment is missing from the system callback’s account-free command list, so the callback loads an account before this read-only command runs. The tests invoke the function directly and bypass that callback.

Please skip account initialization for this command and add a CLI-dispatch test with RPC configured but no wallet/private key.

3. P2: Complete the agreed ownership and wiring checks.

Issue #244 includes owner() checks on ownable application contracts and fee-router/treasury wiring. The approved amendment adds ProxyAdmin ownership checks rather than replacing the original requirements.

The implementation only checks ProxyAdmin.owner() and does not compare FeeRouter.treasury() against the recorded treasury. Changes to application ownership or the fee router’s treasury destination therefore go unchecked.

Please implement these agreed comparisons with their expected values, or explicitly agree a narrower scope before marking #244/BL33 complete.

These findings are based on inspection of the implementation, tests and approved scope.

Please address these findings and sync the branch with develop. The updated head, fresh CI and remaining verification need to be checked before approval.

[robertocarlous]

All three P2 findings from the re-review have been addressed in cca4eece:

Finding 1 — wrong getter names in _WIRING_ABI
_WIRING_ABI now matches the actual DinCoordinator public getters: dinToken(), dinValidatorStakeContract(), and feeRouter(). The old names (dinValidatorStake, dinModelRegistry) would have caused every wiring call to revert. The wiring comparison also now checksums both sides (to_checksum_address(recorded) == to_checksum_address(actual)) so mixed-case inputs from ABI decoders or fixtures never produce false DRIFT.

Finding 2 — verify-deployment missing from account-free command list
"verify-deployment" is now in the explicit account-free list in the system callback (line 75). TestAccountFreeDispatch::test_no_wallet_loaded confirms invoking the command through the Typer app never calls get_en_w3_account_console.

Finding 3 — missing application proxy owner() and FeeRouter.treasury() checks

• Check 4b now iterates _PROXIES and calls owner() on each proxy address directly, emitting OK/DRIFT per proxy.
• Check 5b calls FeeRouter.treasury() and compares against the recorded dinTreasury address, with a zero-address guard that emits WARN and skips rather than erroring when dinFeeRouter is unset.

All 22 tests pass (pytest tests/test_verify_deployment.py).

[robertocarlous]

Cc: @Santiagocetran pls can you review this again

[Santiagocetran]

Thanks, Robbert. Rechecked d5895de. The getter correction, account-free dispatch and application-owner checks address those findings. The treasury comparison also detects mismatches with a complete deployment record.

Two points remain before approval:

1. The FeeRouter.treasury comparison is inside the coordinator block. A file recording dinFeeRouter and dinTreasury but omitting dinCoordinator skips this comparison and can exit 0 despite treasury drift. Please move the independent comparison outside that guard and add a regression. This concerns partial records, not the complete file generated by DeployPlatform.

2. #244 and the guide still include registry wiring, but the checker doesn’t verify it. Please cover the agreed relationships through the registry’s actual getters, or agree a narrower scope and update the documentation accordingly.

Please also sync with current develop and obtain fresh CI. Run 40082902897 passed against the earlier baseline. The new deploy/upgrade metadata-writing path still needs verification: the existing script tests avoid run() and don’t exercise that JSON-writing cycle.

These findings are based on source inspection, observed CI and the existing isolated simulation, not a live-chain deployment test.

[robertocarlous]

Addressed in 12f01d8a.

FeeRouter.treasury inside coordinator block

The treasury comparison is now an independent block (Check 5b) — it runs whenever dinFeeRouter and dinTreasury are present in the deployments file, regardless of whether dinCoordinator is recorded. The regression test_fires_without_coordinator confirms that the check fires and reports [OK] when dinCoordinator is absent from deps; it would fail against the old code because the coordinator guard would skip it.

Registry wiring missing

DINModelRegistry.dinValidatorStake() is now checked as an independent block (Check 5c), using a new _REGISTRY_ABI fragment. TestRegistryWiring covers the OK and DRIFT paths.

JSON-writing cycle

DeployPlatformHarness (in DeployPlatformScript.t.sol) exposes the internal _writeDeployments without going through run(). test_writeDeployments_writesImplAndCodehashMetadata calls deploy() (existing path) then harness.writeDeployments(), reads back the written JSON, and asserts dinTreasuryImpl, dinTreasuryImplCodehash, and owner are all set correctly. The test uses DEPLOYMENTS_NETWORK=test-impl-metadata to write foundry/deployments/test-impl-metadata.json rather than clobbering localhost.json.

Guide table

Updated to reflect the correct getter names (dinValidatorStakeContract, feeRouter) and the two new checks, with a note that Check 5b runs independently of dinCoordinator.

All 25 Python tests and all 11 Foundry DeployPlatformScriptTest tests pass.

[Santiagocetran]

Thanks, Robbert. Rechecked 6512498 against my last review.

The independent treasury comparison and missing-coordinator regression address the first finding. The registry-to-stake check uses the actual getter with OK/DRIFT coverage. The deploy writer test also exercises the real JSON-writing/readback path. Those points are addressed.

One verification item remains: the separate UpgradePlatform metadata-writing path. Could you add focused readback evidence showing that the recorded implementation and codehash match the upgraded implementation, and that commit metadata is written when GIT_COMMIT_HASH is supplied? A test of the production metadata-writing block would be sufficient. Factoring it into a testable helper is fine; no live-network deployment is required.

Also, the sync currently changes BL-32 back to “Idea.” Please retain develop’s “Fixed” entry for #255 while preserving your BL-33 update.

CI passed after your sync. Develop has since advanced with #258, so please refresh the branch and obtain CI against the updated base before final approval.

This recheck is based on source inspection and observed CI, not a deployment or upgrade run.

[robertocarlous]

@Santiagocetran Both items from the review addressed in 50aa75ef:

UpgradePlatform metadata test (foundry/test/UpgradePlatformScript.t.sol):

_writeUpgradeMetadata extracted from run() as an internal function (same logic, same guards, same file-write calls — no behavior change). UpgradePlatformHarness exposes it so UpgradePlatformScriptTest.test_writeUpgradeMetadata_writesImplCodehashAndCommit can call the production block directly without going through the broadcast/upgrade path.

The test covers all three requested properties:

• dinTokenImpl read back from the JSON matches the address of the deployed DinTokenV2 passed to the block
• dinTokenImplCodehash read back matches extcodehash(newImpl) computed inline in the test
• .deployedAtCommit is written when a non-empty commit string is passed; the round-trip is asserted byte-for-byte

All env-polluting calls (vm.setEnv("DEPLOYMENTS_NETWORK", …) and vm.setEnv("GIT_COMMIT_HASH", …)) are combined in the one function with a comment explaining why, consistent with the pattern already in DeployPlatformScriptTest.

BACK_LOG.md conflict:

• BL-32: develop's "Mostly fixed — PR No. 255" entry retained as-is
• BL-33: this PR's "Fixed — PR No. 254" entry preserved
• BL-30, BL-31, BL-34, BL-35: develop's updated statuses (Abraham's Oct 26 decisions) taken

[Santiagocetran]

Thanks, Robbert! Rechecked 6512498 → 50aa75e against the remaining review items and issue No. 244’s approved amendments. The new UpgradePlatform test calls the production metadata-writing helper and reads back the implementation address, extcodehash and non-empty commit string. The BL-32 sync regression is resolved; the final backlog diff updates only BL-33. Earlier getter, account-free dispatch, ownership, treasury and registry findings remain addressed. I also compared the checker’s wiring and slasher ABI fragments against the bundled contract ABIs; they match.

Verification: CI run 41210002474 passed all four checks. Its Solidity log explicitly shows both metadata-writing/readback tests passing (518 Foundry tests total); Python reports 631 passed. The local documentation link check also passes. The trial merge into develop at 8f291c1 is clean. This is source and CI-backed verification, not a new live-network deployment or upgrade run.

No new blocking findings. This approval supersedes my earlier changes-requested review. Ready for the merge queue, where fresh CI will validate against current develop.

[Santiagocetran]

Thanks, Robbert, for addressing the review feedback and adding the upgrade-metadata test! I’ve approved PR No. 254 at 50aa75e, superseding my earlier changes-requested review. The remaining verification and backlog items are resolved, all four CI checks are green, and the merge with current develop is clean. Ready for the merge queue—no additional sync is needed just because develop has advanced; queue CI will validate the combined result. Thanks for your patience!

[umeradl]

@robertocarlous, cc @Santiagocetran: develop is red after this PR's merge (cf80216). Two new tests race on a shared environment variable.

Failing runs. 2 of 3 CI runs since this merged:

• develop push on cf80216: run 41476524829, Solidity: 517 passed, 1 failed.
• The merge-queue run for PR No. 304 on top of this PR: run 41475597415. This dropped 304 from the queue, even though 304 changes no Solidity.
• This PR's own queue run passed (run 41475140104), so the failure is intermittent rather than deterministic.

[FAIL: vm.readFile: failed to open file ".../foundry/deployments/test-impl-metadata.json": No such file or directory (os error 2)]
DeployPlatformScriptTest.test_writeDeployments_writesImplAndCodehashMetadata()

Root cause. DEPLOYMENTS_NETWORK is process-wide, and two different test contracts set it to different values:

• foundry/test/DeployPlatformScript.t.sol:180: vm.setEnv("DEPLOYMENTS_NETWORK", "test-impl-metadata")
• foundry/test/UpgradePlatformScript.t.sol:62: vm.setEnv("DEPLOYMENTS_NETWORK", "test-upgrade-metadata")

forge test runs test contracts in parallel. If the Upgrade test's setEnv lands between the Deploy test's setEnv and its harness.writeDeployments(...), the Deploy test writes test-upgrade-metadata.json and then fails to read test-impl-metadata.json. The "keep every reader in this one function" rule in both docstrings only serializes readers inside one contract, not across two. The DeploymentsPath.t.sol tests also read DEPLOYMENTS_NETWORK (lines 44–68), so they can see either value as well.

Suggested fix. Take these tests off the environment variable entirely. Give _writeDeployments (and _writeUpgradeMetadata) an internal overload that takes the target path or network name explicitly. Keep the existing signatures for the real script path, which resolves through DeploymentsPath. Then have each harness call the explicit overload with its own file name. Each test then writes and reads its own file, with no shared state.

To check the fix, run forge test --match-contract 'DeployPlatformScriptTest|UpgradePlatformScriptTest|DeploymentsPath' several times in a loop locally; it's easy to reproduce under parallel load.

Per MAINTAINING No. 4.6, this needs fixing today, either a fix on top (through a PR) or a revert of the two tests. Until it's fixed, any PR in the merge queue can be dropped at random. Logged in Discussion 170.