Feature/foundry platform deployments

[robertocarlous]

Summary

Implements issue #34 — Foundry Platform Deploy/Upgrade Parity

Adds Foundry equivalents of the Hardhat platform deploy/upgrade workflow so the four platform contracts (DinToken, DinCoordinator, DinValidatorStake, DINModelRegistry) can be deployed and upgraded via forge script on a local anvil chain.

What's included:

V2 upgrade stubs (foundry/src/upgrade/) — minimal V2 contracts for all four platform contracts, used to validate storage-layout safety in tests and the upgrade script

DeployPlatform.s.sol — deploys all four contracts behind TransparentUpgradeableProxy, wires them together in the correct order, and writes foundry/deployments/.json in the same address schema as the hardhat deploy script
UpgradePlatform.s.sol — upgrades a single proxy to its V2 implementation; contract selected via CONTRACT= env var, reads proxy address from the deployments JSON

Forge test suite (foundry/test/DeployPlatform.t.sol, 40 tests) — covers proxy wiring assertions, re-initializer protection on both proxy and implementation, and upgrade correctness (state preservation + access control) for all four contracts
dincli system import-deployments --foundry — new flag that auto-resolves to foundry/deployments/.json (with local → localhost mapping); --file and --foundry together are rejected as mutually exclusive; default hardhat behaviour is unchanged

E2e verified: anvil → forge script DeployPlatform.s.sol --broadcast → dincli system import-deployments --foundry → din_info.json updated with correct addresses

[robertocarlous]

Hi @umermjd11

Pls can you check this

[umeradl]

Code review — Foundry platform deploy/upgrade parity

Reviewed at commit 45dcda1 (feature/foundry-platform-deployments). Scope: issue #34. Everything below was reproduced in a clean local checkout (fresh submodule init + npm ci), not taken from the PR description.

What's good

