deprecate: remove sim_getConsensusContract dependency#142
Conversation
Deprecate initializeConsensusSmartContract() as a no-op and stop making sim_getConsensusContract RPC calls. The consensus contract is now resolved entirely from static chain definitions. Removed the method's fire-and-forget call from client creation and the awaited calls from writeContract and deployContract. Updated all tests accordingly.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR deprecates initializeConsensusSmartContract (replacing it with a no-op that warns), removes its invocations from client and contract workflows, updates tests to assert the deprecation and remove runtime-fetch logic, adds coverage to .gitignore, adds a Vitest coverage devDependency, and deletes a temporary vitest tsconfig. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/chains-actions.test.ts (1)
4-19:⚠️ Potential issue | 🟡 MinorKeep this fixture aligned with
GenLayerChain.The helper now only fills part of the GenLayer chain shape and then hides the mismatch with
as any. That weakens the test against the SDK’s real contract; please add the remaining GenLayer-specific fields or type the fixture asGenLayerChain.As per coding guidelines,
**/*chain*.{ts,tsx}: Extend viem'sChaintype with GenLayer-specific properties:isStudio,consensusMainContract/consensusDataContract,stakingContract,defaultNumberOfInitialValidators, anddefaultConsensusMaxRotations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/chains-actions.test.ts` around lines 4 - 19, The fixture created by makeClient currently only provides a partial Chain shape and uses "as any"; replace or type it as GenLayerChain and populate the GenLayer-specific properties required by the SDK: add isStudio (boolean), consensusMainContract and consensusDataContract (with address, abi, bytecode shapes already used), stakingContract, defaultNumberOfInitialValidators, and defaultConsensusMaxRotations, and ensure rpcUrls and chain.id remain; update the return type of makeClient to GenLayerChain (instead of any) so the test fixture matches the real GenLayerChain shape.tests/contracts-actions.test.ts (1)
64-89:⚠️ Potential issue | 🟡 MinorMake the deprecated initializer fail fast in these harnesses.
These clients now provide a resolved
initializeConsensusSmartContract, sowriteContract()can regress and call the deprecated method again without breaking this suite. Prefer a throwing mock here, or assertnot.toHaveBeenCalled()after each path.Suggested fix
- initializeConsensusSmartContract: vi.fn().mockResolvedValue(undefined), + initializeConsensusSmartContract: vi.fn(async () => { + throw new Error("unexpected initializeConsensusSmartContract call"); + }),Also applies to: 223-242, 301-320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/contracts-actions.test.ts` around lines 64 - 89, The client mock's initializeConsensusSmartContract is currently mocked to resolve (mockResolvedValue), allowing writeContract to silently call the deprecated initializer; change initializeConsensusSmartContract to a failing mock (e.g., vi.fn().mockRejectedValue(new Error('deprecated initializer called')) or vi.fn().mockImplementation(() => { throw new Error('deprecated initializer called') })) so tests fail fast if the deprecated path is invoked, and update the other client mocks found in the same test (the other client occurrences) similarly; alternatively, after exercising each code path assert initializeConsensusSmartContract.not.toHaveBeenCalled() to ensure the deprecated initializer was not used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/chains/actions.ts`:
- Around line 3-14: The parameter name for chainActions is unused causing a
no-unused-vars lint failure; rename the function parameter client to _client in
the chainActions declaration to keep the same signature and semantics while
satisfying the linter (referencing the chainActions function and its deprecated
method initializeConsensusSmartContract).
---
Outside diff comments:
In `@tests/chains-actions.test.ts`:
- Around line 4-19: The fixture created by makeClient currently only provides a
partial Chain shape and uses "as any"; replace or type it as GenLayerChain and
populate the GenLayer-specific properties required by the SDK: add isStudio
(boolean), consensusMainContract and consensusDataContract (with address, abi,
bytecode shapes already used), stakingContract,
defaultNumberOfInitialValidators, and defaultConsensusMaxRotations, and ensure
rpcUrls and chain.id remain; update the return type of makeClient to
GenLayerChain (instead of any) so the test fixture matches the real
GenLayerChain shape.
In `@tests/contracts-actions.test.ts`:
- Around line 64-89: The client mock's initializeConsensusSmartContract is
currently mocked to resolve (mockResolvedValue), allowing writeContract to
silently call the deprecated initializer; change
initializeConsensusSmartContract to a failing mock (e.g.,
vi.fn().mockRejectedValue(new Error('deprecated initializer called')) or
vi.fn().mockImplementation(() => { throw new Error('deprecated initializer
called') })) so tests fail fast if the deprecated path is invoked, and update
the other client mocks found in the same test (the other client occurrences)
similarly; alternatively, after exercising each code path assert
initializeConsensusSmartContract.not.toHaveBeenCalled() to ensure the deprecated
initializer was not used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 309b96b1-6f84-4b7f-b0b1-2700f87b22c2
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
.gitignorepackage.jsonsrc/chains/actions.tssrc/client/client.tssrc/contracts/actions.tssrc/types/clients.tstests/chains-actions.test.tstests/client.test.tstests/contracts-actions.test.tstsconfig.vitest-temp.json
💤 Files with no reviewable changes (3)
- tsconfig.vitest-temp.json
- src/contracts/actions.ts
- src/client/client.ts
1 participant
What
initializeConsensusSmartContract()method as a no-op that emits a warningsim_getConsensusContractRPC call entirely from the codebasewriteContract()anddeployContract()Why
The library should rely on static chain definitions rather than making dynamic RPC calls. This removes a runtime dependency on the
sim_getConsensusContractmethod being available on the chain, simplifying client initialization and reducing unnecessary network calls.Testing done
Decisions made
Kept the method signature for backwards compatibility but made it a pure no-op with a deprecation warning. This allows existing code to continue calling the method without breaking while encouraging migration to chain definitions.
Summary by CodeRabbit
Deprecations
Tests
Chores