feat(subgraph): add StakeSet entity and handler tweak #1901
Conversation
WalkthroughThis pull request introduces the new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 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 (
|
kemuru
changed the title
✅ 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 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-university 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
🧹 Nitpick comments (3)
subgraph/core/src/SortitionModule.ts (2)
36-37: Add error logging for missing general court.The early return if the general court is not found could lead to silent failures. Consider adding error logging to help with debugging.
- if (!generalCourt) return; + if (!generalCourt) { + log.error("General court not found when processing StakeSet event. TX: {}", [event.transaction.hash.toHex()]); + return; + }
39-46: Add parameter validation before entity creation.Consider validating event parameters before creating the entity to ensure data integrity.
+ if (event.params._amount.isZero() && event.params._courtID.isZero()) { + log.warning("StakeSet event with zero amount and courtID. TX: {}", [event.transaction.hash.toHex()]); + } + const stakeSet = new StakeSetEntity(event.transaction.hash.toHex() + "-" + event.logIndex.toString()); stakeSet.address = event.params._address; stakeSet.courtID = event.params._courtID; stakeSet.stake = event.params._amount; stakeSet.newTotalStake = generalCourt.effectiveStake; stakeSet.blocknumber = event.block.number; stakeSet.timestamp = event.block.timestamp; stakeSet.logIndex = event.logIndex; stakeSet.save();subgraph/core/schema.graphql (1)
357-366: Review of the NewStakeSetEntity
- Entity Structure: The new
StakeSetentity is defined with all the required fields. The fields foraddress,courtID,stake,newTotalStake,timestamp, andlogIndexall match the expected data types.- ID Field Comment: The comment on the
idfield indicates it is constructed from the transaction hash and log index. For improved clarity, consider explicitly using a delimiter like'-'(e.g.,event.transaction.hash.toHex() + '-' + event.logIndex.toString()) rather than+ - +.- Naming Consistency: The field
blocknumberis defined in all lowercase. To maintain consistency with the camelCase naming found throughout the schema (e.g.,transactionHash,createdAt,blockNumberin other parts), it is recommended to rename this field toblockNumber.Suggested Diff Change for Consistency:
- blocknumber: BigInt! + blockNumber: BigInt!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
subgraph/core-neo/subgraph.yaml(1 hunks)subgraph/core-university/subgraph.yaml(1 hunks)subgraph/core/schema.graphql(1 hunks)subgraph/core/src/SortitionModule.ts(2 hunks)subgraph/core/subgraph.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: contracts-testing
🔇 Additional comments (5)
subgraph/core/src/SortitionModule.ts (2)
7-7: LGTM! Clear type distinction between event and entity.The renaming of
StakeSettoStakeSetEventand separate import ofStakeSetEntityimproves code clarity by clearly distinguishing between event and entity types.Also applies to: 9-9
27-47: LGTM! Well-structured event handling.The function correctly:
- Updates juror stake information
- Creates and persists the StakeSet entity with all relevant data
- Uses a unique, deterministic ID based on transaction hash and log index
subgraph/core/subgraph.yaml (1)
152-152: LGTM! Consistent entity configuration across subgraphs.The addition of the
StakeSetentity to the SortitionModule's entities list is consistent across all subgraph configurations and properly aligns with the handler implementation inSortitionModule.ts.subgraph/core-university/subgraph.yaml (1)
151-153: Addition of StakeSet Entity: The newStakeSetentity is correctly added to theentitieslist under theSortitionModuledata source. Please confirm that the corresponding GraphQL type definitions and event handler implementations (handleStakeSet) are updated consistently to reflect the intended staking data model.subgraph/core-neo/subgraph.yaml (1)
151-153: StakeSet Entity Integration Validated: The addition of theStakeSetentity under theSortitionModuleis implemented in line with the subgraph enhancements. Ensure that the new entity aligns with the GraphQL schema and that the event handler (handleStakeSet) properly processes theStakeSetevent as expected.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
subgraph/core-university/src/SortitionModule.ts (1)
17-27: Consider adding error logging and validation.The implementation correctly creates and persists the StakeSetEntity with all required fields. However, consider these improvements:
- Add error logging when user is not found to help with debugging:
- if (!juror) return; + if (!juror) { + log.warning('User not found for address {}', [jurorAddress]); + return; + }
- Consider adding validation for the stake amount:
+ if (event.params._amount.lt(ZERO)) { + log.warning('Invalid stake amount {} for address {}', [event.params._amount.toString(), jurorAddress]); + return; + } const stakeSet = new StakeSetEntity(event.transaction.hash.toHex() + "-" + event.logIndex.toString());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
subgraph/core-university/src/SortitionModule.ts(1 hunks)subgraph/core/schema.graphql(1 hunks)subgraph/core/src/SortitionModule.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- subgraph/core/schema.graphql
- subgraph/core/src/SortitionModule.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
🔇 Additional comments (2)
subgraph/core-university/src/SortitionModule.ts (2)
1-2: LGTM! Clear type distinction between event and entity.The renaming of
StakeSettoStakeSetEventimproves code clarity by distinguishing between the event and entity types. The new imports are necessary for the added functionality.
8-8: LGTM! Function signature updated correctly.The function signature change aligns with the import renaming and is consistent with the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
subgraph/core/schema.graphql (1)
357-366: TheStakeSetentity looks good, but consider improving naming consistency.The implementation properly marks the entity as immutable and correctly links to the
Userentity as suggested in previous comments. However, the field naming is inconsistent:blocknumberuses lowercase whilecourtIDuses camelCase.Consider renaming
blocknumbertoblockNumberfor consistency with the other camelCase field names:- blocknumber: BigInt! + blockNumber: BigInt!subgraph/core/src/SortitionModule.ts (1)
36-38: Consider adding error handling.While
ensureUserandensureJurorTokensPerCourtlikely handle errors internally, it might be worth adding error logging for monitoring purposes, especially for production environments.const stakeSet = new StakeSetEntity(event.transaction.hash.toHex() + "-" + event.logIndex.toString()); const jurorTokensPerCourt = ensureJurorTokensPerCourt(jurorAddress, "1"); + // Log the event for monitoring + log.debug("StakeSet event processed for juror {} with court {} and amount {}", [ + jurorAddress, + courtID, + event.params._amount.toString() + ]); stakeSet.juror = juror.id;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
subgraph/core-university/src/SortitionModule.ts(1 hunks)subgraph/core/schema.graphql(1 hunks)subgraph/core/src/SortitionModule.ts(2 hunks)subgraph/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- subgraph/package.json
⏰ Context from checks skipped due to timeout of 90000ms (15)
- 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: Redirect rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
🔇 Additional comments (6)
subgraph/core-university/src/SortitionModule.ts (4)
1-2: Good import renaming for clarity.Renaming the import from
StakeSettoStakeSetEventhelps distinguish between the event and the entity, reducing potential confusion.
8-11: Good use ofensureUser.Storing the result of
ensureUserin a variable for later use is a good practice. This helps avoid duplicate calls and makes the code more readable.
17-27: Well-structured entity creation.The
StakeSetentity creation is well implemented. It correctly follows the ID format specified in the schema and captures all necessary data from the event and related entities.
18-18:❓ Verification inconclusive
Verify court ID used in
ensureJurorTokensPerCourt.The function uses a hardcoded court ID "1", but the
courtIDfrom the event is stored in the entity. Consider if this is intentional or if thecourtIDfrom the event should be used instead.
🏁 Script executed:
#!/bin/bash # Check for other instances of ensureJurorTokensPerCourt to understand usage patterns rg -A 3 "ensureJurorTokensPerCourt\(" --type tsLength of output: 2764
Revisit the court ID value passed into ensureJurorTokensPerCourt
It appears that in
subgraph/core-university/src/SortitionModule.tsthe call to:const jurorTokensPerCourt = ensureJurorTokensPerCourt(jurorAddress, "1");uses the hardcoded string
"1", even though the event carries a_courtID(which is later set instakeSet.courtID). In contrast, other parts of the codebase (e.g. insubgraph/core/src/entities/JurorTokensPerCourt.ts) pass a variable value (likecourt.idorcourtID).Please verify if this hardcoded court ID is intentional. If the court ID from the event should actually be used consistently, consider updating the call to:
const jurorTokensPerCourt = ensureJurorTokensPerCourt(jurorAddress, event.params._courtID);Otherwise, please add a comment explaining why a default value of
"1"is appropriate in this instance.subgraph/core/src/SortitionModule.ts (2)
1-9: Good import organization.The import renaming from
StakeSettoStakeSetEventis consistent with the changes in the university version and helps distinguish between the event and entity.
27-46: Well-implemented entity creation.The implementation correctly creates and populates a new
StakeSetEntitywith all required information. The approach is consistent with the university version of the handler.
|
Code Climate has analyzed commit 809b085 and detected 2 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
subgraph/core-university/src/SortitionModule.ts(1 hunks)subgraph/core/src/SortitionModule.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- subgraph/core/src/SortitionModule.ts
⏰ Context from checks skipped due to timeout of 90000ms (15)
- 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: 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-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
subgraph/core-university/src/SortitionModule.ts (3)
1-2: Good import refactoring for clarity.Renaming the imported event to
StakeSetEventis a good practice to avoid naming conflicts with the new entity type. This makes the code more maintainable and readable.
8-8: Appropriate function signature update.The function signature has been correctly updated to use the renamed
StakeSetEventtype, maintaining type consistency with the imports.
17-17: Good implementation of user handling.Using
ensureUserhere and storing the result in a variable follows good practices for code reusability and clarity. This matches the previous review suggestion to addensureUserin this context.
this is needed for the Staking Rewards script (for pnk-merkle-drop) in V2
PR-Codex overview
This PR focuses on updating the subgraph's schema and functionality to accommodate a new
StakeSetentity, reflecting changes in staking events and enhancing data management for jurors.Detailed summary
versioninsubgraph/package.jsonto0.12.0.StakeSetentity in multiplesubgraph.yamlfiles.StakeSettype insubgraph/core/schema.graphql.handleStakeSetfunction to create and saveStakeSetEntity.StakeSetEntityin relevant TypeScript files.Summary by CodeRabbit
New Features
StakeSet, to enhance staking tracking with detailed information (e.g., stake amounts, updated totals, timestamps) for stake events, offering more granular insights for data monitoring.Refactor
Chores