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

Release v2-testnet-4.0.6 #1845

Merged
merged 64 commits into from
Jan 27, 2025
Merged

Release v2-testnet-4.0.6 #1845

merged 64 commits into from
Jan 27, 2025

Conversation

jaybuidl
Copy link
Member

@jaybuidl jaybuidl commented Jan 22, 2025

PR-Codex overview

This PR focuses on code refactoring, improvements in query handling, and updates to contract versions and configurations in the Kleros project. It includes the addition of new features, adjustments to existing logic, and enhancements to user interfaces.

Detailed summary

  • Updated regex pattern in web/src/utils/commify.ts.
  • Changed version function to override in several Solidity contracts.
  • Added staleTime to various queries in hooks for better data management.
  • Introduced new courts and updated configurations in JSON files.
  • Enhanced file uploader messages in the UI.
  • Improved error handling and query execution in GraphqlBatcher.
  • Added a script for processing policy URIs.
  • Updated package versions and dependencies in package.json.

The following files were skipped due to too many changes: web/src/pages/Cases/CaseDetails/Evidence/index.tsx, contracts/src/proxy/UUPSProxy.sol, contracts/src/arbitration/dispute-kits/DisputeKitGated.sol, kleros-sdk/test/getDispute.test.ts, web/src/assets/svgs/socialmedia/github.svg, web/src/assets/svgs/socialmedia/discord.svg, contracts/src/proxy/UUPSProxiable.sol, yarn.lock, contracts/config/policies.v2.devnet.json, contracts/config/policies.v2.testnet.json, contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol, contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol, contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol, contracts/config/policies.v2.mainnet-neo.json, web/src/assets/svgs/mini-guides/dispute-resolver/well-done.svg

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

Summary by CodeRabbit

Based on the comprehensive summary, here are the high-level release notes:

  • New Features

    • Added a new DisputeKitGated contract with token-based access control for dispute resolution.
    • Introduced version tracking for multiple contracts.
    • Added a "Refuse to Arbitrate" option in dispute resolution workflows.
    • Implemented a new custom hook useSpamEvidence for fetching spam evidence data.
  • Improvements

    • Simplified contract inheritance and initialization processes.
    • Updated network configurations to use Infura endpoints.
    • Enhanced dispute kit and sortition module upgrade mechanisms.
    • Improved loading state handling in web interfaces.
    • Updated GraphQL queries to sort by disputeID.
    • Enhanced styling for Markdown content in the Description component.
    • Enhanced functionality of the populate:courts and populate:policy-registry tasks with new parameters.
  • Dependencies

    • Updated various package versions.
    • Added new dependencies for web and SDK components.
  • Deployment

    • Modified deployment scripts for Kleros Core, Sortition Module, and Dispute Kits.
    • Introduced new proxy contracts for upgradeable components.
  • Bug Fixes

    • Refined error handling in dispute resolution contracts.
    • Updated GraphQL query sorting mechanisms.

Harman-singh-waraich and others added 30 commits December 13, 2024 16:06
Fix: Ensure that SDK getDispute() returns the RTA/Invalid answer
…ment

feat: better appeal loading mechanism
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 comments (1)
contracts/config/courts.v2.testnet.json (1)

Court parameters show significant inconsistencies across environments

The verification revealed several concerns:

  • Curation court has different alpha values (3100 in testnet vs 5000 in mainnet)
  • Inconsistent scaling ratios between mainnet and testnet:
    • Stake ratios vary from 13x to 45x
    • Fee ratios vary from 240x to 740x

These variations may not properly simulate mainnet conditions during testing.

🔗 Analysis chain

Line range hint 19-33: Verify consistency of parameters across environments.

The parameters for child courts (Curation, English Language, Corte de Disputas, Oracle) should be validated against their mainnet counterparts to ensure proper testing conditions.

Also applies to: 35-48, 51-65, 67-80

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Compare court parameters across environments
# Expected: Parameters should follow similar ratios while being appropriately scaled