• The forge test suite genuinely mirrors the hardhat upgrade tests: proxy wiring, double-init protection on both proxy and raw implementation, and state-preservation + access-control checks across upgrades for all four contracts. All tests pass (see results below).
• foundry/deployments/localhost.json matches the hardhat address schema key-for-key — verified against import-deployments's required keys, and a live dincli system import-deployments --foundry run picked it up without modification.
• The dincli/cli/system.py change is clean: default hardhat behaviour untouched, --file/--foundry mutual exclusion handled with a clear error, per-flow "run the deploy script first" hints, and the existing local → localhost network mapping reused rather than duplicated.
• The .gitignore fix (!script/*.sol) closes a pre-existing trap where forge scripts under foundry/script/ were silently untracked.
• test_allProxyAdminsOwnedByDeployer gets the OZ v5 proxy-admin model exactly right (one ProxyAdmin per proxy, owner is the invariant) — see the proxyAdmin question below though.

Bugs fixed (all three were blocking; fixed and re-verified, now landed on develop)

No. 1 — DeployPlatform.s.sol failed on a fresh clone: foundry/deployments/ doesn't exist.
vm.writeJson doesn't create parent directories, and foundry/deployments/localhost.json isn't committed. On a clean checkout the script deployed and wired all four proxies, then reverted at the final step (vm.writeJson: failed to open file ".../foundry/deployments/localhost.json": No such file or directory). Fix: vm.createDir(outDir, true) before vm.writeJson in DeployPlatform.s.sol — covered by the existing read-write fs_permission on deployments/. Re-verified with the directory removed entirely: the script recreates it and writes a valid localhost.json.

No. 2 — @custom:oz-upgrades-from annotation was broken in all four V2 stubs, so UpgradePlatform.s.sol failed for every contract.
A plain // comment sat between the two /// NatSpec lines in each stub, splitting the doc block so @custom:oz-upgrades-from never reached solc — upgrades-core aborted with "does not specify what contract it upgrades from" for all four. The forge tests didn't catch this because they always pass opts.referenceContract explicitly; only the script exercises the annotation path. Fix: moved the // explanation above the NatSpec block in all four files under foundry/src/upgrade/. Re-verified end-to-end for all four contracts after a fresh forge clean + redeploy — each CONTRACT=<name> forge script script/UpgradePlatform.s.sol --broadcast ... completed with "ONCHAIN EXECUTION COMPLETE & SUCCESSFUL", version() returning 2 through the proxy.

No. 9 — both scripts' own "Usage (from repo root)" comment was broken: running the exact documented command failed to compile.
foundry.toml/remappings.txt live in foundry/, not the repo root, and forge picks the project root from cwd, not from the script path argument — invoked exactly as documented, every @openzeppelin/.../forge-std/... import failed. Fix: changed both usage comments to cd foundry && forge script script/<Script>.s.sol ..., for consistency with how the hardhat flow is already documented (cd hardhat && npx hardhat run ...). Re-verified with forge clean + the exact documented commands for both scripts — both complete successfully.

Also fixed — two of the non-blocking suggestions (No. 7, No. 8)

No. 7 — import order in DeployPlatform.t.sol. The DinCoordinatorV2/DinValidatorStakeV2/DINModelRegistryV2 imports sat mid-file, after the MockTaskContract definition. Moved them up next to the DinTokenV2 import, with the rest of the imports at the top.

No. 8 — stale build-info gotcha. Reproduced again independently while re-verifying this pass (ValidateCommandError: Found multiple contracts with name src/DinValidatorStake.sol:DinValidatorStake, right after a forge build without a preceding forge clean) — confirms it's a real, repeatable trap, not a one-off. Added a forge clean step to DeployPlatform.s.sol's documented usage comment, right after starting anvil, so anyone following the header doesn't hit it.

Verification performed (independent, this pass)

• forge test (full suite, after forge clean — see No. 8 below): 40/40 passing (27 added by this PR + 13 pre-existing).
• dincli's tests/dincli/ integration harness, extended to pick the platform deploy toolchain (foundry vs hardhat) via a new PLATFORM_DEPLOY_TOOLCHAIN setting, run against both:
  • Foundry (forge script DeployPlatform.s.sol against a local chain, then dincli system import-deployments --foundry): 8/8 passing (Phase 1 — platform deploy/import/ABI-dump).
  • Hardhat (existing deploy-platform.ts path, unchanged): 8/8 passing — confirms the default hardhat flow is untouched.
  • Full run Phase 1 → Phase 3 (platform deploy through model registration) under the new foundry default: 35/35 passing.
• UpgradePlatform.s.sol verified end-to-end for all four contracts (version() → 2 through the proxy).

Landed

Given the fixes are one-line/doc-comment changes with no design disagreement, they're already applied and merged directly into develop (same pattern as the PR #22 audit merge) rather than waiting on a branch push-and-re-review cycle:

• b40fe2e — feat(foundry): add platform deploy/upgrade scripts + V2 upgrade stubs (PR #35) — your original files (author: you), with the No. 1/2/9 fixes above folded in.
• 7468bd5 — test(dincli): pick platform deploy toolchain (foundry/hardhat) in the harness — the tests/dincli/ extension used to verify this end-to-end.
• 8d61bf0 — fix(foundry): move V2 stub imports to top + document forge clean step — the No. 7/No. 8 fixes above.

Thanks for a genuinely solid submission, Robert — the architecture is right, the dincli integration is clean, and the test suite is a faithful, thorough port of the hardhat upgrade tests.

One thing worth discussing — proxyAdmin in the deployments JSON

test_allProxyAdminsOwnedByDeployer correctly asserts that under OZ v5 each TransparentUpgradeableProxy gets its own ProxyAdmin (four distinct contracts here, not one shared admin). But foundry/deployments/localhost.json (and dincli's proxy_admin field it feeds) only records one — DinToken's admin — same as the pre-existing hardhat/scripts/deploy-platform.ts (getProxyAdminAddress(dinTokenAddress)), so this isn't something this PR introduced.

It doesn't currently cause a functional bug: nothing reads that field to drive an upgrade — UpgradePlatform.s.sol resolves each proxy's admin itself via Upgrades.upgradeProxy, and dincli's own comment already says proxy_admin is "informational" only. But it is a bit of a footgun for anyone who does read it expecting it to be "the" admin for all four proxies (e.g. to call the ProxyAdmin contract directly for DinCoordinator, DinValidatorStake, or DINModelRegistry — they'd get the wrong contract).

Given it's inherited from PR #13's hardhat tooling rather than new here, I don't think it should block this PR — but is it worth a small shared follow-up (hardhat + foundry) to record all four admin addresses (proxyAdminToken/proxyAdminCoordinator/proxyAdminStake/proxyAdminRegistry, or similar), or at least rename the single field so it's unambiguous that it's DinToken's only? Curious what you think, since you clearly understood the v5 model correctly in the test.

Suggestions (non-blocking, carried over from initial review — up to you whether these land here or as follow-ups)

• No. 3 — _writeDeployments in DeployPlatform.s.sol hardcodes the output filename (literally concatenates "/localhost.json"), so it writes the same file regardless of what you actually deployed to. Hardhat's side genuinely does not have this problem: deploy-platform.ts calls savePlatformAddresses(hre.network.name, ...) (hardhat/deploy/helpers.ts), deriving the filename from hre.network.name — pointing --network at a different Hardhat network automatically changes the output file, no code change needed. Forge scripts have no equivalent first-class network concept to key off of the same way — they only take --rpc-url <url> (optionally a foundry.toml [rpc_endpoints] alias, but nothing inside the script can see which alias, or even which raw URL, it was invoked with — our own tests/dincli/test_01_platform.py passes a raw --rpc-url string, confirming there's no "network name" value available). A real fix would need either block.chainid-based mapping to known filenames, or an explicit env var read via vm.envString, same pattern as CONTRACT in UpgradePlatform.s.sol. Fine for the stated local-anvil scope — worth flagging the local-only limitation in the script header before anyone points it at Sepolia.
• No. 4 — DeployPlatform.s.sol step 7 comment says the ProxyAdmin is "shared across all four proxies" — that's the v4 model; under v5 each proxy has its own (see the proxyAdmin discussion above). Worth fixing alongside whatever we land on there.
• No. 5 — No Documentation/ updates for the --foundry flow yet.
• No. 6 — Still open, not covered by test_import_deployments_into_din_info (tests/dincli/test_01_platform.py): that test always passes an explicit --file <path>, deliberately — the harness's cwd is an isolated scratch dir, not the repo root, so it never exercises import_deployments's own default-path resolution (--foundry/--hardhat/no-flag → foundry|hardhat/deployments/<network>.json relative to cwd) or the --foundry/--hardhat/--file mutual-exclusion error — none of those code paths run in that test. What's still worth adding is a small typer.testing.CliRunner unit test (matching the existing pattern in tests/test_dintoken.py/tests/test_ipfs_config.py — no live chain, just monkeypatch + CliRunner.invoke) covering: (a) --foundry/--hardhat/no-flag resolve to the expected relative path, and (b) passing two of --file/--foundry/--hardhat together exits 1 with the mutual-exclusion message.

(No. 7 and No. 8 from the initial review are fixed now — see above.)

Test results (this pass)

• forge test: 40 passed, 0 failed (after forge clean — see No. 8).
• dincli integration harness: 8/8 (foundry), 8/8 (hardhat, regression check), 35/35 (foundry, full Phase 1–3 run).
• Both scripts' documented usage commands run verbatim, successfully, from a fresh forge clean.

---

[umeradl]

Merged manually to develop ✅

@robertocarlous — this is landed on develop now, with you as the commit author on the original files (issue #34).

Commits:

• b40fe2e — feat(foundry): add platform deploy/upgrade scripts + V2 upgrade stubs (PR #35) — your files, with the No. 1/No. 2/No. 9 blocking fixes from the review folded in
• 7468bd5 — test(dincli): pick platform deploy toolchain (foundry/hardhat) in the harness — the tests/dincli/ extension used to verify this end-to-end
• 8d61bf0 — fix(foundry): move V2 stub imports to top + document forge clean step — the No. 7/No. 8 non-blocking fixes

Full review with what was found and fixed is in the comment above. One open question left there for you — the proxyAdmin field in the deployments JSON only recording one of the four proxies' admins (inherited from the hardhat tooling, not new here, but worth discussing).

Closing this out since the code is on develop directly rather than merged via this branch (implements #34). Great work on this, thank you.

[robertocarlous]

Hi @umeradl

Good catch on the proxyAdmin footgun — I agree it's worth fixing properly rather than just leaving a disclaimer.

My preference is to record all four addresses explicitly (proxyAdminToken, proxyAdminCoordinator, proxyAdminStake, proxyAdminRegistry) rather than just renaming the single field. Since OZ v5 genuinely deploys four distinct ProxyAdmin contracts, the JSON should reflect that accurately a renamed single field still misleads anyone who reads it expecting to find the admin for a specific proxy.

The fix needs to touch both the foundry deploy script and hardhat's deploy-platform.ts / helpers.ts together to keep the two schemas in sync, otherwise import-deployments would need to handle two different shapes. I can open a small follow-up issue and PR for that if useful it's a contained change.

[umeradl]

Yes please open an issue. Fix both hardhat and foundry for it, and fix dincli to import these into din_info.json — and the test harness if needed. If you can get to it, go ahead; otherwise I'll fix it myself and open the PR.

[robertocarlous]

Do not worry, i will do that from my end

[robertocarlous]

Hi @umeradl

I have fixed the proxy admin addresses

Here is a short PR for it

#52