Fix create - #134
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe simulator now uses separate TypeScript configurations for building and typechecking, updates its build script, generalizes the static factory signature, and adjusts witness override generic parameters. ChangesSimulator TypeScript and factory updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/simulator/src/factory/createSimulator.ts`:
- Around line 202-213: Update the static create method’s public
overload/signature to return the concrete constructor-derived subclass type
instead of Promise<Simulator>, while retaining a permissive implementation
signature for subclass overrides. Ensure await DerivedSimulator.create(...)
preserves DerivedSimulator-specific members, using the existing this constructor
type and create implementation.
🪄 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: 6b4ed645-9947-4a26-acb0-bef8e4951660
📒 Files selected for processing (5)
packages/simulator/package.jsonpackages/simulator/src/factory/createSimulator.tspackages/simulator/test/integration/Witness.test.tspackages/simulator/tsconfig.build.jsonpackages/simulator/tsconfig.json
| * @returns The constructed simulator (subclass-aware via `this`). | ||
| */ | ||
| static async create<T extends Simulator>( | ||
| static async create( |
There was a problem hiding this comment.
🔵 followup — super.create(tuple, options) is no longer checked against TArgs.
Every subclass has exactly one call site that assembles the tuple, and it is now unchecked:
type Args = readonly [owner: Uint8Array, salt: Uint8Array];
static async create(owner: Uint8Array, salt: Uint8Array, options = {}) {
return super.create([owner], options) as Promise<Sub>; // forgot `salt`
}- on
main:TS2345: Source has 1 element(s) but target requires 2 - with this PR: compiles, then fails at runtime inside
config.contractArgs(...contractArgs)
Direct calls on the base class lose the same protection. Base.create({ backend: 'dry' }) and five-argument calls both type-check now. main rejected both.
A typed sibling keeps override compatibility and the arg checking. I compiled this against all three sample simulators (clean), and the bad call above errors again:
static async create(
this: new (deps: BackendDeps<P, L>) => Simulator,
...args: unknown[]
): Promise<Simulator> {
const [contractArgs, options] = args as [TArgs?, SimulatorOptions<P, W>?];
return (this as unknown as typeof Simulator)._create.call(this, contractArgs, options);
}
/** Typed construction path; subclass `create` overrides should call this. */
static async _create(
this: new (deps: BackendDeps<P, L>) => Simulator,
contractArgs: TArgs = [] as unknown as TArgs,
options: SimulatorOptions<P, W> = {},
): Promise<Simulator> {
const deps = await prepareBackend(contractArgs, options);
return new this(deps);
}Subclasses then call super._create([owner, salt], options). Underscore-public matches the existing _backend / _signers convention for declaration emit. Purely additive, so it can also land after this PR.
added by claude (dev3-code-review)
There was a problem hiding this comment.
Yes, this is better 👍
| // Permissive so subclasses can override `create` with contract-specific | ||
| // params/returns without tripping TS's static-side `extends` check. | ||
| ...args: unknown[] | ||
| ): Promise<Simulator> { |
There was a problem hiding this comment.
🔵 followup — a subclass that does not override create loses its own instance type.
class NoOverride extends Base {
bump() { return this.circuits.impure.setVal(1n); }
}
await (await NoOverride.create([1n])).bump();
// this PR: TS2339: Property 'bump' does not exist on type '...Simulator'
// main: fine, `T` was inferred from `this`Overriding create becomes mandatory for every subclass, including ones that only add circuit methods. Blast radius is small today (composedTokens.ts and sharedInitCollision.ts are the only non-overriders in compact-contracts), but it narrows the API contract. Worth a README line so people meet it as documentation rather than as a type error.
added by claude (dev3-code-review)
There was a problem hiding this comment.
Indirectly addressed above. Improved readme doc d9fea4f
| @@ -199,13 +199,18 @@ export function createSimulator< | |||
| * @param options - Backend selection, witnesses, private state, live world. | |||
| * @returns The constructed simulator (subclass-aware via `this`). | |||
There was a problem hiding this comment.
⚪ nitpick — this @returns is no longer accurate. The return type is now the base Simulator, so it is not subclass-aware. That is precisely what the widening gave up. Suggest something like "the base simulator type; subclass create overrides narrow it".
added by claude (dev3-code-review)
| const UINT_OVERRIDE = 333n; | ||
|
|
||
| const overrideWitnesses = (): IWitnessWitnesses<WitnessPrivateState> => ({ | ||
| const overrideWitnesses = (): IWitnessWitnesses< |
There was a problem hiding this comment.
⚪ nitpick — unknown is correct here, but it reads as arbitrary. WitnessWitnesses is <L>() => IWitnessWitnesses<L, WitnessPrivateState>, so ReturnType<typeof WitnessWitnesses> erases L to unknown, and that erased type is the simulator's W. A named alias in WitnessWitnesses.ts would make the reason visible:
export type WitnessWitnessSet = ReturnType<typeof WitnessWitnesses>;The test then reads const overrideWitnesses = (): WitnessWitnessSet => ({ ... }).
added by claude (dev3-code-review)
0xisk
left a comment
0xisk
left a comment
There was a problem hiding this comment.
Reviewed ed07aec against origin/main (bcce9b5). 3 files, +15/−7.
Verified locally: reproduced the baseline TS2417 on all three sample simulators, confirmed this PR clears it, and ran the dry suite (10 files, 90/90 pass). I also tested two narrower fixes and both still fail, so ...args: unknown[] does look like the minimum that unblocks the override. Per-line notes are inline.
🔴 blocking — the fix has no regression guard
packages/simulator/tsconfig.json has include: ["src/**/*"], so yarn types never compiles test/integration/*Simulator.ts. yarn test is vitest, which strips types without checking them. The create signature can regress and CI stays green.
The same cause makes the Witness.test.ts hunk inert. It fixes a real arity bug (IWitnessWitnesses<L, P> takes two params), but nothing ever compiles that file, so it neither proves the fix nor guards it.
❔ question — why was 9d4281e reverted?
9d4281e had the split that answers the above: tsconfig.build.json for emit, tsconfig.json as noEmit + bundler + include: ["src/**/*", "test/**/*"]. ed07aec removed it. I ran that config verbatim against this branch and it was clean, apart from local @types/node resolution noise on my machine. If CI failed on it, that is worth recording in the PR body. If it was dropped to keep the diff small, it is the fix for the blocking item.
🔵 followup — README is stale on this exact API
packages/simulator/README.md still shows new MySimulator([...], {...}) (L37), BaseSimulatorOptions (L93, L262, L270), and a four-argument createSimulator<P, L, W, TArgs> that is missing the TContract slot (L59). Pre-existing rather than introduced here, but this PR is about create's contract and the README never documents create.
added by claude (dev3-code-review)
…nto fix-create-improve-type-check
Added a regression test. I updated the description with the initial fix that the reason something like this wasn't included was because it would involve a non-trivial refactor on how types are checked. What this PR now proposes is an ugly fix
I'd argue that it's implied in the description (
I updated LMK if you disagree with any of these points |
2 participants
The base
createforced a fixed param shape and a generic return, so subclass overrides fail ts static-side checks. This PR makescreatemore permissive. Without doing a heavy refactor of typechecking, the evidence that this fix works is in the simulators inintegration/. Without the fix, there's an ide error. With the fix, no errorSummary by CodeRabbit