feat: publish the pallet-revive genesis with each release - #253
Conversation
CI Summary
4naly3er AnalysisMedium (5)
Low (10)
Gas (14)
Informational (11)
Slither AnalysisHigh (3)
Medium (43)
Low (73)
Informational (64)
Deploy ContractsDeployed addresses vs the committed manifestExpected is the committed manifest; actual is this CI deployment of the same pipeline.
Labelsdependencies, other, type: docs |
mordamax
changed the title
aa33d85 to
b9ff6cb
Compare
|
@mordamax please check PR template please |
aae3786 to
e722db8
Compare
e722db8 to
6617fdb
Compare
6617fdb to
9ebffb8
Compare
github-actions
Bot
added
the
dependencies
|
all done also dropped one line in |
There was a problem hiding this comment.
Reviewed the genesis builder and the workflow changes. Solid work overall: the extractor is pure and well tested, the parity check against the canonical manifest is a real safety property, and pulling foundry setup into a composite action removes real duplication. Three inline notes below, one correctness and two on duplication and guarding.
| const present = new Set(accounts.map((a) => normalizeAddr(a.address))); | ||
| const broken = []; | ||
| for (const acct of accounts) { | ||
| const word = acct.storage?.[EIP1967_IMPL_SLOT]; |
There was a problem hiding this comment.
This guard reads only the EIP-1967 slot, but the regression the comment above names was a beacon, not an EIP-1967 proxy. An OpenZeppelin UpgradeableBeacon is not a proxy and does not use this slot: it holds _implementation in a plain slot (slot 1, after Ownable._owner), which is exactly how the fixture models it in the test file.
So if a store implementation behind a beacon fails to deploy, referencedContracts drops it (no code to follow) and this check cannot see it either (wrong slot). The precise previewnet failure this file exists to prevent stays unguarded for beacons; only EIP-1967 proxies are covered.
Fix is to widen the predicate: flag any account whose storage holds a pointer to an address that carries no code in the genesis, not just the EIP-1967 slot. One predicate then covers both beacons and proxies, and the check stops being narrower than its own justification.
There was a problem hiding this comment.
good catch - it only read the EIP-1967 slot, and a beacon keeps its impl in slot 1
widened: EIP-1967 on every account, plus slot 1 on manifest-named beacons (both real ones end in "Beacon")
didn't take the broader "any pointer to a codeless address" - that flags every EOA pointer, owner/operator and so on, the fixture has one on purpose
| # The owner key is taken from DOTNS_ADMIN_KEY if set, otherwise derived from | ||
| # DOTNS_ADMIN_MNEMONIC. It ends up owning the registry, resolvers, registrar, store factory | ||
| # and beacons in the genesis storage, so the script refuses to run without one. | ||
| - name: Build pallet-revive genesis |
There was a problem hiding this comment.
The genesis path is duplicated between this file and publish-prerelease.yml: the job-level DOTNS_TLD: test, this build step, the zip line, the release files: entry, and the expected-assets line. Five touch-points across two files that must stay in sync, which is the same problem the setup-foundry composite action in this PR removes for the toolchain.
The comment on the env block ("five steps name the file and must not disagree") describes the smell rather than removing it. The two workflows differ only on deployments.json, so the shared part is most of it, and a composite action or reusable workflow would collapse it.
Not blocking. Given the PR already sets this pattern for the toolchain, the genesis wiring is the natural next candidate.
There was a problem hiding this comment.
agreed, but only the build step can move into a composite action - files:, the zip line, the body text and expected-assets are workflow-level and a composite can't emit those, so it collapses 1 of 5. leaving it for now
| exit 1 | ||
| fi | ||
|
|
||
| GENESIS_OUT="$OUT/dotns-genesis-$DOTNS_TLD.json" |
There was a problem hiding this comment.
The only thing stopping a test genesis reaching a production chain is this filename suffix plus the prose in the release body. Nothing inside the artifact records which TLD it carries, so a rename defeats the guard.
Given the stakes the header calls out ("how a test registry ends up on a chain that wanted a real one"), consider writing the TLD into the file itself, e.g. a top-level "tld": "test" field the consumer or chain-spec tooling can assert against. The filename stays as documentation; the field is the check.
Minor, since CI only builds test today.
There was a problem hiding this comment.
added, top-level "tld" in the artifact
note for whoever wires the consumer: PPN does patch.revive = <whole file>, so it has to inject .accounts only or tld lands in the pallet config as an unknown field. nothing consumes the new file yet so nothing breaks today
3 participants
Description
Extends the release artifact with pallet-revive genesis state, so a chain can carry DotNS from block zero instead of deploying it afterwards.
build-genesis.shruns the existing deploy stages against anvil, dumps the EVM state and converts it to aGenesisConfigaccount list.paritytech/preview-net-v1does this today by cloning this repo at a movingmasterwith its own copy ofFACTORY_DEPLOYER_KEY. Building it here ties the artifact to the same commit and keys as the ABIs beside it, and lets that repo drop ~790 lines and two secrets.Follows the pattern #242 set — standalone asset, in the strict pre-publish check, plus a small path-filtered PR workflow (
genesis-extractor-test.yml) that tests the extractor, mirroringrelease-metadata.yml. Addresses are deliberately not re-emitted;deployments.jsonstays the only copy.The filename carries the TLD (
dotns-genesis-test.json) becauseDOTNS_TLDis baked into the registry initialiser, so it suits test networks only — the release body says so. Address parity againstdeployments/paseo-assethub/420420417.jsonis asserted, so a wrong factory key fails the build.Type
Scope
Related Issues
Follows #242, which made the release carry addresses. This adds genesis state to the same artifact.
Fixes
Checklist
Code
forge buildpassesforge testpassesTesting
Security
selfdestructordelegatecallDocumentation
Breaking Changes
How to test
Ran locally: five deploy stages, 33 genesis accounts, 19 names matching the live manifest, 9 extractor tests.
Notes
No Solidity changed, so the contract checkboxes are inherited state rather than something this diff exercises.
deployall.sh, which also has a factory-exists check and theEXPECTED_CREATE3_FACTORYguard. Drivingdeploy:allwould be DRY-er but needs the keystore flow and thepaseo_localalias, which I could not test locally. The constraint if you collapse it is in the script: the signer must stay the admin key, notdeploy-contracts.yml's public anvil account..github/PULL_REQUEST_TEMPLATE.mdhas a trailing space in its filename, so GitHub never loads it and new PRs open blank. Happy to fix separately.