Various contract scripts improvements #1853
Conversation
WalkthroughThis pull request introduces changes to the Kleros contract configuration and scripts. The modifications include simplifying court names in the Changes
Possibly related PRs
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
🪧 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-testnet-devtools failed. Why did it fail? →
|
❌ Deploy Preview for kleros-v2-testnet failed. Why did it fail? →
|
❌ Deploy Preview for kleros-v2-university failed. Why did it fail? →
|
❌ Deploy Preview for kleros-v2-neo 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: 1
🧹 Nitpick comments (6)
contracts/scripts/utils/contracts.ts (3)
17-21: Consider removing or implementing the UNIVERSITY core.The code includes
Cores.UNIVERSITYeven though it's explicitly marked as "not supported." If there's no plan to support it in the near future, you might remove it from the enum to avoid confusion.
29-46: Unify error messages for unsupported core types.The default case error message specifies "must be one of base, neo," while a separate case for UNIVERSITY references a different message. Consider consolidating these for clearer maintainability, for instance:
case Cores.UNIVERSITY: - throw new Error("University core is not supported"); + throw new Error("Invalid core type, must be one of base, neo");
23-65: Return an explicit type for better IDE support.Returning an inline object is convenient, but consider returning a typed interface for clearer consumption:
interface Contracts { core: KlerosCore | KlerosCoreNeo; sortition: SortitionModule | SortitionModuleNeo; disputeKitClassic: DisputeKitClassic; disputeResolver: DisputeResolver; // ... } export const getContracts = async (...): Promise<Contracts> => { ... }contracts/scripts/changeGovernor.ts (3)
28-33: Clarify the error handling for unknown core types.The code gracefully handles uppercase input (
.toUpperCase()), but might remain susceptible to unexpected user mistakes like extra spaces or special characters. Consider adding more robust input validation or enumerating all core options in the help text.
39-54: Prefer explicit contract interface.
contractInstanceis typed asany; strongly typing it (or accepting a union of known contract types) improves readability and reduces errors.
56-58: Implement the Sortition contract governor change.There is a TODO comment indicating a need to call
await updateGovernor("Sortition", sortition). Completing this ensures consistent governor management across all listed contracts.Do you want me to open an issue or propose a PR to include this invocation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
contracts/config/policies.v2.mainnet-neo.json(2 hunks)contracts/hardhat.config.ts(1 hunks)contracts/package.json(1 hunks)contracts/scripts/changeGovernor.ts(1 hunks)contracts/scripts/utils/contracts.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: dependency-review
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
🔇 Additional comments (4)
contracts/hardhat.config.ts (1)
19-19: Keep new script imports consistent with existing code style.Requiring
"./scripts/changeGovernor"is consistent with other script imports. If you prefer ES modules, you can switch the entire file toimportstatements.contracts/package.json (1)
61-61: Verify the impact of removing the core type flag.The
--core-type neoflag has been removed from thepopulate:policies:mainnetNeoscript. Please ensure this change doesn't affect the policy population process.Run this script to check for any dependencies on the core type flag:
✅ Verification successful
Removal of --core-type flag is safe
The flag was redundant as the script already handles neo-specific configuration through the
--from v2_mainnet_neoparameter, which correctly loads the neo policies frompolicies.v2.mainnet-neo.json.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for dependencies on the core type flag in the populate script # Search for references to core type in the populate script rg -A 5 "core.*type" "scripts/populate-policy-registry.ts" # Check if other scripts use the core type parameter rg -A 5 "core.*type" "scripts/"Length of output: 166
Script:
#!/bin/bash # Find the populate script file fd "populate.*policy.*registry" # Search for any usage of "--core-type" flag in the entire codebase rg --type ts --type js "\-\-core\-type" # Search for any script that handles policy registry population rg "populate.*policy.*registry" -t ts -t jsLength of output: 290
Script:
#!/bin/bash # Examine the populate policy registry script implementation cat contracts/scripts/populatePolicyRegistry.ts # Check git history for any previous usage of core-type git log -p --all -- contracts/scripts/populatePolicyRegistry.ts | grep -A 5 "core.*type"Length of output: 4204
contracts/config/policies.v2.mainnet-neo.json (2)
184-189: LGTM! Court names have been simplified consistently.The removal of the "Court" suffix from court names makes them more concise while maintaining their meaning. The changes are consistent across all modified courts:
- "Humanity Court" → "Humanity"
- "Development Court" → "Development"
- "Solidity Court" → "Solidity"
- "Javascript Court" → "Javascript"
Also applies to: 192-197, 200-205, 208-213
189-189: Verify the accessibility and content of the new IPFS URIs.Please ensure that the new IPFS URIs are accessible and contain the correct content for each court.
Run this script to verify the IPFS URIs:
Also applies to: 197-197, 205-205, 213-213
✅ Verification successful
All IPFS URIs are accessible and contain valid court-specific content ✅
All four URIs are accessible through IPFS and contain properly formatted JSON with appropriate content for each specialized court (Humanity, Development, Solidity, and Javascript). The content structure is consistent and includes all required fields (name, purpose, rules, required skills, and court identifier).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify accessibility and content of new IPFS URIs # List of new IPFS URIs to check URIS=( "QmfH68LJWRQ7UEJqFGDKDpR6hmxmmJrbz2EHJMgqtCgFo6" "QmdiQGftN4Mxtocvf1ENxeEvVzU62AGR3knzfhMDb85iTh" "QmbKfy5vF5jZ5GFqFKgUxnYsbAjJdtsDfp2UJLwxzDokmb" "Qmaf4NzAvyVa4biu7MwaGTTwCe46XVSdBa3t3Uu2soFToz" ) # Check each URI using IPFS gateway for uri in "${URIS[@]}"; do echo "Checking $uri..." curl -s "https://ipfs.io/ipfs/$uri" | jq '.' doneLength of output: 3532
contracts/scripts/changeGovernor.ts
Outdated
| task("change-governor", "Changes the governor for all the contracts") | ||
| .addPositionalParam("newGovernor", "The address of the new governor") | ||
| .addOptionalParam( | ||
| "coreType", | ||
| "The type of core to use between base, neo, university (default: base)", | ||
| Cores.BASE.toString() | ||
| ) |
There was a problem hiding this comment.
💡 Codebase verification
Add address validation using existing utilities
Add validation for the newGovernor address using the isAddress function from viem before proceeding with the transaction. The codebase already uses this pattern in other files.
- In
contracts/scripts/changeGovernor.ts, add validation after getting the address:
import { isAddress } from "viem";
// ...
const newGovernor = taskArgs.newGovernor;
if (!isAddress(newGovernor)) {
throw new Error("Invalid governor address provided");
}🔗 Analysis chain
Validate the new governor address argument.
It would be safer to confirm newGovernor is a valid Ethereum address before proceeding, reducing risk of invalid transactions.
Use a script to confirm references to newGovernor. For instance:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching the usage of newGovernor to verify any type checks or validations
rg --context 3 "newGovernor"
Length of output: 2390
Script:
#!/bin/bash
# Search for contract implementations of changeGovernor and address validation
ast-grep --pattern 'function changeGovernor($$$) {
$$$
}'
# Look for address validation utilities
rg -g '*.ts' -g '*.sol' --context 3 'isAddress|validateAddress|address validation'
Length of output: 5310
jaybuidl
force-pushed
the
chore/contract-scripts-improvements
branch
from
e1660aa to
6370e1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
contracts/scripts/utils/contracts.ts (3)
17-21: Consider documenting or removing the UNIVERSITY core type.The
UNIVERSITYcore type is defined in the enum but throws an error when used. Either:
- Add documentation explaining why it's included but not supported, or
- Remove it if it's not intended to be used.
export enum Cores { BASE, NEO, - UNIVERSITY, }
45-45: Update error message to match all possible enum values.The error message doesn't accurately reflect all possible values from the
Coresenum.- throw new Error("Invalid core type, must be one of base, neo"); + throw new Error("Invalid core type, must be one of: BASE, NEO");
23-65: Consider improving type safety and documentation.A few suggestions to enhance the code:
- Use TypeScript's exhaustive type checking for the switch statement
- Document which contracts can be null in the return type
- Consider using a more type-safe approach for contract type assertions
Here's how you could improve the type safety:
type ContractReturnType = { core: KlerosCore | KlerosCoreNeo; sortition: SortitionModule | SortitionModuleNeo; disputeKitClassic: DisputeKitClassic; disputeResolver: DisputeResolver; disputeTemplateRegistry: DisputeTemplateRegistry; chainlinkRng: ChainlinkRNG | null; randomizerRng: RandomizerRNG | null; blockHashRNG: BlockHashRNG | null; pnk: PNK; batcher: TransactionBatcher; }; export const getContracts = async ( hre: HardhatRuntimeEnvironment, coreType: Cores ): Promise<ContractReturnType> => { // ... existing implementation ... // Exhaustive type check const _exhaustiveCheck: never = coreType; return { // ... existing return ... }; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
contracts/config/policies.v2.mainnet-neo.json(2 hunks)contracts/hardhat.config.ts(1 hunks)contracts/package.json(1 hunks)contracts/scripts/changeGovernor.ts(1 hunks)contracts/scripts/keeperBot.ts(3 hunks)contracts/scripts/utils/contracts.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- contracts/hardhat.config.ts
- contracts/package.json
- contracts/scripts/changeGovernor.ts
- contracts/scripts/keeperBot.ts
- contracts/config/policies.v2.mainnet-neo.json
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: SonarCloud
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
| const chainlinkRng = await ethers.getContractOrNull<ChainlinkRNG>("ChainlinkRNG"); | ||
| const randomizerRng = await ethers.getContractOrNull<RandomizerRNG>("RandomizerRNG"); | ||
| const blockHashRNG = await ethers.getContractOrNull<BlockHashRNG>("BlockHashRNG"); |
There was a problem hiding this comment.
💡 Codebase verification
Error handling required: RNG contract retrieval missing null-checks
The grep search confirms that while most contracts use retrieval methods that inherently throw errors upon failure (like getContract), the RNG contracts in contracts/scripts/utils/contracts.ts are retrieved via getContractOrNull. Since this returns null on failure, there’s no subsequent check or error handling—which could lead to silent failures during deployment verification. Consider adding explicit null checks or error handling logic for these RNG contracts.
🔗 Analysis chain
Consider adding error handling for contract retrieval failures.
The code uses getContractOrNull for RNG contracts but doesn't handle potential deployment verification failures for other contracts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if any contracts are consistently failing to be retrieved
rg -A 2 "getContract\(" . | grep -v "getContractOrNull"
Length of output: 20759
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
contracts/scripts/utils/contracts.ts (1)
59-61:⚠️ Potential issueAdd null checks for RNG contract retrieval.
The RNG contracts are retrieved using
getContractOrNullbut lack null checks, which could lead to runtime errors.- const chainlinkRng = await ethers.getContractOrNull<ChainlinkRNG>("ChainlinkRNG"); - const randomizerRng = await ethers.getContractOrNull<RandomizerRNG>("RandomizerRNG"); - const blockHashRNG = await ethers.getContractOrNull<BlockHashRNG>("BlockHashRNG"); + const chainlinkRng = await ethers.getContractOrNull<ChainlinkRNG>("ChainlinkRNG").catch(() => null); + const randomizerRng = await ethers.getContractOrNull<RandomizerRNG>("RandomizerRNG").catch(() => null); + const blockHashRNG = await ethers.getContractOrNull<BlockHashRNG>("BlockHashRNG").catch(() => null); + + if (!chainlinkRng && !randomizerRng && !blockHashRNG) { + console.warn("No RNG contracts found"); + }
🧹 Nitpick comments (4)
contracts/scripts/utils/execution.ts (1)
19-34: Consider adding error handling and transaction receipt logging.The execute function handles the core logic well, but could benefit from additional error handling and logging.
export const execute = async (tx: ContractTransaction) => { const hre = require("hardhat"); const { ethers } = hre; const contract = await ethers.getContractAt(governableAbi, tx.to); const governor = await contract.governor(); const isContract = (await ethers.provider.getCode(governor)).length > 2; if (isContract) { // Don't execute, just log the tx. It must be submitted for execution separately. console.log("tx = %O", tx); } else { // Execute the tx const signer = (await ethers.getSigners())[0]; - await signer.sendTransaction(tx); + try { + const response = await signer.sendTransaction(tx); + const receipt = await response.wait(); + console.log(`Transaction executed successfully. Hash: ${receipt.hash}`); + } catch (error) { + console.error("Failed to execute transaction:", error); + throw error; + } } };contracts/scripts/populatePolicyRegistry.ts (1)
111-114: Consider adding batch processing for policy updates.The script processes policies sequentially. Consider implementing batch processing using the TransactionBatcher contract to optimize gas costs.
for await (const policy of policiesV2) { console.log("Populating policy for %s Court (%d): %s", policy.name, policy.court, policy.uri); - await policyRegistry.setPolicy.populateTransaction(policy.court, policy.name, policy.uri).then(execute); + const tx = await policyRegistry.setPolicy.populateTransaction(policy.court, policy.name, policy.uri); + const batcher = await ethers.getContract("TransactionBatcher"); + await batcher.populateTransaction.batchTransactions([tx]).then(execute); }contracts/scripts/populateCourts.ts (2)
263-263: Extract magic number to a named constant.The hardcoded value
5intoBeHex(5)should be extracted to a named constant for better maintainability.+const DEFAULT_SORTITION_SUM_TREE_K = 5; + // ... later in the code ... - ethers.toBeHex(5), // Not accessible on-chain, but has always been set to the same value so far. + ethers.toBeHex(DEFAULT_SORTITION_SUM_TREE_K), // Not accessible on-chain, but has always been set to the same value so far.
223-238: Improve error handling for changeCourtParameters.The error handling could be more specific and provide better error messages.
try { await core.changeCourtParameters .populateTransaction( court.id, court.hiddenVotes, court.minStake, court.alpha, court.feeForJuror, court.jurorsForCourtJump, [court.timesPerPeriod[0], court.timesPerPeriod[1], court.timesPerPeriod[2], court.timesPerPeriod[3]] ) .then(execute); } catch (error) { - console.error("Error changing court parameters: %s", error); + if (error.code === 'CALL_EXCEPTION') { + console.error(`Failed to change parameters for court ${court.id}: Contract call reverted`); + } else if (error.code === 'NETWORK_ERROR') { + console.error(`Network error while changing parameters for court ${court.id}`); + } else { + console.error(`Unexpected error changing parameters for court ${court.id}: ${error.message}`); + } + throw error; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
contracts/config/courts.v2.devnet.json(1 hunks)contracts/config/courts.v2.mainnet-neo.json(1 hunks)contracts/config/policies.v2.devnet.json(1 hunks)contracts/config/policies.v2.mainnet-neo.json(2 hunks)contracts/scripts/changeGovernor.ts(1 hunks)contracts/scripts/populateCourts.ts(5 hunks)contracts/scripts/populatePolicyRegistry.ts(2 hunks)contracts/scripts/utils/contracts.ts(1 hunks)contracts/scripts/utils/execution.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/scripts/changeGovernor.ts
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: SonarCloud
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
- GitHub Check: Mend Security Check
🔇 Additional comments (11)
contracts/scripts/utils/execution.ts (1)
3-17: LGTM! Well-defined ABI for governor function.The ABI definition is correct and properly typed for interacting with governable contracts.
contracts/scripts/utils/contracts.ts (1)
20-26: LGTM! Well-structured enum and type definition.The Cores enum and Core type are well-defined using const assertions for type safety.
contracts/config/courts.v2.devnet.json (1)
81-97: New Automated Curation Court Entry Added.
A new court object for "Automated Curation" has been introduced with the following configuration:
- id: 6
- parent: 2 (i.e. a child of “Curation”)
- minStake, alpha, feeForJuror, jurorsForCourtJump, timesPerPeriod: set to values consistent with similar courts in this configuration.
Ensure that the values align with governance requirements for devnet and that any downstream code using this value is updated accordingly.
contracts/config/courts.v2.mainnet-neo.json (1)
481-497: New Automated Curation Court Entry Added for Mainnet.
A new "Automated Curation" entry has been added with the following key parameters:
- id: 31
- parent: 10 (expected to be the "Curation" court in the mainnet config)
- minStake: "2600000000000000000000"
- alpha: "290"
- feeForJuror: "170000000000000"
- jurorsForCourtJump: "3"
- timesPerPeriod: [140400, 291600, 291600, 194400]
These values seem to be tailored for mainnet requirements. Please verify that these numbers meet your intended governance parameters for production use.
contracts/config/policies.v2.devnet.json (1)
40-48: New Automated Curation Policy Entry Added.
A new policy entry for "Automated Curation" has been inserted using the following details:
- name: "Automated Curation"
- purpose: Clearly explains that this court is designed for handling rapid micro-tasks (e.g. content moderation and gaming disputes)
- rules: (currently empty, which is acceptable if this is pending further detail)
- requiredSkills: Outlines the skills required for AI jurors, including data processing efficiency, compliance with Kleros rules, and temporal awareness
- court: 6
- uri: "/ipfs/QmNm6w4itnvMoWQXcz3CAQmjSF4nP5w6uTwGAQ1Z5YoUKJ"
The entry appears to be consistent with the devnet court settings. Consider detailing any specific rules later if needed.
contracts/config/policies.v2.mainnet-neo.json (6)
184-190: Court Renaming: Humanity Update.
The entry formerly known as "Humanity Court" has been renamed to "Humanity" and its URI has been updated to "/ipfs/QmfH68LJWRQ7UEJqFGDKDpR6hmxmmJrbz2EHJMgqtCgFo6". This naming simplification improves clarity.
191-198: Court Renaming: Development Update.
The "Development Court" entry has been renamed to "Development" and its URI updated to "/ipfs/QmdiQGftN4Mxtocvf1ENxeEvVzU62AGR3knzfhMDb85iTh". This change streamlines the naming.
200-206: Court Renaming: Solidity Update.
The "Solidity Court" has been simplified to "Solidity" and now uses the new URI "/ipfs/QmbKfy5vF5jZ5GFqFKgUxnYsbAjJdtsDfp2UJLwxzDokmb". These updates enhance consistency with other courts.
207-214: Court Renaming: Javascript Update.
The entry formerly referred to as "Javascript Court" has been renamed to "Javascript" with an updated URI "/ipfs/Qmaf4NzAvyVa4biu7MwaGTTwCe46XVSdBa3t3Uu2soFToz". This makes the naming more concise and consistent across the configuration.
232-238: Court Renaming: Oracle Update.
The "Oracle Court" entry has been updated to "Oracle" and its URI has been changed to "/ipfs/QmZqV3TJNZtYTZ74fcVTNT5uEwrsv2aDkGGVB5XUS32VD9". Please verify that any references in the system to the Oracle configuration are also updated.
239-246: New Automated Curation Court Policy Entry Added.
A new policy entry for the "Automated Curation" court is added with the following details:
- name: "Automated Curation"
- purpose: Describes its role in handling rapid, micro-task disputes
- rules: (currently empty)
- requiredSkills: Specifies capabilities for AI agents (data processing efficiency; compliance with Kleros rules; temporal awareness)
- court: 31
- uri: "/ipfs/QmV4TYUwUFgpMMKfWgDQjBtC5Dn5cRGraDnVatccSb6LMx"
This entry aligns the mainnet policies with the updated courts configuration.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
contracts/scripts/getDisputeTemplate.ts (2)
5-7: Add input validation for templateId parameter.The templateId is accepted as a string but used with BigInt. Consider adding validation to ensure it's a valid numeric string.
task("get-dispute-template", "Gets a dispute template by ID") .addPositionalParam("templateId", "The ID of the template to query") .setAction(async function ({ templateId }: { templateId: string }, hre: HardhatRuntimeEnvironment) { + if (!/^\d+$/.test(templateId)) { + throw new Error("Template ID must be a valid numeric string"); + }
22-31: Improve output formatting and add type safety.The output could be more readable with JSON formatting, and type safety should be added for event args.
// Get the most recent event const event = events[events.length - 1]; + const { _templateId, _templateTag, _templateData, _templateDataMappings } = event.args; - console.log("Template Details:"); - console.log("----------------"); - console.log(`Template ID: ${event.args._templateId}`); - console.log(`Template Tag: ${event.args._templateTag}`); - console.log(`Template Data: ${event.args._templateData}`); - console.log(`Template Data Mappings: ${event.args._templateDataMappings}`); + console.log(JSON.stringify({ + templateDetails: { + templateId: _templateId.toString(), + templateTag: _templateTag, + templateData: _templateData, + templateDataMappings: _templateDataMappings + } + }, null, 2));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/hardhat.config.ts(1 hunks)contracts/scripts/getDisputeTemplate.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/hardhat.config.ts
⏰ 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-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- 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: SonarCloud
- GitHub Check: Analyze (javascript)
| // Query the events | ||
| const filter = disputeTemplateRegistry.filters.DisputeTemplate(BigInt(templateId)); | ||
| const events = await disputeTemplateRegistry.queryFilter(filter); | ||
|
|
||
| if (events.length === 0) { | ||
| console.log(`No template found with ID ${templateId}`); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add comprehensive error handling for event querying.
The event query could fail due to network issues or contract reverts. Consider adding try-catch block and handling specific error cases.
// Query the events
- const filter = disputeTemplateRegistry.filters.DisputeTemplate(BigInt(templateId));
- const events = await disputeTemplateRegistry.queryFilter(filter);
+ let events;
+ try {
+ const filter = disputeTemplateRegistry.filters.DisputeTemplate(BigInt(templateId));
+ events = await disputeTemplateRegistry.queryFilter(filter);
+ } catch (error) {
+ console.error("Failed to query events:", error.message);
+ return;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Query the events | |
| const filter = disputeTemplateRegistry.filters.DisputeTemplate(BigInt(templateId)); | |
| const events = await disputeTemplateRegistry.queryFilter(filter); | |
| if (events.length === 0) { | |
| console.log(`No template found with ID ${templateId}`); | |
| return; | |
| } | |
| // Query the events | |
| let events; | |
| try { | |
| const filter = disputeTemplateRegistry.filters.DisputeTemplate(BigInt(templateId)); | |
| events = await disputeTemplateRegistry.queryFilter(filter); | |
| } catch (error) { | |
| console.error("Failed to query events:", error.message); | |
| return; | |
| } | |
| if (events.length === 0) { | |
| console.log(`No template found with ID ${templateId}`); | |
| return; | |
| } |
| const { ethers } = hre; | ||
|
|
||
| // Get the contract instance | ||
| const disputeTemplateRegistry = await ethers.getContract<IDisputeTemplateRegistry>("DisputeTemplateRegistry"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for contract retrieval.
The contract retrieval could fail if the contract is not deployed. Consider adding try-catch block.
const { ethers } = hre;
// Get the contract instance
+ let disputeTemplateRegistry;
+ try {
const disputeTemplateRegistry = await ethers.getContract<IDisputeTemplateRegistry>("DisputeTemplateRegistry");
+ } catch (error) {
+ console.error("Failed to get DisputeTemplateRegistry contract:", error.message);
+ return;
+ }Committable suggestion skipped: line range outside the PR's diff.
jaybuidl
force-pushed
the
chore/contract-scripts-improvements
branch
from
02594c5 to
8940812
Compare
jaybuidl
force-pushed
the
chore/contract-scripts-improvements
branch
from
8940812 to
708f000
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
contracts/scripts/changeGovernor.ts (1)
41-56: Enhance error handling in updateGovernor function.The error handling could be improved by:
- Adding transaction receipt validation
- Including specific error types for common failure scenarios
- Adding retry logic for transient failures
const updateGovernor = async (contractName: string, contractInstance: any) => { print.info(`Changing governor for ${contractName}`); const spinner = print.spin(`Executing transaction for ${contractName}...`); try { const tx = await contractInstance.changeGovernor(newGovernor); - await tx.wait(); + const receipt = await tx.wait(); + if (receipt.status === 0) { + throw new Error('Transaction failed'); + } spinner.succeed(`Governor changed for ${contractName}, tx hash: ${tx.hash}`); } catch (error) { + if (error.code === 'NETWORK_ERROR') { + spinner.fail(`Network error while changing governor for ${contractName}. Retrying...`); + // Add retry logic here + return; + } if (error instanceof Error) { spinner.fail(`Failed to change governor for ${contractName}: ${error.message}`); } else { spinner.fail(`Failed to change governor for ${contractName}: ${String(error)}`); } + throw error; // Re-throw to handle at task level } };contracts/scripts/utils/contracts.ts (1)
31-33: Type safety improvement needed for contract variables.The type declarations for contract variables could be more specific to prevent potential type-related issues.
- let core: KlerosCore | KlerosCoreNeo | KlerosCoreUniversity; - let sortition: SortitionModule | SortitionModuleNeo | SortitionModuleUniversity; - let disputeKitClassic: DisputeKitClassic; + let core: KlerosCore | KlerosCoreNeo | KlerosCoreUniversity | undefined; + let sortition: SortitionModule | SortitionModuleNeo | SortitionModuleUniversity | undefined; + let disputeKitClassic: DisputeKitClassic | undefined;contracts/scripts/populateCourts.ts (2)
224-234: Consider batching multiple court parameter changes.The current implementation processes court parameter changes sequentially. Consider using the TransactionBatcher for multiple court updates to optimize gas usage.
+ const batchedTxs = []; try { - await core.changeCourtParameters - .populateTransaction( + const tx = await core.changeCourtParameters.populateTransaction( court.id, court.hiddenVotes, court.minStake, court.alpha, court.feeForJuror, court.jurorsForCourtJump, [court.timesPerPeriod[0], court.timesPerPeriod[1], court.timesPerPeriod[2], court.timesPerPeriod[3]] - ) - .then(execute); + ); + batchedTxs.push(tx); + if (batchedTxs.length === 5) { // Batch size of 5 + await batcher.executeBatch(batchedTxs).then(execute); + batchedTxs.length = 0; + }
263-263: Document the magic number in createCourt.The hardcoded value
5should be documented with its purpose and any implications of changing it.- ethers.toBeHex(5), // Not accessible on-chain, but has always been set to the same value so far. + ethers.toBeHex(5), // COURT_TREE_HEIGHT: Fixed value for the height of the court's sortition sum tree. + // WARNING: Changing this value requires careful consideration as it affects the court's capacity and gas costs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
contracts/config/courts.v2.devnet.json(1 hunks)contracts/config/courts.v2.mainnet-neo.json(1 hunks)contracts/config/policies.v2.devnet.json(1 hunks)contracts/config/policies.v2.mainnet-neo.json(2 hunks)contracts/hardhat.config.ts(1 hunks)contracts/package.json(1 hunks)contracts/scripts/changeGovernor.ts(1 hunks)contracts/scripts/getDisputeTemplate.ts(1 hunks)contracts/scripts/keeperBot.ts(3 hunks)contracts/scripts/populateCourts.ts(5 hunks)contracts/scripts/populatePolicyRegistry.ts(2 hunks)contracts/scripts/utils/contracts.ts(1 hunks)contracts/scripts/utils/execution.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- contracts/hardhat.config.ts
- contracts/scripts/getDisputeTemplate.ts
- contracts/scripts/utils/execution.ts
- contracts/scripts/populatePolicyRegistry.ts
- contracts/package.json
- contracts/scripts/keeperBot.ts
- contracts/config/courts.v2.devnet.json
- contracts/config/policies.v2.devnet.json
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
🔇 Additional comments (10)
contracts/scripts/changeGovernor.ts (2)
11-11: Add address validation using existing utilities.The
newGovernoraddress should be validated before proceeding with the transaction.
58-59: Consider implementing the SortitionModule upgrade.The TODO comment indicates a pending upgrade for SortitionModule. This should be tracked and implemented to ensure complete governor migration.
Would you like me to help create an issue to track the SortitionModule upgrade implementation?
contracts/scripts/utils/contracts.ts (1)
59-61: Error handling required: RNG contract retrieval missing null-checks.The grep search confirms that while most contracts use retrieval methods that inherently throw errors upon failure, the RNG contracts are retrieved via
getContractOrNull. Since this returnsnullon failure, there's no subsequent check or error handling—which could lead to silent failures during deployment verification.contracts/config/courts.v2.mainnet-neo.json (1)
481-497: New Court "Automated Curation" Added.
The new court is correctly added with the expected parameters:
- id: 31
- parent: 10 (referring to the "Curation" court)
- Parameters:
minStake,alpha,feeForJuror,jurorsForCourtJump, andtimesPerPeriodare set per the PR specifications.Please verify that downstream scripts and references (e.g., in policies files or governance modules) properly integrate with this addition.
contracts/config/policies.v2.mainnet-neo.json (6)
183-190: Simplify Court Name: "Humanity".
The court formerly named "Humanity Court" is now updated to "Humanity". The new naming aligns with the design decision to simplify court names. Ensure that any references to this court (by name or URI) in contracts or front-end code are correspondingly updated.
191-198: Simplify Court Name: "Development".
The update changes the name from "Development Court" to "Development" and updates the URI. The modifications are clear and consistent with the intent to simplify naming conventions.
199-206: Simplify Court Name: "Solidity".
The court’s name is now updated to "Solidity", and the URI has been changed accordingly. This adjustment is consistent with the naming updates outlined in the PR objectives.
207-214: Simplify Court Name: "Javascript".
The update removes the "Court" suffix from "Javascript Court", resulting in "Javascript", along with a corresponding URI update. This change should improve clarity and consistency across the configuration.
232-238: Simplify Court Name: "Oracle".
The court formerly designated as "Oracle Court" is now renamed to "Oracle", with an updated URI. The changes follow the PR objective for name simplification. Verify that any integrations (e.g., in contract calls or front-end references) are adjusted to match the new identifier.
239-246: Addition of "Automated Curation" Court in Policies.
A new policy entry for the "Automated Curation" court has been introduced with detailed information in itspurposeandrequiredSkills. The configuration sets:
- name: "Automated Curation"
- court id: 31
- uri:
/ipfs/QmV4TYUwUFgpMMKfWgDQjBtC5Dn5cRGraDnVatccSb6LMxThis addition is consistent with the changes in
courts.v2.mainnet-neo.jsonand aligns with the objectives to manage micro-tasks and rapid decision cases.
|
Code Climate has analyzed commit 245f07d and detected 5 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
contracts/scripts/changeGovernor.ts (1)
7-10:⚠️ Potential issueAdd address validation for newGovernor parameter.
The task should validate that the provided
newGovernoraddress is a valid Ethereum address before proceeding with any transactions.Add validation using the
isAddressfunction fromviem:+import { isAddress } from "viem"; task("change-governor", "Changes the governor for all the contracts") .addPositionalParam("newGovernor", "The address of the new governor") .addOptionalParam("coreType", "The type of core to use between base, neo, university (default: base)", Cores.BASE) .setAction(async (taskArgs, hre) => { const newGovernor = taskArgs.newGovernor; + if (!isAddress(newGovernor)) { + throw new Error("Invalid governor address provided"); + }
🧹 Nitpick comments (4)
contracts/scripts/changeGovernor.ts (4)
14-18: Enhance confirmation message with more details.The confirmation message should be more explicit about the impact of changing the governor across multiple contracts.
- message: "Are you sure you want to proceed?", + message: "This will change the governor address for multiple contracts. Are you sure you want to proceed?",
24-28: Improve type safety for core type validation.Consider using TypeScript's type system more effectively to prevent runtime errors.
- const coreType = Cores[taskArgs.coreType.toUpperCase() as keyof typeof Cores]; - if (coreType === undefined) { - console.error("Invalid core type, must be one of base, neo, university"); - return; - } + type CoreType = keyof typeof Cores; + const upperCoreType = taskArgs.coreType.toUpperCase(); + if (!Object.keys(Cores).includes(upperCoreType)) { + throw new Error(`Invalid core type '${taskArgs.coreType}'. Must be one of: ${Object.keys(Cores).map(k => k.toLowerCase()).join(', ')}`); + } + const coreType = Cores[upperCoreType as CoreType];
31-41: Add type definitions for contract instances.Consider adding explicit type definitions for the contract instances to improve type safety and code maintainability.
+ interface Contracts { + core: Contract; + disputeKitClassic: Contract; + disputeResolver: Contract; + disputeTemplateRegistry: Contract; + policyRegistry: Contract; + chainlinkRng?: Contract; + randomizerRng?: Contract; + snapshotProxy: Contract; + } const { core, disputeKitClassic, // ... - } = await getContracts(hre, coreType); + }: Contracts = await getContracts(hre, coreType);
62-69: Optimize contract updates execution.Consider these improvements to the execution flow:
- Add parallel processing for better performance
- Implement rollback mechanism for failed transactions
- Add transaction summary at the end
- await updateGovernor("KlerosCore", core); - await updateGovernor("DisputeKitClassic", disputeKitClassic); - // ... + const contracts = [ + { name: "KlerosCore", instance: core }, + { name: "DisputeKitClassic", instance: disputeKitClassic }, + { name: "DisputeResolver", instance: disputeResolver }, + { name: "DisputeTemplateRegistry", instance: disputeTemplateRegistry }, + { name: "PolicyRegistry", instance: policyRegistry }, + { name: "KlerosCoreSnapshotProxy", instance: snapshotProxy }, + ...(chainlinkRng ? [{ name: "ChainlinkRNG", instance: chainlinkRng }] : []), + ...(randomizerRng ? [{ name: "RandomizerRNG", instance: randomizerRng }] : []), + ]; + + const results = await Promise.allSettled( + contracts.map(({ name, instance }) => updateGovernor(name, instance)) + ); + + const failed = results.filter((r): r is PromiseRejectedResult => r.status === 'rejected'); + if (failed.length > 0) { + throw new Error(`Failed to update governor for ${failed.length} contracts`); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/scripts/changeGovernor.ts(1 hunks)contracts/scripts/utils/contracts.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/scripts/utils/contracts.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: contracts-testing
🔇 Additional comments (1)
contracts/scripts/changeGovernor.ts (1)
59-60: Address the TODO comment about SortitionModule.The TODO comment indicates missing functionality that needs to be implemented.
Would you like me to help implement the changeGovernor functionality for the SortitionModule or create an issue to track this task?
| const updateGovernor = async (contractName: string, contractInstance: any) => { | ||
| print.info(`Changing governor for ${contractName}`); | ||
|
|
||
| const spinner = print.spin(`Executing transaction for ${contractName}...`); | ||
| try { | ||
| const tx = await contractInstance.changeGovernor(newGovernor); | ||
| await tx.wait(); | ||
| spinner.succeed(`Governor changed for ${contractName}, tx hash: ${tx.hash}`); | ||
| } catch (error) { | ||
| if (error instanceof Error) { | ||
| spinner.fail(`Failed to change governor for ${contractName}: ${error.message}`); | ||
| } else { | ||
| spinner.fail(`Failed to change governor for ${contractName}: ${String(error)}`); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and transaction validation.
The current implementation could be improved with better error handling and transaction receipt validation.
const updateGovernor = async (contractName: string, contractInstance: any) => {
+ if (!contractInstance?.changeGovernor) {
+ throw new Error(`Contract ${contractName} does not implement changeGovernor`);
+ }
print.info(`Changing governor for ${contractName}`);
const spinner = print.spin(`Executing transaction for ${contractName}...`);
try {
const tx = await contractInstance.changeGovernor(newGovernor);
- await tx.wait();
+ const receipt = await tx.wait();
+ if (receipt.status === 0) {
+ throw new Error('Transaction reverted');
+ }
spinner.succeed(`Governor changed for ${contractName}, tx hash: ${tx.hash}`);
} catch (error) {
+ const errorMessage = error instanceof Error ? error.message : String(error);
+ console.error(`Transaction details for ${contractName}:`, error);
- if (error instanceof Error) {
- spinner.fail(`Failed to change governor for ${contractName}: ${error.message}`);
- } else {
- spinner.fail(`Failed to change governor for ${contractName}: ${String(error)}`);
- }
+ spinner.fail(`Failed to change governor for ${contractName}: ${errorMessage}`);
+ throw error; // Re-throw to stop execution
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const updateGovernor = async (contractName: string, contractInstance: any) => { | |
| print.info(`Changing governor for ${contractName}`); | |
| const spinner = print.spin(`Executing transaction for ${contractName}...`); | |
| try { | |
| const tx = await contractInstance.changeGovernor(newGovernor); | |
| await tx.wait(); | |
| spinner.succeed(`Governor changed for ${contractName}, tx hash: ${tx.hash}`); | |
| } catch (error) { | |
| if (error instanceof Error) { | |
| spinner.fail(`Failed to change governor for ${contractName}: ${error.message}`); | |
| } else { | |
| spinner.fail(`Failed to change governor for ${contractName}: ${String(error)}`); | |
| } | |
| } | |
| }; | |
| const updateGovernor = async (contractName: string, contractInstance: any) => { | |
| if (!contractInstance?.changeGovernor) { | |
| throw new Error(`Contract ${contractName} does not implement changeGovernor`); | |
| } | |
| print.info(`Changing governor for ${contractName}`); | |
| const spinner = print.spin(`Executing transaction for ${contractName}...`); | |
| try { | |
| const tx = await contractInstance.changeGovernor(newGovernor); | |
| const receipt = await tx.wait(); | |
| if (receipt.status === 0) { | |
| throw new Error('Transaction reverted'); | |
| } | |
| spinner.succeed(`Governor changed for ${contractName}, tx hash: ${tx.hash}`); | |
| } catch (error) { | |
| const errorMessage = error instanceof Error ? error.message : String(error); | |
| console.error(`Transaction details for ${contractName}:`, error); | |
| spinner.fail(`Failed to change governor for ${contractName}: ${errorMessage}`); | |
| throw error; // Re-throw to stop execution | |
| } | |
| }; |
PR-Codex overview
This PR introduces new functionalities for managing courts and policies within the Kleros system, including adding a new court type, changing the governor of contracts, and enhancing transaction execution methods.
Detailed summary
changeGovernorscript to change contract governors.getDisputeTemplatetask for querying dispute templates.Automated Curationcourt in configuration files.populateCourtsandpopulatePolicyRegistrytasks to use transaction methods.Summary by CodeRabbit
Release Notes
Configuration Updates
Script Enhancements
Package Management
Code Improvements
vartoconst.