Feat/leaderboard offset #2198
Feat/leaderboard offset #2198
Conversation
WalkthroughThis pull request introduces a governance-controlled leaderboard offset system. It adds a new Changes
Sequence DiagramsequenceDiagram
participant Gov as Governor/Admin
participant Contract as LeaderboardOffset
participant Chain as Blockchain
participant Subgraph as Subgraph Indexer
participant DB as User Entity (DB)
Gov->>Contract: setOffset(juror, offset, arbitrator)
activate Contract
Contract->>Contract: require onlyGovernor
Contract->>Chain: emit Offset(user, offset, arbitrator)
deactivate Contract
Chain-->>Subgraph: Event: Offset
activate Subgraph
Subgraph->>DB: Load User by address
Note over Subgraph: User found
Subgraph->>DB: leaderboardOffset += offset
Subgraph->>DB: totalCoherentVotes += offset
Subgraph->>Subgraph: computeCoherenceScore()
Subgraph->>DB: Update coherenceScore
Subgraph->>DB: Save User
deactivate Subgraph
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (5)
contracts/src/governance/LeaderboardOffset.sol (1)
48-50: Consider validating address parameters in setOffset.The function only emits an event, but accepting zero addresses for
jurororarbitratorcould lead to invalid data being indexed by the subgraph and confusion in off-chain systems.Consider adding validation:
function setOffset(address juror, int256 offset, address arbitrator) external onlyGovernor { + if (juror == address(0) || arbitrator == address(0)) revert InvalidGovernor(); emit Offset(juror, offset, arbitrator); }Alternatively, define a more specific error like
InvalidAddress()if you prefer clearer error messages.subgraph/core/tests/leaderboard-offset.test.ts (1)
13-25: Consider expanding test coverage for edge cases.The current tests cover basic positive and negative offset scenarios effectively. Consider adding tests for:
- Zero offset: Verify that applying an offset of 0 leaves the leaderboard unchanged.
- Multiple offsets: Test cumulative behavior when multiple offset events are applied to the same user.
- Non-existent user: Verify the error logging path when
handleLeaderboardOffsetis called for a user that doesn't exist (currently line 7-9 in LeaderboardOffset.ts logs an error but this path is untested).Example test for the non-existent user scenario:
+ test("Should log error when user does not exist", () => { + const nonExistentAddress = Address.fromString("0x0000000000000000000000000000000000000999"); + const event = createOffsetEvent(nonExistentAddress, BigInt.fromI32(100)); + handleLeaderboardOffset(event); + // Note: matchstick provides logStore assertions to verify error logs + });subgraph/core/tests/leaderboard-offset-utils.ts (1)
7-31: LGTM with minor observation.The
createOffsetEventutility function is well-structured. Note that Line 24 sets thearbitratorparameter to the same address as theuserparameter. While this is acceptable for testing purposes and keeps the test setup simple, it doesn't reflect real-world scenarios where the arbitrator would typically be a different address (e.g., the KlerosCore contract address).Consider adding an optional
arbitratorparameter if future tests need to distinguish between user and arbitrator addresses.Optional enhancement:
-export const createOffsetEvent = (user: Address, offset: BigInt): Offset => { +export const createOffsetEvent = (user: Address, offset: BigInt, arbitrator?: Address): Offset => { let mockEvent = newMockEvent(); let offsetEvent = new Offset( mockEvent.address, mockEvent.logIndex, mockEvent.transactionLogIndex, mockEvent.logType, mockEvent.block, mockEvent.transaction, mockEvent.parameters, mockEvent.receipt ); offsetEvent.parameters = new Array(); let userParam = new ethereum.EventParam("user", ethereum.Value.fromAddress(user)); let offsetParam = new ethereum.EventParam("offset", ethereum.Value.fromSignedBigInt(offset)); - let arbitratorParam = new ethereum.EventParam("arbitrator", ethereum.Value.fromAddress(user)); + let arbitratorParam = new ethereum.EventParam("arbitrator", ethereum.Value.fromAddress(arbitrator || user)); offsetEvent.parameters.push(userParam); offsetEvent.parameters.push(offsetParam); offsetEvent.parameters.push(arbitratorParam); return offsetEvent; };contracts/tasks/verify-all.ts (2)
17-21: Consider more robust proxy detection.The current implementation skips proxies based solely on the naming convention
endsWith("_Proxy"). This approach is fragile and may not catch all proxy deployments, especially if naming conventions change or if proxies use different suffixes (e.g.,_Implementation,Proxy, etc.).Consider checking deployment metadata or contract type for more reliable proxy detection, if available through hardhat-deploy.
Alternative approach:
- // skip proxy files (we only skip by naming convention) - if (name.endsWith("_Proxy")) { - console.log(`Skipping proxy: ${name}`); + // skip proxy files (check both naming convention and deployment metadata) + if (name.endsWith("_Proxy") || name.includes("Proxy") || deployment.implementation) { + console.log(`Skipping proxy/implementation: ${name}`); continue; }
14-41: Consider adding progress indication for user experience.For repositories with many deployments, verification can take considerable time. Adding progress indication would improve the user experience.
const allDeployments = await deployments.all(); const entries = Object.entries(allDeployments); + const total = entries.length; + let current = 0; for (const [name, deployment] of entries) { + current++; + console.log(`\n[${current}/${total}] Verifying ${name}...`); if (contract && name !== contract) continue;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
contracts/deploy/00-home-chain-leaderboard-offset.ts(1 hunks)contracts/deployments/arbitrumSepoliaDevnet/LeaderboardOffset.json(1 hunks)contracts/hardhat.config.ts(3 hunks)contracts/package.json(2 hunks)contracts/src/governance/LeaderboardOffset.sol(1 hunks)contracts/tasks/verify-all.ts(1 hunks)subgraph/.gitignore(1 hunks)subgraph/core/schema.graphql(1 hunks)subgraph/core/src/LeaderboardOffset.ts(1 hunks)subgraph/core/src/entities/User.ts(1 hunks)subgraph/core/subgraph.template.yaml(1 hunks)subgraph/core/subgraph.yaml(1 hunks)subgraph/core/tests/leaderboard-offset-utils.ts(1 hunks)subgraph/core/tests/leaderboard-offset.test.ts(1 hunks)subgraph/matchstick.yaml(1 hunks)subgraph/package.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-07T10:48:16.774Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 1739
File: web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx:22-26
Timestamp: 2024-11-07T10:48:16.774Z
Learning: In the `Coherency` component (`web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx`), `totalResolvedVotes` is always greater than or equal to `totalCoherentVotes`. When both are zero, `0/0` results in `NaN`, which is acceptable in this context.
Applied to files:
subgraph/core/schema.graphql
🧬 Code graph analysis (3)
contracts/tasks/verify-all.ts (1)
contracts/deployments/utils.ts (1)
deployments(3-16)
subgraph/core/tests/leaderboard-offset.test.ts (2)
subgraph/core/tests/leaderboard-offset-utils.ts (2)
createUser(33-51)createOffsetEvent(7-31)subgraph/core/src/LeaderboardOffset.ts (1)
handleLeaderboardOffset(5-13)
contracts/deploy/00-home-chain-leaderboard-offset.ts (2)
contracts/deploy/utils/getContractOrDeploy.ts (1)
getContractOrDeploy(6-20)contracts/deploy/utils/index.ts (1)
isSkipped(32-38)
🪛 Biome (2.1.2)
subgraph/core/tests/leaderboard-offset-utils.ts
[error] 1-1: 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/leaderboard-offset.test.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)
⏰ 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). (1)
- GitHub Check: SonarCloud
🔇 Additional comments (13)
subgraph/.gitignore (1)
1-3: LGTM! Standard subgraph artifact exclusions.The ignore patterns appropriately exclude build artifacts and generated files.
subgraph/matchstick.yaml (1)
1-3: LGTM! Matchstick configuration aligns with project structure.The path configuration correctly references the core subgraph tests and manifest.
subgraph/package.json (1)
13-13: LGTM! Test script simplified via matchstick.yaml configuration.The removal of
cd coreis correct sincematchstick.yamlnow specifies the test paths.subgraph/core/src/entities/User.ts (1)
43-43: LGTM! Proper initialization of leaderboardOffset field.The field is correctly initialized to ZERO, consistent with other User entity fields and the schema requirement.
contracts/src/governance/LeaderboardOffset.sol (1)
1-68: Clean and focused governance contract.The contract design is minimal and appropriate for its purpose of emitting offset events. The governor access control, validation in constructor and
updateGovernor, and error handling are all correctly implemented.subgraph/core/schema.graphql (1)
82-82: LGTM! Schema field consistent with entity initialization.The non-null
BigInttype is appropriate for the leaderboardOffset field, which is always initialized to ZERO.subgraph/core/subgraph.template.yaml (1)
276-295: LGTM! LeaderboardOffset data source properly configured.The event signature matches the contract definition, and the mapping configuration follows the established pattern for other data sources.
contracts/package.json (1)
83-83: Dependency version verification complete — no issues found.The
@nomicfoundation/hardhat-verify@^2.0.14dependency is correctly pinned for Hardhat 2.26.5. No security vulnerabilities exist for this package. Upgrading to 3.0.7 would require migrating to Hardhat 3, which is outside this change's scope.subgraph/core/src/LeaderboardOffset.ts (1)
5-13: LGTM!The handler implementation is correct and straightforward:
- Proper error handling for missing users with early return
- Correct use of
plus()for BigInt arithmetic to accumulate offsets- Saves the updated entity appropriately
contracts/hardhat.config.ts (1)
303-305: LGTM!The addition of the
etherscanconfiguration section at the root level is correct for the@nomicfoundation/hardhat-verifyplugin. The dual configuration (bothverify.etherscanat lines 298-302 andetherscanat lines 303-305) appears intentional to support both hardhat-deploy's verification and the newer hardhat-verify plugin.subgraph/core/tests/leaderboard-offset-utils.ts (1)
33-51: LGTM!The
createUserutility properly initializes all User entity fields with appropriate default values, including the newleaderboardOffsetfield set toZERO. This provides a clean baseline for testing offset behavior.contracts/deploy/00-home-chain-leaderboard-offset.ts (1)
14-18: Document governance transfer expectations.The deployment sets the
deployeras the initial governor (line 16), granting them full control over offset emission. Ensure this is intentional and document the expected governance transfer process if the governor role should be transferred to a multisig or DAO after deployment.Consider adding a comment in the deployment script or updating documentation to clarify:
- Whether the deployer is intended to remain the permanent governor
- If not, the process and timeline for transferring governance to the appropriate entity
await getContractOrDeploy(hre, "LeaderboardOffset", { from: deployer, + // Note: deployer is set as initial governor; transfer governance using updateGovernor() if needed args: [deployer], log: true, });contracts/deployments/arbitrumSepoliaDevnet/LeaderboardOffset.json (1)
1-194: Deployment artifact looks correct.This is an auto-generated deployment artifact file. The ABI, deployment address (0x811eC94d73445Df262D3Bf43571B85caD122bBD7), and start block (217101551) correctly match the configuration in
subgraph/core/subgraph.yaml(lines 281 and 283).
jaybuidl
added
Type: Feature🗿
Package: Web
jaybuidl
dismissed
coderabbitai[bot]’s stale review
The base branch was changed.
jaybuidl
left a comment
jaybuidl
left a comment
There was a problem hiding this comment.
Let's apply the offset to the total coherence.
❌ 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-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
subgraph/core/tests/leaderboard-offset.test.ts (1)
8-38: Consider adding edge case tests.The current test coverage is good for happy paths. Consider adding tests for edge cases such as:
- Non-existent user: Verify the handler's error logging when a user doesn't exist
- Multiple offsets: Apply multiple offsets sequentially to verify cumulative behavior
- Large offsets: Test with very large positive and negative values
- Negative coherent votes: Apply a negative offset that would make
totalCoherentVotesnegativeThese additional tests would improve robustness and coverage of boundary conditions.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
subgraph/core/src/LeaderboardOffset.ts(1 hunks)subgraph/core/tests/leaderboard-offset.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- subgraph/core/src/LeaderboardOffset.ts
🧰 Additional context used
🧬 Code graph analysis (1)
subgraph/core/tests/leaderboard-offset.test.ts (2)
subgraph/core/tests/leaderboard-offset-utils.ts (2)
createUser(33-51)createOffsetEvent(7-31)subgraph/core/src/LeaderboardOffset.ts (1)
handleLeaderboardOffset(6-21)
🪛 Biome (2.1.2)
subgraph/core/tests/leaderboard-offset.test.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)
⏰ 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-neo
- GitHub Check: Redirect 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
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
🔇 Additional comments (4)
subgraph/core/tests/leaderboard-offset.test.ts (4)
1-4: LGTM! Static analysis warning is a false positive.The imports are appropriate for Graph Protocol subgraph testing. The Biome warning about shadowing the global
BigIntis a false positive—BigIntfrom@graphprotocol/graph-tsis the intended type for AssemblyScript-based subgraph development and is standard practice in this context.
8-11: LGTM! Proper test isolation.The
beforeEachhook correctly clears the store before each test, ensuring proper test isolation.
13-25: LGTM! Basic offset tests are correct.The positive and negative offset tests correctly verify that offsets are applied to the user's leaderboard score. The test logic aligns with the handler implementation.
27-37: The coherenceScore calculation is correct. With totalCoherentVotes=70 and totalResolvedVotes=80, the formula (70 / (80 + 10)) × 100 = 77.777..., which rounds to 78 as expected.
Includes:
Pending:
leaderboardOffsetproperty, pending subgraph deploymentcloses #2178
PR-Codex overview
This PR introduces a new
LeaderboardOffsetcontract to manage juror score offsets and updates to the subgraph to support this functionality, including new event handling and tests.Detailed summary
LeaderboardOffsetcontract with offset event management.Userentity inUser.tsto includeleaderboardOffset.schema.graphqlto includeleaderboardOffset.handleLeaderboardOffsetfunction inLeaderboardOffset.ts.Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.