New dispute kit gated for Argentinian consumer protection lawyers #2189
Conversation
For accredited Argentinian consumer protection lawyers
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ 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-testnet-devtools failed. Why did it fail? →
|
| Caution Review failedThe pull request is closed. WalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram participant KC as KlerosCore / KlerosCoreUniversity participant DK as IDisputeKit / DisputeKitClassicBase participant DKG as DisputeKitGatedArgentinaConsumerProtection rect rgb(230,240,255) Note over KC,DK: Core passes round.nbVotes into draw KC->>DK: draw(_disputeID, startIndex, round.nbVotes) end rect rgb(245,250,245) Note over DK: Base draw selects juror and runs _postDrawCheck(..., _roundNbVotes) DK->>DK: select juror -> _postDrawCheck(..., _roundNbVotes) DK-->>KC: (drawnAddress, fromSubcourtID) end rect rgb(245,235,245) Note over DKG: Gated DK records consumer-protection draws KC->>DKG: draw(..., _roundNbVotes) DKG->>DKG: super.draw(...) => drawnAddress alt drawn holds consumer-protection token DKG->>DKG: drawnConsumerProtectionLawyer[localDispute][localRound] = true else DKG->>DKG: may trigger re-draw or clear marker if round complete end DKG-->>KC: (drawnAddress, fromSubcourtID) end Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/src/arbitration/university/KlerosCoreUniversity.sol (1)
1061-1063: Guard against division by zero in convertEthToTokenAmount.If a token is marked accepted but rateInEth==0, this will revert. Validate rate configured (non‑zero) before use or revert early.
function convertEthToTokenAmount(IERC20 _toToken, uint256 _amountInEth) public view returns (uint256) { - return (_amountInEth * 10 ** currencyRates[_toToken].rateDecimals) / currencyRates[_toToken].rateInEth; + uint64 rate = currencyRates[_toToken].rateInEth; + if (rate == 0) revert TokenNotAccepted(); // or dedicated error + return (_amountInEth * 10 ** currencyRates[_toToken].rateDecimals) / rate; }
🧹 Nitpick comments (2)
contracts/src/arbitration/university/KlerosCoreUniversity.sol (1)
812-817: Avoid raw send; use a safe send helper consistently.send(...) ignores failures; prefer SafeSend (as in KlerosCore) to prevent silent fund loss.
- payable(owner).send(round.totalFeesForJurors); + // use SafeSend.safeSend with wNative analogue or SafeERC20 for ERC20 path onlyAlso applies to: 868-873, 900-905
contracts/src/arbitration/dispute-kits/DisputeKitGatedArgentinaConsumerProtection.sol (1)
99-117: Slight gas/readability: cache round.Avoid repeated dispute.rounds[localRoundID] lookups; store Round storage round = dispute.rounds[localRoundID].
- uint256 localRoundID = dispute.rounds.length - 1; + uint256 localRoundID = dispute.rounds.length - 1; + Round storage round = dispute.rounds[localRoundID]; @@ - if ( - dispute.rounds[localRoundID].votes.length == _roundNbVotes && + if ( + round.votes.length == _roundNbVotes && !drawnConsumerProtectionLawyer[localDisputeID][localRoundID] ) { @@ - dispute.rounds[localRoundID].votes.pop(); + round.votes.pop();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
contracts/src/arbitration/KlerosCore.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGatedArgentinaConsumerProtection.sol(1 hunks)contracts/src/arbitration/interfaces/IDisputeKit.sol(1 hunks)contracts/src/arbitration/university/KlerosCoreUniversity.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- 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: Redirect rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
🔇 Additional comments (4)
contracts/src/arbitration/KlerosCore.sol (1)
756-760: Core passes nbVotes to DK.draw — OK.Matches the new interface; loop semantics unchanged; continue on zero address still correct.
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
254-263: Base DK draw signature update — OK.Visibility/public override and unused param pattern are correct; maintains behavior.
contracts/src/arbitration/interfaces/IDisputeKit.sol (1)
53-60: Verified — all DisputeKit implementations updated to 3-arg signature.All DisputeKit implementations now use the new signature with
_roundNbVotes:
- DisputeKitGatedArgentinaConsumerProtection uses all three parameters
- DisputeKitClassicBase accepts the parameter (marked unused with comment, valid pattern)
- No legacy 2-arg implementations remain in codebase
contracts/src/arbitration/university/KlerosCoreUniversity.sol (1)
605-605: All DK implementations verified to use the correct 3‑arg draw signature.The interface
IDisputeKit.draw()expects(uint256 _coreDisputeID, uint256 _nonce, uint256 _roundNbVotes). BothDisputeKitClassicBaseandDisputeKitGatedArgentinaConsumerProtectionimplement this correctly with 3 parameters. Other dispute kits inherit fromDisputeKitClassicBaseand thus maintain the same signature. The call at line 605 in KlerosCoreUniversity passes 3 arguments as required.
contracts/src/arbitration/dispute-kits/DisputeKitGatedArgentinaConsumerProtection.sol Show resolved Hide resolved
contracts/src/arbitration/dispute-kits/DisputeKitGatedArgentinaConsumerProtection.sol Show resolved Hide resolved
contracts/src/arbitration/dispute-kits/DisputeKitGatedArgentinaConsumerProtection.sol Outdated Show resolved Hide resolved
contracts/src/arbitration/dispute-kits/DisputeKitGatedArgentinaConsumerProtection.sol Show resolved Hide resolved
contracts/src/arbitration/dispute-kits/DisputeKitGatedArgentinaConsumerProtection.sol Show resolved Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contracts/test/foundry/DisputeKitGatedArgentinaConsumerProtection_Drawing.t.sol (1)
154-168: Consider removing debug console.log statements.The test includes several
console.logstatements that appear to be debugging artifacts. These should be removed before merging to production to keep test output clean.- // Debug: Log the values being encoded - console.log("argentinaCourt:", argentinaCourt); - console.log("DEFAULT_NB_OF_JURORS:", DEFAULT_NB_OF_JURORS); - console.log("ARGENTINA_DK_ID:", ARGENTINA_DK_ID); - // Verify the arbitration cost is correct - use uint256 for all values bytes memory extraData = abi.encodePacked( uint256(argentinaCourt), uint256(DEFAULT_NB_OF_JURORS), uint256(ARGENTINA_DK_ID) ); - console.log("extraData length:", extraData.length); uint256 cost = core.arbitrationCost(extraData); - console.log("Actual cost:", cost); - console.log("Expected cost:", 0.03 ether * DEFAULT_NB_OF_JURORS); assertEq(cost, 0.03 ether * DEFAULT_NB_OF_JURORS, "Wrong arbitration cost");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/test/foundry/DisputeKitGatedArgentinaConsumerProtection_Drawing.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Appeals.t.sol(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- 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: hardhat-tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (12)
contracts/test/foundry/KlerosCore_Appeals.t.sol (4)
345-345: LGTM! Correctly updated test to match new draw signature.The test properly verifies that a dispute kit rejects draw calls after the dispute has jumped to another kit, now using the updated 3-parameter signature.
494-494: LGTM! Test update is consistent with new draw signature.
718-718: LGTM! Correctly updated for recurring DK jump test.
797-797: LGTM! Test updates complete and consistent.All draw calls in the appeals test suite have been correctly updated to use the new 3-parameter signature with
_roundNbVotes.contracts/test/foundry/DisputeKitGatedArgentinaConsumerProtection_Drawing.t.sol (8)
47-130: LGTM! Well-structured test setup.The setUp function comprehensively covers all necessary test scenarios:
- Six test accounts representing different token ownership combinations
- Proper dispute kit and court initialization
- Correct token deployment and configuration
- Adequate PNK distribution for staking
174-206: LGTM! Test correctly validates rejection of draws without consumer protection lawyers.The test properly verifies that when only regular accredited lawyers (without consumer protection tokens) are staked, the drawing process cannot complete because it cannot satisfy the requirement of having at least one consumer protection lawyer per round.
209-244: LGTM! Test correctly validates successful drawing with consumer protection lawyers.
247-285: LGTM! Test correctly validates mixed lawyer scenario.The test appropriately uses extra iterations (
* 5) to handle potential retries when drawing from a mixed pool of regular and consumer protection lawyers.
288-329: LGTM! Test correctly validates jurors holding both token types.
332-363: LGTM! Test correctly validates rejection of jurors without tokens.
366-445: LGTM! Comprehensive test of multi-round consumer protection requirement.The test correctly validates that each round independently satisfies the consumer protection lawyer requirement, including proper handling of appeals and round escalation (3 jurors → 7 jurors).
448-502: LGTM! Mapping test and helper function are well-implemented.The test correctly validates the
drawnConsumerProtectionLawyermapping state transitions, and the helper function properly verifies the consumer protection lawyer requirement across multiple tests.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/src/arbitration/dispute-kits/DisputeKitGated.sol (1)
111-126: OOB/garbage read of tokenId (length check off by 32 bytes).You read at
add(_extraData, 0xA0)but only guard for< 160. Require ≥192 or readtokenIdconditionally.- // Need at least 160 bytes to safely read the parameters - if (_extraData.length < 160) return (address(0), false, 0); + // Need at least 160 bytes to read tokenGate/isERC1155; 192 if tokenId is present. + if (_extraData.length < 160) return (address(0), false, 0); assembly { // solium-disable-line security/no-inline-assembly - let packedTokenGateIsERC1155 := mload(add(_extraData, 0x80)) // 4th parameter at offset 128 - tokenId := mload(add(_extraData, 0xA0)) // 5th parameter at offset 160 (moved up) + let packedTokenGateIsERC1155 := mload(add(_extraData, 0x80)) // 4th parameter at offset 128 // Unpack address from lower 160 bits and bool from bit 160 tokenGate := and(packedTokenGateIsERC1155, 0xffffffffffffffffffffffffffffffffffffffff) isERC1155 := and(shr(160, packedTokenGateIsERC1155), 1) } + if (_extraData.length >= 192) { + assembly { tokenId := mload(add(_extraData, 0xA0)) } // 5th parameter at offset 160 + } else { + tokenId = 0; + }
♻️ Duplicate comments (3)
contracts/src/arbitration/dispute-kits/DisputeKitGatedArgentinaConsumerProtection.sol (3)
55-59: Validate token addresses on init (avoid ABI decode revert at runtime).Reject zero/EOA tokens up-front.
) external initializer { __DisputeKitClassicBase_initialize(_owner, _core, _wNative); - accreditedLawyerToken = _accreditedLawyerToken; - accreditedConsumerProtectionLawyerToken = _accreditedConsumerProtectionLawyerToken; + if (_accreditedLawyerToken == address(0) || _accreditedLawyerToken.code.length == 0) + revert TokenNotSupported(_accreditedLawyerToken); + if (_accreditedConsumerProtectionLawyerToken == address(0) || _accreditedConsumerProtectionLawyerToken.code.length == 0) + revert TokenNotSupported(_accreditedConsumerProtectionLawyerToken); + accreditedLawyerToken = _accreditedLawyerToken; + accreditedConsumerProtectionLawyerToken = _accreditedConsumerProtectionLawyerToken; }
73-83: Harden governance setters to prevent bricking via zero/EOA addresses.Add the same validation as init.
function changeAccreditedLawyerToken(address _accreditedLawyerToken) external onlyByOwner { - accreditedLawyerToken = _accreditedLawyerToken; + if (_accreditedLawyerToken == address(0) || _accreditedLawyerToken.code.length == 0) + revert TokenNotSupported(_accreditedLawyerToken); + accreditedLawyerToken = _accreditedLawyerToken; } @@ function changeAccreditedConsumerProtectionLawyerToken( address _accreditedConsumerProtectionLawyerToken ) external onlyByOwner { - accreditedConsumerProtectionLawyerToken = _accreditedConsumerProtectionLawyerToken; + if (_accreditedConsumerProtectionLawyerToken == address(0) || _accreditedConsumerProtectionLawyerToken.code.length == 0) + revert TokenNotSupported(_accreditedConsumerProtectionLawyerToken); + accreditedConsumerProtectionLawyerToken = _accreditedConsumerProtectionLawyerToken; }
112-138: Non‑terminating draw when no CP lawyers exist + unsafe token calls.
- If no CP lawyers in the pool, the “last draw must be CP” rule causes perpetual redraws. Add a bounded retry fallback.
- Also guard
balanceOfon EOAs/zero and short‑circuitsuper._postDrawCheckfirst.- ) internal view override returns (bool) { - if (IBalanceHolder(accreditedConsumerProtectionLawyerToken).balanceOf(_juror) == 0) { + ) internal view override returns (bool) { + // Short-circuit base checks first (e.g., singleDrawPerJuror) + if (!super._postDrawCheck(_round, _coreDisputeID, _juror, _roundNbVotes)) return false; + + bool isCPLawyer = ( + accreditedConsumerProtectionLawyerToken.code.length > 0 && + IBalanceHolder(accreditedConsumerProtectionLawyerToken).balanceOf(_juror) > 0 + ); + if (!isCPLawyer) { // The juror is not a consumer protection lawyer. uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; Dispute storage dispute = disputes[localDisputeID]; uint256 localRoundID = dispute.rounds.length - 1; if ( - dispute.rounds[localRoundID].votes.length == _roundNbVotes - 1 && - !drawnConsumerProtectionLawyer[localDisputeID][localRoundID] + dispute.rounds[localRoundID].votes.length == _roundNbVotes - 1 && + !drawnConsumerProtectionLawyer[localDisputeID][localRoundID] ) { - // This is the last draw iteration and we still have not drawn a consumer protection lawyer. - // Reject this draw so that another iteration can try again later. - return false; + // Last draw attempt; if we still have none, allow bounded extra attempts before accepting. + uint256 retries = extraDrawRetries[localDisputeID][localRoundID]; + if (retries < MAX_EXTRA_DRAW_RETRIES) { + unchecked { extraDrawRetries[localDisputeID][localRoundID] = retries + 1; } + return false; + } } - if (IBalanceHolder(accreditedLawyerToken).balanceOf(_juror) == 0) { + // Must at least hold the broader accredited lawyer token. + if ( + !(accreditedLawyerToken.code.length > 0 && + IBalanceHolder(accreditedLawyerToken).balanceOf(_juror) > 0) + ) { // The juror does not hold either of the tokens. return false; } } - return super._postDrawCheck(_round, _coreDisputeID, _juror, _roundNbVotes); + return true; }Add storage for retries and a sane cap:
contract DisputeKitGatedArgentinaConsumerProtection is DisputeKitClassicBase { @@ - mapping(uint256 localDisputeID => mapping(uint256 localRoundID => bool)) public drawnConsumerProtectionLawyer; // Maps the local dispute and round ID to the boolean indicating if the consumer protection lawyer was drawn. + mapping(uint256 localDisputeID => mapping(uint256 localRoundID => bool)) public drawnConsumerProtectionLawyer; // Maps the local dispute and round ID to the boolean indicating if the consumer protection lawyer was drawn. + mapping(uint256 localDisputeID => mapping(uint256 localRoundID => uint256)) private extraDrawRetries; + uint256 public constant MAX_EXTRA_DRAW_RETRIES = 64;
🧹 Nitpick comments (2)
contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol (1)
45-53: Validate POH address in initialize().Guard against zero/EOA to avoid decode issues later.
function initialize( @@ - ) external initializer { + ) external initializer { __DisputeKitClassicBase_initialize(_owner, _core, _wNative); - poh = _poh; + require(address(_poh) != address(0) && address(_poh).code.length > 0, "Invalid PoH"); + poh = _poh; singleDrawPerJuror = true; }contracts/src/arbitration/dispute-kits/DisputeKitGated.sol (1)
132-151: GuardbalanceOfagainst EOAs/zero addresses.A supported token can still be misconfigured; calling an interface function on an EOA/zero reverts on ABI decode. Short-circuit when
tokenGate.code.length == 0.- // Check juror's token balance - if (isERC1155) { + // Check juror's token balance + if (tokenGate.code.length == 0) return false; + if (isERC1155) { return IBalanceHolderERC1155(tokenGate).balanceOf(_juror, tokenId) > 0; } else { return IBalanceHolder(tokenGate).balanceOf(_juror) > 0; }Optionally also validate in governance:
function changeSupportedTokens(address[] memory _tokens, bool _supported) external onlyByOwner { for (uint256 i = 0; i < _tokens.length; i++) { - supportedTokens[_tokens[i]] = _supported; + if (_supported && _tokens[i] != NO_TOKEN_GATE) { + require(_tokens[i].code.length > 0, "Token must be a contract"); + } + supportedTokens[_tokens[i]] = _supported; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(3 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGated.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGatedArgentinaConsumerProtection.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: hardhat-tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (1)
256-278: LGTM! Signature update correctly implemented.The
_postDrawCheckfunction signature has been properly updated to accept the new_roundNbVotesparameter and forwards it to the superclass implementation. The existing token-gating validation logic remains intact and continues to function as expected.contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol (1)
69-77: Param plumbed correctly.Forwarding
_roundNbVotestosuper._postDrawCheckand AND-ing withpoh.isRegistered(_juror)looks good.contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (2)
645-680: Court jump now keyed off_currentRoundNbVotes. Verify intended behavior.Using the round’s nbVotes to decide court jump is a policy change. Ensure tests cover boundary: exactly equal to
jurorsForJumpand appeal sequences.
252-284: Draw plumbing verified: all dispute kits align on_roundNbVotesparameter.All
_postDrawCheckoverrides include the_roundNbVotesparameter (DisputeKitGated, DisputeKitGatedArgentinaConsumerProtection, DisputeKitGatedShutter, DisputeKitSybilResistant). DisputeKitClassic and DisputeKitShutter inherit from DisputeKitClassicBase without override, inheriting the base implementation. DisputeKitClassicBase.draw() correctly passes_roundNbVotesto_postDrawCheck, and DisputeKitGatedArgentinaConsumerProtection.draw() correctly forwards it in the super call. Signatures and function plumbing are consistent across all implementations.
contracts/src/arbitration/dispute-kits/DisputeKitGatedArgentinaConsumerProtection.sol Show resolved Hide resolved
|
PR-Codex overview
This PR focuses on enhancing the
drawfunctionality across variousDisputeKitimplementations in the Kleros arbitration system. It adds a new parameter for the number of votes in the drawing process and ensures that this parameter is used consistently in related checks and logic.Detailed summary
drawfunction signatures in multiple contracts to include_roundNbVotes.drawto pass the new_roundNbVotesparameter._postDrawCheckto incorporate_roundNbVotesin checks for juror eligibility.DisputeKitGatedArgentinaConsumerProtection, implementing specific logic for drawing jurors based on token ownership.Summary by CodeRabbit
New Features
Refactor
Tests