Skip to content

feat: make StoreFactory upgradeable so store upgrades survive into R2 - #298

Open
re-gius wants to merge 4 commits into
masterfrom
re-gius/issue-297-storefactory-uups
Open

feat: make StoreFactory upgradeable so store upgrades survive into R2#298
re-gius wants to merge 4 commits into
masterfrom
re-gius/issue-297-storefactory-uups

Conversation

@re-gius

@re-gius re-gius commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Description

Closes #297. StoreFactory was the only non-upgradeable authority in the deployment; it is now UUPS like the other twelve contracts, so both its beacon ownership and its per-user store bindings follow the same owner-operated upgrade path as the rest of R1. See the issue for the failure modes.

The three immutables (labelStoreBeacon, userStoreBeacon, protocolRegistry) became storage, ahead of the existing mappings and lists and followed by a uint256[50] __gap. Beacon and implementation minting moved from the constructor into initialize(address initialOwner, address protocolRegistry_), which runs by delegatecall, so address(this) is the proxy and both beacons belong to it. IStoreFactory is comment-only: no selector, event, error, or getter signature moved.

One check had to be added. While protocolRegistry was a constructor immutable, the CREATE3 adoption comparison from #261 covered it byte for byte. Behind a proxy it is storage the initialiser wrote, and a proxy's runtime code is identical whatever it was initialised with, so initialize(realOwner, foreignRegistry) would have passed every existing assertion. _verifyStoreImplementations now reads the pointer back, and both the wire stage and the single-shot deployer pass the registry they wired.

Type

  • Bug fix
  • Feature
  • Breaking change
  • Documentation
  • Chore
  • Refactor
  • Security

Scope

  • Registration
  • Resolver
  • Store
  • Proof of Personhood
  • Deployment scripts
  • Tests

Related Issues

Closes #297

Fixes

Closes #297

Checklist

Code

  • Follows project style
  • forge build passes
  • forge test passes
  • No new compiler warnings

Testing

  • New tests added for changed behavior
  • Fuzz tests added where applicable
  • Invariant tests verified

Security

  • No new selfdestruct or delegatecall
  • Access control reviewed
  • No storage layout conflicts (for upgradeable contracts)

Documentation

  • NatSpec updated on changed interfaces
  • README updated if needed

Breaking Changes

  • No breaking changes
  • Breaking changes documented below

Breaking changes:

  • StoreFactory is constructed through a proxy, not directly: new StoreFactory(registry, owner) becomes an implementation plus an ERC1967Proxy initialised with initialize(owner, registry). Argument order follows the post-fix: harden the deploy pipeline against contract substitution #261 convention, owner first.
  • Its address moves. The CREATE3 salt goes from StoreFactory:contract to StoreFactory:proxy, and both beacon addresses move with it because they are now minted by the proxy. The committed manifest has to be regenerated: deploy CI will fail with a MOVED table naming the new values, and deployments/paseo-assethub/420420417.json is updated from that run. Not done in this PR.

How to test

forge test --mt test_initialize_mints_beacons_owned_by_the_proxy
forge test --mt test_upgrade_is_owner_gated_and_preserves_bindings
forge test --mt test_rejects_a_factory_initialised_against_a_foreign_registry

Notes

Three tests in DeterministicDeployment.t.sol are repointed: they used StoreFactory because it had constructor arguments and address-derived beacon immutables, and it now has neither. The foreign-constructor-args rejection moved to DotnsPopLens, and the range-skip regression targets UUPSUpgradeable.__self.

@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

CI Summary

Check Result
4naly3er Analysis Found 37 issues: 5 medium, 8 low, 13 gas, 11 informational - View Report
Slither Analysis Found 192 issues: 4 high, 40 medium, 89 low, 59 informational - View Report
Contract Tests (Unit + Fuzz) All tests passed (660 total) - View Report
Contract Tests (Invariant) All tests passed (58 total) - View Report
Gas Report 9 contracts analyzed - View Report
Coverage Failed - Tests failed, cannot compute coverage
Documentation Passed - 66 pages generated - View Docs
Format & Lint Passed - Code formatted correctly
File Validation Passed - All tracked files valid
Deploy Contracts Failed - addresses differ from the committed manifest
PR Title PR Title Valid
Labels Unknown
Secret Scan Passed - No secrets detected

@re-gius

re-gius commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

The deploy job's manifest check fails, with four MOVED rows. Three are this PR and are expected: StoreFactory moves because its CREATE3 salt goes from :contract to :proxy, and LabelStoreBeacon / UserStoreBeacon follow because the proxy now mints them. I'll update those three from the Actual column. The fourth, DotnsPopLens, is not from this branch: 387ce2ea ("repoint DotnsPopLens manifest entry to the redeployed lens") landed on master last night and hand-edited that entry to an instance that was redeployed on-chain, so master's own deploy run has been failing since (failure 387ce2ea against success f0be180f before it). That row also can't be fixed by copying the Actual column, since a CREATE3 address is a pure function of factory plus salt and a fresh run will always land 0xfe5A45f7… rather than the committed 0xAE374b07…. So it needs a separate decision, and I don't think this PR should be the one to make it. Three options as I see them: restore the manifest to the CREATE3 address and let the live popLens registry key carry the redeployment, since that key is what consumers actually read; bump DOTNS_SALT_VERSION for the lens so the pipeline derives the address that is already live; or exempt hand-redeployed entries from the reproduction check, as _-prefixed metadata already is.@sphamjoli, which would you prefer?

@sphamjoli

Copy link
Copy Markdown
Member

@re-gius reviewing will respond

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feat]: Make StoreFactory upgradeable (UUPS) so beacon ownership and user store bindings survive into R2

2 participants