Skip to content

Phase 1 of fuzzing: scaffolding + first real test (#64)#130

Draft
ivanvpan wants to merge 4 commits into
mainfrom
64-fuzzing-phase-1-clean
Draft

Phase 1 of fuzzing: scaffolding + first real test (#64)#130
ivanvpan wants to merge 4 commits into
mainfrom
64-fuzzing-phase-1-clean

Conversation

@ivanvpan
Copy link
Copy Markdown

Summary

Phase 1 of the fuzz-testing initiative tracked in #64. Adds a small but real fuzz harness for EqualDistributionStrategy, wires it into CI as a parallel job, and tunes the project's default fuzz settings. No production source files are touched.

What changed

test/fuzz/ (new directory)

  • EqualDistributionStrategy.t.sol — 4 tests:
    • testFuzz_distribute_equalSplitAndDust — happy path; asserts each recipient gets amount / n, the strategy keeps amount % n as dust, and distributionId increments by exactly 1.
    • testFuzz_distribute_reverts_insufficientYieldamount < n reverts before any transfer.
    • test_distribute_reverts_zeroAmount — zero amount reverts.
    • testFuzz_distribute_reverts_notDistributionManager — only the configured manager can call distribute.
  • helpers/MockERC20.sol — minimal mintable ERC-20.
  • helpers/MockDistManagerForFuzz.sol — minimal stub that exposes recipientRegistry() and acts as the authorized caller.
  • README.md — how to run, CI behavior, fork policy.

foundry.toml

  • [profile.default] now sets fuzz = { runs = 1024 } with comments noting the convention and when to revisit.

.github/workflows/test.yml

  • New parallel fuzz job: forge test --match-path 'test/fuzz/**' -vvv (no ETH_RPC_URL — these tests are fork-free).

Why this scope

Existing tests cover voting, time-weighted power, registries, and cycle logic deeply, but distribution strategies and managers are mostly tested at the deploy/init level only. Phase 1 picks the smallest meaningful target on that gap: real production strategy + registry behind ERC1967 proxies, a minimal mock manager and ERC-20, no RPC fork. The intention is to land infrastructure (directory, profile, CI job) plus one well-shaped suite that future phases extend.

Dilemmas / decisions made along the way

  1. Profile strategy ([profile.ci] vs [profile.fuzz] vs none).

    • Workflow has historically set FOUNDRY_PROFILE=ci, but foundry.toml had no [profile.ci], so it was a no-op.
    • Considered adding [profile.fuzz] to support "deeper fuzz on demand," then reconsidered: locally is exactly where developers want more fuzzing, and adding a profile splits where fuzz settings live.
    • Decided to keep one knob in [profile.default] for now (fuzz = { runs = 1024 }) and explicitly leave [profile.ci] absent. Re-evaluate once forge test gets slow (a comment in foundry.toml records the conditions to add a profile).
  2. CI placement (one job vs two).

    • Could have merged fuzz into the existing check job (one compile, simpler).
    • Chose a parallel fuzz job to keep the main test path identical to before fuzz work and to avoid coupling ETH_RPC_URL and fork-test concerns to the fuzz path. Trade-off: a second checkout/compile, but PR latency improves with parallelism.
  3. Vendored libs.

    • Tempting to mirror profile changes into lib/chainlink/contracts/foundry.toml etc.
    • Confirmed those configs only matter when you build inside those subtrees; running forge test from the root uses the root foundry.toml. Vendored libs are intentionally not modified.
  4. FOUNDRY_PROFILE: ci left in workflow.

    • Considered removing it for symmetry (since there is no [profile.ci]).
    • Kept it as repo convention; the comment in foundry.toml explains the no-op merge so it's not surprising.
  5. Mocks vs forks.

    • Phase 1 fuzz is fork-free by policy (deterministic, no RPC rate limits, fast in CI). Existing TestWrapper fork suites are unchanged.
    • Cost: the strategy is pre-funded by minting a MockERC20 directly to it instead of going manager → safeTransfer → strategy. Documented inline so readers don't mistake it for a faithful production trace.
  6. MockDistManagerForFuzz shape.

    • First version had both a public recipientRegistry_ state var and an explicit recipientRegistry() getter (two public surfaces for one value).
    • Simplified to a single public immutable recipientRegistry; the auto-getter matches IDistributionManager.recipientRegistry() selector. Also renamed the file to MockDistManagerForFuzz.sol to avoid filename clash with test/mocks/MockDistributionManager.sol.
  7. What to assert.

    • Could go beyond conservation (event matching, fuzz on caller for happy path, etc.).
    • For Phase 1, scope was kept to a small set of clear invariants documented inline (distributionId, per-recipient share, total paid, dust, access control). Larger property suites are deferred to Phase 2/3.
  8. Plan documents not committed.

    • Drafted FUZZING_PLAN.md and FUZZING_PLAN_PHASE1.md while shaping this work.
    • Not included in this PR — they remain local notes; happy to land a slimmed-down version separately if reviewers want it in-tree.

Test plan

Locally

forge test --match-path 'test/fuzz/**' -vv

Same command CI runs in the new job

forge test --match-path 'test/fuzz/**' -vvv

ivanvpan and others added 4 commits April 24, 2026 16:59
Introduce test/fuzz with MockERC20, MockDistributionManagerForFuzz, and fuzz cases for equal split, dust, and revert paths.

Made-with: Cursor
Add access-control revert test, rename mock to avoid filename clash with test/mocks/, use makeAddr for labeled recipients, drop redundant mint in revert path, convert zero-amount case to a plain unit test, and add inline comments documenting invariants. Expand foundry.toml comment on the ci-profile convention.

Made-with: Cursor
@RonTuretzky
Copy link
Copy Markdown
Contributor

@ivanvpan be aware there is already an I
Open or for this

@daopunk
Copy link
Copy Markdown

daopunk commented Apr 27, 2026

@ivanvpan you should match your format/linter to the master so you don't end up pushing many changes that are just inline params changing to stacked params. so it's easier to review the actual changes.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants