remove ledger dep in sample witnesses, update sims#73
Open
andrew-fleming wants to merge 1 commit intoOpenZeppelin:mainfrom
Open
remove ledger dep in sample witnesses, update sims#73andrew-fleming wants to merge 1 commit intoOpenZeppelin:mainfrom
andrew-fleming wants to merge 1 commit intoOpenZeppelin:mainfrom
Conversation
WalkthroughThe pull request generalizes witness interfaces and factory functions by introducing a ledger type parameter Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can use oxc to improve the quality of JavaScript and TypeScript code reviews.Add a configuration file to your project to customize how CodeRabbit runs oxc. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/simulator/test/integration/WitnessSimulator.ts (1)
6-9: Carry the concrete witness type through the simulator generics.Line 34 binds the factory to
WitnessLedger, but lines 23 and 44 still use the uninstantiatedReturnType<typeof WitnessWitnesses>. This widens the witness type again outside the factory call, so this file does not preserve the ledger-specific witness typing end-to-end.Extract a concrete
Witnessestype alias fromIWitnessWitnesses<WitnessLedger, WitnessPrivateState>and use it to replace both occurrences ofReturnType<typeof WitnessWitnesses>in thecreateSimulatorcall (line 23) andBaseSimulatorOptionsconstructor parameter (line 44). This will propagate the ledger-specific witness type consistently through the entire simulator setup.♻️ Proposed fix
import { + type IWitnessWitnesses, WitnessPrivateState, WitnessWitnesses, } from '../fixtures/sample-contracts/witnesses/WitnessWitnesses'; /** Concrete ledger type extracted from the generated artifact */ type WitnessLedger = ReturnType<typeof ledger>; +type Witnesses = IWitnessWitnesses<WitnessLedger, WitnessPrivateState>; const WitnessSimulatorBase = createSimulator< WitnessPrivateState, - ReturnType<typeof ledger>, - ReturnType<typeof WitnessWitnesses>, + WitnessLedger, + Witnesses, WitnessContract<WitnessPrivateState>, WitnessArgs >({ @@ export class WitnessSimulator extends WitnessSimulatorBase { constructor( options: BaseSimulatorOptions< WitnessPrivateState, - ReturnType<typeof WitnessWitnesses> + Witnesses > = {}, ) {Also applies to: 14–15, 20–25, 34–45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/simulator/test/integration/WitnessSimulator.ts` around lines 6 - 9, The simulator currently widens witness typing by using ReturnType<typeof WitnessWitnesses> in createSimulator and BaseSimulatorOptions; instead, add a concrete alias like "type Witnesses = IWitnessWitnesses<WitnessLedger, WitnessPrivateState>" (using WitnessLedger and WitnessPrivateState from the import) and replace both occurrences of ReturnType<typeof WitnessWitnesses> with this Witnesses alias so the concrete ledger-specific witness type flows through createSimulator(...) and the BaseSimulatorOptions constructor consistently; update references to WitnessWitnesses only for the factory binding and use the new Witnesses type for the generic parameters.packages/simulator/test/integration/SampleZOwnableSimulator.ts (1)
9-12: Carry the concrete witness type through the simulator generics.Line 40 fixes the factory call with
SampleZOwnableWitnesses<SampleZOwnableLedger>(), but Lines 29-30 and Line 52 still useReturnType<typeof SampleZOwnableWitnesses>. This drops the concrete ledger type everywhere else in the simulator, so type safety is only enforced at the factory call itself.♻️ Proposed fix
import { + type ISampleZOwnableWitnesses, SampleZOwnablePrivateState, SampleZOwnableWitnesses, } from '../fixtures/sample-contracts/witnesses/SampleZOwnableWitnesses'; /** Concrete ledger type extracted from the generated artifact */ type SampleZOwnableLedger = ReturnType<typeof ledger>; +type Witnesses = ISampleZOwnableWitnesses< + SampleZOwnableLedger, + SampleZOwnablePrivateState +>; const SampleZOwnableSimulatorBase = createSimulator< SampleZOwnablePrivateState, - ReturnType<typeof ledger>, - ReturnType<typeof SampleZOwnableWitnesses>, + SampleZOwnableLedger, + Witnesses, SampleZOwnable<SampleZOwnablePrivateState>, SampleZOwnableArgs >({ @@ export class SampleZOwnableSimulator extends SampleZOwnableSimulatorBase { constructor( ownerId: Uint8Array, instanceSalt: Uint8Array, options: BaseSimulatorOptions< SampleZOwnablePrivateState, - ReturnType<typeof SampleZOwnableWitnesses> + Witnesses > = {}, ) {Also applies to: 20-21, 26-31, 40-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/simulator/test/integration/SampleZOwnableSimulator.ts` around lines 9 - 12, The simulator generics are losing the concrete ledger type by using ReturnType<typeof SampleZOwnableWitnesses> in multiple places; replace those occurrences with the concrete generic form SampleZOwnableWitnesses<SampleZOwnableLedger> so the ledger type is carried through. Specifically, update all generic type references (including the simulator class/type parameter usages and any variable/const typings that currently use ReturnType<typeof SampleZOwnableWitnesses>) to use SampleZOwnableWitnesses<SampleZOwnableLedger>, ensuring the factory call remains SampleZOwnableWitnesses<SampleZOwnableLedger>() and all related symbols (SampleZOwnablePrivateState, SampleZOwnableWitnesses, SampleZOwnableLedger, and the simulator class/type) consistently reference the concrete witness type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/simulator/test/integration/SampleZOwnableSimulator.ts`:
- Around line 9-12: The simulator generics are losing the concrete ledger type
by using ReturnType<typeof SampleZOwnableWitnesses> in multiple places; replace
those occurrences with the concrete generic form
SampleZOwnableWitnesses<SampleZOwnableLedger> so the ledger type is carried
through. Specifically, update all generic type references (including the
simulator class/type parameter usages and any variable/const typings that
currently use ReturnType<typeof SampleZOwnableWitnesses>) to use
SampleZOwnableWitnesses<SampleZOwnableLedger>, ensuring the factory call remains
SampleZOwnableWitnesses<SampleZOwnableLedger>() and all related symbols
(SampleZOwnablePrivateState, SampleZOwnableWitnesses, SampleZOwnableLedger, and
the simulator class/type) consistently reference the concrete witness type.
In `@packages/simulator/test/integration/WitnessSimulator.ts`:
- Around line 6-9: The simulator currently widens witness typing by using
ReturnType<typeof WitnessWitnesses> in createSimulator and BaseSimulatorOptions;
instead, add a concrete alias like "type Witnesses =
IWitnessWitnesses<WitnessLedger, WitnessPrivateState>" (using WitnessLedger and
WitnessPrivateState from the import) and replace both occurrences of
ReturnType<typeof WitnessWitnesses> with this Witnesses alias so the concrete
ledger-specific witness type flows through createSimulator(...) and the
BaseSimulatorOptions constructor consistently; update references to
WitnessWitnesses only for the factory binding and use the new Witnesses type for
the generic parameters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c24306b6-29fc-43a8-92b8-4e73ca32901e
📒 Files selected for processing (4)
packages/simulator/test/fixtures/sample-contracts/witnesses/SampleZOwnableWitnesses.tspackages/simulator/test/fixtures/sample-contracts/witnesses/WitnessWitnesses.tspackages/simulator/test/integration/SampleZOwnableSimulator.tspackages/simulator/test/integration/WitnessSimulator.ts
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
1 participant
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.
Summary by CodeRabbit