fix: create missing Pool on demand to avoid null-Pool abort#249
Conversation
📝 WalkthroughWalkthroughThis change adds a shared ChangesPool lifecycle centralization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant RoundHandler
participant RewardHandler
participant TicketHandler
participant createOrLoadPool
participant PoolStore
RoundHandler->>createOrLoadPool: Initialize pool for active transcoder
RewardHandler->>createOrLoadPool: Retrieve pool for reward
TicketHandler->>createOrLoadPool: Retrieve pool for ticket fees
createOrLoadPool->>PoolStore: Load or save pool
createOrLoadPool-->>RewardHandler: Pool entity for reward updates
createOrLoadPool-->>TicketHandler: Pool entity for fee updates
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🚀 Subgraph Studio preview deployed
curl -H 'Content-Type: application/json' \
-d '{"query":"{ protocol(id: \"0\") { inflation } }"}' \
https://api.studio.thegraph.com/query/31909/livepeer-ci/pr-249-8d58daf-29782364581 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mappings/ticketBroker.ts (1)
68-71: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winFix erroneous reserve deduction due to premature zeroing of deposit.
broadcaster.depositis reassigned toZERO_BDbefore it is subtracted fromfaceValueto calculate thedifference. Consequently, thedifferenceevaluates to the entirefaceValue, leading to an over-deduction from the broadcaster's reserve. Calculate the difference before resetting the deposit.🐛 Proposed fix
if (faceValue.gt(broadcaster.deposit)) { - broadcaster.deposit = ZERO_BD; - - let difference = faceValue.minus(broadcaster.deposit); + let difference = faceValue.minus(broadcaster.deposit); + broadcaster.deposit = ZERO_BD; if (difference.gt(broadcaster.reserve)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mappings/ticketBroker.ts` around lines 68 - 71, In the insufficient-deposit branch, calculate difference using the original broadcaster.deposit before assigning ZERO_BD. Update the sequence around broadcaster.deposit and difference so the reserve deduction reflects only the uncovered amount, then reset broadcaster.deposit to ZERO_BD.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/mappings/ticketBroker.ts`:
- Around line 68-71: In the insufficient-deposit branch, calculate difference
using the original broadcaster.deposit before assigning ZERO_BD. Update the
sequence around broadcaster.deposit and difference so the reserve deduction
reflects only the uncovered amount, then reset broadcaster.deposit to ZERO_BD.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54c63cbb-bd7c-462a-8efc-e8566a4022c1
📒 Files selected for processing (4)
src/mappings/bondingManager.tssrc/mappings/roundsManager.tssrc/mappings/ticketBroker.tsutils/helpers.ts
f7a9b2b to
4e6ab06
Compare
Defensive patch. newRound() builds a Pool per active transcoder via a non-deterministic eth_call enumeration that can miss one, leaving reward() to abort on the null Pool and ticketBroker to drop fees. createOrLoadPool() rebuilds the missing Pool on demand in newRound(), reward(), and ticketBroker() so no handler aborts. The deterministic root fix is tracked in #248.
4e6ab06 to
7c6b016
Compare
Problem
reward()aborts on a nullPool(the halt at block 144056642) andticketBrokersilently drops the fees when a round'sPoolwas never built.newRoundbuilds Pools by walking the on-chain pool with aneth_callenumeration, which is non-deterministic and can come up short. See #248 for the root cause.Fix
Add
createOrLoadPool(round, transcoderAddress)and use it innewRound,reward, andticketBroker, so the Pool is always built through one code path and no handler aborts on a null Pool.totalStake/rewardCut/feeShare) once. Handlers only add their own accumulator (reward→rewardTokens,ticketBroker→fees), never re-touching the snapshot.!— the helper returns a non-null Pool and defaults the non-nullable fields if the Transcoder is ever missing, sosave()can't abort (matches The Graph'sunexpected-nulllinter guidance).Bond/register logs), so Pools get real values. The default branch is a failsafe.Notes
Transaction.tonullable fix (fix: make Transaction.to nullable for contract-creation txs #245) — 144056642 comes before theto-bug block (485100965).