# Function to extract and format court parameters
extract_params() {
  local file=$1
  local court=$2
  jq -r --arg court "$court" '.[] | select(.name==$court) | "\(.minStake) \(.feeForJuror) \(.alpha)"' "$file"
}

# Compare parameters for each court across environments
for court in "Curation" "English Language" "Corte de Disputas de Consumo y Vecindad" "Oracle Court"; do
  echo "=== $court ==="
  echo "Testnet:"
  extract_params "contracts/config/courts.v2.testnet.json" "$court"
  echo "Devnet:"
  extract_params "contracts/config/courts.v2.devnet.json" "$court"
  echo "Mainnet:"
  extract_params "contracts/config/courts.v2.mainnet-neo.json" "$court"
  echo
done

Length of output: 5374

🧹 Nitpick comments (10)
web/src/pages/Courts/CourtDetails/Description.tsx (1)

22-54: Consider improving style maintainability.

The styling implementation looks good, but could be enhanced for better maintainability:

  • Consider consolidating duplicate margin values (16px 0 16px 0) into a shared CSS variable
  • Define font sizes using CSS variables for consistent typography management

Example refactor:

 const StyledReactMarkdown = styled(ReactMarkdown)`
+  --heading-margin: 16px 0;
+  --heading-large: 20px;
+  --heading-medium: 18px;
+  --text-regular: 16px;
+
   p {
     word-break: break-word;
   }

   ul,
   ol {
     li + li {
       margin-top: 8px;
     }
   }

   h1 {
-    margin: 16px 0 16px 0;
-    font-size: 20px;
+    margin: var(--heading-margin);
+    font-size: var(--heading-large);
     line-height: 26px;
   }

   h2 {
-    margin: 16px 0 16px 0;
-    font-size: 20px;
+    margin: var(--heading-margin);
+    font-size: var(--heading-large);
     line-height: 26px;
   }

   h3 {
-    margin: 16px 0 16px 0;
-    font-size: 18px;
+    margin: var(--heading-margin);
+    font-size: var(--heading-medium);
     line-height: 24px;
   }

   a {
-    font-size: 16px;
+    font-size: var(--text-regular);
   }
`;
contracts/scripts/populatePolicyRegistry.ts (1)

98-102: Add bounds validation for start and end indices.

The slicing logic is correct, but we should add validation to ensure start is within bounds and end doesn't exceed the array length.

 const start = taskArgs.start;
+if (start < 0 || start >= policiesV2.length) {
+  throw new Error(`Invalid start index: ${start}. Must be between 0 and ${policiesV2.length - 1}`);
+}
 const end = taskArgs.maxNumberOfCourts ? start + taskArgs.maxNumberOfCourts : policiesV2.length;
+const actualEnd = Math.min(end, policiesV2.length);
-console.log(`Keeping only the first ${end - start} courts, starting from ${start}`);
-policiesV2 = policiesV2.slice(start, end);
+console.log(`Keeping only the first ${actualEnd - start} courts, starting from ${start}`);
+policiesV2 = policiesV2.slice(start, actualEnd);
contracts/scripts/populateCourts.ts (1)

153-162: Add bounds validation and improve logging for court ordering.

The slicing logic should validate bounds, and the logging could be more descriptive when reversing the order.

+if (start < 0 || start >= courtsV2.length) {
+  throw new Error(`Invalid start index: ${start}. Must be between 0 and ${courtsV2.length - 1}`);
+}
 const end = taskArgs.maxNumberOfCourts ? start + taskArgs.maxNumberOfCourts : courtsV2.length;
+const actualEnd = Math.min(end, courtsV2.length);
-console.log(`Keeping only the first ${end - start} courts, starting from ${start}`);
+console.log(`Processing courts ${start} to ${actualEnd - 1} (${actualEnd - start} courts)`);
-courtsV2 = courtsV2.slice(start, end);
+courtsV2 = courtsV2.slice(start, actualEnd);

 if (taskArgs.reverse) {
-  console.log("Reversing the order of courts");
+  console.log("Reversing the order of courts to handle dependencies (child courts first)");
   courtsV2.reverse();
 }
