ci(solidity): clear the remaining type errors and gate tsc in CI - #4207
Open
mswilkison wants to merge 4 commits into
Open
ci(solidity): clear the remaining type errors and gate tsc in CI#4207mswilkison wants to merge 4 commits into
mswilkison wants to merge 4 commits into
Conversation
`random-beacon/tsconfig.json` sets `noImplicitAny: true`, but typechain 7
emits its ABI literals as bare `const _abi = [{ inputs: [], ... }]`. Under
that flag an empty array literal infers `any[]`, so every factory the
generator produced raised TS7018 -- 21 errors, all of them in generated
code that no one can edit. typechain 8 emits the same literals `as const`
and the whole class disappears.
The alternative was dropping `noImplicitAny`, which would have bought a
clean run by checking our own code less. This keeps the flag.
`@typechain/hardhat` is pinned to ^7 rather than the current ^9 because
9 defaults to the `ethers-v6` target and this package is still on v5.
The bump changes the output layout: v8 nests modules under their source
path instead of emitting flat files, so five deep imports move. Four are
Solidity library namespaces (`Groups`, `BeaconDkg`), which still have to
come from the module of a contract that uses them; the fifth,
`RandomBeaconGovernance`, is an ordinary contract type and now comes from
the barrel with its neighbours.
`yarn test` is unchanged at 955 passing, 0 failing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`submitRelayEntry` takes `bytes`, and the test handed it a `BigNumber` built from 32 random bytes, which `tsc` rejects as not `BytesLike`. It was not only a typing problem. ethers renders a `BigNumber` argument through `toHexString()`, which drops leading zero bytes, so about one run in 256 submitted an entry shorter than 32 bytes. The stub only keccak-hashes the value, so nothing failed and nothing noticed. The random bytes are a `Uint8Array`, already `BytesLike`, so they go in directly. `yarn test` is unchanged at 673 passing, 0 failing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing in CI ran the type checker. `yarn lint` is eslint, and hardhat transpiles tests without checking them, so a type error reached main only if someone happened to run `tsc` by hand. Both packages are at zero errors as of the two commits before this one, and this keeps them there. The step goes in `contracts-lint`, which already runs `yarn build` -- `typecheck` needs the generated `typechain/` to exist. Worth being precise about what this enforces: neither tsconfig sets `strict`, so zero is zero under the settings the packages already use. random-beacon additionally has `noImplicitAny`; ecdsa does not, and turning it on there would surface 112 errors (27 in typechain 6 output, 85 in hand-written code). Raising ecdsa to typechain 8 and then to `noImplicitAny` is worth doing separately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`yarn install` in this package drops a 1 MB `.yarn/install-state.gz` that shows up as untracked and is easy to sweep into a commit. random-beacon already ignores it; the root `.gitignore` rule does not apply here because a pattern containing a slash is anchored to its own directory. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Jul 27, 2026
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
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.
Stacked on #4206. The tail of the Node 24 chain: with TypeScript raised to 5.9.3, both solidity packages can be brought to zero
tscerrors and held there.Nothing in CI ran the type checker.
yarn lintis eslint, and hardhat transpiles tests without checking them, so a type error only surfaced if someone rantscby hand.What was broken
random-beacontypechain/ecdsatest/WalletRegistry.RandomBeacon.test.tsrandom-beacon— every error wasTS7018in generated factories.random-beacon/tsconfig.jsonsetsnoImplicitAny: true, and typechain 7 emits ABI literals as bareconst _abi = [{ inputs: [], ... }]; under that flag an empty array literal infersany[]. So the package was type-checking generated output more strictly than its generator supported, in files nobody can edit.typechain 8 emits the same literals
as constand the class disappears. The alternative — droppingnoImplicitAny— would have bought a clean run by checking our own code less, so it is not what this does.Two notes on the bump:
@typechain/hardhatis pinned to^7, not the current^9, because 9 defaults to theethers-v6target and this package is still on v5.Groups,BeaconDkg) which still have to come from the module of a contract that uses them; the fifth,RandomBeaconGovernance, is an ordinary contract type and now comes from the barrel with its neighbours.ecdsa— one real bug.submitRelayEntrytakesbytes, and the test passed aBigNumberbuilt from 32 random bytes. Beyond not beingBytesLike, ethers renders aBigNumberargument throughtoHexString(), which drops leading zero bytes — so roughly one run in 256 submitted an entry shorter than 32 bytes. The stub only keccak-hashes the value, so nothing failed and nothing noticed. The random bytes are already aUint8Array, so they now go in directly.The gate
yarn typecheck(tsc --noEmit -p tsconfig.json) added to both packages, wired into the existingcontracts-lintjob — which already runsyarn build, and the check needs the generatedtypechain/to exist.Being precise about what this enforces: neither tsconfig sets
strict, so zero is zero under the settings the packages already use.random-beaconadditionally hasnoImplicitAny;ecdsadoes not, and turning it on there would surface 112 errors (27 in typechain 6 output, 85 hand-written). Raisingecdsato typechain 8 and then tonoImplicitAnyis worth doing, separately.Also ignores
.yarn/install-state.gzinecdsa.yarn installdrops 1 MB there and it shows up untracked;random-beaconalready ignores it, and the root rule does not apply because a pattern containing a slash is anchored to its own directory.Verification
Both suites run on the branch, unchanged from before it:
random-beacon: 955 passing, 0 failingecdsa: 673 passing, 0 failingtsc --noEmit: 0 errors in both packages