feat(KC): add foundry test file #1765
Conversation
WalkthroughThe changes in this pull request involve updates to several contracts within the Kleros arbitration framework. Key modifications include changes to data types in event parameters, refinements in staking logic, enhancements in error handling, and the introduction of mock contracts for testing. Additionally, documentation improvements clarify the conditions under which jurors are drawn and the implications of their staking status. A deprecation notice for certain constants and parameters is also included, reflecting an evolution in the contract structure and functionality. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🪛 yamllint (1.35.1).github/workflows/contracts-testing.yml[error] 45-45: trailing spaces (trailing-spaces) [error] 46-46: trailing spaces (trailing-spaces) [error] 50-50: trailing spaces (trailing-spaces) [error] 75-75: trailing spaces (trailing-spaces) 🔇 Additional comments (4).github/workflows/contracts-testing.yml (4)
The checkout configuration is properly secured with a pinned commit hash and includes recursive submodules, which is necessary for Foundry testing. Line range hint The caching configuration is well-structured with:
🧰 Tools🪛 yamllint (1.35.1)[error] 45-45: trailing spaces (trailing-spaces) [error] 46-46: trailing spaces (trailing-spaces) [error] 50-50: trailing spaces (trailing-spaces)
The installation steps look correct, but let's verify the workspace setup. ✅ Verification successfulWorkspace configuration is correctly set up The verification confirms:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify the yarn workspace configuration
# Check if package.json exists and contains the workspace configuration
cat package.json | jq '.workspaces'
# Check if the workspace package exists
fd -t f "package.json" -x cat {} \; | jq 'select(.name=="@kleros/kleros-v2-contracts")'
Length of output: 5628
The coverage setup looks good, but let's verify the coverage command exists in the workspace. ✅ Verification successfulCoverage configuration is properly set up The coverage setup is correctly configured:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify the coverage command configuration
# Check if the coverage script exists in package.json
fd -t f "package.json" -p "contracts" -x cat {} \; | jq '.scripts.coverage'
# Check if the coverage output directory is gitignored
fd -H -t f ".gitignore" -x grep -l "coverage" {} \;
Length of output: 211 Script: #!/bin/bash
# Check the content of coverage script and gitignore patterns
echo "=== Coverage Script Content ==="
fd "coverage.sh" -p "contracts/scripts" -x cat {} \;
echo -e "\n=== Gitignore Patterns ==="
fd ".gitignore" -p "contracts" -x cat {} \;
Length of output: 2117 🧰 Tools🪛 yamllint (1.35.1)[error] 75-75: trailing spaces (trailing-spaces) [error] 78-78: trailing spaces (trailing-spaces) [error] 79-79: trailing spaces (trailing-spaces) Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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-university ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
contracts/src/test/SortitionModuleMock.sol (1)
3-9: Consider adding implementation details to documentationWhile the authorship is documented, it would be helpful to add:
- A brief description of what properties are exposed and why
- Example usage in tests
contracts/src/test/KlerosCoreMock.sol (1)
3-7: Consider enhancing documentationWhile the purpose is documented, it would be helpful to add:
- Description of each getter function's purpose
- Example test scenarios where these functions are useful
contracts/src/libraries/Constants.sol (1)
12-12: Consider removing deprecated constantSince NULL_DISPUTE_KIT is deprecated and its purpose (forest structure) is no longer used, consider removing it in a future update to prevent confusion.
contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol (1)
623-626: LGTM: Comprehensive documentation of minStake behaviorThe added documentation excellently explains the minStake checking behavior and its implications. It properly documents an edge case where jurors might be drawn with less than minStake, while explaining why this is acceptable due to insolvency checks.
Consider adding a code example to illustrate the edge case:
/// Note that we don't check the minStake requirement here because of the implicit staking in parent courts. /// minStake is checked directly during staking process however it's possible for the juror to get drawn /// while having < minStake if it is later increased by governance. /// This issue is expected and harmless since we check for insolvency anyway. +/// Example: +/// 1. Juror stakes 100 tokens (minStake = 100) +/// 2. Governance increases minStake to 150 +/// 3. Juror can still be drawn as insolvency check ensures sufficient stake for voting
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
contracts/lib/forge-std(1 hunks)contracts/src/arbitration/KlerosCoreBase.sol(7 hunks)contracts/src/arbitration/SortitionModuleBase.sol(2 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol(1 hunks)contracts/src/arbitration/interfaces/IDisputeKit.sol(1 hunks)contracts/src/libraries/Constants.sol(2 hunks)contracts/src/test/KlerosCoreMock.sol(1 hunks)contracts/src/test/SortitionModuleMock.sol(1 hunks)contracts/test/arbitration/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- contracts/lib/forge-std
- contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol
🔇 Additional comments (13)
contracts/src/test/SortitionModuleMock.sol (1)
18-22: LGTM! Safe implementation of test utility
The getter function safely exposes read-only access to internal tree properties needed for testing, without modifying state or exposing sensitive data.
contracts/src/test/KlerosCoreMock.sol (2)
16-18: LGTM! Safe implementation of court children getter
The function safely exposes the court hierarchy data needed for testing.
20-24: Verify type consistency across contracts
The function correctly uses uint96 for courtID, matching the base contract. Let's verify this type is consistent across all related contracts.
✅ Verification successful
Note: Adjusting the search to handle Solidity files and using more specific patterns to find type declarations and usages of courtID.
Note: Adjusting the approach to first locate Solidity files and then search within them, since the -t sol flag isn't working.
Based on the comprehensive search results, I can now generate the final response:
Type uint96 for courtID is consistently used across the codebase
The verification confirms that uint96 is used consistently for courtID across all relevant contracts:
- In struct definitions (e.g., in Dispute structs across different versions)
- In function parameters and return values
- In state variables and mappings
- In event declarations
The type consistency is maintained in core contracts, interfaces, and mock implementations, including:
- KlerosCore and its variants
- SortitionModule
- All related interfaces
- Test implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify courtID type consistency across contracts
# Look for courtID declarations to ensure uint96 is used consistently
rg -A 1 "courtID|court[^a-zA-Z]" --type sol
Length of output: 73
Script:
#!/bin/bash
# Search for courtID type declarations and usages across all files
rg -t sol "courtID.*uint|uint.*courtID"
# Also search for function signatures and struct definitions containing courtID
ast-grep --pattern 'function $$$courtID$$$'
ast-grep --pattern 'struct $$$courtID$$$'
Length of output: 159
Script:
#!/bin/bash
# First, find all Solidity files
fd "\.sol$"
# Then search for courtID patterns in those files
fd "\.sol$" --exec grep -A 1 "courtID"
# Also try to find any uint96 type declarations
fd "\.sol$" --exec grep -A 1 "uint96"
Length of output: 56066
contracts/src/libraries/Constants.sol (1)
36-37: Verify the implementation of new staking validation
The new enum value CannotStakeZeroWhenNoStake suggests additional validation logic was added. Let's verify its implementation.
✅ Verification successful
Let me gather more context about the staking validation implementation.
Let me get the complete implementation of the staking function to understand the validation logic better.
Let me get the complete implementation to see where the zero stake validation occurs.
Let me get one final piece of context to see where the actual validation occurs.
Validation for zero stake is properly implemented
The validation is correctly implemented in SortitionModuleBase._setStake() to prevent staking zero amount when there is no existing stake. This validation:
- Returns
StakingResult.CannotStakeZeroWhenNoStakewhen attempting to stake 0 with no current stake - Is handled in
KlerosCoreBaseby reverting withStakingZeroWhenNoStakeerror - Has proper test coverage verifying this behavior
- Helps prevent flaky behavior in the staking system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find the implementation of zero stake validation
# Look for staking-related functions and the usage of this enum value
# Search for staking function implementations
ast-grep --pattern 'function $STAKE_FUNC($_) {
$$$
$RESULT = StakingResult.$_;
$$$
}'
# Search for zero stake validation
rg -A 5 "CannotStakeZeroWhenNoStake"
Length of output: 1586
Script:
#!/bin/bash
# Find the staking function implementation in SortitionModuleBase
ast-grep --pattern 'function $_($$$) {
$$$
if ($AMOUNT == 0 && $$$) {
$$$
return ($$$, StakingResult.CannotStakeZeroWhenNoStake);
}
$$$
}'
# Find the error definition
rg "StakingZeroWhenNoStake"
Length of output: 521
Script:
#!/bin/bash
# Find the complete staking function implementation
rg -A 20 "function setStake" contracts/src/arbitration/SortitionModuleBase.sol
# Check test cases to understand the validation
rg -A 10 "StakingZeroWhenNoStake" contracts/test/foundry/KlerosCore.t.sol
Length of output: 1616
Script:
#!/bin/bash
# Get more context of the staking implementation
rg -B 10 -A 30 "if \(_newStake == 0" contracts/src/arbitration/SortitionModuleBase.sol
# Get the complete test case context
rg -B 20 -A 20 "StakingZeroWhenNoStake" contracts/test/foundry/KlerosCore.t.sol
Length of output: 4062
Script:
#!/bin/bash
# Find where the zero stake validation is implemented
rg -B 20 -A 10 "return \(0, 0, StakingResult.CannotStakeZeroWhenNoStake\)" contracts/src/arbitration/SortitionModuleBase.sol
Length of output: 1698
contracts/test/arbitration/index.ts (1)
33-33: LGTM: Test expectation correctly updated
The test expectation has been properly updated to expect [1] for _supportedDisputeKits, which aligns with the changes in KlerosCoreBase.sol.
contracts/src/arbitration/interfaces/IDisputeKit.sol (1)
44-44: LGTM: Clear deprecation notice added
The deprecation notice for _nbVotes parameter is well-documented, clearly explaining that KlerosCore now handles the count internally.
contracts/src/arbitration/SortitionModuleBase.sol (2)
268-274: LGTM: Good defensive programming practice
The added validation prevents meaningless zero-stake operations when there's no existing stake, which helps avoid potential edge cases and maintains cleaner state.
303-303: LGTM: Fixed potential withdrawal accumulation bug
Direct assignment of pnkWithdrawal instead of incremental addition prevents potential issues with withdrawal amount accumulation.
contracts/src/arbitration/KlerosCoreBase.sol (5)
116-116: LGTM: Improved type consistency
Changed _courtID parameter type from uint256 to uint96 in the CourtCreated event, aligning with the courtID type used throughout the contract.
240-251: LGTM: Proper initialization of dispute kit support
The changes ensure that the General Court is properly initialized with Classic Dispute Kit support, maintaining the contract's invariant that all courts must support the Classic DK.
356-356: LGTM: Enhanced type safety
Added explicit type casting of courtID to uint96, ensuring type consistency when enabling dispute kits.
Line range hint 1066-1107: LGTM: Improved validation logic
The changes enhance the staking validation by:
- Using
>=for court length check, preventing staking in non-existent courts - Adding proper error handling for the zero-stake validation
1181-1181: LGTM: Added descriptive error
Added StakingZeroWhenNoStake error to provide clear feedback when attempting to stake zero tokens with no existing stake.
❌ Deploy Preview for kleros-v2-testnet-devtools failed. Why did it fail? →
|
unknownunknown1
force-pushed
the
feat/foundry-tests
branch
from
f0e4fa4 to
7d8e858
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
contracts/scripts/coverage.sh (1)
41-51: Add error handling for HTML report generationThe HTML report generation should handle potential errors and check for required tools.
Add error checking:
if [ "$CI" != "true" ]; then + if ! command -v genhtml &> /dev/null; then + echo "Error: genhtml command not found. Please install lcov." + exit 1 + } + # Generate the HTML report genhtml coverage/filtered-lcov.info \ --ignore-errors inconsistent \ --rc branch_coverage=1 \ --rc max_message_count=3 \ -o coverage \ --ignore-errors category \ - --ignore-errors format - open coverage/index.html + --ignore-errors format || { + echo "Error: failed to generate HTML report" + exit 1 + } + + # Use xdg-open on Linux, open on macOS + if [[ "$OSTYPE" == "darwin"* ]]; then + open coverage/index.html + else + xdg-open coverage/index.html + fi fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/contracts-testing.yml(2 hunks)contracts/.solcover.js(2 hunks)contracts/package.json(1 hunks)contracts/scripts/coverage.sh(1 hunks)cspell.json(2 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/contracts-testing.yml
[error] 45-45: trailing spaces
(trailing-spaces)
[error] 46-46: trailing spaces
(trailing-spaces)
[error] 50-50: trailing spaces
(trailing-spaces)
[error] 77-77: trailing spaces
(trailing-spaces)
🔇 Additional comments (7)
cspell.json (1)
24-24: LGTM! Dictionary additions are consistent and well-organized.
The new words "crowdfunder", "unstaked", and "Unstaking" are:
- Properly alphabetized
- Follow existing capitalization patterns
- Relevant to the staking and arbitration functionality being modified
Also applies to: 51-52
.github/workflows/contracts-testing.yml (3)
Line range hint 12-14: LGTM! Strong security configurations in place.
The workflow implements security best practices:
- Minimal permissions following principle of least privilege
- Runner hardening with specific egress policies
- Well-defined allowed endpoints for package managers and GitHub
Also applies to: 20-34
🧰 Tools
🪛 yamllint (1.35.1)
[error] 45-45: trailing spaces
(trailing-spaces)
[error] 46-46: trailing spaces
(trailing-spaces)
[error] 50-50: trailing spaces
(trailing-spaces)
43-43: LGTM! Excellent security practice with action version pinning.
All GitHub Actions are now pinned to specific commit hashes instead of version tags, which prevents supply chain attacks. The version tags are properly documented in comments for maintainability.
Also applies to: 47-49, 52-52, 79-79
74-77: Verify the coverage script configuration.
Please ensure:
- The
coveragescript exists in the contracts workspace's package.json - The script properly handles both Hardhat and Foundry test coverage
- Coverage artifacts are generated in the correct path
✅ Verification successful
Coverage script setup is properly configured
The verification confirms that:
- The
coveragescript exists and points toscripts/coverage.sh - The script handles both Hardhat and Forge test coverage comprehensively:
- Generates Forge coverage with lcov report
- Generates Hardhat coverage using solcover.js configuration
- Merges both reports and filters out unnecessary contracts
- Coverage artifacts are correctly generated in the
coveragedirectory with proper filtering of mock and test files - Both Hardhat and Foundry configurations are present
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if coverage script exists and its configuration
cat contracts/package.json | jq '.scripts.coverage'
# Check for coverage configuration files
fd -t f -e json -e js -e yml coverage.config
Length of output: 120
Script:
#!/bin/bash
# Check the coverage script content
cat contracts/scripts/coverage.sh
# Look for coverage-related configurations
fd -t f -e json -e js -e yml coverage
Length of output: 1688
Script:
#!/bin/bash
# Check for solcover.js configuration
cat contracts/.solcover.js
# Check if both Hardhat and Forge are available
ls contracts/hardhat.config.* contracts/foundry.toml
Length of output: 848
🧰 Tools
🪛 yamllint (1.35.1)
[error] 77-77: trailing spaces
(trailing-spaces)
contracts/package.json (1)
25-25: LGTM: Coverage script addition
The new coverage script entry is properly configured to execute the coverage.sh script.
contracts/.solcover.js (1)
8-8: LGTM: Coverage configuration updates
The changes improve the coverage setup by:
- Using LCOV format which is better suited for CI integration
- Properly excluding test and mock directories from coverage analysis
The skipFiles list correctly aligns with the paths filtered in the coverage.sh script.
Let's verify the existence of the excluded directories:
Also applies to: 17-17
✅ Verification successful
Coverage configuration correctly excludes test and mock directories
The excluded directories in .solcover.js are valid and match the actual project structure:
- Test directories:
contracts/src/test/,contracts/test/ - Mock directories:
contracts/src/gateway/mock/,contracts/src/proxy/mock/,contracts/src/rng/mock/ - Token directory:
contracts/src/token/ - Kleros directory:
contracts/src/kleros-v1/
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the excluded directories exist to ensure we're not accidentally skipping coverage for real code
fd -t d "^(test|token|kleros-v1|mock)$" contracts/
Length of output: 212
contracts/scripts/coverage.sh (1)
1-6: LGTM: Proper script setup
Good practices observed:
- Proper shebang line
- Exit on error enabled
- Clean workspace setup
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
.github/workflows/contracts-testing.yml (2)
43-43: Standardize version comment format.The version comments are inconsistent. Some use vX.X.X format while others use vX.X.
Standardize all version comments to use the same format, preferably the most detailed one (vX.X.X):
- # v4.1.0 + # v4.1.0 - # v4.2.2 + # v4.2.2 - # v4.2.0 + # v4.2.0 - # v1.2.0 + # v1.2.0 - # v4.4.3 + # v4.4.3Also applies to: 47-47, 52-52, 67-67, 77-77
Line range hint
52-61: Consider adding package.json to cache key.The cache key only includes package-lock.json and yarn.lock, but changes to package.json can also affect dependencies.
Add package.json to the cache key:
- key: ${{ runner.os }}-build-${{ secrets.CACHE_VERSION }}-${{ env.cache-name }}-${{ hashFiles('**/package-lock.json', '**/yarn.lock') }} + key: ${{ runner.os }}-build-${{ secrets.CACHE_VERSION }}-${{ env.cache-name }}-${{ hashFiles('**/package.json', '**/package-lock.json', '**/yarn.lock') }}🧰 Tools
🪛 yamllint (1.35.1)
[error] 45-45: trailing spaces
(trailing-spaces)
[error] 46-46: trailing spaces
(trailing-spaces)
[error] 50-50: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/contracts-testing.yml(2 hunks)contracts/scripts/coverage.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/scripts/coverage.sh
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/contracts-testing.yml
[error] 45-45: trailing spaces
(trailing-spaces)
[error] 46-46: trailing spaces
(trailing-spaces)
[error] 50-50: trailing spaces
(trailing-spaces)
[error] 75-75: trailing spaces
(trailing-spaces)
🔇 Additional comments (2)
.github/workflows/contracts-testing.yml (2)
69-70:
Critical: sudo usage conflicts with harden-runner configuration.
The sudo apt-get install command will fail because the workflow has disable-sudo: true in the harden-runner configuration.
Fix by removing sudo:
- run: sudo apt-get install -y lcov
+ run: apt-get install -y lcov72-74: Verify coverage script existence.
Ensure that the coverage script is defined in the contracts workspace's package.json.
✅ Verification successful
Coverage script is properly configured and functional
The coverage script exists in package.json and points to an executable shell script that:
- Generates coverage reports from both Hardhat and Forge tests
- Merges and filters the reports appropriately
- Creates HTML reports for local development
- Handles CI environment differently
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the coverage script exists in package.json
jq '.scripts.coverage' contracts/package.json
Length of output: 67
Script:
#!/bin/bash
# Check if the coverage script exists and is executable
ls -l contracts/scripts/coverage.sh
Length of output: 116
Script:
#!/bin/bash
# Check the contents of the coverage script
cat contracts/scripts/coverage.sh
Length of output: 1660
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
contracts/test/rng/index.ts (1)
Line range hint
1-184: Consider architectural improvements for RNG testingTo enhance the testing framework's reliability and maintainability:
- Create a shared test utility module for common RNG testing patterns
- Implement proper transaction receipt handling
- Add test timeouts and retry mechanisms for network-dependent tests
- Consider adding integration tests that verify the interaction between these RNG implementations and KlerosCore
Would you like me to help create a shared test utility module that implements these improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/scripts/coverage.sh(1 hunks)contracts/test/rng/index.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/scripts/coverage.sh
| @@ -113,7 +113,7 @@ abstract contract KlerosCoreBase is IArbitratorV2 { | |||
| event AppealDecision(uint256 indexed _disputeID, IArbitrableV2 indexed _arbitrable); | |||
| event Draw(address indexed _address, uint256 indexed _disputeID, uint256 _roundID, uint256 _voteID); | |||
| event CourtCreated( | |||
| uint256 indexed _courtID, | |||
| uint96 indexed _courtID, | |||
There was a problem hiding this comment.
jaybuidl
dismissed stale reviews from coderabbitai[bot] and themself
via
fed3d93
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/contracts-testing.yml (1)
69-70:⚠️ Potential issueCritical: sudo usage conflicts with harden-runner configuration.
The
sudo apt-get installcommand will fail because the workflow hasdisable-sudo: truein the harden-runner configuration.Fix by removing sudo:
- run: sudo apt-get install -y lcov + run: apt-get install -y lcov
🧹 Nitpick comments (1)
.github/workflows/contracts-testing.yml (1)
45-46: Remove trailing spaces.There are trailing spaces on these lines that should be removed for consistency.
- node-version: 18.x + node-version: 18.x - + - + - +Also applies to: 50-50, 79-79
🧰 Tools
🪛 yamllint (1.35.1)
[error] 45-45: trailing spaces
(trailing-spaces)
[error] 46-46: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/contracts-testing.yml(2 hunks)contracts/scripts/coverage.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/scripts/coverage.sh
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/contracts-testing.yml
[error] 45-45: trailing spaces
(trailing-spaces)
[error] 46-46: trailing spaces
(trailing-spaces)
[error] 50-50: trailing spaces
(trailing-spaces)
[error] 79-79: trailing spaces
(trailing-spaces)
🔇 Additional comments (2)
.github/workflows/contracts-testing.yml (2)
Line range hint 13-14: LGTM! Following security best practices.
The addition of explicit read-only permissions follows the principle of least privilege.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 45-45: trailing spaces
(trailing-spaces)
[error] 46-46: trailing spaces
(trailing-spaces)
[error] 50-50: trailing spaces
(trailing-spaces)
72-78: Verify coverage report integration.
The workflow now includes both Foundry and Hardhat tests with coverage. Please ensure that:
- Coverage reports from both frameworks are properly merged
- No test cases are being double-counted in the final report
✅ Verification successful
Coverage reports are properly integrated
The coverage setup is well-configured with proper merging of both Foundry and Hardhat reports:
- The
coverage.shscript handles both frameworks:- Generates Forge coverage report as
lcov-forge.info - Generates Hardhat coverage report as
lcov-hardhat.info - Merges both reports using
lcovtool - Filters out unnecessary contracts and generates HTML report
- Generates Forge coverage report as
- Path normalization is handled to ensure consistency between reports
- Branch coverage is properly enabled
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check coverage configuration
# Check coverage configuration files
echo "Checking coverage configuration:"
fd -t f -e js -e json -e yml -E "node_modules" . | rg -l "coverage|lcov"
# Check if there are any coverage merging tools or scripts
rg -l "merge.*coverage|combine.*coverage" -g "!node_modules"
Length of output: 210
Script:
#!/bin/bash
# Let's check the package.json and hardhat config for coverage setup
echo "Checking package.json in contracts directory:"
cat contracts/package.json
echo -e "\nChecking hardhat.config files:"
fd hardhat.config -t f
echo -e "\nChecking coverage related scripts and configurations:"
rg -A 5 "coverage" contracts/
Length of output: 10599
|
Code Climate has analyzed commit 0220cc2 and detected 0 issues on this pull request. View more on Code Climate. |
|
Created a testfile that goes through all the functions in KlerosCore contract while also testing most of the functionality of the adjacent contracts (Sortition and DK). Ideally the latter will also need their own test files to test governance and modifiers etc, so I'll add them some time later.
Foundry itself needs to be installed separately. There is no option to put in into package.json
Some minor contract fixes along the way (e.g. changing the type for consistency, adding missing argument to event).
Most of the covered bugs were mentioned in this review
I revisited and updated the review document. One of the highlighted bugs (delayed stakes frontrun) is still present as I need further clarifications before fixing it since it requires some tricky logic change. I created a test specifically to explore this bug. It won't pass once the bug is fixed.
Note that I checked the solution mentioned in my review and it works, but I didn't implement it yet for the aforementioned reason
PR-Codex overview
This PR focuses on enhancing the Kleros arbitration system by adding new mock contracts for testing, updating dispute kit handling, and improving coverage reporting. It also refines error handling and introduces new functionality for managing stakes and courts.
Detailed summary
SortitionModuleMockandKlerosCoreMockfor testing.Summary by CodeRabbit
Release Notes
New Features
KlerosCoreMockandSortitionModuleMock) to facilitate easier testing scenarios.Bug Fixes
KlerosCoreBaseandSortitionModuleBasecontracts to provide clearer feedback on staking conditions.Documentation
Chores
DisputeKitClassicto reflect changes in expected behavior during court creation.