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. |
|
Caution Review failedThe pull request is closed. WalkthroughThe changes update the Changes
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
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🪧 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.
✅ Deploy Preview for kleros-v2-testnet-devtools ready!
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.
|
Code Climate has analyzed commit 411b384 and detected 0 issues on this pull request. View more on Code Climate. |
jaybuidl
dismissed stale reviews from kemuru, coderabbitai[bot], and themself
via
367549c
|
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 updates the version of the
@kleros/kleros-v2-subgraphpackage and modifies the logic in theKlerosCore.tsfile to correctly manage dispute rounds by ensuring the previous round is marked as inactive and renaming variables for clarity.Detailed summary
versioninsubgraph/package.jsonfrom0.15.3to0.15.4.subgraph/core/src/KlerosCore.ts.isCurrentRoundtofalseand saving it.roundIDtonewRoundIDfor clarity.dispute.currentRoundto usenewRoundID.Summary by CodeRabbit
Bug Fixes
Chores