Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Neo config and keeper bot #1746

Merged
merged 6 commits into from
Nov 19, 2024
Merged

Neo config and keeper bot #1746

merged 6 commits into from
Nov 19, 2024

Conversation

jaybuidl
Copy link
Member

@jaybuidl jaybuidl commented Nov 19, 2024

PR-Codex overview

This PR primarily focuses on enhancing the contract deployment scripts and configurations for the Kleros arbitration system, introducing new court types, and refining the dispute resolution process.

Detailed summary

  • Refactored disputeKitClassic.changeCore() execution logic for efficiency.
  • Introduced CORE_TYPE environment variable to switch between KlerosCore and KlerosCoreNeo.
  • Added a Cores enum to manage core types.
  • Expanded court configurations, adding multiple new court types with detailed descriptions and requirements.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

Summary by CodeRabbit

  • New Features

    • Introduced multiple new courts for various dispute types, including Blockchain, Non-Technical, Token Listing, and various translation courts.
    • Expanded dispute resolution capabilities with specialized courts for marketing, data analysis, and statistical modeling, among others.
    • Enhanced the keeper bot to support new core types and improve error handling.
  • Bug Fixes

    • Enhanced error handling in the keeper bot for better resilience against missing contracts.
  • Documentation

    • Updated comments and documentation for clarity in the deployment process and core type management.

@jaybuidl jaybuidl marked this pull request as ready for review November 19, 2024 15:23
Copy link
Contributor

coderabbitai bot commented Nov 19, 2024

Walkthrough

The changes in this pull request involve significant updates to the court configurations within the Kleros ecosystem. New court entries have been added to both the courts.v2.mainnet-neo.json and policies.v2.mainnet-neo.json files, expanding the types of disputes that can be arbitrated. Additionally, modifications to the 00-home-chain-arbitration.ts and keeperBot.ts files enhance the efficiency and flexibility of the arbitration process, including improvements in contract interactions and error handling.

Changes

File Path Change Summary
contracts/config/courts.v2.mainnet-neo.json Added multiple new court entries with varying configurations including minimum stakes, fees, and juror settings. Updated existing court entry for "General Court".
contracts/config/policies.v2.mainnet-neo.json Introduced new court entries with descriptions and requirements for various dispute types, including technical and translation-related courts.
contracts/deploy/00-home-chain-arbitration.ts Streamlined interaction with the DisputeKitClassic contract by reducing redundant fetches and updated comments for clarity.
contracts/scripts/keeperBot.ts Added support for a new core type KlerosCoreNeo and improved error handling and type safety in the getContracts function. Introduced a new enum for core types.

Possibly related PRs

Suggested labels

Type: Maintenance :construction:

Suggested reviewers

  • alcercu

🐰 In the court of hops and dreams,
New entries bloom like sunlit beams.
With stakes and fees, we now extend,
A world of justice we shall send.
So gather round, both near and far,
For every dispute, we raise the bar! 🌟


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c028143 and a218f9e.

📒 Files selected for processing (1)
  • contracts/config/courts.v2.mainnet-neo.json (1 hunks)
🧰 Additional context used
📓 Learnings (1)
contracts/config/courts.v2.mainnet-neo.json (5)
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:3-5
Timestamp: 2024-11-19T17:18:39.007Z
Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (id: 1) intentionally references itself as its parent (`"parent": 1`). This self-reference is acceptable and should not be flagged as an issue in future reviews.
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH.
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In the court hierarchy, child courts' `minStake` must be greater than or equal to their parent court's `minStake`.
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:3-17
Timestamp: 2024-11-19T16:09:52.022Z
Learning: In the Kleros court configuration, it's acceptable for the General Court (id: 1) to have itself as its parent (parent: 1) when there is no court at index 0.
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:3-17
Timestamp: 2024-11-19T16:09:41.467Z
Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (ID: 1) can have its `parent` ID set to itself (`"parent": 1`), as there is no parent court with ID 0 currently.
🔇 Additional comments (4)
contracts/config/courts.v2.mainnet-neo.json (4)

71-71: Verify stake requirements in Blockchain court hierarchy

