Remove artifact ledger from witnesses by andrew-fleming · Pull Request #389 · OpenZeppelin/compact-contracts

andrew-fleming · 2026-03-14T14:23:46Z

This PR proposes to use a generic ledger in the ZOwnablePK witnesses file. This removes the witness dependence on the generated artifact (which is from the mock contract). Updating the simulator is relatively straightforward bc it already has access to the ledger

This PR also updates the simulator package accordingly

Summary by CodeRabbit

Documentation
- Updated simulator documentation and examples with improved type patterns and workflow guidance.
Refactor
- Enhanced type system for witness factories and interfaces to support better ledger type inference across the simulator framework.

coderabbitai · 2026-03-14T14:24:02Z

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7d902174-3ec0-464a-aec2-a27231f1f419

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

Walkthrough

This PR generalizes witness interfaces and factories across the codebase to accept a generic ledger type parameter, replacing hardcoded Ledger dependencies with a type parameter L. Changes propagate through witness definitions, simulators, and related documentation without altering control flow or functional logic.

Changes

Cohort / File(s)	Summary
Core Witness Generalization `contracts/src/access/witnesses/ZOwnablePKWitnesses.ts`	Updated `IZOwnablePKWitnesses` interface to accept generic ledger parameter `<L, P>` instead of hardcoded Ledger. Factory `ZOwnablePKWitnesses` now generic over L. Added public type `ZOwnablePKPrivateState` with helper object exposing `generate()` and `withNonce()` methods. Removed Ledger import.
Test Fixture Witness Generalization `packages/simulator/test/fixtures/sample-contracts/witnesses/SampleZOwnableWitnesses.ts`, `packages/simulator/test/fixtures/sample-contracts/witnesses/WitnessWitnesses.ts`	Made both witness interfaces and factories generic over ledger type L. Updated all context parameters from `WitnessContext<Ledger, P>` to `WitnessContext<L, P>`. Removed Ledger type dependencies from factory signatures.
Simulator Integration `contracts/src/access/test/simulators/ZOwnablePKSimulator.ts`, `packages/simulator/test/integration/SampleZOwnableSimulator.ts`, `packages/simulator/test/integration/WitnessSimulator.ts`	Added type alias extracting ledger type via `ReturnType<typeof ledger>` in each simulator. Updated `witnessesFactory` configurations to instantiate witnesses with the concrete ledger type as generic parameter (e.g., `ZOwnablePKWitnesses<ZOwnablePKLedger>()`). Minor JSDoc formatting changes.
Documentation Updates `CHANGELOG.md`, `packages/simulator/README.md`	Added changelog entry documenting generic ledger type introduction. Updated README examples and Quick Start steps to reflect ledger type extraction and generic witness factory pattern with newly added type aliases and parameterized witness instantiation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Upgrade Simulator, Contracts to 0.29.0 #366: Introduces the same generic ledger type pattern for witness objects and updates simulator configurations to leverage generic witness factories with concrete ledger types, directly overlapping with this PR's core changes.

Suggested reviewers

0xisk

Poem

🐰 With whiskers twitching, I craft with care,
Generic ledgers floating in the air!
No hardcoding here, just L so free,
Type parameters dancing, wild and spree! 🎭

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Remove artifact ledger from witnesses' accurately summarizes the main objective of the PR, which is to replace the generated-artifact-specific ledger dependency with a generic ledger parameter across witness interfaces and implementations.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📝 Coding Plan

Generate coding plan for human review comments

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Tip

CodeRabbit can enforce grammar and style rules using `languagetool`.

Configure the reviews.tools.languagetool setting to enable/disable rules and categories. Refer to the LanguageTool Community to learn more.

coderabbitai

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@CHANGELOG.md`:
- Line 12: The changelog entry "Use generic ledger type in ZOwnablePKWitnesses"
still uses a placeholder "(#)"; update that entry to reference the actual PR or
issue number or URL (e.g., replace "(#)" with the real PR number like "(`#1234`)"
or a full issue/PR link) so the release note is traceable and points to the
correct discussion.

In `@packages/simulator/README.md`:
- Around line 85-86: The comment incorrectly calls the export
MyContractWitnesses a "generic function"; change the wording to call it a
"factory function" instead. Update the surrounding comment where
MyContractWitnesses is referenced to read something like "but for the simulator,
wrap it in a factory function:" so the term matches the implementation (() =>
({})) and avoids implying type generics.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5f32dd5c-fd91-4e7c-8671-35c037a15d3e

