Add technical specification for user collections factory#113
Open
Douglasacost wants to merge 26 commits into
Open
Add technical specification for user collections factory#113Douglasacost wants to merge 26 commits into
Douglasacost wants to merge 26 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an initial technical specification and documentation index for an operator-triggered “User Collections” NFT factory (UUPS-upgradeable factory deploying immutable EIP-1167 ERC-721/ERC-1155 clones), including roles, interfaces, flows, storage notes, security model, testing plan, and ops/deployment guidance.
Changes:
- Introduce a full technical specification for the user collections factory + clone templates.
- Add a documentation README that indexes the specification.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/collections/doc/spec/user-collections-specification.md | New end-to-end technical spec describing architecture, interfaces, security, testing, and operations for the collections factory/clones. |
| src/collections/doc/README.md | Documentation landing page linking to the technical specification. |
LCOV of commit
|
Specifies an operator-triggered NFT collection factory: users pay in fiat off-chain, a trusted backend deploys a fully-isolated EIP-1167 clone (ERC-721 or ERC-1155) on the user's behalf. Factory is UUPS-upgradeable; clones are immutable per release. Both creator and operator hold MINTER_ROLE on each clone; per-collection metadata (baseURI/uri + contractURI) and royalties are owner-mutable until locked one-way. On-chain externalId map prevents duplicate creations from the off-chain ledger. Operational sections describe the actual zkSync flow: deployment via a Forge script + ops/ shell wrapper mirroring ops/deploy_swarm_contracts_zksync.sh, source-code verification via ops/verify_zksync_contracts.py against the zkSync verifier API, and a manual pre-upgrade storage-layout check matching src/swarms/doc/upgradeable-contracts.md. CI today runs forge test on all packages and enforces 95% coverage on swarms only; collection-coverage and storage-layout-diff CI jobs are recorded as deferred items.
These three technical terms are used by the new src/collections/doc/spec/user-collections-specification.md but were not in the project's cspell ignoreWords list. EXTCODEHASH is an EVM opcode (sits next to the existing EXTCODECOPY entry); autonumber is a Mermaid sequence-diagram keyword; dedup is a common abbreviation.
Douglasacost
force-pushed
the
feat/user-collections-spec
branch
from
c869607 to
93e660b
Compare
Updates the user collections specification with nine targeted refinements from design review and wires the collections coverage gate in CI: - Operator MINTER_ROLE auto-granted by the factory on every clone (no longer a backend convention). - Royalty cap intentionally absent: full creator autonomy up to OZ's ERC-2981 100% bound; recorded as a design decision. - ERC-721 mintBatch returns the contiguous tokenIds array for backend reconciliation without log parsing. - Factory setImplementation* and initialize reject zero addresses and non-contracts (NotAContract error). - Clone storage layout: metadataLocked and royaltiesLocked share one slot; pre-upgrade diff must verify byte offsets, not just slot indices. - ERC-1155 mintBatch stays single-recipient (OZ alignment); multi- recipient deferred with explicit trigger condition. - Bytecode-permanence invariants: implementations contain no SELFDESTRUCT, no caller-controlled delegatecall, deployed via CREATE only (no CREATE2 redeploy path). - Coverage CI gate wired now; storage-layout diff CI deferred to the V2 upgrade PR with a baseline-JSON convention for v1. - UUPS upgrade tests tightened: slot change verification, OPERATOR_ROLE rejection, storage preservation via V2 mock, non-UUPS revert. Adds a new design-decision row codifying OZ alignment as a governing principle for src/collections/.
…terfaces First contract draft of the user collections system. Mirrors the spec at src/collections/doc/spec/user-collections-specification.md. - ICollectionFactory, IUserCollection721, IUserCollection1155, and shared CollectionTypes (Standard enum, CreateParams721, CreateParams1155). - UserCollection721: ERC-721 + URIStorage + Burnable + ERC-2981 + AccessControl. Owner/minter roles, mint and mintBatch (returns minted tokenIds), opt-in metadata/royalties locks, packed lock booleans, __gap reserved, _disableInitializers in constructor. - UserCollection1155: ERC-1155 + Supply + Burnable + ERC-2981 + AccessControl. Single-recipient mintBatch matching OZ's _mintBatch shape, same lock semantics. - CollectionFactory: UUPS-upgradeable, OPERATOR_ROLE-gated creation, atomic Clones.clone + initialize + externalId mapping write, setImplementation* with ZeroAddress / NotAContract validation, _authorizeUpgrade gated to DEFAULT_ADMIN_ROLE. Operator MINTER_ROLE auto-grant on every clone (passes msg.sender to clone.initialize). - .cspell.json: allow runbook, selfdestruct, SELFDESTRUCT, proxiable (referenced in spec/contracts). Tests, deployment scripts, and the V2 mock fixtures will land in follow-up commits.
…inits Test coverage for the user collections system: - UserCollection721.t.sol (24 tests): initialize wiring, mint / mintBatch with returned tokenIds asserted contiguous and matching Transfer events, lock semantics on baseURI/contractURI/royalties, role-admin grant + revoke, ERC-2981 non-zero and zero (clear) paths, supportsInterface for ERC-165/721/Metadata/2981/AccessControl, ERC721Burnable scenarios, bytecode-permanence opcode walker (skips PUSH1..PUSH32 immediates). - UserCollection1155.t.sol (18 tests): analogous coverage adapted to ERC-1155 mechanics, including ERC1155Supply / Burnable behavior. - CollectionFactory.t.sol (21 tests): atomic clone+init, externalId reverts, operator auto-grant invariant, setImplementation* validation, and the four UUPS assertions from spec §8.1 — slot change via vm.load, OPERATOR-only rejection, storage preservation through V2 mock, non-UUPS revert via ERC1967InvalidImplementation. - Collections.integration.t.sol: end-to-end happy path from §8.4. - mocks/CollectionFactoryV2Mock.sol + NonUUPSImplementationMock.sol. Drops empty `__<Mixin>_init` calls (ERC721URIStorage, Burnable, ERC2981, AccessControl, ERC1155Supply, etc.) which are no-op shims in OZ v5.6.1 kept only as forward-compat aliases. Comment notes to re-add them on OZ upgrade. Functional inits (`__ERC721_init`, `__ERC1155_init`) and `_disableInitializers()` retained. Coverage: 157/162 = 96.91% lines (CollectionFactory 97.78%, UserCollection1155 96.08%, UserCollection721 96.97%). Clears the 95% threshold enforced by .github/workflows/checks.yml.
…yout baselines
Adds the deployment and upgrade tooling for the user collections system,
mirroring the swarms pattern.
- script/DeployCollectionFactoryZkSync.s.sol: Forge script that deploys
UserCollection721 and UserCollection1155 implementations (CREATE only),
the CollectionFactory logic, and the ERC1967 proxy initialized with
(admin, operator, impl721, impl1155).
- script/UpgradeCollectionFactory.s.sol: three-mode upgrade script with
ACTION env var: UPGRADE_FACTORY (UUPS upgradeToAndCall, with optional
REINIT_DATA), SET_IMPL_721, or SET_IMPL_1155 (admin pointer swap for
future clones only). Includes the full §9.4 pre-upgrade checklist in
the natspec.
- ops/deploy_collection_factory_zksync.sh: orchestration script that
loads .env-test/.env-prod, runs preflight checks, builds with --zksync,
invokes the Forge script, performs cast-based post-deploy sanity
checks (admin/operator role grants, impl pointers, EIP-1967 slot),
delegates source verification to verify_zksync_contracts.py, and
appends the deployed addresses to the env file.
- src/collections/layouts/{CollectionFactory,UserCollection721,
UserCollection1155}.v1.json: storage-layout baselines committed for
the §9.4 baseline-JSON convention. Verifies that lock booleans pack
into one slot (slot 3 offset 0/1 in 721; slot 1 offset 0/1 in 1155)
and that the gap is sized correctly (46 in 721, 47 in 1155 and the
factory).
- .cspell.json: allow codehash, codehashes, immediates, newbase,
newcontract, opping (referenced in test fixtures and contracts).
- spec §10: file layout extended to list all three layout baselines.
Required env vars per spec §9.1: N_FACTORY_ADMIN (multisig holding
DEFAULT_ADMIN_ROLE) and N_FACTORY_OPERATOR (backend service holding
OPERATOR_ROLE).
…clone() Records the spec-level decision and detailed design for swapping per-collection EIP-1167 minimal proxies for canonical OZ ERC1967Proxy with salt=externalId, unblocking zkSync Era deployment. Preserves the per-collection bytecode immutability promise (impls do not inherit UUPSUpgradeable, no ProxyAdmin) and keeps all v1 storage-layout baselines unchanged. Captures spec deltas, test impact, and audit checklist additions for the implementation phase.
…f Clones 13 task plan covering: 721 + 1155 factory refactor with TDD CREATE2-derivation tests, atomic-emits assertions, test-helper switch from Clones to ERC1967Proxy, no-upgrade-selector tests on impls, ERC1967Proxy bytecode-permanence test, integration test sequence-of-events updates, vocabulary pass, spec doc deltas, factoryDependencies CI gate, post-broadcast createCollection smoke test, and final storage-layout baseline verification.
forge build --zksync compiles the entire tree, so SwarmRegistryL1Upgradeable and test/upgrade-demo/TestUpgradeOnAnvil (both use EXTCODECOPY/SSTORE2) need to be moved out of the way before zksolc, then restored on EXIT. Mirrors the move/restore pattern in ops/deploy_swarm_contracts_zksync.sh. Required to unblock zkSync Sepolia dry-run for collections.
…createCollection721 Atomic deploy + init via the proxy's constructor (delegatecall to initialize). Salt = externalId gives off-chain CREATE2 pre-derivation. Preserves the per-collection bytecode-immutability promise — impls do not inherit UUPSUpgradeable, so the EIP-1967 impl slot is constructor- fixed. Required for zkSync Era compatibility (Clones.clone() is not supported on EraVM).
…createCollection1155 Symmetrical 1155 refactor following Task 2's 721 path. Drops the now- unused Clones import; both standards now deploy per-collection ERC1967Proxy via salted CREATE2.
Vocabulary pass clones to collections, deployment-model rewrite, §1.4 upgradeability footnote, §3.4 atomic-flow bullet, §4.1 sequence diagram, §4.4 gas profile, §4.5 new Address Determinism sub-section, §6.2 storage section, §7.2 row 15 split into 15a/15b + new row 16 for OZ proxy import audit posture, §9.1 deploy-script note, §9.4 per-collection EIP-1967 slot check.
Drops stale clone references in UserCollection*.sol, ICollectionFactory.sol, IUserCollection*.sol, CollectionTypes.sol, and doc/README.md NatSpec/comments. Identified as a plan gap by the Task 9 spec-compliance reviewer; the per-collection ERC1967Proxy architecture should be reflected in source-level documentation, not just the spec doc and tests.
CollectionFactory's factoryDependencies must contain at least one entry (ERC1967Proxy). Empty factoryDeps means the factory cannot deploy per-collection proxies on EraVM at runtime — exactly the failure mode that left the original Clones.clone() factory broken on zkSync Era.
Per Task 11 code-review feedback: capture jq's exit status explicitly so a malformed/truncated artifact JSON surfaces a curated log_error instead of '[: : integer expression expected'.
…Sync Calls createCollection721 against the freshly deployed factory and asserts the resulting collection has non-empty code and a populated EIP-1967 impl slot. Catches EraVM-runtime failures that compile-time gates miss (e.g. the original Clones.clone() incompatibility).
Per Task 12 code-review feedback (two HIGH-severity issues): 1. Pass the CreateParams721 tuple literal directly to `cast send` instead of pre-encoding via `cast abi-encode` (which produced a hex blob that `cast send`'s tuple parser would reject). 2. Resolve a signer key that actually holds OPERATOR_ROLE: prefer \$OPERATOR_PRIVATE_KEY, fall back to \$DEPLOYER_PRIVATE_KEY only when it matches \$N_FACTORY_OPERATOR, else skip with a clear warning. Production typically separates the deployer EOA from the operator, so the previous unconditional sign-with-deployer would always AccessControlUnauthorizedAccount-revert in prod.
Two header comments described the implementations as 'cloned per ERC-721/1155 collection' — stale post-refactor. The implementations are deployed once and shared across all per-collection ERC1967Proxy instances. Final stragglers from the Task 13 verification sweep. The L220 comment referencing the original Clones.clone() bug is intentional historical context and stays.
The smoke test sent the tuple as (string,string,address,address[],string,address,uint96,string) but the actual CreateParams721 struct in src/collections/interfaces/CollectionTypes.sol is (address owner, string name, string symbol, string baseURI, string contractURI, address royaltyRecipient, uint96 royaltyBps, address[] additionalMinters). The selector mismatch caused the call to revert with empty data. Verified post-fix on zkSync Sepolia: smoke test now successfully creates a per- collection ERC1967Proxy with all expected invariants (Upgraded → Initialized → RoleGranted → CollectionCreated emit order; EIP-1967 impl slot constructor-fixed; operator auto-granted MINTER_ROLE).
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Specifies an operator-triggered NFT collection factory: users pay in fiat off-chain, a trusted backend deploys a fully-isolated EIP-1167 clone (ERC-721 or ERC-1155) on the user's behalf. Factory is UUPS-upgradeable; clones are immutable per release. Both creator and operator hold MINTER_ROLE on each clone; per-collection metadata and royalties are owner-mutable until locked one-way. On-chain externalId map prevents duplicate creations from the off-chain ledger.