The stake requirements in the Blockchain court branch need verification:

  • Blockchain Court: 7,000 PNK
  • Non-Technical Court: 7,000 PNK (equal to parent)
  • Technical Court: 33,000 PNK (higher than parent)

This follows the rule that child courts must have minStake >= parent court's minStake.

Also applies to: 39-39, 23-23


403-432: LGTM: Consistent configuration for development courts

The Solidity and Javascript courts maintain identical configurations:

  • minStake: 9,500 PNK
  • feeForJuror: 0.011 ETH
  • alpha: 5000
  • jurorsForCourtJump: 63
  • timesPerPeriod: [3.25, 5.06, 5.06, 3.38] days

This consistency is good for maintaining fairness across different development specialties.


1-464: Verify JSON structure and court IDs

The configuration appears well-structured. Let's verify:

  1. JSON validity
  2. Unique court IDs
  3. Valid parent references
✅ Verification successful

Configuration validation successful

The verification checks confirm:

  • Valid JSON structure with no syntax errors
  • No duplicate court IDs (1-29)
  • All parent references point to existing court IDs
  • Proper hierarchical relationships maintained
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Validate JSON structure and court relationships
echo "Checking JSON validity..."
jq '.' contracts/config/courts.v2.mainnet-neo.json > /dev/null

echo "Checking for duplicate court IDs..."
jq -r '.[].id' contracts/config/courts.v2.mainnet-neo.json | sort -n | uniq -d

echo "Checking for invalid parent references..."
jq -r '.[].id' contracts/config/courts.v2.mainnet-neo.json > court_ids.txt
jq -r '.[] | select(.parent != .id) | .parent' contracts/config/courts.v2.mainnet-neo.json | while read parent; do
  if ! grep -q "^$parent$" court_ids.txt; then
    echo "Invalid parent ID: $parent"
  fi
done
rm court_ids.txt

Length of output: 1791


459-464: Consider aligning time periods with other courts

The Blockchain No Técnica court has significantly shorter time periods compared to other courts:

  • Most courts: ~3.25 days, ~6.75 days, ~6.75 days, ~4.5 days
  • Blockchain No Técnica: ~1.25 days, ~2.5 days, ~2.5 days, ~2.5 days

These shorter periods might not provide sufficient time for thorough case review.


🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

netlify bot commented Nov 19, 2024

Deploy Preview for kleros-v2-neo ready!

Name Link
🔨 Latest commit a218f9e
🔍 Latest deploy log https://app.netlify.com/sites/kleros-v2-neo/deploys/673cc8b2091017000871a017
😎 Deploy Preview https://deploy-preview-1746--kleros-v2-neo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Nov 19, 2024

Deploy Preview for kleros-v2-university ready!

Name Link
🔨 Latest commit a218f9e
🔍 Latest deploy log https://app.netlify.com/sites/kleros-v2-university/deploys/673cc8b25fc72e0008765341
😎 Deploy Preview https://deploy-preview-1746--kleros-v2-university.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Nov 19, 2024

Deploy Preview for kleros-v2-testnet ready!

Name Link
🔨 Latest commit a218f9e
🔍 Latest deploy log https://app.netlify.com/sites/kleros-v2-testnet/deploys/673cc8b27d1d6a00081c48aa
😎 Deploy Preview https://deploy-preview-1746--kleros-v2-testnet.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jaybuidl jaybuidl added the Package: Contracts Court smart contracts label Nov 19, 2024
Copy link

netlify bot commented Nov 19, 2024

Deploy Preview for kleros-v2-testnet-devtools ready!

Name Link
🔨 Latest commit a218f9e
🔍 Latest deploy log https://app.netlify.com/sites/kleros-v2-testnet-devtools/deploys/673cc8b28595380008574987
😎 Deploy Preview https://deploy-preview-1746--kleros-v2-testnet-devtools.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (5)
contracts/deploy/00-home-chain-arbitration.ts (1)

Line range hint 104-111: Improve error handling for currency rate changes.

The current implementation catches and logs errors from changeCurrencyRate but continues execution. This could lead to a deployment being considered successful even when currency rates are not properly set.

Consider:

  1. Failing the deployment if currency rates cannot be set
  2. Adding validation of the rates after setting them
  3. Adding retry logic for transient failures

