Deployments export and NPM packaging improvement #1957
Conversation
WalkthroughThis update introduces unified TypeScript modules for instantiating and managing Kleros smart contract instances across multiple Ethereum-compatible networks and deployment environments, supporting both Ethers.js and Viem libraries. New integration tests validate the correctness of contract retrieval for various deployment configurations. The package's build, export, and publishing scripts are improved for consistency and clarity, and new utilities and tests are added for contract address verification. Indexing and re-export modules are provided for streamlined access to deployment artifacts and contract getter functions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Provider
participant getContracts (Ethers/Viem)
participant DeploymentConfig
participant ContractFactory
User->>getContracts: Call with (provider/client, deployment)
getContracts->>DeploymentConfig: Lookup contract addresses/ABIs for deployment
getContracts->>ContractFactory: Instantiate contract objects with provider/client
ContractFactory-->>getContracts: Contract instances
getContracts-->>User: Return contract instances (core, modules, common, RNG, etc.)
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (12)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
❌ Deploy Preview for kleros-v2-university failed. Why did it fail? →
|
❌ Deploy Preview for kleros-v2-testnet failed. Why did it fail? →
|
❌ Deploy Preview for kleros-v2-neo failed. Why did it fail? →
|
❌ Deploy Preview for kleros-v2-testnet-devtools failed. Why did it fail? →
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
contracts/scripts/publish.sh (1)
17-17: Consider implementing version rollbackThe TODO comment indicates a missing feature to roll back version bumps if the publishing process fails. This would prevent version number inconsistencies if errors occur.
function _finally { - # TODO: rollback version bump + if [ $? -ne 0 ] && [ -f "package.json.bak" ]; then + # Restore the original package.json if an error occurred + mv package.json.bak package.json + echo "Rolled back version bump due to error" + fi rm -rf $SCRIPT_DIR/../dist }contracts/test/integration/getContractsViem.test.ts (1)
73-81: Consider mocking external RPC endpoints for more stable testsThe test uses live RPC endpoints, which could make tests unstable if these endpoints are unavailable.
For more reliable tests, consider using mocked clients:
import { createPublicClient, http } from "viem"; import { arbitrum, arbitrumSepolia } from "viem/chains"; import sinon from "sinon"; // Create a stubbed client that doesn't make real network requests const stubbedTransport = sinon.stub().returns({ request: async () => ({ chainId: arbitrumSepolia.id }), }); const arbitrumSepoliaClient = createPublicClient({ chain: arbitrumSepolia, transport: { request: stubbedTransport } as any, });contracts/test/integration/getContractsEthers.test.ts (1)
92-93: Consider using mocked providers for more stable testsSimilar to the Viem tests, using live RPC endpoints could make tests unstable.
For more reliable tests, consider mocking the providers:
import { ethers } from "ethers"; import sinon from "sinon"; // Create a stubbed provider that doesn't make real network requests const arbitrumSepoliaProvider = new ethers.JsonRpcProvider(); sinon.stub(arbitrumSepoliaProvider, "getNetwork").resolves({ chainId: BigInt(arbitrumSepolia.id), name: "arbitrum-sepolia" });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
contracts/deployments/contractsEthers.ts(1 hunks)contracts/deployments/contractsViem.ts(1 hunks)contracts/deployments/index.ts(1 hunks)contracts/package.json(4 hunks)contracts/scripts/publish.sh(2 hunks)contracts/scripts/utils/contracts.ts(4 hunks)contracts/test/integration/getContractsEthers.test.ts(1 hunks)contracts/test/integration/getContractsViem.test.ts(1 hunks)contracts/test/utils/getActualAddress.test.ts(1 hunks)contracts/test/utils/getActualAddress.ts(1 hunks)contracts/tsconfig-release.json(0 hunks)contracts/wagmi.config.devnet.ts(1 hunks)contracts/wagmi.config.mainnet.ts(1 hunks)contracts/wagmi.config.testnet.ts(1 hunks)
💤 Files with no reviewable changes (1)
- contracts/tsconfig-release.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
contracts/test/utils/getActualAddress.test.ts (1)
contracts/test/utils/getActualAddress.ts (1)
getActualAddress(8-23)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
🔇 Additional comments (46)
contracts/test/utils/getActualAddress.ts (1)
1-23: Well-implemented utility function with comprehensive error handling.This utility function is nicely crafted with:
- Clear JSDoc documentation
- Proper TypeScript typing
- Comprehensive error handling with specific error messages
- Clean implementation using dynamic imports with type assertion
The function effectively serves its purpose of retrieving contract addresses from deployment files while providing meaningful errors when issues occur.
contracts/wagmi.config.testnet.ts (1)
2-2: Good transition to local artifact imports.Updating the import path from an external package to a local relative path is a good change that reduces external dependencies and ensures artifact version consistency. This aligns well with the PR objectives of improving NPM packaging.
contracts/wagmi.config.devnet.ts (1)
2-2: Good transition to local artifact imports.Updating the import path from an external package to a local relative path is a good change that reduces external dependencies and ensures artifact version consistency. This aligns well with the PR objectives of improving NPM packaging.
contracts/wagmi.config.mainnet.ts (1)
2-2: Good transition to local artifact imports.Updating the import path from an external package to a local relative path is a good change that reduces external dependencies and ensures artifact version consistency. This aligns well with the PR objectives of improving NPM packaging.
contracts/test/utils/getActualAddress.test.ts (1)
1-22: Well-structured test suite with comprehensive coverageThis test suite for
getActualAddressis well-designed with good coverage of the expected functionality:
- Verifies successful retrieval of a valid Ethereum address
- Validates error handling for non-existent networks
- Confirms proper error handling for non-existent contracts
The tests appropriately use async/await patterns with chai's promise rejection expectations, providing clear assertions and error message validations.
contracts/scripts/publish.sh (6)
1-3: Improved script portability with better shebang and extended globbingThe updated shebang line (
#!/usr/bin/env bash) is more portable across different environments, and enabling extended globbing provides more powerful pattern matching capabilities that will help with the file operations later in the script.
25-28: Good safety check for script execution contextAdding this directory check prevents accidental execution from the wrong location, which could cause unintended consequences. This is a good practice for scripts that perform sensitive operations like publishing packages.
30-38: Improved build process flowThe reordering of the build process to clean and rebuild before other operations ensures a fresh start. The removal of mock artifacts and debug JSON files before regenerating typechain types helps create a cleaner package.
50-53: Enhanced distribution build processUsing
yarn build:allinstead of just TypeScript compilation likely provides a more comprehensive build that includes all necessary artifacts for both CJS and ESM modules, supporting the PR's objective of better packaging.
58-64: Thorough cleanup of unnecessary filesThe additional cleanup steps to remove mock files from the distribution package create a cleaner, more focused package that only contains production-relevant code.
75-77: Safer publishing approach using dry runChanging to
npm publish --dry-runis a safer approach that allows verification of what would be published without actually releasing it to the npm registry. This is particularly helpful during development and testing of the publishing process.contracts/scripts/utils/contracts.ts (4)
30-35: Improved documentation with JSDoc commentsAdding JSDoc comments to the
getContractNamesfunction enhances code documentation, making it more accessible to other developers and providing clear information about parameters and return values.
73-79: Improved documentation with JSDoc commentsSimilar to the previous function, adding JSDoc comments to the
getContractsfunction improves code documentation and provides valuable context for developers.
99-100: Code refactoring to reduce duplicationMoving the declarations of
disputeKitClassicanddisputeResolverout of the switch statement to a single instantiation point reduces code duplication and improves maintainability. This change ensures that these contracts are instantiated consistently regardless of the core type.
131-136: Improved documentation with JSDoc commentsAdding JSDoc comments to the
getContractsFromNetworkandgetContractNamesFromNetworkfunctions completes the documentation effort for this module, providing a consistent approach to code documentation throughout the file.Also applies to: 147-152
contracts/deployments/index.ts (1)
1-17: Well-structured module exports for multiple environments and librariesThis new index file elegantly organizes the package exports in a clean, logical structure:
- Typechain Ethers v6 artifacts grouped by network
- Viem artifacts for different deployment environments
- Re-exports from typechain-types for complete type coverage
- Unified access to contract getters for both Ethers and Viem libraries
This structure supports the PR objective of improving packaging by providing clear and consistent entry points for both CJS and ESM module systems, and it enables developers to seamlessly use either Viem or Ethers v6 libraries with the same underlying deployment artifacts.
contracts/package.json (4)
5-15: Improved package.json configuration for dual module support!The new fields correctly define entry points for different module systems. The
exportsfield is properly configured to support both CommonJS and ES module imports.
16-31: Verify thefilesfield includes necessary directoriesWhile the build output directories (
types,esm,cjs) are properly included, I noticed several source directories are also listed. This is unusual as published packages typically only include build outputs, not source files.Are all these directories intentionally included in the published package? If not, consider moving source directories into the
srcfolder and including only build outputs.
93-96: Well-structured build scripts for multiple module formatsThe build scripts correctly generate outputs for CommonJS, ESM, and TypeScript declaration files. The package.json files in each output directory properly specify the module type.
One minor suggestion for better cross-platform compatibility:
- "build:all": "rm -rf ./dist && yarn build:cjs && yarn build:esm && yarn build:types", + "build:all": "rimraf ./dist && yarn build:cjs && yarn build:esm && yarn build:types",
113-113: Added testing dependenciesThe added dependencies
sinonand@types/sinonare appropriate for mocking in tests.Also applies to: 139-139
contracts/test/integration/getContractsViem.test.ts (8)
1-22: Well-structured test setup with appropriate typesThe test imports, constants, and type definitions are well organized. The
ContractMappingtype is particularly useful for type safety when verifying contracts.
23-69: Comprehensive contract mappings for different deploymentsThe contract mappings for base, university, and neo deployments are thorough and properly account for optional contracts like RNG modules.
84-111: Good helper functions for contract verificationThe helper functions effectively encapsulate the verification logic, making the test cases cleaner and more maintainable.
114-129: Robust address verification against actual deployment filesUsing
getActualAddressto verify against the actual deployment addresses is a robust approach to ensure contract instances have the correct addresses.
131-149: Thorough testing of devnet deploymentThe test covers all important aspects: chain ID verification, contract instance validation, and address verification. Good specific checks for RNG instances that should be present or absent.
151-169: University deployment testing follows same patternThe test for university deployment maintains consistency with the devnet test pattern, which is good for maintainability.
171-189: Testnet and mainnetNeo deployment tests are consistentBoth tests follow the same pattern as previous tests, maintaining consistency throughout the test suite.
Also applies to: 191-209
211-219: Good error case testingTesting the error case for unsupported deployment ensures robust error handling.
contracts/test/integration/getContractsEthers.test.ts (7)
1-34: Well-structured imports and type definitionsThe imports are comprehensive and the type definitions are clear. Using type inference from the
getContractsreturn type forContractMappingis a smart approach to maintain type safety.
42-88: Comprehensive contract mappings for different deploymentsAs with the Viem tests, the contract mappings are thorough and properly account for optional contracts.
95-100: Clever approach to get constructor for instance checksUsing a helper function to get the constructor for type checking is a good workaround for checking instance types.
102-125: Thorough common contract instance verificationThe verification checks all contract instances against their expected types, including conditional checks for optional RNG contracts.
128-171: Good helper functions for address verificationThe helper functions for address verification are well-structured and reusable across test cases.
173-190: Consistent and thorough testing for all deploymentsAll deployment tests follow the same pattern, checking instance types, addresses, and specific RNG availability. This consistency is good for maintainability.
Also applies to: 192-213, 215-232, 234-251
253-258: Good error case testing with async/awaitProperly testing the error case for an unsupported deployment using async/await and chai's expect.
contracts/deployments/contractsEthers.ts (6)
1-85: Comprehensive imports for contract configurations and typesThe imports are well-organized, clearly separating configurations for different deployment environments and contract factory types.
87-100: Clear deployment configuration with chain IDsThe deployments constant clearly maps deployment names to their respective chain IDs using a const assertion for type safety.
102-113: Good address retrieval utility with error handlingThe
getAddressfunction effectively extracts addresses for specific chains and includes proper error handling for missing addresses.
115-141: Well-typed interfaces for contract configurations and instancesThe type definitions for
CommonFactoriesConfigsandCommonFactoriesprovide clear expectations for the inputs and outputs of the factory function.
143-168: Robust common factories functionThe
getCommonFactoriesfunction properly handles optional RNG contracts by returning null when not configured. The function is well-structured and type-safe.
170-267: The main getContracts function handles all deployment scenariosThe switch statement properly handles different deployment types, initializing the appropriate contracts for each scenario. The function correctly throws an error for unsupported deployments.
One minor note: The function is declared as
asyncbut doesn't contain anyawaitoperations. This might be for consistency with other APIs or to support future async operations.contracts/deployments/contractsViem.ts (5)
94-98: Add early‑exit guard for missing addresses (improves DX).
getAddressthrows, but the error message does not convey which contract was missing. Passing the config name (or at least the known addresses) would save time when debugging multi‑chain issues.-function getAddress(config: ContractConfig, chainId: number): `0x${string}` { - const address = config.address[chainId]; - if (!address) throw new Error(`No address found for chainId ${chainId}`); - return address; +function getAddress( + config: ContractConfig, + chainId: number, + label?: string, +): `0x${string}` { + const address = config.address[chainId]; + if (!address) { + throw new Error( + `No address found for chainId ${chainId}${ + label ? ` (contract: ${label})` : "" + }`, + ); + } + return address; }You would then pass the label from
getContractConfig.
[ suggest_optional_refactor ]
171-184: FIXME is a blocker – university deployment re‑uses devnet artefacts.The TODO comments indicate that several contracts (
disputeTemplateRegistry,evidence,policyRegistry, …) are copied from devnet rather than having university‑specific deployments. Shipping this as‑is may break consumers that assume addresses differ between deployments or, worse, point them to a wrong chain.At minimum:
- Document clearly in README / code that these are intentional placeholders.
- Fallback to throwing when the university deployment does not yet have its own contracts to prevent silent misconfiguration.
- disputeTemplateRegistry: getContractConfig({ config: devnetDtrConfig, chainId }), // FIXME: should not be shared with devnet + // TODO(university): replace with dedicated deployment artefacts once available. + // A runtime error is preferable to silently pointing to devnet addresses. + disputeTemplateRegistry: (() => { + throw new Error( + "University deployment missing disputeTemplateRegistry artefact", + ); + })(),[ flag_critical_issue ]
240-244: VerifygetContractcall‑site – signature changed in viem ≥1.4.Recent versions of viem expect either:
getContract({ address, abi, publicClient, walletClient })or
getContract({ address, abi, client: { public: pc, wallet: wc } })The second form is correct only for versions ≥1.4.0. If your package.json still pins an older viem, users will get a TS/RT error (“unknown property client”). Please double‑check the version constraint or switch to the first signature for wider compatibility.
[ request_verification ]
Would you run the snippet below to confirm the installed viem major version?#!/bin/bash jq -r '.dependencies.viem' package.json
230-317: Reduce boilerplate by iterating over config keys.The block instantiating every contract repeats the same pattern 13 × . A tiny utility cuts 30+ LOC, removes the risk of omissions, and keeps the code in sync with
ContractInstances.-const klerosCore = getContract({ ...contractConfigs.klerosCore, ...clientConfig }) -const sortition = getContract({ ...contractConfigs.sortition, ...clientConfig }) -// … -const klerosCoreSnapshotProxy = getContract({ - ...contractConfigs.klerosCoreSnapshotProxy, - ...clientConfig, -}) -return { - klerosCore, - sortition, - disputeKitClassic, - disputeResolver, - disputeTemplateRegistry, - evidence, - policyRegistry, - transactionBatcher, - chainlinkRng, - randomizerRng, - blockHashRng, - pnk, - klerosCoreSnapshotProxy, -}; +const instances = Object.fromEntries( + Object.entries(contractConfigs).map(([key, cfg]) => [ + key, + cfg ? getContract({ ...(cfg as ContractInstance), ...clientConfig }) : undefined, + ]), +) as { [K in keyof typeof contractConfigs]: ReturnType<typeof getContract> }; +return instances;Cleaner, easier to extend (e.g., when you add
fastBridge, you only updateContractInstances).[ suggest_essential_refactor ]
51-64: Consider sourcing chain IDs from artefacts instead of hard‑coding.
deploymentshard‑codesarbitrumSepolia.idfor devnet, university & testnet. When you migrate testnet from Sepolia to Arbitrum One (or another L2), forgetting to update this constant will break consumers silently.Prefer deriving the
chainIdfrom the artefact JSON itself (every*.viemexport already contains the target chain in most setups) or from an env variable.[ offer_architecture_advice ]
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
contracts/deployments/contractsEthers.ts (2)
142-239: Contract instantiation function looks good but could benefit from explicit type annotations.The
getContractsfunction successfully implements contract instantiation across multiple deployment environments. It closely mirrors the Viem implementation while adapting for Ethers.js patterns.Consider adding an explicit return type to increase type safety and improve documentation:
- export const getContracts = async (provider: ethers.Provider, deployment: DeploymentName) => { + export const getContracts = async (provider: ethers.Provider, deployment: DeploymentName): Promise<{ + klerosCore: KlerosCore | KlerosCoreNeo | KlerosCoreUniversity; + sortition: SortitionModule | SortitionModuleNeo | SortitionModuleUniversity; + disputeKitClassic: DisputeKitClassic; + disputeResolver: DisputeResolver; + disputeTemplateRegistry: DisputeTemplateRegistry; + evidence: EvidenceModule; + policyRegistry: PolicyRegistry; + transactionBatcher: TransactionBatcher; + chainlinkRng: ChainlinkRNG | null; + randomizerRng: RandomizerRNG | null; + blockHashRng: BlockHashRNG; + pnk: PNK; + klerosCoreSnapshotProxy: KlerosCoreSnapshotProxy; + }> => {
127-139: Consider using undefined instead of null for optional contracts.The function currently returns
nullfor optional contracts that are not available in a particular deployment. In TypeScript, it's generally more conventional to useundefinedfor optional values.Consider updating the return types and values:
type CommonFactories = { disputeKitClassic: DisputeKitClassic; disputeResolver: DisputeResolver; disputeTemplateRegistry: DisputeTemplateRegistry; evidence: EvidenceModule; policyRegistry: PolicyRegistry; transactionBatcher: TransactionBatcher; - chainlinkRng: ChainlinkRNG | null; - randomizerRng: RandomizerRNG | null; + chainlinkRng: ChainlinkRNG | undefined; + randomizerRng: RandomizerRNG | undefined; blockHashRng: BlockHashRNG; pnk: PNK; klerosCoreSnapshotProxy: KlerosCoreSnapshotProxy; }; // And in the implementation: chainlinkRng: configs.chainlinkRngConfig ? ChainlinkRNG__factory.connect(getAddress(configs.chainlinkRngConfig, chainId), provider) - : null, + : undefined, randomizerRng: configs.randomizerRngConfig ? RandomizerRNG__factory.connect(getAddress(configs.randomizerRngConfig, chainId), provider) - : null, + : undefined,contracts/test/integration/getContractsEthers.test.ts (4)
92-93: Hardcoded RPC URLs could be centralized or made configurable.Using hardcoded URLs for production RPC endpoints can create maintenance issues if these endpoints change, and may expose tests to rate limiting during frequent test runs.
Consider centralizing these URLs or making them configurable:
- const arbitrumSepoliaProvider = new ethers.JsonRpcProvider("https://sepolia-rollup.arbitrum.io/rpc"); - const arbitrumProvider = new ethers.JsonRpcProvider("https://arb1.arbitrum.io/rpc"); + // Import from a central configuration file or environment variables + import { RPC_URLS } from "../../config"; + + const arbitrumSepoliaProvider = new ethers.JsonRpcProvider(RPC_URLS.arbitrumSepolia); + const arbitrumProvider = new ethers.JsonRpcProvider(RPC_URLS.arbitrum);
58-72: Redundancy in contract mappings could be reduced with inheritance patterns.The
universityContractMappingandneoContractMappingobjects share most properties withbaseContractMappingbut override a few specific entries. This creates redundancy.Consider using object spread to derive these mappings:
const baseContractMapping: ContractMapping = { klerosCore: { name: "KlerosCore" }, sortition: { name: "SortitionModule" }, // ... other properties }; - const universityContractMapping: ContractMapping = { - klerosCore: { name: "KlerosCoreUniversity" }, - sortition: { name: "SortitionModuleUniversity" }, - disputeKitClassic: { name: "DisputeKitClassicUniversity" }, - disputeResolver: { name: "DisputeResolverUniversity" }, - disputeTemplateRegistry: { name: "DisputeTemplateRegistry" }, - // ... duplicate properties - }; + const universityContractMapping: ContractMapping = { + ...baseContractMapping, + klerosCore: { name: "KlerosCoreUniversity" }, + sortition: { name: "SortitionModuleUniversity" }, + disputeKitClassic: { name: "DisputeKitClassicUniversity" }, + disputeResolver: { name: "DisputeResolverUniversity" }, + }; - const neoContractMapping: ContractMapping = { - klerosCore: { name: "KlerosCoreNeo" }, - sortition: { name: "SortitionModuleNeo" }, - disputeKitClassic: { name: "DisputeKitClassicNeo" }, - disputeResolver: { name: "DisputeResolverNeo" }, - // ... duplicate properties - }; + const neoContractMapping: ContractMapping = { + ...baseContractMapping, + klerosCore: { name: "KlerosCoreNeo" }, + sortition: { name: "SortitionModuleNeo" }, + disputeKitClassic: { name: "DisputeKitClassicNeo" }, + disputeResolver: { name: "DisputeResolverNeo" }, + chainlinkRng: { name: "ChainlinkRNG", optional: false }, + randomizerRng: { name: "RandomizerRNG", optional: false }, + };Also applies to: 74-88
253-258: Consider making the error message assertion more specific.The error message assertion is using a regex that matches multiple possible error messages. While this works, it might hide subtle differences in error behavior.
Consider making this assertion more specific:
- await expect(getContracts(arbitrumSepoliaProvider, "invalid")).to.be.rejectedWith( - /Unsupported deployment|Cannot destructure property/ - ); + await expect(getContracts(arbitrumSepoliaProvider, "invalid")).to.be.rejectedWith( + "Unsupported deployment: invalid" + );If there are legitimate different error paths, add separate test cases for each scenario to explicitly document the expected behavior.
184-185: Test expectations differ between deployment types.The tests for different deployments have different expectations for RNG contracts:
- Devnet:
chainlinkRngnot null,randomizerRngnull- University:
chainlinkRngnot null,randomizerRngnull- Testnet:
chainlinkRngnot null,randomizerRngnull- MainnetNeo: Both not null
These differences are implicit and not explained in the test comments.
Consider adding comments to explain why different deployments have different RNG contracts available, or at least make the pattern more explicit:
// Verify contract instances expect(contracts.klerosCore).to.be.instanceOf(getConstructor(KlerosCoreNeo__factory, arbitrumProvider)); expect(contracts.sortition).to.be.instanceOf(getConstructor(SortitionModuleNeo__factory, arbitrumProvider)); verifyCommonContractInstances(contracts, arbitrumProvider); + // MainnetNeo has both RNG contracts available expect(contracts.chainlinkRng).to.not.be.null; expect(contracts.randomizerRng).to.not.be.null;Also applies to: 207-208, 226-227, 245-246
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (8)
contracts/deployments/contractsEthers.ts(1 hunks)contracts/deployments/contractsViem.ts(1 hunks)contracts/deployments/index.ts(1 hunks)contracts/deployments/utils.ts(1 hunks)contracts/package.json(4 hunks)contracts/scripts/publish.sh(3 hunks)contracts/test/integration/getContractsEthers.test.ts(1 hunks)contracts/test/integration/getContractsViem.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- contracts/deployments/utils.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- contracts/test/integration/getContractsViem.test.ts
- contracts/deployments/index.ts
- contracts/package.json
- contracts/scripts/publish.sh
- contracts/deployments/contractsViem.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
contracts/deployments/contractsEthers.ts (4)
contracts/deployments/utils.ts (4)
ContractConfig(20-23)getAddress(25-29)DeploymentName(18-18)deployments(3-16)contracts/deployments/index.ts (2)
getContracts(18-18)getContracts(19-19)contracts/deployments/contractsViem.ts (1)
getContracts(202-289)contracts/scripts/utils/contracts.ts (1)
getContracts(79-129)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: SonarCloud
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
contracts/deployments/contractsEthers.ts (1)
142-147: Why is this function marked as async when it doesn't contain awaits?The
getContractsfunction is marked asasyncbut doesn't contain anyawaitexpressions. This makes it unnecessarily return a Promise.Is there a specific reason for marking this function as
async? If it's to maintain a consistent interface with another implementation that does useawait, that makes sense. Otherwise, removing theasynckeyword would make the function more straightforward.contracts/test/integration/getContractsEthers.test.ts (3)
95-100: Smart helper function for constructor testing.The
getConstructorhelper function is a clever approach to testing class instances without exposing TypeScript-specific implementation details.
127-153: Comprehensive address verification.The test utilities for verifying contract addresses are thorough. They check for proper format, non-zero value, and match against expected deployment addresses.
173-259: Well-structured test cases for each deployment environment.The test cases for different deployment environments are well-structured and consistent. They verify chain IDs, contract instances, and addresses, which provides thorough coverage of the
getContractsfunction.
|
Code Climate has analyzed commit e832cc7 and detected 30 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
|
Improvements
PR-Codex overview
This PR primarily updates the import paths for the
IHomeGatewayinterface and enhances the deployment configurations and utility functions for handling contract instances across different networks. Additionally, it introduces tests for contract address retrieval.Detailed summary
IHomeGatewayincontracts/wagmi.config.devnet.ts,contracts/wagmi.config.mainnet.ts, andcontracts/wagmi.config.testnet.ts.contracts/deployments/index.tsfor various network configurations.deploymentsobject incontracts/deployments/utils.tswith chain IDs for different networks.getActualAddressfunction incontracts/test/utils/getActualAddress.tsto retrieve deployed contract addresses.getActualAddressincontracts/test/utils/getActualAddress.test.ts.contracts/scripts/publish.shfor improved error handling and build processes.contracts/package.jsonto0.9.3and updated module paths.contracts/deployments/contractsEthers.tsandcontracts/deployments/contractsViem.ts.contracts/test/integration/getContractsEthers.test.ts.Summary by CodeRabbit