Page MenuHomePhabricator

[ecash-herald] Support daily summary msgs
ClosedPublic

Authored by bytesofman on Oct 15 2024, 11:32.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC263ae951c8e7: [ecash-herald] Support daily summary msgs
Summary

Send a daily summary msg every 144 blocks

ecash-herald in its current form is a useful reference or alert system for power users. I think we could get more impact from higher level summaries of daily on chain activity. This is more representative of eCash KPIs.

Start with a simple template. Going forward, will be an editorial question what we should be tracking.

Test Plan

npm test, review msg sample

image.png (603×492 px, 67 KB)

after updating diff:

image.png (647×492 px, 79 KB)

Diff Detail

Repository
rABC Bitcoin ABC
Branch
herald-daily-summaries
Lint
Lint Errors
SeverityLocationCodeMessage
Errorapps/ecash-herald/src/parse.js:2228ESLINTno-unused-vars
Errorapps/ecash-herald/src/parse.js:2287ESLINTno-unused-vars
Errorapps/ecash-herald/test/parse.test.js:48ESLINTno-unused-vars
Unit
No Test Coverage
Build Status
Buildable 30601
Build 60718: Build Diffecash-herald-tests
Build 60717: arc lint + arc unit

Event Timeline

better logs, patch getDailySummary script to accept arbitrary blockheight

bytesofman added inline comments.
apps/ecash-herald/src/events.js
107 ↗(On Diff #50108)

note: this functionality (is the daily summary triggered if blockheight is a multiple of 144) is not tested, as imo would really just be testing mock chronik client and this specific if statement

we unit test building the function, and we have the script getDailySummaryTxs to confirm the behavior here

apps/ecash-herald/test/mocks/dailyTxs.js
8 ↗(On Diff #50108)

don't really need to review this file but it also isn't really generated

going forward, will add txs to this mock that we expect to be somehow described by summarizeTxHistory

Fabien requested changes to this revision.Oct 15 2024, 13:14
Fabien added a subscriber: Fabien.
Fabien added inline comments.
apps/ecash-herald/src/parse.js
2313 ↗(On Diff #50108)

Might not be 24 hours. Actually it would be interesting to have the opposite value, e.g. how many blocks in 24h (otherwise how many time for the 144 blocks) so we can kinda see the impact of heartbeat

2317 ↗(On Diff #50108)

Not sure what is wrong here but the value from your screenshot is obviously off by a lot

2324 ↗(On Diff #50108)

Would be nice to show also as a percentage, same for staking rewards. Can be another diff if you don't want to add too much to this one

This revision now requires changes to proceed.Oct 15 2024, 13:14
bytesofman marked an inline comment as done.

show %s for miners and stakers, better formatting, show the thru-blockheight

apps/ecash-herald/src/parse.js
2158 ↗(On Diff #50115)

This is actually hardcoded at the callsite so you could as well pass down the value. But I like the idea that it can be a time interval so I'm OK if you want to keep it that way to support future improvements.

apps/ecash-herald/src/parse.js
2313 ↗(On Diff #50108)

Updated the description. imo 24 hrs is better, but this is a more sophisticated algorithm. I think better to do this in its own diff, since would want tests for that.

2317 ↗(On Diff #50108)

not the same as the screenshot earlier but, just running it,

image.png (647×492 px, 71 KB)

This is what the map looks like:

sortedMinerMap Map(34) {
  'Mining-Dutch' => 66,
  'solopool.org' => 22,
  'Molepool' => 9,
  'Cminors-Pools' => 7,
  'unknown, ...863u' => 6,
  'ViaBTC, Mined by 192233' => 3,
  'ViaBTC, Mined by karan23' => 2,
  'p2p-spb' => 2,
  'ViaBTC, Mined by vetal737' => 2,
  'ViaBTC, Mined by niessuh' => 1,
  'ViaBTC, Mined by sadeghdd' => 1,
  'ViaBTC, Mined by itts' => 1,
  'ViaBTC, Mined by hossein111982' => 1,
  'ViaBTC, Mined by dam200' => 1,
  'ViaBTC, Mined by bsnvse2' => 1,
  'ViaBTC, Mined by sashabel' => 1,
  'ViaBTC, Mined by abdo2020' => 1,
  'ViaBTC, Mined by ded001ok' => 1,
  'nodeStratum' => 1,
  'ViaBTC, Mined by visn02' => 1,
  'ViaBTC, Mined by vhab' => 1,
  'ViaBTC, Mined by vatavata' => 1,
  'ViaBTC, Mined by vitaliy2024' => 1,
  'ViaBTC, Mined by mahdii63' => 1,
  'ViaBTC, Mined by danieli7' => 1,
  'ViaBTC, Mined by rmozh88' => 1,
  'ViaBTC, Mined by moordoc' => 1,
  'ViaBTC, Mined by marchenkoda' => 1,
  'ViaBTC, Mined by simoblak' => 1,
  'ViaBTC, Mined by nocomblink' => 1,
  'ViaBTC, Mined by aminrs' => 1,
  'ViaBTC, Mined by 9143959441' => 1,
  'ViaBTC, Mined by alexey83' => 1,
  'ViaBTC, Mined by chaster228' => 1
}

total 144 blocks

I added a "top 3" label to clarify that not all miners are displayed

2324 ↗(On Diff #50108)

easy enough to put this in now

group viabtc miners in line item

Fabien added inline comments.
apps/ecash-herald/src/parse.js
2324 ↗(On Diff #50119)

imo the miner count is superfluous

This revision is now accepted and ready to land.Oct 15 2024, 20:07