Example improvement:

  try {
    await changeCurrencyRate(core, await pnk.getAddress(), true, 12225583, 12);
    await changeCurrencyRate(core, await dai.getAddress(), true, 60327783, 11);
    await changeCurrencyRate(core, await weth.getAddress(), true, 1, 1);
+   // Verify rates were set correctly
+   const pnkRate = await core.rates(await pnk.getAddress());
+   const daiRate = await core.rates(await dai.getAddress());
+   const wethRate = await core.rates(await weth.getAddress());
+   if (!pnkRate || !daiRate || !wethRate) {
+     throw new Error("Failed to verify currency rates");
+   }
  } catch (e) {
-   console.error("failed to change currency rates:", e);
+   console.error("Failed to change currency rates:", e);
+   throw e; // Fail deployment if rates cannot be set
  }
contracts/config/policies.v2.mainnet-neo.json (3)

81-82: Add missing descriptions and summaries for Data Analysis and Statistical Modeling courts

The description and summary fields are empty for these specialized courts. This could impact documentation generation and frontend display.

Consider adding appropriate descriptions to maintain consistency with other courts and provide clear guidance for jurors.

Also applies to: 89-90


Line range hint 1-229: Consider restructuring court IDs to reflect hierarchy

The current sequential numbering (1-29) doesn't reflect the hierarchical relationships between courts. For example, translation courts (14-22) could be grouped under a common parent, and technical courts (25-27) could share a common prefix.

Consider implementing a hierarchical ID system (e.g., 1000 for General Court, 1100 for Blockchain, 1110 for Technical) to better represent court relationships and make it easier to add new courts in the future.


176-182: Ensure consistent translation of court terminology

While it's appropriate to have Spanish descriptions for Spanish-language courts, ensure that technical terms like "court", "policy", and "skills" are consistently translated throughout the Spanish court definitions.

Consider creating a glossary of standard translations for technical terms to maintain consistency across all language-specific courts.

Also applies to: 216-222

contracts/scripts/keeperBot.ts (1)

49-63: Enhance type safety by using string enums for CORE_TYPE

Currently, the CORE_TYPE environment variable is a string, and its value is being used to index the Cores enum after converting to uppercase. This can lead to runtime errors if CORE_TYPE has unexpected values.

Consider defining Cores as a string enum with explicit string values. This change improves type safety and allows for direct comparison without the need to manipulate strings, reducing the risk of runtime errors.

Apply this diff to implement string enums:

-enum Cores {
-  BASE,
-  NEO,
-}
+enum Cores {
+  BASE = "base",
+  NEO = "neo",
+}

...

-const coreType = Cores[CORE_TYPE.toUpperCase() as keyof typeof Cores];
+const coreType = CORE_TYPE.toLowerCase() as Cores;