contracts/scripts/setPoliciesURIs.sh (2)

9-15: Enhance error handling and logging.

The file extension validation could be improved with more descriptive error messages and explicit logging of skipped files.

 # Process each input file
 for INPUT_FILE in "$@"; do
     # Validate file extension
     if [[ ! "$INPUT_FILE" =~ \.json$ ]]; then
-        echo "Error: Input file $INPUT_FILE must have a .json extension"
+        echo "Error: Input file '$INPUT_FILE' must have a .json extension. Skipping this file..."
+        echo "----------------------------------------"
         continue
     fi

67-70: Enhance cleanup and add status summary.

Add comprehensive cleanup and provide a status summary of the operations performed.

-rm "$HASHES_FILE"
-echo "Done! URIs updated in $INPUT_FILE"
-echo "----------------------------------------"
+# Cleanup
+rm -f "$HASHES_FILE"
+[ -f "${INPUT_FILE}.bak" ] && rm "${INPUT_FILE}.bak"
+
+# Optional: cleanup individual policy files if no longer needed
+# rm -rf "$POLICIES_DIR"
+
+# Print status summary
+echo "Operation completed successfully:"
+echo "- Input file: $INPUT_FILE"
+echo "- Policies processed: $(jq 'length' "$INPUT_FILE")"
+echo "- Policy files directory: $POLICIES_DIR"
+echo "----------------------------------------"
web/src/styles/global-style.ts (1)

Line range hint 101-105: Consider removing explicit visibility property.

The visibility: visible declaration is redundant as this is the default value for all elements.

  svg, img {
    display: inline-block;
    vertical-align: middle;
-    visibility: visible;
  }
contracts/config/policies.v2.devnet.json (3)

12-12: Consider adding rules for the Curation court.

The "rules" field is empty. Consider adding basic curation guidelines to help jurors make consistent decisions.


28-28: Consider adding rules for the Spanish Consumer court.

The "rules" field is empty. Consider adding basic guidelines specific to consumer and neighborhood disputes to help jurors make consistent decisions.


36-36: Update example dates in Oracle court rules.

The rules contain examples using dates from 2024, which is the current year. Consider:

  1. Using past dates for historical examples
  2. Using relative time references (e.g., "current year", "next year") for future examples
contracts/config/policies.v2.mainnet-neo.json (1)

201-201: Add purpose descriptions for development courts.

The "purpose" field is empty for both the Solidity and Javascript courts. Consider adding descriptions to help users understand when to use these courts.

Also applies to: 209-209

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4ed43ef and 25dce8a.

📒 Files selected for processing (15)
  • contracts/config/courts.v2.devnet.json (1 hunks)
  • contracts/config/courts.v2.mainnet-neo.json (2 hunks)
  • contracts/config/courts.v2.testnet.json (4 hunks)
  • contracts/config/policies.v2.devnet.json (1 hunks)
  • contracts/config/policies.v2.mainnet-neo.json (1 hunks)
  • contracts/config/policies.v2.testnet.json (1 hunks)
  • contracts/config/policies.v2/Curation-Court-Policy.json (0 hunks)
  • contracts/config/policies.v2/English-Language-Court-Policy.json (0 hunks)
  • contracts/config/policies.v2/General-Court-Policy.json (0 hunks)
  • contracts/package.json (5 hunks)
  • contracts/scripts/populateCourts.ts (4 hunks)
  • contracts/scripts/populatePolicyRegistry.ts (2 hunks)
  • contracts/scripts/setPoliciesURIs.sh (1 hunks)
  • web/src/pages/Courts/CourtDetails/Description.tsx (5 hunks)
  • web/src/styles/global-style.ts (2 hunks)
💤 Files with no reviewable changes (3)
  • contracts/config/policies.v2/Curation-Court-Policy.json
  • contracts/config/policies.v2/English-Language-Court-Policy.json
  • contracts/config/policies.v2/General-Court-Policy.json
