From cfdb451df1e91a861fe180b972960038468d7d44 Mon Sep 17 00:00:00 2001 From: PelleKrab Date: Thu, 9 Jul 2026 15:48:46 -0700 Subject: [PATCH 1/3] docs: add AGENTS.md with agent working conventions Generated with Claude Code Co-Authored-By: Claude --- AGENTS.md | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) create mode 100644 AGENTS.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 000000000..08fd13e5b --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,91 @@ +# 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: `forge test --match-path ` (single file), `--match-test ` (single test), no filter for the full suite. + +## Local toolchain vs CI — KNOWN FOOTGUN + +CI installs Foundry through a pinned GitHub Action with `version: stable`; your local forge may differ and produce different output for `forge fmt` and, in some cases, hashes. + +- `just build`, `just build-dev`, `just lint`, and `just pre-pr` can shell out to `forge fmt` and reformat files **outside your change** (named-argument call expansion in scripts is the usual victim). `just check` uses `forge fmt --check` and should not write. Always inspect `git diff` after formatting-capable targets. +- To fix a CI lint/fmt failure: restore the file to the branch's accepted version or hand-apply CI's reported diff. Do **not** re-run `forge fmt` after restoring unless you intend to re-check the local toolchain output. +- `just semver-lock` does **not** run `forge fmt`, so it is safe from the reformat footgun. +- `just semver-lock` uses `forge build --force`, which avoids stale cached artifacts. The repo sets `bytecode_hash = 'none'`, but not every semver contract pins an exact pragma, so do not overstate cross-toolchain bytecode determinism. +- Some aggregate Justfile targets may reference branch-specific checker scripts. If a target fails because a referenced script is absent, report the Justfile issue and run the targeted checks that exist on the current branch. + +## 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. From d0874ea6f4025ad843d2ed851e1c9d04b061f7e3 Mon Sep 17 00:00:00 2001 From: PelleKrab Date: Fri, 10 Jul 2026 09:28:35 -0700 Subject: [PATCH 2/3] docs: add mise for Foundry version pinning and update test command - Replace raw forge test with just test in AGENTS.md - Remove local toolchain footgun section; add mise instead - Add .mise.toml to pin foundry to stable, matching CI Generated with Claude Code Co-Authored-By: Claude --- .mise.toml | 5 +++++ AGENTS.md | 13 ++----------- 2 files changed, 7 insertions(+), 11 deletions(-) create mode 100644 .mise.toml diff --git a/.mise.toml b/.mise.toml new file mode 100644 index 000000000..921c2195b --- /dev/null +++ b/.mise.toml @@ -0,0 +1,5 @@ +# Pins the Foundry version to match CI. Install mise (https://mise.jdx.dev) then run: +# mise install — installs pinned tools +# mise use — activates them in your shell +[tools] +foundry = "stable" diff --git a/AGENTS.md b/AGENTS.md index 08fd13e5b..c4c68cb46 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -12,17 +12,8 @@ tests win. - `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: `forge test --match-path ` (single file), `--match-test ` (single test), no filter for the full suite. - -## Local toolchain vs CI — KNOWN FOOTGUN - -CI installs Foundry through a pinned GitHub Action with `version: stable`; your local forge may differ and produce different output for `forge fmt` and, in some cases, hashes. - -- `just build`, `just build-dev`, `just lint`, and `just pre-pr` can shell out to `forge fmt` and reformat files **outside your change** (named-argument call expansion in scripts is the usual victim). `just check` uses `forge fmt --check` and should not write. Always inspect `git diff` after formatting-capable targets. -- To fix a CI lint/fmt failure: restore the file to the branch's accepted version or hand-apply CI's reported diff. Do **not** re-run `forge fmt` after restoring unless you intend to re-check the local toolchain output. -- `just semver-lock` does **not** run `forge fmt`, so it is safe from the reformat footgun. -- `just semver-lock` uses `forge build --force`, which avoids stale cached artifacts. The repo sets `bytecode_hash = 'none'`, but not every semver contract pins an exact pragma, so do not overstate cross-toolchain bytecode determinism. -- Some aggregate Justfile targets may reference branch-specific checker scripts. If a target fails because a referenced script is absent, report the Justfile issue and run the targeted checks that exist on the current branch. +- 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 From cd431b140cca1315d9e61e8c87aa56c0c54cecc8 Mon Sep 17 00:00:00 2001 From: PelleKrab Date: Fri, 10 Jul 2026 17:08:38 -0700 Subject: [PATCH 3/3] fix(ci): pin foundry to 1.5.1 in mise and CI --- .github/workflows/test.yml | 2 +- .mise.toml | 18 ++++++++++++++---- 2 files changed, 15 insertions(+), 5 deletions(-) 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 index 921c2195b..709c65daa 100644 --- a/.mise.toml +++ b/.mise.toml @@ -1,5 +1,15 @@ -# Pins the Foundry version to match CI. Install mise (https://mise.jdx.dev) then run: -# mise install — installs pinned tools -# mise use — activates them in your shell +# 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 = "stable" +# Foundry (forge/cast/anvil/chisel) — pinned for deterministic build +# artifacts, ABI/storage snapshots, and semver-lock hashes. +foundry = "1.5.1"