feat(subgraph/web): time travel query refactor #1939
Conversation
WalkthroughThis pull request introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant D as Dispute Creation Handler
participant C as CourtCounter Entity
participant U as updateCourtCumulativeMetric
participant A as Appeal Decision Handler
participant J as Juror Stake Updater
participant S as updateCourtStateVariable
D->>C: Increment numberDisputes
D->>U: Update "numberDisputes" metric with delta & timestamp
A->>C: Update numberVotes
A->>U: Update "numberVotes" metric with delta & timestamp
J->>S: Update effectiveStake state with new value & timestamp
sequenceDiagram
participant UI as Web UI
participant Hook as useHomePageBlockQuery
participant Server as GraphQL Server
UI->>Hook: Request past court data with pastTimestamp
Hook->>Server: Execute query using pastTimestamp (via courtCounters)
Server-->>Hook: Return updated data
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 (
|
✅ 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-neo failed. Why did it fail? →
|
❌ Deploy Preview for kleros-v2-university failed. Why did it fail? →
|
✅ Deploy Preview for kleros-v2-testnet-devtools 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: 1
🧹 Nitpick comments (4)
subgraph/core/src/datapoint.ts (1)
95-132: Consider validation for unrecognized metrics.
This new function correctly updates the “numberDisputes” and “numberVotes” fields of theCourtCounterentity, along with creating a daily snapshot. However, if an unexpectedmetricvalue is passed, the function silently does nothing. Consider handling unknown metrics explicitly (e.g., logging, throwing, or ignoring with justification) to avoid silent errors.web/src/hooks/queries/useHomePageBlockQuery.ts (3)
55-60: Guard against empty arrays when calling[0].
Although these utility functions look clean, calling[0]on an empty array will yieldundefined. Consider returning early or defaulting to a sentinel object ifcourtscan be empty.
77-77: Rename Set to maintain clarity.
Using aSet<number>to track processed indices is fine. However, naming itprocessedIndicesor similar could improve clarity in this block.
94-100: Reduce repeated function calls to improve performance.
Repeatedly callingprocessCourt(parentIndex)can be costly since it’s invoked multiple times in the same object literal. Store the return value in a local variable first to avoid repeated recursion.const parentCourt = processCourt(parentIndex); processedCourts[id] = { ...courtWithTree, treeNumberDisputes: courtWithTree.treeNumberDisputes + parentCourt.treeNumberDisputes, treeNumberVotes: courtWithTree.treeNumberVotes + parentCourt.treeNumberVotes, // ... };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
subgraph/core/schema.graphql(1 hunks)subgraph/core/src/KlerosCore.ts(3 hunks)subgraph/core/src/datapoint.ts(2 hunks)subgraph/core/src/entities/JurorTokensPerCourt.ts(2 hunks)web/src/hooks/queries/useHomePageBlockQuery.ts(5 hunks)web/src/hooks/queries/useHomePageExtraStats.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
web/src/hooks/queries/useHomePageBlockQuery.ts (1)
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1744
File: web/src/hooks/queries/useHomePageBlockQuery.ts:71-71
Timestamp: 2025-04-03T18:37:26.185Z
Learning: In `web/src/hooks/queries/useHomePageBlockQuery.ts`, the non-null assertions on `blockNumber!` and `genesisBlock!` within `queryFn` are safe because `isEnabled` ensures that `queryFn` only runs when either `blockNumber` or `genesisBlock` is defined.
🧬 Code Definitions (3)
subgraph/core/src/entities/JurorTokensPerCourt.ts (1)
subgraph/core/src/datapoint.ts (1)
updateCourtStateVariable(134-168)
subgraph/core/src/KlerosCore.ts (1)
subgraph/core/src/datapoint.ts (1)
updateCourtCumulativeMetric(96-132)
web/src/hooks/queries/useHomePageExtraStats.ts (1)
web/src/hooks/queries/useHomePageBlockQuery.ts (1)
useHomePageBlockQuery(170-187)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
- GitHub Check: contracts-testing
🔇 Additional comments (18)
subgraph/core/schema.graphql (1)
245-252: LGTM! A well-designed entity for court metrics tracking.The addition of the
CourtCounterentity is well-structured with appropriate fields for storing cumulative metrics and timestamps. It will provide valuable time-series data points for court metrics.subgraph/core/src/entities/JurorTokensPerCourt.ts (2)
3-3: Appropriate import for the newupdateCourtStateVariablefunction.The import is correctly added to bring in the required function for updating court state variables.
97-97: Good integration of court state tracking.The addition of
updateCourtStateVariablecall ensures that whenever a juror's stake is updated, the court's effective stake is properly tracked in the new time-series data structure. This will allow for historical queries of court metrics.subgraph/core/src/KlerosCore.ts (4)
26-26: Appropriate import for the new update function.The import is correctly added to bring in the required function for updating court cumulative metrics.
85-85: Good addition for dispute creation tracking.The call to
updateCourtCumulativeMetricensures that new disputes are properly recorded in the time-series data structure, allowing for historical querying of dispute counts.
89-89: Proper tracking of votes during dispute creation.This tracking of votes at dispute creation time enhances the system's ability to provide historical court metrics related to voting activities.
231-231: Consistent tracking of votes during appeal decisions.The addition of vote tracking during appeal decisions ensures that all vote metrics are consistently captured in the time-series data, regardless of when they occur in the dispute lifecycle.
web/src/hooks/queries/useHomePageExtraStats.ts (3)
8-8: Good shift from block-based to timestamp-based tracking.Changing from
pastBlockNumbertopastTimestampsimplifies the data model and improves the accuracy of time-based queries.
11-16: Improved time calculation logic.The new implementation directly calculates the past timestamp based on the current time, which is more intuitive and eliminates dependencies on block-related calculations.
19-19: Updated query to use timestamp-based filtering.The modified call to
useHomePageBlockQueryproperly passes the calculated timestamp instead of a block number, completing the transition to timestamp-based historical queries.subgraph/core/src/datapoint.ts (1)
2-2: Ensure consistent usage of imported entities.
The addition ofCourtCounteralongsideCounteris consistent with the changes below, where these entities are referenced. No concerns here.web/src/hooks/queries/useHomePageBlockQuery.ts (7)
8-8: Timestamp-based query parameter looks good.
Changing the query to accept$pastTimestampaligns with the new time travel approach. Verify that all references to the oldblockNumberparameter in downstream code have been removed or updated.
20-20: Querying courtCounters by timestamp is consistent with the new logic.
ThepastCourtsfield now relies ontimestamp_lte: $pastTimestamp. This is appropriate for the new time-travel functionality. Ensure that descending order bytimestampis indeed the intended behavior, especially if multiple snapshots exist on the same day.
32-32: Type alias for CourtCounter is a helpful addition.
Creating a dedicatedCourtCountertype reference from GraphQL results is good for clarity, ensuring type safety across the codebase.
66-74: Map builder for past courts works as intended.
Storing only the first encountered court snapshot (descending timestamp) ensures we capture the most recent state when!allTime. This logic seems correct, but be mindful of concurrency if multiple snapshots share the same exact timestamp to avoid overwriting.
83-90: Conditional usage of diff logic is appropriate.
UsingaddTreeValuesWithDiffonly when a matchingpastCourtsnapshot is found ensures that you account for partial historical data. This aligns well with the newly introducedCourtCounterlogic.
141-168: Be mindful of integer truncation with BigInt averaging.
Using(presentCourtWithTree.effectiveStake + pastEffectiveStake) / 2ntruncates decimals if the sum is odd. This is likely acceptable, but confirm this is intentional. If you want a more precise average, you might need floating arithmetic.
170-187: New time-based hook logic looks solid.
The updateduseHomePageBlockQueryhook thoroughly replaces block-number logic with timestamp logic. It is conditionally enabled only if a validpastTimestampis provided orallTimeis true. Confirm that providingpastTimestamp= 0 does not cause unexpected queries for negative or null time ranges.
|
Code Climate has analyzed commit 0e47701 and detected 14 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
♻️ Duplicate comments (1)
subgraph/core/src/datapoint.ts (1)
134-168: 🛠️ Refactor suggestionExpand handling for additional variables and fix inconsistent initialization.
This function only explicitly handles
effectiveStakebut is structured to potentially handle more variables. There's also an inconsistency in how it initializes new counters.export function updateCourtStateVariable(courtId: string, newValue: BigInt, timestamp: BigInt, variable: string): void { + const validVariables = ["effectiveStake"]; // Add more variables as they are introduced + if (!validVariables.includes(variable)) { + return; // Early return for invalid variables + } // Load or create the current CourtCounter (ID: courtId-0) let currentCounter = CourtCounter.load(courtId + "-0"); if (!currentCounter) { currentCounter = new CourtCounter(courtId + "-0"); currentCounter.court = courtId; currentCounter.numberDisputes = ZERO; currentCounter.numberVotes = ZERO; - currentCounter.effectiveStake = newValue; + currentCounter.effectiveStake = variable === "effectiveStake" ? newValue : ZERO; currentCounter.timestamp = ZERO; + currentCounter.save(); // Save was missing here } else { if (variable === "effectiveStake") { currentCounter.effectiveStake = newValue; } currentCounter.save(); } // Update daily snapshot let dayID = timestamp.toI32() / 86400; let dayStartTimestamp = dayID * 86400; let dailyCounter = CourtCounter.load(courtId + "-" + dayStartTimestamp.toString()); if (!dailyCounter) { dailyCounter = new CourtCounter(courtId + "-" + dayStartTimestamp.toString()); dailyCounter.court = courtId; dailyCounter.numberDisputes = currentCounter.numberDisputes; dailyCounter.numberVotes = currentCounter.numberVotes; - dailyCounter.effectiveStake = newValue; + dailyCounter.effectiveStake = variable === "effectiveStake" ? newValue : currentCounter.effectiveStake; dailyCounter.timestamp = BigInt.fromI32(dayStartTimestamp); + dailyCounter.save(); // Save was missing here } else { if (variable === "effectiveStake") { dailyCounter.effectiveStake = newValue; } dailyCounter.save(); } }The current code unconditionally sets
effectiveStaketonewValuewhen creating a new counter, but for existing counters, it only updates ifvariable === "effectiveStake". This could lead to unexpected behavior in the future.
🧹 Nitpick comments (1)
subgraph/core/src/datapoint.ts (1)
96-132: Consider refactoring for reusability and flexibility.This function currently handles only two metrics (
numberDisputesandnumberVotes). If additional metrics are added in the future, the function will need to be modified. Consider refactoring to make it more flexible:- export function updateCourtCumulativeMetric(courtId: string, delta: BigInt, timestamp: BigInt, metric: string): void { + export function updateCourtCumulativeMetric(courtId: string, delta: BigInt, timestamp: BigInt, metric: string): void { + const validMetrics = ["numberDisputes", "numberVotes"]; + if (!validMetrics.includes(metric)) { + return; // Early return for invalid metrics + } // Load or create the current CourtCounter (ID: courtId-0) let currentCounter = CourtCounter.load(courtId + "-0"); if (!currentCounter) { currentCounter = new CourtCounter(courtId + "-0"); currentCounter.court = courtId; currentCounter.numberDisputes = ZERO; currentCounter.numberVotes = ZERO; currentCounter.effectiveStake = ZERO; currentCounter.timestamp = ZERO; } - if (metric === "numberDisputes") { - currentCounter.numberDisputes = currentCounter.numberDisputes.plus(delta); - } else if (metric === "numberVotes") { - currentCounter.numberVotes = currentCounter.numberVotes.plus(delta); - } + // Use dynamic property access + const currentValue = currentCounter.get(metric) as BigInt; + currentCounter.set(metric, currentValue.plus(delta)); currentCounter.save(); // Update daily snapshot let dayID = timestamp.toI32() / 86400; // Seconds to days let dayStartTimestamp = dayID * 86400; let dailyCounter = CourtCounter.load(courtId + "-" + dayStartTimestamp.toString()); if (!dailyCounter) { dailyCounter = new CourtCounter(courtId + "-" + dayStartTimestamp.toString()); dailyCounter.court = courtId; dailyCounter.numberDisputes = currentCounter.numberDisputes.minus(delta); // State before this update dailyCounter.numberVotes = currentCounter.numberVotes.minus(delta); dailyCounter.effectiveStake = currentCounter.effectiveStake; dailyCounter.timestamp = BigInt.fromI32(dayStartTimestamp); } - if (metric === "numberDisputes") { - dailyCounter.numberDisputes = dailyCounter.numberDisputes.plus(delta); - } else if (metric === "numberVotes") { - dailyCounter.numberVotes = dailyCounter.numberVotes.plus(delta); - } + // Use dynamic property access + const dailyValue = dailyCounter.get(metric) as BigInt; + dailyCounter.set(metric, dailyValue.plus(delta)); dailyCounter.save(); }Also, extracting the day timestamp calculation to a helper function would reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
subgraph/core/src/datapoint.ts(2 hunks)web/src/hooks/queries/useHomePageBlockQuery.ts(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- 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: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
🔇 Additional comments (10)
web/src/hooks/queries/useHomePageBlockQuery.ts (8)
8-9: GraphQL query updated to use timestamp-based filteringThe query structure has been refactored to use timestamps instead of block numbers for historical data retrieval, with clear separation between present and past data. This approach aligns with The Graph's recommended practice as time-travel queries using block numbers are being deprecated.
Also applies to: 20-33
38-38: New type definition added for court counter dataGood addition of the
CourtCountertype to represent the new data structure returned from thecourtCountersquery, improving type safety.
61-66: Updated sorting helper functions using modern JavaScriptThe sorting helper functions now use the newer
toSorted()array method which is more concise and avoids mutating the original array.
69-81: Enhanced data processing with Map-based lookupsThe implementation now uses Map objects for both present and past courts, which provides more efficient lookups compared to array iterations. The conditional processing for past courts is a good optimization for the non-allTime case.
83-110: Refactored court processing with improved recursive approachThe court processing logic has been enhanced with:
- Memoization via the
processedCourtsMap- Proper handling of the new data structure
- Conditional logic to use either diff or direct values based on availability of past data
This should result in more efficient and accurate court data processing.
150-168: Added fallback mechanisms and error handling for missing dataGood addition of fallback logic and warning messages when snapshot data is missing or shows no activity. This improves reliability and debugging capabilities.
170-174: Improved metric calculation using average effective stakeThe implementation now uses the average of past and present effective stake for calculating per-PNK metrics, which provides a more accurate representation over the time period.
192-209: Updated hook signature to use bigint for timestampThe hook signature has been updated to use
bigintfor the timestamp parameter, which is appropriate for handling large timestamp values. The implementation also correctly handles theallTimecase by using "0" as the pastTimestamp.subgraph/core/src/datapoint.ts (2)
2-2: Looks good - Import statement updated correctly.The import statement has been properly updated to include the new
CourtCounterentity, which is used in the newly added functions.
121-122: Verify the initial state calculation for daily counters.When creating a new daily counter, you're setting the initial values by subtracting the current delta from the current counter's values. This assumes the delta has already been applied to the current counter, which seems correct based on the code flow, but worth verifying:
Are you certain that the formula
currentCounter.numberDisputes.minus(delta)correctly represents the state before the update? This approach assumes that the delta has already been added to the current counter.
|
needs subgraph redeployment (or deploy preview will fail)
luckily I had time to compare the data before they deprecate the time travel query (which they will in a couple of hours). everything matches. tested on my personal subgraph.
Screen.Recording.2025-04-03.at.20.32.03.mov
PR-Codex overview
This PR focuses on updating the
subgraphto handle new metrics forCourtCounter, enhancing the data model and queries related to court statistics in the Kleros system.Detailed summary
versioninsubgraph/package.jsonto0.15.0.CourtCounterentity insubgraph/core/schema.graphql.updateCourtCumulativeMetricandupdateCourtStateVariablefunctions insubgraph/core/src/datapoint.ts.updateJurorStakeandhandleDisputeCreationfunctions to utilize new metrics.useHomePageExtraStatsto use timestamps instead of block numbers.web/src/hooks/queries/useHomePageBlockQuery.tsto fetch data based on timestamps.Summary by CodeRabbit
New Features
CourtCounterfor enhanced tracking of court metrics, including disputes, votes, and effective stake.Chores