📥 Commits

Reviewing files that changed from the base of the PR and between 7b257c4 and c74f42f.

📒 Files selected for processing (8)

CHANGELOG.md
contracts/src/access/test/simulators/ZOwnablePKSimulator.ts
contracts/src/access/witnesses/ZOwnablePKWitnesses.ts
packages/simulator/README.md
packages/simulator/test/fixtures/sample-contracts/witnesses/SampleZOwnableWitnesses.ts
packages/simulator/test/fixtures/sample-contracts/witnesses/WitnessWitnesses.ts
packages/simulator/test/integration/SampleZOwnableSimulator.ts
packages/simulator/test/integration/WitnessSimulator.ts

coderabbitai · 2026-03-14T14:34:22Z

CHANGELOG.md


 ### Changes

+- Use generic ledger type in ZOwnablePKWitnesses (#)


⚠️ Potential issue | 🟡 Minor

Replace the placeholder reference in the changelog entry.

(#) is still a placeholder; please point this to the actual PR/issue so the release note is traceable.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 12, The changelog entry "Use generic ledger type in ZOwnablePKWitnesses" still uses a placeholder "(#)"; update that entry to reference the actual PR or issue number or URL (e.g., replace "(#)" with the real PR number like "(`#1234`)" or a full issue/PR link) so the release note is traceable and points to the correct discussion.

coderabbitai · 2026-03-14T14:34:22Z

packages/simulator/README.md

+// But for the simulator, wrap it in a generic function:
 export const MyContractWitnesses = () => ({});


⚠️ Potential issue | 🟡 Minor

Fix wording: this is a factory function, not a generic function.

The comment says “generic function”, but () => ({}) is non-generic. Rename it to “factory function” to avoid confusion.

Suggested doc fix

-// But for the simulator, wrap it in a generic function: +// But for the simulator, wrap it in a factory function: export const MyContractWitnesses = () => ({});

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

// But for the simulator, wrap it in a generic function:

export const MyContractWitnesses = () => ({});

// But for the simulator, wrap it in a factory function:

export const MyContractWitnesses = () => ({});

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@packages/simulator/README.md` around lines 85 - 86, The comment incorrectly calls the export MyContractWitnesses a "generic function"; change the wording to call it a "factory function" instead. Update the surrounding comment where MyContractWitnesses is referenced to read something like "but for the simulator, wrap it in a factory function:" so the term matches the implementation (() => ({})) and avoids implying type generics.

emnul

Nice way to make the witness files cleaner!

andrew-fleming added 4 commits March 14, 2026 10:58

remove ledger artifact from wit, update sim

c7d06c0

remove ledger artifact from wit, update sim in zownable

7e27f80

update sim readme

7ea82e9

add changelog entry

c74f42f

andrew-fleming requested review from a team as code owners March 14, 2026 14:23

andrew-fleming added 2 commits March 14, 2026 11:31

improve witness docs

f802a54

update changelog

bcb4ce1

coderabbitai bot reviewed Mar 14, 2026

View reviewed changes

This was referenced Mar 14, 2026

Packaging witness files #390

Open

Shielded AccessControl #190

Open

Merge branch 'main' into remove-artifact-ledger-from-witnesses

1b4f5c3

emnul approved these changes Mar 17, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove artifact ledger from witnesses#389

Remove artifact ledger from witnesses#389
andrew-fleming wants to merge 7 commits intoOpenZeppelin:mainfrom
andrew-fleming:remove-artifact-ledger-from-witnesses

andrew-fleming commented Mar 14, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Mar 14, 2026 •

edited

Loading

Review skipped

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Mar 14, 2026

Uh oh!

coderabbitai bot Mar 14, 2026

Uh oh!

emnul left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		### Changes

		- Use generic ledger type in ZOwnablePKWitnesses (#)

		// But for the simulator, wrap it in a generic function:
		export const MyContractWitnesses = () => ({});

Conversation

andrew-fleming commented Mar 14, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Mar 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Mar 14, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Mar 14, 2026

Choose a reason for hiding this comment

Uh oh!

emnul left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

andrew-fleming commented Mar 14, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Mar 14, 2026 •

edited

Loading