Feat/leaderboard offset #2198
Feat/leaderboard offset #2198
Conversation
WalkthroughIntroduces the LeaderboardOffset contract to enable governance-controlled score offsets for jurors, with a corresponding Hardhat deployment script, subgraph integration to track offsets per user, and new verification and test utilities. Changes
Sequence DiagramsequenceDiagram
participant Gov as Governor
participant LeaderboardOffset as LeaderboardOffset Contract
participant Subgraph as Subgraph Handler
participant DB as User Entity
Gov->>LeaderboardOffset: setOffset(juror, offset, arbitrator)
LeaderboardOffset->>LeaderboardOffset: onlyGovernor check
LeaderboardOffset->>Subgraph: emit Offset event
Subgraph->>DB: load User by event.params.user
Subgraph->>DB: leaderboardOffset += event.params.offset
DB->>DB: save updated entity
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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
|
Includes:
Pending:
leaderboardOffsetproperty, pending subgraph deploymentcloses #2178
PR-Codex overview
This PR introduces a new
LeaderboardOffsetcontract to manage leaderboard offsets for users, along with related functionality in the subgraph. It enhances user data handling and includes tests for the new feature.Detailed summary
leaderboardOffsetfield toUserentity inUser.tsandschema.graphql.handleLeaderboardOffsetfunction to update user offsets.LeaderboardOffsetcontract withsetOffsetandupdateGovernorfunctions.Summary by CodeRabbit
Release Notes
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.