🚧 Files skipped from review as they are similar to previous changes (1)
  • contracts/package.json
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: dependency-review
  • GitHub Check: contracts-testing
  • GitHub Check: Analyze (javascript)
  • GitHub Check: SonarCloud
  • GitHub Check: SonarCloud
  • GitHub Check: Scorecards analysis
  • GitHub Check: Analyze (javascript)
🔇 Additional comments (16)
web/src/pages/Courts/CourtDetails/Description.tsx (4)

69-71: LGTM! Interface changes improve clarity.

The renaming from description/summary to purpose/rules makes the policy structure more intuitive and better reflects the content's purpose.


79-79: LGTM! Tab configuration correctly updated.

The visibility conditions have been properly updated to match the new interface properties.

Also applies to: 91-91


122-124: LGTM! Routes properly aligned with interface changes.

The route paths and elements have been correctly updated to use the new policy properties.


135-135: LGTM! Markdown formatting properly updated.

The function correctly uses the new styled component while maintaining the same markdown processing logic.

contracts/scripts/populateCourts.ts (1)

138-140: Verify the removal of parametersProductionToDev for V2_DEVNET.

The V2_DEVNET case no longer applies the parametersProductionToDev transformation. Please confirm if this is intentional, as it might affect development environment parameters.

web/src/styles/global-style.ts (2)

97-99: LGTM! Consistent theme-based styling for horizontal rules.

The styling maintains consistency with the theme system.


108-114: LGTM! Consistent text styling for list items.

The styling maintains consistency with other text elements and uses theme-based colors.

contracts/config/courts.v2.testnet.json (1)

Line range hint 3-15: LGTM! General Court configuration is appropriate for testnet.

The parameters are well-balanced for a testnet environment with appropriate stake requirements and time periods.

contracts/config/courts.v2.devnet.json (2)

12-15: LGTM! Appropriate time periods for development testing.

The reduced time periods (120s, 240s, 240s, 600s) are well-suited for development testing while maintaining the relative ratios between periods.

Also applies to: 28-31, 44-47, 60-63, 76-79


7-9: Verify the significant reduction in fees and stakes.

The substantial reduction in minStake and feeForJuror values needs validation to ensure it doesn't impact the economic model testing.

Also applies to: 23-25, 39-41, 55-57, 71-73

✅ Verification successful

The reduced economic parameters in devnet are appropriately scaled

The 100x reduction in stakes and fees is uniformly applied across all courts while maintaining the same relative ratios between courts as in testnet. This scaling makes the system more accessible for testing while preserving the economic model's core properties.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Analyze the economic model scaling
# Expected: Values should maintain similar ratios while being appropriately scaled

# Calculate and compare ratios between courts
jq -r '
  def ratio(a; b): (a | tonumber) / (b | tonumber);
  . as $courts |
  $courts[] | select(.name=="General Court") as $base |
  $courts[] | {
    court: .name,
    stake_ratio: ratio(.minStake; $base.minStake),
    fee_ratio: ratio(.feeForJuror; $base.feeForJuror)
  }
' contracts/config/courts.v2.devnet.json

Length of output: 754


Script:

#!/bin/bash
# Compare economic model ratios between devnet and testnet
echo "=== Devnet Configuration ==="
jq -r '[.[] | {name: .name, minStake, feeForJuror}]' contracts/config/courts.v2.devnet.json

echo -e "\n=== Testnet Configuration ==="
jq -r '[.[] | {name: .name, minStake, feeForJuror}]' contracts/config/courts.v2.testnet.json

Length of output: 1512

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

Line range hint 451-465: LGTM! Court rename maintains existing parameters.

The renamed court "Corte de Disputas de Consumo y Vecindad" preserves its configuration, ensuring continuity.


467-480: Verify Oracle Court parameters for mainnet security.

The new Oracle Court's parameters should be validated to ensure they provide adequate economic security for mainnet deployment.

✅ Verification successful

Oracle Court parameters provide adequate economic security for mainnet

The Oracle Court's parameters are well-calibrated for mainnet deployment:

  • Stake requirement (5000 ETH) appropriately positions it between General Court and specialized technical courts
  • Juror fees align with similar courts like the Blockchain Court
  • Security parameters (alpha, jurorsForJump) are properly tuned for specialized oracle dispute resolution
  • Time periods maintain consistency with the parent court structure
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Validate Oracle Court security parameters
# Expected: Parameters should provide adequate economic security

# Extract and analyze Oracle Court parameters in relation to other courts
jq -r '
  def avg(arr): (arr | add) / (arr | length);
  def stddev(arr; mean): sqrt((arr | map((. - mean) * (. - mean)) | add) / (arr | length));
  
  . as $courts |
  ($courts | map(.minStake | tonumber) | avg) as $avgStake |
  ($courts | map(.feeForJuror | tonumber) | avg) as $avgFee |
  
  $courts[] | select(.name=="Oracle Court") as $oracle |
  {
    stake_vs_avg: ($oracle.minStake | tonumber) / $avgStake,
    fee_vs_avg: ($oracle.feeForJuror | tonumber) / $avgFee,
    parent_court: ($courts[] | select(.id==($oracle.parent | tonumber)) | .name)
  }
' contracts/config/courts.v2.mainnet-neo.json

Length of output: 1063


Script:

#!/bin/bash
# Get all courts data for comparison
echo "=== All Courts Overview ==="
jq -r '.[] | select(.name != null) | "\(.name):\n  stake: \(.minStake)\n  fee: \(.feeForJuror)\n  parent: \(.parent)"' contracts/config/courts.v2.mainnet-neo.json

echo -e "\n=== Oracle Court Specific Analysis ==="
# Get Oracle Court and its parent court details
jq -r '.[] | select(.name=="Oracle Court" or .id==1) | "\(.name):\n  stake: \(.minStake)\n  fee: \(.feeForJuror)\n  alpha: \(.alpha)\n  jurorsForJump: \(.jurorsForCourtJump)\n  periods: \(.timesPerPeriod|join(","))"' contracts/config/courts.v2.mainnet-neo.json

Length of output: 3574

contracts/config/policies.v2.devnet.json (2)

20-20: ⚠️ Potential issue

Fix grammar in English Language court rules.

There's a grammatical error in the rules: "are to accepted" should be "are to be accepted".

Apply this diff to fix the grammar:

-  "rules": "- If the disputed content is of significant size (> 10 000 words), parties in the dispute should point out specific parts of the content which are being disputed. Otherwise, jurors should refuse to arbitrate.\n\n- All variations of English (UK, US, Australia, etc) are to accepted unless a target audience is specifically mentioned in the contract.",
+  "rules": "- If the disputed content is of significant size (> 10 000 words), parties in the dispute should point out specific parts of the content which are being disputed. Otherwise, jurors should refuse to arbitrate.\n\n- All variations of English (UK, US, Australia, etc) are to be accepted unless a target audience is specifically mentioned in the contract.",

Likely invalid or redundant comment.


29-29: Verify the external link in Spanish Consumer court.

The requiredSkills field contains an external link to Buenos Aires consumer rights. Ensure this link is accessible and consider:

  1. Adding a backup link or embedding key rights directly in case the external site changes
  2. Including consumer rights references from other Spanish-speaking regions
✅ Verification successful

External link is accessible and appropriate

The link to Buenos Aires consumer rights is working correctly and is hosted on an official government domain. While backup links could be helpful, using the official government source is appropriate for this jurisdiction.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify the accessibility of the consumer rights link
curl -I https://buenosaires.gob.ar/principales-derechos-de-los-consumidores

Length of output: 1496

contracts/config/policies.v2.testnet.json (1)

Line range hint 1-237: Changes mirror devnet configuration.

The changes in this file are identical to those in contracts/config/policies.v2.devnet.json. The same review comments apply here.

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

192-197: Verify impact of court name changes.

The following courts have been renamed to remove the "xDai" prefix:

  • "xDai Development Court" → "Development Court"
  • "xDai Solidity Court" → "Solidity Court"
  • "xDai Javascript Court" → "Javascript Court"

Ensure that all references to these courts in the codebase, documentation, and UI have been updated accordingly.

Also applies to: 200-205, 208-213

✅ Verification successful

Court name changes are correctly implemented

The old court names with "xDai" prefix exist only in v1 configuration files for gnosischain, while the new names are correctly used in v2 configuration files for mainnet. This separation is expected and proper.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for references to the old court names
echo "Searching for references to old court names..."
rg -i "xDai Development Court|xDai Solidity Court|xDai Javascript Court"

Length of output: 753

@jaybuidl
Copy link
Member Author

@coderabbitai resolve

Copy link
Contributor

coderabbitai bot commented Jan 27, 2025

✅ Actions performed

Comments resolved and changes approved.

coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 27, 2025
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: 0

🧹 Nitpick comments (1)
web/src/utils/commify.ts (1)

4-4: LGTM! The regex improvement enhances input validation.

The new regex pattern is more robust as it enforces at least one digit and properly validates decimal numbers. This prevents edge cases like empty strings or lone decimal points.

Consider adding input type validation before the regex check for better error handling:

 export function commify(value: string | number): string {
   const comps = String(value).split(".");

+  if (typeof value !== 'string' && typeof value !== 'number') {
+    return "0";
+  }
   if (!String(value).match(/^-?\d+(\.\d+)?$/)) {
     return "0";
   }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2343dfa and ec16bb7.

⛔ Files ignored due to path filters (1)
  • web/src/assets/svgs/mini-guides/dispute-resolver/well-done.svg is excluded by !**/*.svg
📒 Files selected for processing (1)
  • web/src/utils/commify.ts (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: contracts-testing
  • GitHub Check: SonarCloud
  • GitHub Check: Analyze (javascript)
  • GitHub Check: dependency-review
  • GitHub Check: Scorecards analysis
  • GitHub Check: SonarCloud
  • GitHub Check: Analyze (javascript)

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: 0

🧹 Nitpick comments (8)
web/src/context/GraphqlBatcher.tsx (1)

25-39: Inline @ts-ignore usage may obscure real typing issues.
While the try-catch block appropriately prevents a single error from failing the entire batch (consistent with the retrieved learning), consider removing or refining the @ts-ignore comment once proper typings are established. This helps maintain stronger type safety across your codebase.

web/src/hooks/queries/useCourtDetails.ts (1)

3-3: LGTM! Consider adding type safety for query key.

The addition of staleTime configuration is a good practice for optimizing cache behavior.

Consider using an array literal type for the query key to improve type safety:

-    queryKey: [`courtDetails${id}`],
+    queryKey: ['courtDetails', id] as const,

Also applies to: 39-39

web/src/hooks/queries/useDisputeMaintenanceQuery.ts (1)

5-5: Standardize import paths across files.

While the implementation is correct, the import path for STALE_TIME differs from other files (src/consts vs consts/index). Consider standardizing the import paths across all files for better maintainability.

-import { STALE_TIME } from "src/consts";
+import { STALE_TIME } from "consts/index";

Also applies to: 44-44

web/src/hooks/queries/useDisputeDetailsQuery.ts (1)

3-3: Optimize query key structure for better caching.

The implementation looks good, but the query key structure could be improved for better caching and type safety.

Consider restructuring the query key:

-    queryKey: [`disputeDetailsQuery${id}`],
+    queryKey: ['disputeDetails', id?.toString()] as const,

This structure:

  1. Makes the key more predictable
  2. Ensures consistent type conversion
  3. Improves cache key readability

Also applies to: 51-51

web/src/hooks/queries/useCourtTree.ts (1)

5-5: Consider making the root court ID configurable.

While the implementation is correct, there are a few potential improvements:

  1. Make the root court ID configurable:
-  query CourtTree {
-    court(id: "1") {
+  query CourtTree($rootId: ID!) {
+    court(id: $rootId) {
  1. Since this query doesn't use variables, consider simplifying the fetch call:
-      await graphqlBatcher.fetch({ id: crypto.randomUUID(), document: courtTreeQuery, variables: {} }),
+      await graphqlBatcher.fetch({ id: crypto.randomUUID(), document: courtTreeQuery, variables: { rootId: "1" } }),

Also applies to: 43-43

web/src/consts/index.ts (1)

8-10: Consider aligning STALE_TIME with REFETCH_INTERVAL.

The current configuration marks data as stale (1s) much earlier than the refetch interval (5s). This might lead to unnecessary background refetches when users revisit tabs or refocus the window. Consider setting STALE_TIME equal to or greater than REFETCH_INTERVAL to optimize caching behavior.

 export const REFETCH_INTERVAL = 5000;
-export const STALE_TIME = 1000;
+export const STALE_TIME = 5000;
web/src/hooks/queries/useUser.ts (2)

6-6: Standardize import paths across files.

Other files import from 'consts/index' while this file imports from 'src/consts'. Maintain consistency in import paths.

-import { STALE_TIME } from "src/consts";
+import { STALE_TIME } from "consts/index";

62-62: LGTM! Consider longer stale time for user data.

The staleTime addition is correct. Since user profile data (total disputes, coherence score) changes infrequently, you might benefit from a longer stale time specifically for this query to reduce unnecessary refetches.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ec16bb7 and d4aaa7c.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (12)
  • web/package.json (1 hunks)
  • web/src/consts/index.ts (2 hunks)
  • web/src/context/GraphqlBatcher.tsx (2 hunks)
  • web/src/hooks/queries/useAllCasesQuery.ts (2 hunks)
  • web/src/hooks/queries/useClassicAppealQuery.ts (2 hunks)
  • web/src/hooks/queries/useCourtDetails.ts (2 hunks)
  • web/src/hooks/queries/useCourtTree.ts (2 hunks)
  • web/src/hooks/queries/useDisputeDetailsQuery.ts (2 hunks)
  • web/src/hooks/queries/useDisputeMaintenanceQuery.ts (2 hunks)
  • web/src/hooks/queries/useJurorStakeDetailsQuery.ts (2 hunks)
  • web/src/hooks/queries/useUser.ts (2 hunks)
  • web/src/hooks/queries/useVotingHistory.ts (2 hunks)
🧰 Additional context used
📓 Learnings (2)
web/src/hooks/queries/useAllCasesQuery.ts (1)
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1850
File: web/src/context/GraphqlBatcher.tsx:25-41
Timestamp: 2025-01-27T11:25:08.246Z
Learning: In GraphQL batch operations using Promise.all, individual request errors should be caught and handled (e.g., returning {data: {}}) rather than thrown, to prevent a single failure from causing the entire batch to fail.
web/src/context/GraphqlBatcher.tsx (1)
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1850
File: web/src/context/GraphqlBatcher.tsx:25-41
Timestamp: 2025-01-27T11:25:08.246Z
Learning: In GraphQL batch operations using Promise.all, individual request errors should be caught and handled (e.g., returning {data: {}}) rather than thrown, to prevent a single failure from causing the entire batch to fail.
⏰ Context from checks skipped due to timeout of 90000ms (8)
  • GitHub Check: SonarCloud
  • GitHub Check: Analyze (javascript)
  • GitHub Check: contracts-testing
  • GitHub Check: dependency-review
  • GitHub Check: SonarCloud
  • GitHub Check: Analyze (javascript)
  • GitHub Check: contracts-testing
  • GitHub Check: Scorecards analysis
🔇 Additional comments (10)
web/src/context/GraphqlBatcher.tsx (3)

3-4: Imports look appropriate.
The newly added imports from @graphql-tools/batch-execute and @graphql-tools/utils provide well-tested batching and execution utilities for GraphQL. This approach can streamline requests and reduce network overhead.


41-41: Batch executor creation.
Using createBatchingExecutor(executor) correctly encapsulates the shared executor logic. The code here is succinct and clear.


44-53: Partial error handling confirmation.
When mapping the batched data, ensure that any potential per-request errors return an object with an empty data field (as done in the executor) to avoid halting the entire batch. The current structure correctly aligns with the retrieved learnings about handling partial failures in GraphQL batch operations.

web/src/hooks/queries/useAllCasesQuery.ts (2)

5-5: Importing STALE_TIME.
This import introduces a standardized freshness period, boosting consistency across query hooks and reducing repeated literal values for caching.


24-24: Setting staleTime in query options.
Defining staleTime reduces unnecessary refetches and improves performance for data unlikely to change rapidly. Confirm that users can tolerate slightly stale data within this interval.

web/src/hooks/queries/useJurorStakeDetailsQuery.ts (2)

3-3: Consolidated imports for query intervals.
Bringing REFETCH_INTERVAL and STALE_TIME together ensures consistent definitions for revalidation and staleness across multiple queries.


32-32: Adoption of staleTime in juror stake details query.
Adding staleTime can efficiently reduce repeated fetches for frequently accessed data like stake details. Verify that it accurately reflects how fresh users expect this data to be.

web/src/hooks/queries/useVotingHistory.ts (1)

3-3: LGTM! Consider data freshness requirements.

The addition of staleTime configuration is correct. However, since this query handles voting history which is critical for decision-making, ensure that the stale time value aligns with your data freshness requirements.

Also applies to: 62-62

web/src/hooks/queries/useClassicAppealQuery.ts (1)

3-3: Verify fee data consistency with stale time.

While the staleTime addition is correct, be cautious with caching appeal fee data. Consider implementing optimistic updates or reducing stale time specifically for this query to prevent potential race conditions during fee updates.

Also applies to: 47-47

web/package.json (1)

81-82: Verify the new GraphQL dependencies.

The addition of GraphQL batching tools suggests an optimization for GraphQL queries. Let's verify the versions and usage:

Consider documenting the batching strategy in the README to help other developers understand:

  • The rationale behind adding these dependencies
  • How they improve query performance
  • Best practices for using them effectively
✅ Verification successful

GraphQL dependencies are properly implemented and secure

The verification shows:

  • Both packages are used correctly in GraphqlBatcher.tsx with proper batching implementation
  • Latest stable versions are being used
  • No security concerns found in the implementation or reported vulnerabilities
  • Good integration with proper error handling and type safety
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check versions, security advisories, and usage of new GraphQL dependencies

# Check latest versions and download counts
echo "Checking latest versions and popularity..."
npm view @graphql-tools/batch-execute versions --json
npm view @graphql-tools/utils versions --json
npm view @graphql-tools/batch-execute downloads
npm view @graphql-tools/utils downloads

# Check for security advisories
echo "Checking for security advisories..."
npm audit @graphql-tools/[email protected]
npm audit @graphql-tools/[email protected]

# Verify usage in the codebase
echo "Checking usage in the codebase..."
rg -l "@graphql-tools/batch-execute|@graphql-tools/utils" "web/src"

Length of output: 74887


Script:

#!/bin/bash

# Check for security advisories through npm registry API
echo "Checking security advisories..."
curl -s https://registry.npmjs.org/-/npm/v1/security/advisories/search?package=@graphql-tools/batch-execute
curl -s https://registry.npmjs.org/-/npm/v1/security/advisories/search?package=@graphql-tools/utils

# Verify usage patterns with proper output capture
echo -e "\nChecking usage patterns..."
rg -l '@graphql-tools/batch-execute|@graphql-tools/utils' web/src/

Length of output: 643


Script:

#!/bin/bash

# Check implementation details
echo "Checking implementation in GraphqlBatcher.tsx..."
cat web/src/context/GraphqlBatcher.tsx

# Check GitHub advisories
echo -e "\nChecking GitHub security advisories..."
gh api \
  -H "Accept: application/vnd.github+json" \
  -H "X-GitHub-Api-Version: 2022-11-28" \
  /repos/ardatan/graphql-tools/security/advisories

Length of output: 3042

Copy link

@jaybuidl jaybuidl merged commit be6d5f9 into master Jan 27, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants