You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
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.
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?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Closes #297.
StoreFactorywas 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 auint256[50] __gap. Beacon and implementation minting moved from the constructor intoinitialize(address initialOwner, address protocolRegistry_), which runs by delegatecall, soaddress(this)is the proxy and both beacons belong to it.IStoreFactoryis comment-only: no selector, event, error, or getter signature moved.One check had to be added. While
protocolRegistrywas 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, soinitialize(realOwner, foreignRegistry)would have passed every existing assertion._verifyStoreImplementationsnow reads the pointer back, and both the wire stage and the single-shot deployer pass the registry they wired.Type
Scope
Related Issues
Closes #297
Fixes
Closes #297
Checklist
Code
forge buildpassesforge testpassesTesting
Security
selfdestructordelegatecallDocumentation
Breaking Changes
Breaking changes:
StoreFactoryis constructed through a proxy, not directly:new StoreFactory(registry, owner)becomes an implementation plus anERC1967Proxyinitialised withinitialize(owner, registry). Argument order follows the post-fix: harden the deploy pipeline against contract substitution #261 convention, owner first.StoreFactory:contracttoStoreFactory: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, anddeployments/paseo-assethub/420420417.jsonis updated from that run. Not done in this PR.How to test
Notes
Three tests in
DeterministicDeployment.t.solare repointed: they usedStoreFactorybecause it had constructor arguments and address-derived beacon immutables, and it now has neither. The foreign-constructor-args rejection moved toDotnsPopLens, and the range-skip regression targetsUUPSUpgradeable.__self.