diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index f15fee4c2..e1df41ad5 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -26,7 +26,7 @@ jobs: - name: Install Foundry uses: foundry-rs/foundry-toolchain@82dee4ba654bd2146511f85f0d013af94670c4de # v1.4.0 with: - version: stable + version: v1.5.1 - name: Install Go uses: actions/setup-go@d35c59abb061a4a6fb18e82ac0862c26744d6ab5 # v5.5.0 diff --git a/.mise.toml b/.mise.toml new file mode 100644 index 000000000..709c65daa --- /dev/null +++ b/.mise.toml @@ -0,0 +1,15 @@ +# mise tool versions — https://mise.jdx.dev +# +# This file pins the toolchain versions used across the contracts repo. +# Contributors should install mise and run `mise install` at the repo root so +# that everyone executes builds, tests, and snapshot generation with +# byte-identical tooling. +# +# When bumping any version here, also update the matching pin in +# `.github/workflows/test.yml` (or anywhere else CI installs the same tool) so +# that local and CI runs stay aligned. + +[tools] +# Foundry (forge/cast/anvil/chisel) — pinned for deterministic build +# artifacts, ABI/storage snapshots, and semver-lock hashes. +foundry = "1.5.1" diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 000000000..c4c68cb46 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,82 @@ +# AGENTS.md — base/contracts + +Working conventions for agents on this repo, distilled from prior sessions. +These are defaults for new or touched work, not permission to churn unrelated +code. When this file conflicts with the current tree, nearby code and enforced +tests win. + +## Tooling + +- `just snapshots` — regenerates ABI, storage layout, and semver-lock in one shot. Run after source changes that affect API, storage, or semver snapshots. +- `just semver-lock` — full source build + semver hash. Run before pushing semver-tagged `src/` changes or when `snapshots/semver-lock.json` may change. Do not use `just semver-lock-no-build` after source edits; it reads existing artifacts and can write stale hashes. +- `just semver-lock-no-build` — hash only, from existing artifacts. +- CI regenerates `snapshots/semver-lock.json` and diffs it against the committed file. ABI/storage snapshots are generated by `scripts/autogen`, but check the current workflow before assuming CI diffs every snapshot type. +- Snapshot generation is handled by Go scripts under `scripts/autogen`. +- Tests: `just test` (full suite), `just test --match-path ` (single file), `just test --match-test ` (single test). +- mise is used to keep local and CI Foundry versions in sync. Run `mise install` once after cloning. + +## Contract structure conventions + +### Style-guide ordering inside a contract + +For new or touched contract bodies, prefer: + +Type declarations → Constants → Immutables → State variables → Events → Errors → Constructor → external → external view → external pure → public → public view → public pure → internal → internal view → internal pure → private → private view → private pure. + +### Visibility + +- A concrete implementation **not meant to be inherited** should mark implementation internals `private`, not `internal`. +- Inline single-caller and single-line helpers by default; extract only when logic is genuinely shared across multiple callers. + +### Interface / implementation split + +- Deploy-script-backed protocol implementations generally inherit base contracts and shared mixins (proxy-owned base, initializable, reinitializable, semver), not their own `I` interface. +- When a contract has a paired interface, keep ABI-relevant declarations in sync with that interface. Do not invent a new interface only to satisfy this note. +- The `__constructor__() external` declaration in interfaces is a tooling artifact for ABI/snapshot generation, not callable. Do not remove it. +- For new semver functions in recent protocol work, prefer `function version() public pure virtual returns (string memory)`. + +### API / event surface + +- Use explicit `return x;`, not named returns. +- Trim events: drop params already carried in log metadata (e.g. block number). +- Add indexed query getters when arbitrary entries need querying, not just tail reads. + +### Naming + +- Errors: prefer `ContractName_ErrorName` for new protocol contracts. +- Events: no prefix (e.g. `SomethingRegistered`, `TimestampSet`). +- Test contracts: `ContractName_FunctionName_Test` for standard contract behavior suites; standalone unit tests may use `NameTest`. +- Test functions: prefer `test_functionName_scenario_succeeds` / `_reverts` for new tests. +- Commits: conventional — `type(scope): description` (e.g. `refactor(L1):`, `fix(L1):`). +- Branding: prefer `offchain` / `onchain` in new proof/protocol wording. + +## Access control + +- The proxy-owned base mixin provides `_assertOnlyProxyAdminOwner()` and `_assertOnlyProxyAdminOrProxyAdminOwner()`. +- For `ProxyAdminOwnedBase`, owner = `proxyAdmin().owner()`, read from the proxy-owner storage slot. +- Secondary roles in proxy-owned protocol contracts are plain `address` fields with inline `msg.sender` checks. Inline one-line local role checks. + +## Proxy deployment + +- For fresh deployment of an initialized proxy, use `upgradeToAndCall` (atomic upgrade + init in one tx). Separate `upgradeTo` then `initialize` opens a window of uninitialized state; mutations in that window corrupt persistent state permanently. Separate upgrade-only paths can be valid for admin migrations, tests, or proxies that are not being initialized. +- Upgradeable implementation constructors call `_disableInitializers()`. +- Contracts that inherit `ReinitializableBase` initialize with `reinitializer(initVersion())`. The `reinitializer` modifier fires **before** the function body, so the "already initialized" revert precedes any owner/admin assert — any caller triggers it. Contracts that use plain OpenZeppelin `initializer` are valid exceptions. + +## Adding a new `Initializable` contract + +Standard deploy-script-backed `src/` contracts with `initialize()` must be accounted for in the repo-wide reinitialization test that scans `src/` via FFI. The test has explicit exclusions for categories such as L2/predeploys, periphery, and contracts with custom initialization state. + +- **Deployed via the standard deploy script**: add both `Impl` and `Proxy` entries. Proxy address comes from the shared test setup; impl address via the EIP-1967 implementation-slot helper on the proxy. +- **Not yet in the deploy script**: add the source path to the excludes list (fixed capacity — mind the array size) with a comment. +- Proxied-contract detection uses the `@custom:proxied` devdoc NatSpec tag — a proxied contract missing that tag won't get its proxy entry checked. +- **Always run `forge test --match-test test_cannotReinitialize_succeeds`** when adding a standard initialized contract, in addition to the contract's own tests. A narrow `--match-path` run misses it. +- New production contracts must be wired into the standard deploy script, not permanently excluded from the tracking test. + +## Test conventions + +- Tests for standard deploy-script-deployed system contracts should inherit the shared `CommonTest` base and use the deploy-script-deployed instances it exposes (via the shared `Setup`) — **not** `new Contract()` + a hand-rolled proxy. This makes the deploy script a real dependency of those tests. +- Mirror an existing contract test of the same type: call `super.setUp()`, read parameters from the deploy config, and get the implementation via the EIP-1967 implementation-slot helper. +- If an initializer edge-case test for a deploy-script-backed contract truly needs a fresh uninitialized proxy, deploy **only** a proxy and point it at the already-deployed implementation (via the implementation-slot helper). Do not `new Contract()` in that test file; that dodges the deploy-script dependency. +- Re-initialize / initializer edge-case tests: reset the initializable slot with `vm.store(addr, bytes32(0), bytes32(0))` behind a small helper, then prank the proxy admin. Skip on forks with `skipIfForkTest(...)`. +- Run the **full** suite after adding a contract, not just its own file (cross-cutting registration tests live outside the contract's file). +- Kill redundant/duplicate tests: if an exact-value assertion subsumes a weaker one, keep one.