Fix: Update isCurrentRound for previous round on appeal #2007
Conversation
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.
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
❌ Deploy Preview for kleros-v2-university failed. Why did it fail? →
|
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThe changes introduce explicit state management for dispute rounds in the KlerosCore subgraph, ensuring only one round is marked as current at a time. A new test utility for creating mock AppealDecision events is added, and comprehensive tests are implemented to verify correct round updates and entity state after handling an appeal decision. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant Handler as handleAppealDecision
participant Store as Subgraph Store
participant Contract as KlerosCore Contract
Test->>Handler: Trigger with mock AppealDecision event
Handler->>Store: Load Dispute and previous Round
Handler->>Store: Set previous Round isCurrentRound = false
Handler->>Contract: Fetch new round info (getRoundInfo)
Handler->>Store: Create new Round entity (isCurrentRound = true)
Handler->>Store: Update Dispute's currentRound and currentRoundIndex
Handler->>Store: Update Court's numberVotes
Handler-->>Test: Test asserts entity states
Suggested labels
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
subgraph/core/tests/kleros-core-utils.ts (1)
11-11: Consider using a more descriptive parameter nameThe 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 hashsubgraph/core/tests/kleros-core.test.ts (1)
11-11: Consider using a more descriptive import nameSimilar 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
BigIntin 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
📒 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
isCurrentRoundflag to false, you ensure only one round is marked as current at any given time.
228-230: Improved variable naming for better clarityThe change from
roundIDtonewRoundIDclarifies 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
isCurrentRoundflag 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
createRoundFromRoundInfoare in the initial dispute handler and inhandleAppealDecision.- In the initial handler, there’s no prior round to clear, and
createRoundFromRoundInfosetsisCurrentRound = true.- In
handleAppealDecision, the previous round’sisCurrentRoundis explicitly set tofalsebefore creating the new round withisCurrentRound = true.- No other code paths or handlers invoke
createRoundFromRoundInfo.All round-creation paths correctly manage the
isCurrentRoundflag. No further changes are needed.subgraph/core/tests/kleros-core.test.ts (4)
24-118: Well-structured test setupThe 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 mockingThe mocking of contract calls is well-implemented and covers all the necessary interactions for the
handleAppealDecisionfunction. The detailed comments explaining the structure of the return values make the test more maintainable.
169-224: Thorough test assertionsThe test assertions comprehensively verify that the
handleAppealDecisionfunction correctly updates all relevant entities: the previous round'sisCurrentRoundflag 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 toolKeeping 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.
| appealDecisionEvent.block.number = blockNumber; | ||
| appealDecisionEvent.block.timestamp = timestamp; | ||
| appealDecisionEvent.logIndex = logIndex; | ||
| appealDecisionEvent.transaction.hash = Address.fromString(transactionHash); |
There was a problem hiding this comment.
Fix the transaction hash type conversion
The transaction hash is incorrectly converted to an Address type. Transaction hashes should be Bytes values, not Addresses.
- appealDecisionEvent.transaction.hash = Address.fromString(transactionHash);
+ appealDecisionEvent.transaction.hash = ethereum.Value.fromString(transactionHash).toBytes();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| appealDecisionEvent.transaction.hash = Address.fromString(transactionHash); | |
| appealDecisionEvent.transaction.hash = ethereum.Value.fromString(transactionHash).toBytes(); |
🤖 Prompt for AI Agents
In subgraph/core/tests/kleros-core-utils.ts at line 19, the transaction hash is
incorrectly converted to an Address type. Change the conversion to use
Bytes.fromHexString or the appropriate Bytes constructor instead of
Address.fromString to correctly represent the transaction hash as a Bytes value.
❌ Deploy Preview for kleros-v2-testnet-devtools failed. Why did it fail? →
|
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.
|
Code Climate has analyzed commit 3925b6f and detected 0 issues on this pull request. View more on Code Climate. |
|
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:
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:
PR-Codex overview
This PR focuses on updating the handling of rounds in the
KlerosCorecontract. It adds functionality to manage the previous round when a new round is created, ensuring that the previous round is marked as not current.Detailed summary
dispute.currentRound.previousRound.isCurrentRoundtofalseif the previous round exists and saved the changes.roundIDtonewRoundIDfor clarity.dispute.currentRoundto usenewRoundID.Summary by CodeRabbit
Bug Fixes
Tests