Conversation
mikezupper
left a comment
mikezupper
left a comment
There was a problem hiding this comment.
All of the changes look good. The changes shows consideration for backwards compatibility.
Caveat: I am not able to run these change to ensure they work with the current subgraph.
|
@rickstaa I am unable to use the test link provided (Test in Graph Studio), is there some other mechanism I can run some tests? Also, have we tested running this subgraph from scratch? I know its a bit out of scope for your change, but I had many issues when I attempted this process last year. |
Added gateway fee tracking with daily snapshots and rolling-window metrics like transcoders, and stored the active gateway set on Protocol so newRound can recompute 30/60/90 day sums efficiently.
rickstaa
force-pushed
the
feat/track-gateway-fees
branch
from
bf0e9f1 to
2762102
Compare
|
🚀 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-168-7e4d7c4-19509005431 |
Ensure that peopel can query the day a broadcaster joined the network (i.e. funded reserve/deposit).
|
🚀 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-168-b602361-19536247588 |
There was a problem hiding this comment.
This functionally looks all good, however I had a few comments that'd be worth your reviewal @rickstaa.
Edit: One further thing I would add is that the use of the term volume in the fields for fees feels a bit ambigious. It seems to track the fees paid out to broadcasters, as well as winning ticket redemptions (which I believe are also a form of fees). I'm not sure whether you want to bundle these together, but I'd suggest the following:
- Rename relevant fields to something less ambigious (e.g.
totalFeesPaidETH) - If nuanced tracking of what fees were paid out by which events (rounds vs tickets), then update the types and mappings accordingly. For example there could be one field for each form of payout, alongside a total fees field.
Bear in mind I have less context on the protocol and data needs of the subgraph than yourself, so I might be wrong with some of these suggestions :)
utils/helpers.ts
Outdated
| protocol.save(); | ||
| } | ||
|
|
||
| // Ensure backwards compatibility |
There was a problem hiding this comment.
What is the rationale behind this backwards compatibility logic?
Are you planning to utilise grafting for these subgraphs?
There was a problem hiding this comment.
@jmulq I initially (incorrectly) assumed grafting was enabled by default. After reviewing the grafting docs, it appears it’s not, and that new subgraph deployments reindex from scratch while the previous deployment remains available. Can you confirm this and that it’s safe to remove this logic? I’ve already applied the commit.
One question to double-check: most of our apps query the Subgraph ID (not a pinned Deployment ID—see explorer). Based on the querying docs (link), my understanding is that publishing a new version should not cause downtime: the network may continue routing queries to the previous deployment while the new one syncs, with the main caveats being potential version skew or schema-breaking changes. Can you confirm this matches your understanding?
There was a problem hiding this comment.
I can confirm you are safe to remove this logic.
Yep your understanding is correct. Until there is a synced indexer on the latest deployment, it will route traffic to n-1.
Simplify and improve documentation around the logic that captures the first active day for a gateway. Co-authored-by: jmulq <james.mulqueeny@outlook.com>
|
🚀 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-168-27a2d04-20483564032 |
Co-authored-by: jmulq <james.mulqueeny@outlook.com>
Grafting is not enabled, so the compatibility code is unnecessary. Co-authored-by: jmulq <james.mulqueeny@outlook.com>
rickstaa
force-pushed
the
feat/track-gateway-fees
branch
from
8b3829f to
4728c85
Compare
@jmulq Thanks for your quick review. Your reasoning makes sense. I created a low priority issue so the community can pick this up if they want. |
|
@mehrdadmms this pull request can be merged and you can work with @yondonfu to deploy it after @jmulq has looked at #168 (comment) and approvied it. This will unblock livepeer/explorer#291 which is not part of the original Explorer RFP but is a nice to have for the community. You can decide when you want to schedule it in. |
jmulq
left a comment
jmulq
left a comment
There was a problem hiding this comment.
Looks good to me following the prior reviewal.
4 participants
This pull request adds gateway fee tracking with daily snapshots and rolling-window metrics similar to what we currently do for transcoders. It also stores the active gateway set on
Protocolso thatnewRoundcan efficiently recompute the 30/60/90-day aggregates. The goal is to make it straightforward to build a gateway dashboard in the explorer.@victorges, @yondonfu, @mikezupper, @ecmulli does this approach make sense, and do you see any potential issues, variables which you want to see added? I kept the existing “broadcaster” naming to avoid divergence for now, we can rename and deprecate it later if needed, but it’s not a priority.
TODOs