Fix/use answer id in voting #1839
Conversation
WalkthroughThis pull request introduces comprehensive changes across multiple components of the Kleros SDK and web application, focusing on enhancing the dispute resolution process. The modifications primarily involve restructuring how answers are handled, introducing a new Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (16)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
❌ Deploy Preview for kleros-v2-testnet-devtools failed. Why did it fail? →
|
❌ Deploy Preview for kleros-v2-neo failed. Why did it fail? →
|
❌ Deploy Preview for kleros-v2-university failed. Why did it fail? →
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
web/src/pages/Cases/CaseDetails/Appeal/Classic/StageExplainer.tsx (1)
69-70: Fix text formatting and array rendering issues.
- Grammar: Remove the space before the colon
- The map function might render null values in the array, which could lead to unwanted commas
Consider this improvement:
- Following choices were funded in the stage 1 :{" "} - <small>{options?.map((option) => (option?.funded ? option.title : null))}</small> + Following choices were funded in stage 1:{" "} + <small> + {options + ?.filter((option) => option?.funded) + .map((option) => option.title) + .join(", ")} + </small>web/src/pages/Cases/CaseDetails/Appeal/Classic/index.tsx (1)
50-50: Improve type safety for amount conversion.The current type assertion could be unsafe. Consider adding runtime validation:
- <Fund amount={amount as `${number}`} setAmount={setAmount} setIsOpen={setIsPopupOpen} /> + <Fund + amount={/^\d+$/.test(amount) ? (amount as `${number}`) : "0"} + setAmount={setAmount} + setIsOpen={setIsPopupOpen} + />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
kleros-sdk/src/dataMappings/utils/disputeDetailsSchema.ts(1 hunks)kleros-sdk/src/dataMappings/utils/populateTemplate.ts(2 hunks)kleros-sdk/src/utils/getDispute.ts(0 hunks)subgraph/core/schema.graphql(2 hunks)subgraph/core/src/DisputeKitClassic.ts(2 hunks)subgraph/core/src/entities/ClassicRound.ts(4 hunks)web/src/components/Verdict/DisputeTimeline.tsx(2 hunks)web/src/hooks/queries/useClassicAppealQuery.ts(1 hunks)web/src/hooks/useClassicAppealContext.tsx(5 hunks)web/src/pages/Cases/CaseDetails/Appeal/AppealHistory.tsx(2 hunks)web/src/pages/Cases/CaseDetails/Appeal/Classic/Fund.tsx(1 hunks)web/src/pages/Cases/CaseDetails/Appeal/Classic/Options/StageOne.tsx(2 hunks)web/src/pages/Cases/CaseDetails/Appeal/Classic/Options/StageTwo.tsx(2 hunks)web/src/pages/Cases/CaseDetails/Appeal/Classic/StageExplainer.tsx(2 hunks)web/src/pages/Cases/CaseDetails/Appeal/Classic/index.tsx(3 hunks)web/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsx(2 hunks)web/src/pages/Cases/CaseDetails/Voting/Classic/OptionsContainer.tsx(4 hunks)web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsx(5 hunks)web/src/pages/Cases/CaseDetails/Voting/VotesDetails/AccordionTitle.tsx(1 hunks)web/src/pages/Cases/CaseDetails/Voting/VotesDetails/index.tsx(1 hunks)web/src/utils/getVoteChoice.ts(1 hunks)
💤 Files with no reviewable changes (1)
- kleros-sdk/src/utils/getDispute.ts
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
- GitHub Check: dependency-review
🔇 Additional comments (25)
web/src/pages/Cases/CaseDetails/Voting/VotesDetails/index.tsx (2)
Line range hint
141-196: Enhance test coverage for voting scenarios.Given this component's critical role in displaying vote details and the PR's focus on answer IDs, ensure comprehensive test coverage for:
- Hidden votes handling
- Committed votes display
- Different voting periods
- Various answer ID formats
- Edge cases (undefined choices, missing justifications)
Let's check the existing test coverage:
Would you like me to help create additional test cases to cover these scenarios?
117-117: Verify type safety and error handling for the choice parameter.The change from
parseInt(choice)to passingchoicedirectly aligns with using answer IDs. However, we should ensure proper type safety and error handling.Let's verify the implementation of
getVoteChoiceand its usage:Additionally, consider adding type guards or error handling:
{!isUndefined(choice) && typeof choice === 'string' && ( <VotedText dir="auto">{getVoteChoice(choice, answers)}</VotedText> )}web/src/utils/getVoteChoice.ts (1)
4-9: Function correctly updated to handle vote IDs as strings using BigIntThe
getVoteChoicefunction now accepts thevoteparameter as a string and usesBigIntfor comparison withanswer.id. This ensures accurate matching when dealing with hexadecimal IDs. The implementation is correct, and the use of optional chaining and null checking is appropriate.subgraph/core/src/entities/ClassicRound.ts (3)
27-38: Effective encapsulation withensureAnswerfunctionThe introduction of the
ensureAnswerfunction enhances code reuse and readability by encapsulating the creation and retrieval ofAnswerentities. This approach simplifies answer management withinClassicRoundand promotes maintainability.
Line range hint
43-64: Refactored logic improves clarity and consistencyBy utilizing the
ensureAnswerfunction inupdateCountsAndGetCurrentRuling, the code reduces duplication and ensures consistent handling ofAnswerentities. This refactoring enhances the clarity of the count update logic.
80-83: Consistent use ofensureAnswerin funding updateThe update to
updateChoiceFundingFromContributionEventappropriately leverages theensureAnswerfunction to manageAnswerentities consistently. This change improves code consistency across different functions handling answers.web/src/pages/Cases/CaseDetails/Appeal/AppealHistory.tsx (1)
41-47: Improved robustness by using option IDs instead of array indices.The changes enhance code reliability by:
- Using unique option IDs instead of array indices
- Safely handling undefined options with optional chaining
- Directly accessing option properties instead of relying on separate arrays
web/src/pages/Cases/CaseDetails/Appeal/Classic/Options/StageOne.tsx (2)
44-47: Well-structured funding determination logic.The required funding calculation is clearly tied to the option's ID, making the code more maintainable and less prone to errors.
48-54: Consistent use of option properties and ID-based comparisons.Good practices observed:
- Using unique IDs for React keys
- Direct property access for option data
- Clear boolean conditions for selection state
web/src/pages/Cases/CaseDetails/Appeal/Classic/Options/StageTwo.tsx (2)
38-38: Good use of useMemo for derived state.Memoizing the choice calculation prevents unnecessary recalculations when unrelated state changes.
41-43: Consider adding cleanup to useEffect.While the effect logic is clean, it's missing a cleanup function which might be needed if setSelectedOption or setAmount need to be cleaned up when the component unmounts.
Could you verify if these setter functions need cleanup in your component's lifecycle?
kleros-sdk/src/dataMappings/utils/disputeDetailsSchema.ts (2)
36-36: Breaking Change:idfield is now required in AnswerSchemaThe removal of
.optional()makes theidfield mandatory. This is a breaking change that might affect existing implementations.Run this script to check for potential impacts:
✅ Verification successful
The
idfield requirement aligns with existing implementationsAfter thorough verification of the codebase, all existing implementations already provide and rely on the
idfield, making this a non-breaking change that formalizes the existing usage pattern.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Answer type usage without id field rg -A 5 "Answer.*{" --type typescriptLength of output: 121
Script:
#!/bin/bash # Search for Answer type/interface definitions and usages rg -l "type Answer|interface Answer|: Answer|: Answer\[\]" --type typescript # Search for AnswerSchema usage rg -l "AnswerSchema" --type typescript # Get content of files that use Answer or AnswerSchema echo "=== Files containing Answer or AnswerSchema ===" rg "(type Answer|interface Answer|: Answer|: Answer\[\]|AnswerSchema)" -A 3 --type typescriptLength of output: 423
Script:
#!/bin/bash # Search for Answer type/interface definitions and usages rg "(type Answer|interface Answer|: Answer|: Answer\[\])" -A 5 --glob "*.{ts,tsx}" # Search for AnswerSchema usage with context echo "=== AnswerSchema Usage ===" rg "AnswerSchema" -A 5 --glob "*.{ts,tsx}" # Search for imports of Answer type echo "=== Answer Imports ===" rg "import.*Answer.*from" --glob "*.{ts,tsx}"Length of output: 11356
42-47: Verify the special case handling of RefuseToArbitrateAnswerThe introduction of
RefuseToArbitrateAnswerwithid: "0x0"needs verification to ensure consistent handling across the codebase.Run this script to check the usage:
✅ Verification successful
Special case handling of RefuseToArbitrateAnswer is properly implemented and verified
The implementation in
populateTemplate.tsensures that:
- RefuseToArbitrateAnswer with id "0x0" is always the first option
- Any custom answers with id 0 are filtered out and replaced with the standard one
- Invalid dispute data cases are handled by directing users to this option
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for RefuseToArbitrateAnswer usage rg "RefuseToArbitrateAnswer" --type typescript # Search for "0x0" handling in voting logic rg '"0x0"' --type typescriptLength of output: 147
Script:
#!/bin/bash # Search for RefuseToArbitrateAnswer usage with context rg -l "\.ts$|\.js$" | xargs rg -l "RefuseToArbitrateAnswer" # Search for hex value usage with context rg -A 3 -B 3 '(0x0|0x00)' --glob '*.{ts,js}' # Search for files importing from disputeDetailsSchema rg -A 1 -B 1 "from.*disputeDetailsSchema" --glob '*.{ts,js}'Length of output: 59602
Script:
#!/bin/bash # Get content of populateTemplate.ts cat kleros-sdk/src/dataMappings/utils/populateTemplate.ts # Search for dispute resolution or voting logic rg -A 5 -B 5 "vote|ruling|resolution" --glob '*.ts' # Search for other special answer IDs rg -A 2 -B 2 'id: "(0x[0-9a-fA-F]+)"' --glob '*.ts'Length of output: 76799
web/src/pages/Cases/CaseDetails/Voting/VotesDetails/AccordionTitle.tsx (1)
76-76: LGTM: Removed unnecessary parseInt for choice parameterThe removal of
parseIntaligns with the transition to using hex IDs for answers. The change maintains type consistency with the updatedgetVoteChoicefunction.web/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsx (2)
47-47: Type safety improvement: Using bigint for choice parameterThe transition to
bigintimproves type safety and aligns with using hex IDs. The salt storage format has been updated accordingly.Also applies to: 59-59
64-67: Improved error handling with publicClient checkThe additional check for
publicClientenhances safety by ensuring both clients are available before proceeding with the contract write operation.web/src/pages/Cases/CaseDetails/Voting/Classic/OptionsContainer.tsx (2)
15-15: Type safety improvement: Consistent use of bigint and SDK typesThe changes improve type safety by:
- Using the Answer type from SDK
- Converting to bigint for choice handling
- Using BigInt(-1) as default state
Also applies to: 45-45, 53-53
81-82: Verify edge case handling for Refuse to ArbitrateThe "Refuse to Arbitrate" option uses
BigInt(0), which should be verified against theRefuseToArbitrateAnswer.id("0x0") defined in the schema.Run this script to check consistency:
Also applies to: 95-96
✅ Verification successful
Edge case handling for "Refuse to Arbitrate" is correctly implemented
The usage of
BigInt(0)in the UI correctly matches theRefuseToArbitrateAnswer.id("0x0") defined in the schema. The conversion between hex string and BigInt is consistent with blockchain number handling practices.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential mismatches between BigInt(0) and "0x0" rg -A 5 'BigInt\(0\)' --type typescript # Check RefuseToArbitrateAnswer.id usage ast-grep --pattern 'RefuseToArbitrateAnswer.id'Length of output: 124
Script:
#!/bin/bash # Search for BigInt(0) usage in TypeScript files rg -g '*.{ts,tsx}' 'BigInt\(0\)' -B 2 -A 2 # Search for "0x0" usage rg -g '*.{ts,tsx}' '"0x0"' -B 2 -A 2 # Search for RefuseToArbitrateAnswer imports and usage rg -g '*.{ts,tsx}' 'RefuseToArbitrateAnswer' -B 2 -A 2 # Look for type definitions and constants ast-grep --pattern 'const RefuseToArbitrateAnswer = { $$$ }' ast-grep --pattern 'type RefuseToArbitrateAnswer = { $$$ }'Length of output: 9785
web/src/pages/Cases/CaseDetails/Appeal/Classic/Fund.tsx (1)
76-76: LGTM: Safe access to answer IDThe change correctly uses optional chaining to access the ID property of the selected option, maintaining the same fallback value for safety.
web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsx (1)
23-23: LGTM: Added Answer type importProper import of the Answer type from kleros-sdk improves type safety.
subgraph/core/src/DisputeKitClassic.ts (1)
105-114:⚠️ Potential issueVerify the fee rewards calculation
The change from using
localRound.paidFees[choice.toI32()]toanswer.paidFeerepresents a significant architectural shift. While this improves the data model, please ensure:
- All existing fee calculations are migrated correctly
- No funds are lost in the transition
Run this script to verify the fee calculations:
web/src/components/Verdict/DisputeTimeline.tsx (1)
108-108: LGTM: Improved timeline accuracyThe changes correctly:
- Use the winning choice directly for party display
- Update the appeal win condition to use the current ruling
- Display the correct party based on the dispute's current ruling
Also applies to: 127-130
subgraph/core/schema.graphql (1)
269-276: Well-structured Answer entity with clear relationships.The new Answer entity provides a robust structure for tracking individual answers with their funding status and vote counts. The relationship with ClassicRound is properly established.
web/src/hooks/queries/useClassicAppealQuery.ts (1)
28-33: Query structure properly aligned with schema changes.The query correctly retrieves all necessary fields from the new Answer entity structure, maintaining consistency with the schema changes.
web/src/hooks/useClassicAppealContext.tsx (1)
16-16: Type definition properly extends Answer entity.The Option type correctly extends the Answer type with UI-specific fields.
| const getOptions = (dispute?: DisputeDetails, classicDispute?: ClassicAppealQuery["dispute"]) => { | ||
| if (!dispute) return []; | ||
| const currentLocalRound = getCurrentLocalRound(classicDispute); | ||
| const classicAnswers = currentLocalRound?.answers; | ||
|
|
||
| const options = dispute.answers.map((answer) => { | ||
| const classicAnswer = classicAnswers?.find((classicAnswer) => BigInt(classicAnswer.answerId) == BigInt(answer.id)); | ||
| // converting hexadecimal id to stringified bigint to match id fomr subgraph | ||
| return { | ||
| ...answer, | ||
| id: BigInt(answer.id).toString(), | ||
| paidFee: classicAnswer?.paidFee ?? "0", | ||
| funded: classicAnswer?.funded ?? false, | ||
| }; | ||
| }); | ||
| return options; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for ID conversion.
The BigInt conversion of answer.id could throw if the ID is invalid. Consider adding error handling:
const options = dispute.answers.map((answer) => {
+ try {
const classicAnswer = classicAnswers?.find((classicAnswer) => BigInt(classicAnswer.answerId) == BigInt(answer.id));
return {
...answer,
id: BigInt(answer.id).toString(),
paidFee: classicAnswer?.paidFee ?? "0",
funded: classicAnswer?.funded ?? false,
};
+ } catch (error) {
+ console.error(`Failed to process answer: ${answer.id}`, error);
+ return null;
+ }
-});
+}).filter((option): option is NonNullable<typeof option> => option !== null);📝 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.
| const getOptions = (dispute?: DisputeDetails, classicDispute?: ClassicAppealQuery["dispute"]) => { | |
| if (!dispute) return []; | |
| const currentLocalRound = getCurrentLocalRound(classicDispute); | |
| const classicAnswers = currentLocalRound?.answers; | |
| const options = dispute.answers.map((answer) => { | |
| const classicAnswer = classicAnswers?.find((classicAnswer) => BigInt(classicAnswer.answerId) == BigInt(answer.id)); | |
| // converting hexadecimal id to stringified bigint to match id fomr subgraph | |
| return { | |
| ...answer, | |
| id: BigInt(answer.id).toString(), | |
| paidFee: classicAnswer?.paidFee ?? "0", | |
| funded: classicAnswer?.funded ?? false, | |
| }; | |
| }); | |
| return options; | |
| const getOptions = (dispute?: DisputeDetails, classicDispute?: ClassicAppealQuery["dispute"]) => { | |
| if (!dispute) return []; | |
| const currentLocalRound = getCurrentLocalRound(classicDispute); | |
| const classicAnswers = currentLocalRound?.answers; | |
| const options = dispute.answers.map((answer) => { | |
| try { | |
| const classicAnswer = classicAnswers?.find((classicAnswer) => BigInt(classicAnswer.answerId) == BigInt(answer.id)); | |
| // converting hexadecimal id to stringified bigint to match id fomr subgraph | |
| return { | |
| ...answer, | |
| id: BigInt(answer.id).toString(), | |
| paidFee: classicAnswer?.paidFee ?? "0", | |
| funded: classicAnswer?.funded ?? false, | |
| }; | |
| } catch (error) { | |
| console.error(`Failed to process answer: ${answer.id}`, error); | |
| return null; | |
| } | |
| }).filter((option): option is NonNullable<typeof option> => option !== null); | |
| return options; |
|
Code Climate has analyzed commit 86b02a7 and detected 4 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
|
requires subgraph redeploy, currently using my personal studio endpoint https://api.studio.thegraph.com/query/71663/core-dev/version/latest
requires kleros-sdk redeploy
Please pay close attention to
PR-Codex overview
This PR focuses on enhancing the handling of answers in the classic appeal system, introducing a new
Answerentity, and updating various components to work with this structure. It also refines vote handling and improves the overall data flow in the application.Detailed summary
Answerentity with properties:id,answerId,count,paidFee,funded.useClassicAppealQueryto includeanswers.stringinstead ofnumber.Answerstructure.populateTemplateto include a standard "Refuse to Arbitrate" option.Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Technical Changes