switch (coreType) {
  case Cores.NEO:
    // ...
  case Cores.BASE:
    // ...
  default:
    throw new Error("Invalid core type, must be one of base, neo");
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between b03eb5a and c028143.

📒 Files selected for processing (4)
  • contracts/config/courts.v2.mainnet-neo.json (1 hunks)
  • contracts/config/policies.v2.mainnet-neo.json (1 hunks)
  • contracts/deploy/00-home-chain-arbitration.ts (1 hunks)
  • contracts/scripts/keeperBot.ts (7 hunks)
🔇 Additional comments (10)
contracts/deploy/00-home-chain-arbitration.ts (2)

97-102: LGTM! Improved efficiency in core management.

The refactored implementation is more efficient and safer:

  • Prevents unnecessary transactions by checking the current core state
  • Properly typed contract instance with TypeScript
  • Clear and maintainable code structure

Line range hint 12-91: Consider documenting and validating configuration values.

The deployment script contains several critical configuration values (RNG_LOOKAHEAD, minStakingTime, maxFreezingTime, minStake, alpha, etc.) that would benefit from:

  1. Documentation explaining their significance and impact
  2. Validation to ensure values are within acceptable ranges
  3. Consideration for making them configurable via environment variables or config files

This would improve maintainability and reduce the risk of deployment errors.

Let's verify if these values are documented elsewhere:

contracts/config/courts.v2.mainnet-neo.json (2)

211-352: LGTM! Well-structured translation courts configuration

The translation courts are well-organized with:

  • Consistent minimum stake (9000 ETH) across all language pairs
  • Graduated fee and alpha values based on language complexity
  • Uniform timing parameters suitable for translation tasks

355-464: LGTM! Well-structured Spanish courts configuration

The Spanish courts configuration is appropriate with:

  • Reasonable minimum stakes relative to their English counterparts
  • Consistent timing parameters within court categories
  • Proper parent-child relationships
contracts/config/policies.v2.mainnet-neo.json (1)

68-69: Verify the validity of the unusually long IPFS URI

The URI for the Onboarding court appears to be significantly longer than other URIs in the file and contains what looks like additional hash segments.

✅ Verification successful

Let me analyze the results and generate a final response.

The IPFS URI format is valid and consistent with other URIs in the codebase

The URI in question follows the standard IPFS URI format /ipfs/<hash>/<optional-path>. The analysis shows that:

  1. The base IPFS hash QmbC7uhDEC33V8zyp8u6xozuD3GwtMp4Eaw25EzscEJk3R is a valid CIDv0 hash
  2. The additional path segment appears to be a valid identifier for the specific court policy
  3. The URI length is consistent with other policy URIs in the codebase that contain additional path segments
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the IPFS URI format and accessibility

# Extract and check all IPFS URIs
rg -o '/ipfs/[a-zA-Z0-9]+' | while read -r uri; do
  # Count characters in hash
  hash_length=$(echo "$uri" | cut -d'/' -f3 | wc -c)
  echo "URI: $uri, Hash length: $hash_length"
done

Length of output: 108445

contracts/scripts/keeperBot.ts (5)

1-10: Imports correctly updated to include Neo core and modules

The added imports for KlerosCoreNeo and SortitionModuleNeo are necessary for supporting the new core type and are correctly implemented.


27-27: CORE_TYPE environment variable with default value

Defining CORE_TYPE with a default value of "base" ensures backward compatibility and proper configuration if the environment variable is not set.


95-98: Defining the Cores enum

The Cores enum is defined to distinguish between BASE and NEO core types. With the suggested changes to use string enums, this definition will be clearer and less error-prone.


Line range hint 186-200: Robust error handling with optional chaining

Using optional chaining (randomizerRng?.target and blockHashRNG?.target) in the isRngReady function helps prevent runtime errors when randomizerRng or blockHashRNG are undefined or null. This improves the robustness of the function.


457-457: Adding type annotation to delay function

Explicitly typing the ms parameter in the delay function as number enhances type safety and helps prevent bugs related to incorrect parameter types.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Outside diff range and nitpick comments (7)
contracts/deploy/00-home-chain-arbitration.ts (1)

97-102: LGTM! Good refactoring for improved readability.

The introduction of disputeKitContract variable and the conditional check for core address change improves code clarity and efficiency.

Consider these improvements for better robustness:

 // changeCore() only if necessary
 const disputeKitContract = (await ethers.getContract("DisputeKitClassic")) as DisputeKitClassic;
 const currentCore = await disputeKitContract.core();
 if (currentCore !== klerosCore.address) {
   console.log(`disputeKit.changeCore(${klerosCore.address})`);
-  await disputeKitContract.changeCore(klerosCore.address);
+  try {
+    const tx = await disputeKitContract.changeCore(klerosCore.address);
+    await tx.wait();
+    // Verify the change
+    const newCore = await disputeKitContract.core();
+    if (newCore !== klerosCore.address) {
+      throw new Error("Core address change verification failed");
+    }
+  } catch (error) {
+    console.error("Failed to change core address:", error);
+    throw error;
+  }
 }
contracts/config/courts.v2.mainnet-neo.json (2)

211-225: Consider standardizing translation court parameters.

All translation courts (Spanish, French, Portuguese, German, Russian, Korean, Japanese, Turkish, Chinese) have identical minStake (9000000000000000000000) but varying feeForJuror and alpha values. Consider standardizing these parameters unless there's a specific reason for the variations:

  • Lower tier (Spanish, French, Portuguese): feeForJuror=3100000000000000, alpha=1500
  • Mid tier (German, Russian): feeForJuror=3900000000000000, alpha=1800
  • Higher tier (Korean, Japanese, Turkish, Chinese): feeForJuror=5000000000000000, alpha=2300

If these variations are intentional based on market rates or complexity, consider documenting the rationale. If not, consider standardizing the parameters.

Also applies to: 227-241, 243-257, 259-273, 275-289, 291-305, 307-321, 323-337, 339-353


1-464: Review court hierarchy for potential bottlenecks.

The current court structure has most courts (19 out of 29) directly under the General Court. This flat hierarchy might create bottlenecks in appeal handling and reduce the effectiveness of court specialization.

Consider restructuring the hierarchy to group related courts:

  • Move all translation courts under a new "Translation Services" parent court
  • Group technical courts (Solidity, Javascript) under the Development Court
  • Create a separate parent court for curation-related courts

A more structured hierarchy can improve:

  • Appeal handling efficiency
  • Juror specialization paths
  • Court management and governance
contracts/config/policies.v2.mainnet-neo.json (4)

81-82: Add missing descriptions and summaries

Several courts have empty description and summary fields:

  • Data Analysis (lines 81-82)
  • Statistical Modeling (lines 89-90)
  • Blockchain No Técnica (lines 225-226)

These fields should be populated to maintain consistency and provide clear guidance for jurors.

Also applies to: 89-90, 225-226


191-214: Standardize code size limits across technical courts

The technical courts have inconsistent code size limits for arbitration:

  • Solidity Court: 500 lines
  • Javascript Court: 700 lines
  • Development Court: No explicit limit

Consider standardizing these limits or providing justification for the differences in the court descriptions.


50-50: Fix grammatical errors in policies

There's a recurring grammatical error in the translation court policies:

-"All variations of English (UK, US, Australia, etc) are to accepted"
+"All variations of English (UK, US, Australia, etc) are to be accepted"

Also applies to: 130-130, 138-138, 146-146, 154-154, 162-162, 170-170


195-195: Fix typos in technical court requirements

There are spelling errors in the technical courts' required skills:

-"good level of programmation"
+"good level of programming"
-"solidity intermediate developers"
+"intermediate Solidity developers"
-"javascript intermediate developers"
+"intermediate JavaScript developers"

Also applies to: 203-203, 211-211

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between b03eb5a and c028143.

📒 Files selected for processing (4)
  • contracts/config/courts.v2.mainnet-neo.json (1 hunks)
  • contracts/config/policies.v2.mainnet-neo.json (1 hunks)
  • contracts/deploy/00-home-chain-arbitration.ts (1 hunks)
  • contracts/scripts/keeperBot.ts (7 hunks)
🔇 Additional comments (5)
contracts/deploy/00-home-chain-arbitration.ts (1)

97-102: Verify Neo configuration integration.

The PR objectives mention a distinction between KlerosCore and KlerosCoreNeo, but this deployment script doesn't seem to handle different core types yet.

Let's verify the core type handling:

contracts/config/courts.v2.mainnet-neo.json (1)

1-464: Verify economic parameters across courts.

The wide variation in economic parameters needs verification:

  • minStake ranges from 2.3K to 70K tokens
  • feeForJuror ranges from 0.7K to 64K wei
  • alpha ranges from 1300 to 10000

Consider documenting the rationale for these variations and verify that they align with:

  • Expected case complexity per court
  • Required juror expertise level
  • Market rates for similar services
  • Economic security requirements
✅ Verification successful

Let me analyze the distribution data and generate the final response.

Economic parameters across courts show intentional stratification

The distribution analysis reveals a well-structured pattern:

  • minStake shows clear tiers:

    • Entry-level courts (2.3K-2.9K tokens): General, Onboarding, Curation
    • Mid-tier courts (5.1K-9.5K tokens): Language, Development, Marketing
    • Specialist courts (15K-70K tokens): Technical, Data Analysis, Token Listing
  • feeForJuror follows similar stratification:

    • Basic tasks: 700K-3.1M wei
    • Standard cases: 3.9M-11M wei
    • Complex cases: 18M-64M wei
  • alpha values cluster logically:

    • Most courts (14) use standard 5000
    • General courts use maximum 10000
    • Specialized courts use lower values (1300-3333) for finer-grained control

The economic parameters demonstrate a coherent structure that aligns with court hierarchy, expertise requirements, and case complexity.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Analyze economic parameters distribution
echo "MinStake Distribution:"
jq -r '.[] | .minStake' contracts/config/courts.v2.mainnet-neo.json | sort -n | uniq -c

echo "\nFeeForJuror Distribution:"
jq -r '.[] | .feeForJuror' contracts/config/courts.v2.mainnet-neo.json | sort -n | uniq -c

echo "\nAlpha Distribution:"
jq -r '.[] | .alpha' contracts/config/courts.v2.mainnet-neo.json | sort -n | uniq -c

Length of output: 1420

contracts/config/policies.v2.mainnet-neo.json (1)

Line range hint 1-229: LGTM: Court hierarchy and numbering

The court numbering system is well-structured:

contracts/scripts/keeperBot.ts (2)

Line range hint 186-196: Good use of optional chaining to prevent runtime errors

The use of optional chaining when accessing randomizerRng?.target and blockHashRNG?.target prevents potential runtime errors if these variables are null or undefined.


457-457: Type annotation enhances type safety

Explicitly declaring the type of ms in the delay function improves type safety.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (9)
contracts/deploy/00-home-chain-arbitration.ts (2)

97-102: LGTM! Consider adding error handling.

The optimization to store the contract instance in a variable is a good improvement that reduces redundant contract fetches and improves code readability.

Consider wrapping the core change operation in a try-catch block for consistent error handling, similar to how currency rate changes are handled:

  // changeCore() only if necessary
  const disputeKitContract = (await ethers.getContract("DisputeKitClassic")) as DisputeKitClassic;
  const currentCore = await disputeKitContract.core();
  if (currentCore !== klerosCore.address) {
    console.log(`disputeKit.changeCore(${klerosCore.address})`);
-   await disputeKitContract.changeCore(klerosCore.address);
+   try {
+     await disputeKitContract.changeCore(klerosCore.address);
+   } catch (e) {
+     console.error("failed to change core:", e);
+   }
  }

Line range hint 12-65: Consider extracting hardcoded values to configuration files.

Given that this PR introduces new court configurations in courts.v2.mainnet-neo.json, consider moving the hardcoded deployment parameters to configuration files for better maintainability and network-specific customization. This includes:

  • RNG parameters (LOOKAHEAD)
  • Timing parameters (minStakingTime, maxFreezingTime)
  • Economic parameters (minStake, alpha, feeForJuror)
  • Court parameters (jurorsForCourtJump)

This would allow for:

  1. Easier parameter tuning per environment
  2. Better alignment with court configurations
  3. Reduced risk of misconfiguration
  4. Simplified maintenance

Would you like me to propose a configuration structure for these parameters?

contracts/config/courts.v2.mainnet-neo.json (3)

214-218: Standardize translation court parameters.

The translation courts have varying alpha values (1500, 1800, 2300) and fees (3100000000000000 to 5000000000000000) without clear justification. Consider standardizing these parameters unless there's a specific reason for the differences.

Also applies to: 230-234, 246-250, 262-266, 278-282, 294-298, 310-314, 326-330, 342-346


37-42: Review court hierarchy for technical disputes.

The "Non-Technical" court (id: 3) is a child of "Blockchain" court (id: 2), while "Technical" court (id: 5) is also a child of "Blockchain". This structure might cause confusion about which court should handle borderline cases.

Consider:

  1. Making both courts direct children of the General Court
  2. Adding clear jurisdiction guidelines in the court policies

Also applies to: 69-74


1-464: Consider adding schema validation.

The configuration file would benefit from JSON schema validation to ensure all required fields are present and values are within acceptable ranges.

Consider adding a schema file that defines:

  • Required fields
  • Value ranges for numeric fields
  • Allowed values for categorical fields
  • Parent-child relationship constraints
contracts/config/policies.v2.mainnet-neo.json (3)

80-82: Add missing descriptions and summaries for courts

The following courts have empty description and/or summary fields which could lead to confusion for jurors:

  • Data Analysis (court 11)
  • Statistical Modeling (court 12)
  • Blockchain No Técnica (court 29)

Consider adding appropriate descriptions to provide clear guidance for jurors.

Also applies to: 88-90, 224-226


50-50: Fix typos and grammatical errors

Several typos and grammatical errors were found:

  • Line 50: "are to accepted" should be "are to be accepted"
  • Line 194: "Developper" should be "Developer"
  • Line 195: "refuse the proposal" would be clearer as "reject the submission"
  • Line 211: "be comfortable with" should be "be comfortable using"

Also applies to: 194-194, 195-195, 211-211


196-197: Standardize policy file naming convention

The xDai courts use inconsistent naming patterns for their policy files:

  • Some use PascalCase: "xDai-Development-Court-Policy.json"
  • Others use different patterns: "xDai-Spanish-Curation-Court-Policy.json"

Consider standardizing the naming convention across all policy files for better maintainability.

Also applies to: 204-205, 212-213, 220-221, 228-229

contracts/scripts/keeperBot.ts (1)

49-63: Improve robustness in handling CORE_TYPE environment variable

The retrieval of coreType may result in undefined if CORE_TYPE does not match 'BASE' or 'NEO' (case-insensitive), potentially leading to unexpected behavior before reaching the default case. Consider adding a check for undefined after mapping CORE_TYPE to the Cores enum to explicitly handle invalid values.

Apply this diff to improve error handling:

const coreTypeKey = CORE_TYPE.toUpperCase();
const coreType = Cores[coreTypeKey as keyof typeof Cores];
+if (coreType === undefined) {
+  throw new Error(`Invalid CORE_TYPE '${CORE_TYPE}', must be one of 'base', 'neo'`);
+}
switch (coreType) {
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between b03eb5a and c028143.

📒 Files selected for processing (4)
  • contracts/config/courts.v2.mainnet-neo.json (1 hunks)
  • contracts/config/policies.v2.mainnet-neo.json (1 hunks)
  • contracts/deploy/00-home-chain-arbitration.ts (1 hunks)
  • contracts/scripts/keeperBot.ts (7 hunks)
🔇 Additional comments (8)
contracts/deploy/00-home-chain-arbitration.ts (1)

Line range hint 1-102: Verify implementation of core type handling.

The PR objectives mention the introduction of a new CORE_TYPE to distinguish between KlerosCore and KlerosCoreNeo, along with a Cores enum. However, these changes are not reflected in this deployment script. This might affect how different core types are handled during deployment.

Let's verify if these changes exist in other files:

✅ Verification successful

Based on the search results, I can see that:

  1. The CORE_TYPE and Cores enum are implemented in the keeper bot and populate courts scripts:
contracts/scripts/keeperBot.ts:const CORE_TYPE = env.optional("CORE_TYPE", "base");
contracts/scripts/keeperBot.ts:  const coreType = Cores[CORE_TYPE.toUpperCase() as keyof typeof Cores];
contracts/scripts/keeperBot.ts:enum Cores {
contracts/scripts/populateCourts.ts:enum Cores {
  1. The deployment script has a separate file for KlerosCoreNeo deployment at contracts/deploy/00-home-chain-arbitration-neo.ts

  2. The KlerosCoreNeo contract exists and is properly implemented with its own proxy and implementation contracts.

This indicates that the core type handling is already implemented in the codebase, just not in this particular deployment script since it's the base version. The KlerosCoreNeo has its own dedicated deployment script.

The core type handling is properly implemented across the codebase

The PR objectives mentioning the introduction of CORE_TYPE and Cores enum are reflected in:

  1. The keeper bot script for runtime core type selection
  2. The populate courts script for court initialization
  3. Separate deployment scripts for different core types (base vs neo)
  4. Proper contract implementations with their respective proxies
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for core type related changes

# Look for CORE_TYPE or Cores enum definitions
rg -i "CORE_TYPE|enum\s+Cores"

# Look for KlerosCoreNeo references
rg "KlerosCoreNeo"

# Look for any type definitions related to cores
ast-grep --pattern 'type $_ = $$$Core$$$'

Length of output: 600399

contracts/config/courts.v2.mainnet-neo.json (2)

459-464: Review shorter time periods for "Blockchain No Técnica" court.

The time periods (108000, 216000) are significantly shorter than other courts, which could lead to rushed decisions. Most courts use longer periods (280800, 583200) to ensure thorough deliberation.


135-138: Verify stake-to-fee ratios for low-stake courts.

The Onboarding and Curation courts have relatively low minimum stakes (2.3M and 2.6M) compared to their fees (700K and 2.4M). This ratio might not provide sufficient economic incentive for honest voting.

Also applies to: 151-154

✅ Verification successful

Stake-to-fee ratios are within acceptable range

The analysis of the court configurations shows:

  • Onboarding Court: 2300 ANJ stake vs 0.0007 ANJ fee (ratio ~3285:1)
  • Curation Court: 2600 ANJ stake vs 0.0024 ANJ fee (ratio ~1083:1)

These ratios provide sufficient economic incentive for honest voting, as jurors would need to stake significantly more than what they could earn in fees, ensuring they have enough "skin in the game" to vote honestly.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Analyze stake-to-fee ratios across all courts
jq -r '.[] | "\(.name): stake=\(.minStake), fee=\(.feeForJuror), ratio=\(.minStake | tonumber / (.feeForJuror | tonumber))"' courts.v2.mainnet-neo.json | sort -t: -k2 -n

Length of output: 256


Script:

#!/bin/bash
# First, let's find the correct path to the courts config file
fd courts.v2.mainnet-neo.json

# Then analyze stake-to-fee ratios
fd courts.v2.mainnet-neo.json --exec jq -r '.[] | "\(.name): stake=\(.minStake), fee=\(.feeForJuror), ratio=\(.minStake | tonumber / (.feeForJuror | tonumber))"' {} | sort -t: -k2 -n

Length of output: 374


Script:

#!/bin/bash
# Let's first check the structure of the JSON file
cat contracts/config/courts.v2.mainnet-neo.json | head -n 20

# Then we'll try to analyze the content differently
jq '.' contracts/config/courts.v2.mainnet-neo.json | head -n 20

Length of output: 844


Script:

#!/bin/bash
# Let's try with a modified jq query to handle the array structure correctly
jq -r '.[] | select(.name == "Onboarding" or .name == "Curation") | "\(.name): stake=\(.minStake), fee=\(.feeForJuror), ratio=\(.minStake | tonumber / (.feeForJuror | tonumber))"' contracts/config/courts.v2.mainnet-neo.json

# Also get the full court configuration for these specific courts
jq -r '.[] | select(.name == "Onboarding" or .name == "Curation")' contracts/config/courts.v2.mainnet-neo.json

Length of output: 1001

contracts/scripts/keeperBot.ts (5)

1-10: Imports updated to support new core types

The addition of KlerosCoreNeo, SortitionModuleNeo, and other related imports correctly reflect the inclusion of the new core type functionality.


95-99: Enum Cores correctly defines core types

The introduction of the Cores enum provides a clear and type-safe way to handle core types.


186-186: Enhanced error handling with optional chaining

Using optional chaining (?.) in if (currentRng === randomizerRng?.target) prevents runtime errors if randomizerRng is null or undefined, improving code safety.


196-196: Improved safety with optional chaining in RNG check

Applying optional chaining in else if (currentRng === blockHashRNG?.target) ensures that the code handles cases where blockHashRNG might be null or undefined, preventing potential errors.


457-457: Explicit parameter typing enhances code clarity

Adding the type annotation (ms: number) to the delay function parameter improves code readability and ensures proper type checking.

coderabbitai[bot]
coderabbitai bot previously approved these changes Nov 19, 2024
Copy link

codeclimate bot commented Nov 19, 2024

Code Climate has analyzed commit a218f9e and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 3

View more on Code Climate.

@jaybuidl jaybuidl added this pull request to the merge queue Nov 19, 2024
Merged via the queue into dev with commit 884674e Nov 19, 2024
27 of 28 checks passed
@jaybuidl jaybuidl deleted the feat/keeper-bot-neo branch November 20, 2024 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant