Skip to content

Fix: Update isCurrentRound for previous round on appeal #2007

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

Merged
merged 4 commits into from
May 28, 2025

Conversation

alcercu
Copy link
Contributor

@alcercu alcercu commented May 22, 2025

The isCurrentRound field in the Round entity was not being set to false for the previous round when a new round was created due to an appeal.

This commit modifies the handleAppealDecision handler in KlerosCore.ts to:

  1. Load the round that was current before the appeal.
  2. Set its isCurrentRound field to false and save it.
  3. Proceed to create the new round, which will have isCurrentRound set to true by the createRoundFromRoundInfo function.

A new test suite, kleros-core.test.ts, was added with a specific test case to verify this behavior. The test ensures that after an appeal:

  • The previous round's isCurrentRound is false.
  • The new round's isCurrentRound is true.
  • The dispute's currentRound and currentRoundIndex are correctly updated.
  • Court statistics are updated.

PR-Codex overview

This PR updates the version of the @kleros/kleros-v2-subgraph package and modifies the logic in the KlerosCore.ts file to correctly manage dispute rounds by ensuring the previous round is marked as inactive and renaming variables for clarity.

Detailed summary

  • Updated version in subgraph/package.json from 0.15.3 to 0.15.4.
  • Added logic to load the previous round of a dispute in subgraph/core/src/KlerosCore.ts.
  • Marked the previous round as inactive by setting isCurrentRound to false and saving it.
  • Renamed roundID to newRoundID for clarity.
  • Updated dispute.currentRound to use newRoundID.

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

Summary by CodeRabbit

  • Bug Fixes

    • Improved dispute round management to ensure only one round is marked as current at any time during appeals.
  • Chores

    • Updated package version to 0.15.4.

The isCurrentRound field in the Round entity was not being set to false for the previous round when a new round was created due to an appeal.

This commit modifies the handleAppealDecision handler in KlerosCore.ts to:
1. Load the round that was current before the appeal.
2. Set its isCurrentRound field to false and save it.
3. Proceed to create the new round, which will have isCurrentRound set to true by the createRoundFromRoundInfo function.

A new test suite, kleros-core.test.ts, was added with a specific test case to verify this behavior. The test ensures that after an appeal:
- The previous round's isCurrentRound is false.
- The new round's isCurrentRound is true.
- The dispute's currentRound and currentRoundIndex are correctly updated.
- Court statistics are updated.
@alcercu alcercu requested a review from a team as a code owner May 22, 2025 09:19
Copy link

netlify bot commented May 22, 2025

Deploy Preview for kleros-v2-testnet ready!

Name Link
🔨 Latest commit 367549c
🔍 Latest deploy log https://app.netlify.com/projects/kleros-v2-testnet/deploys/6836e166cc59150008e87a7c
😎 Deploy Preview https://deploy-preview-2007--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 project configuration.

Copy link

netlify bot commented May 22, 2025

Deploy Preview for kleros-v2-university failed. Why did it fail? →

Name Link
🔨 Latest commit 367549c
🔍 Latest deploy log https://app.netlify.com/projects/kleros-v2-university/deploys/6836e167a1b94e00070c1ebd

Copy link

netlify bot commented May 22, 2025

Deploy Preview for kleros-v2-neo ready!

Name Link
🔨 Latest commit 367549c
🔍 Latest deploy log https://app.netlify.com/projects/kleros-v2-neo/deploys/6836e16657fbe90008308032
😎 Deploy Preview https://deploy-preview-2007--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 project configuration.

Copy link
Contributor

coderabbitai bot commented May 22, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

The changes update the handleAppealDecision function to explicitly manage the isCurrentRound flag for dispute rounds, ensuring only one round is current at a time. Additionally, the package version is incremented from 0.15.3 to 0.15.4, with no other modifications to package metadata.

Changes

File(s) Change Summary
subgraph/core/src/KlerosCore.ts Refined round state management in handleAppealDecision; ensures only one round is current.
subgraph/package.json Updated package version from 0.15.3 to 0.15.4.

Sequence Diagram(s)

sequenceDiagram
    participant AppealDecisionEvent
    participant KlerosCore
    participant Dispute
    participant Round

    AppealDecisionEvent->>KlerosCore: handleAppealDecision(event)
    KlerosCore->>Dispute: Load current dispute
    KlerosCore->>Round: Load previous round (currentRound)
    alt Previous round exists
        KlerosCore->>Round: Set isCurrentRound = false
        KlerosCore->>Round: Save previous round
    end
    KlerosCore->>Dispute: Update currentRound to newRoundID
    KlerosCore->>Dispute: Update currentRoundIndex
    KlerosCore->>Dispute: Save dispute
    KlerosCore->>Round: Fetch new round info, create new round entity
Loading

Suggested labels

Type: Bug :bug:, Package: Web

Suggested reviewers

  • kemuru

Poem

A round of applause for rounds made neat,
Only one now claims the current seat!
With version bumped, the code refined,
Disputes and rounds are well-aligned.
🐇✨
Hopping forward, bugs outshined!


📜 Recent review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 29ef9a1 and 367549c.

📒 Files selected for processing (1)
  • subgraph/package.json (1 hunks)

🪧 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.
    • Explain this complex logic.
    • 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 explain this code block.
    • @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 explain its main purpose.
    • @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.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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 generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this 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
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: 1

🧹 Nitpick comments (2)
subgraph/core/tests/kleros-core-utils.ts (1)

11-11: Consider using a more descriptive parameter name

The static analysis warning about shadowing the global "BigInt" property is valid. While this won't cause functional issues, it's good practice to use more descriptive parameter names.

- transactionHash: string = "0x0000000000000000000000000000000000000000000000000000000000000000" // Default transaction hash
+ txHash: string = "0x0000000000000000000000000000000000000000000000000000000000000000" // Default transaction hash
subgraph/core/tests/kleros-core.test.ts (1)

11-11: Consider using a more descriptive import name

Similar to the utility file, there's a static analysis warning about shadowing the global "BigInt" property. Consider using a more descriptive import name.

- import { Address, BigInt, ethereum } from "@graphprotocol/graph-ts";
+ import { Address, BigInt as GraphBigInt, ethereum } from "@graphprotocol/graph-ts";

Then update all references to BigInt in the file.

🧰 Tools
🪛 Biome (1.9.4)

[error] 11-11: Do not shadow the global "BigInt" property.

Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.

(lint/suspicious/noShadowRestrictedNames)

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 1ca9066 and 29ef9a1.

📒 Files selected for processing (3)
  • subgraph/core/src/KlerosCore.ts (1 hunks)
  • subgraph/core/tests/kleros-core-utils.ts (1 hunks)
  • subgraph/core/tests/kleros-core.test.ts (1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
subgraph/core/tests/kleros-core-utils.ts

[error] 2-2: Do not shadow the global "BigInt" property.

Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.

(lint/suspicious/noShadowRestrictedNames)

subgraph/core/tests/kleros-core.test.ts

[error] 11-11: Do not shadow the global "BigInt" property.

Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.

(lint/suspicious/noShadowRestrictedNames)

⏰ Context from checks skipped due to timeout of 90000ms (16)
  • GitHub Check: SonarCloud
  • GitHub Check: contracts-testing
  • GitHub Check: Analyze (javascript)
  • GitHub Check: Redirect rules - kleros-v2-neo
  • GitHub Check: Redirect rules - kleros-v2-university
  • GitHub Check: Redirect rules - kleros-v2-testnet
  • GitHub Check: Header rules - kleros-v2-neo
  • GitHub Check: Header rules - kleros-v2-university
  • GitHub Check: Redirect rules - kleros-v2-testnet
  • GitHub Check: Header rules - kleros-v2-testnet
  • GitHub Check: Pages changed - kleros-v2-neo
  • GitHub Check: Pages changed - kleros-v2-university
  • GitHub Check: Header rules - kleros-v2-testnet
  • GitHub Check: Pages changed - kleros-v2-testnet
  • GitHub Check: Pages changed - kleros-v2-testnet
  • GitHub Check: Mend Security Check
🔇 Additional comments (8)
subgraph/core/tests/kleros-core-utils.ts (1)

5-30: Function implementation looks good!

This utility function properly creates a mock AppealDecision event with all the necessary parameters for testing. The default transaction hash parameter is a nice touch for test flexibility.

subgraph/core/src/KlerosCore.ts (3)

219-225: Great fix for the round state management!

This change correctly addresses the issue where previous rounds weren't being marked as non-current when appeals occurred. By explicitly loading the previous round and setting its isCurrentRound flag to false, you ensure only one round is marked as current at any given time.


228-230: Improved variable naming for better clarity

The change from roundID to newRoundID clarifies that this is the ID for the newly created round. This makes the code more readable and maintainable.


213-244:

✅ Verification successful

Verify all code paths handle round state correctly

While the current fix addresses the immediate issue, it would be good to ensure all code paths where rounds are created maintain consistent state. Let's verify that other handlers that create new rounds also manage the isCurrentRound flag properly.


🏁 Script executed:

#!/bin/bash
# Find all places where rounds are created or where isCurrentRound might be set

# Look for createRoundFromRoundInfo calls
echo "Checking all places where rounds are created:"
rg -A 3 -B 3 "createRoundFromRoundInfo" ./subgraph/core/src/

# Look for places where isCurrentRound is set
echo "Checking all places where isCurrentRound is set:"
rg "isCurrentRound" ./subgraph/core/src/

Length of output: 2541


isCurrentRound flag handling verified across all round creation paths

I’ve confirmed that:

  • The only two uses of createRoundFromRoundInfo are in the initial dispute handler and in handleAppealDecision.
  • In the initial handler, there’s no prior round to clear, and createRoundFromRoundInfo sets isCurrentRound = true.
  • In handleAppealDecision, the previous round’s isCurrentRound is explicitly set to false before creating the new round with isCurrentRound = true.
  • No other code paths or handlers invoke createRoundFromRoundInfo.

All round-creation paths correctly manage the isCurrentRound flag. No further changes are needed.

subgraph/core/tests/kleros-core.test.ts (4)

24-118: Well-structured test setup

The test setup is thorough and creates all the necessary entities with appropriate values. The initialization of Court, Dispute, and Round entities with realistic values provides a good foundation for testing the appeal decision handling.


120-163: Comprehensive contract mocking

The mocking of contract calls is well-implemented and covers all the necessary interactions for the handleAppealDecision function. The detailed comments explaining the structure of the return values make the test more maintainable.


169-224: Thorough test assertions

The test assertions comprehensively verify that the handleAppealDecision function correctly updates all relevant entities: the previous round's isCurrentRound flag is set to false, the dispute's current round and index are updated, and a new round is created with the correct attributes.


222-223: Good use of commented debugging tool

Keeping the logStore() call commented out is a good practice. It's available when needed for debugging but doesn't clutter the test output during normal runs.

Copy link

netlify bot commented May 22, 2025

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

Name Link
🔨 Latest commit 367549c
🔍 Latest deploy log https://app.netlify.com/projects/kleros-v2-testnet-devtools/deploys/6836e1664948c6000811eb15
😎 Deploy Preview https://deploy-preview-2007--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 project configuration.

The isCurrentRound field in the Round entity was not being set to false for the previous round when a new round was created due to an appeal.

This commit modifies the handleAppealDecision handler in KlerosCore.ts to:
1. Load the round that was current before the appeal.
2. Set its isCurrentRound field to false and save it.
3. Proceed to create the new round, which will have isCurrentRound set to true by the createRoundFromRoundInfo function.

Newly added test files (kleros-core.test.ts and kleros-core-utils.ts) were removed from this commit due to feedback regarding outdated testing tooling. The core logic fix remains.
@tractorss tractorss linked an issue May 28, 2025 that may be closed by this pull request
coderabbitai[bot]
coderabbitai bot previously approved these changes May 28, 2025
jaybuidl
jaybuidl previously approved these changes May 28, 2025
kemuru
kemuru previously approved these changes May 28, 2025
Copy link

codeclimate bot commented May 28, 2025

Code Climate has analyzed commit 411b384 and detected 0 issues on this pull request.

View more on Code Climate.

@jaybuidl jaybuidl dismissed stale reviews from kemuru, coderabbitai[bot], and themself via 367549c May 28, 2025 10:11
@jaybuidl jaybuidl merged commit 30382a0 into dev May 28, 2025
5 of 7 checks passed
@jaybuidl jaybuidl deleted the fix/update-iscurrentround branch May 28, 2025 10:12
Copy link

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.

isCurrentRound is never set to false for previous Rounds in subgraph
3 participants