Page MenuHomePhabricator
Differential D17202

[ecash-herald] Remember to pass the staking info param for daily summary msg
ClosedPublic
Actions

Authored by bytesofman on Tue, Nov 26, 06:15.

Details

Reviewers
emack
Group Reviewers
Restricted Project
Commits
rABC9a35304c1008: [ecash-herald] Remember to pass the staking info param for daily summary msg
Summary

Code landed in D17195 but did not implement in event handler

Test Plan

npm test

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bytesofman created this revision.Tue, Nov 26, 06:15
Herald added a reviewer: Restricted Project. · View Herald TranscriptTue, Nov 26, 06:15
bytesofman published this revision for review.Tue, Nov 26, 06:17
Harbormaster completed remote builds in B31320: Diff 51103.Tue, Nov 26, 06:17
emack requested changes to this revision.Tue, Nov 26, 10:33
emack added a subscriber: emack.
emack added inline comments.
apps/ecash-herald/src/events.ts
372 ↗(On Diff #51103)

but you're not returning here, and it's proceeding to summarizeTxHistory below.

This revision now requires changes to proceed.Tue, Nov 26, 10:33
bytesofman requested review of this revision.Tue, Nov 26, 13:23
bytesofman marked an inline comment as done.
bytesofman added inline comments.
apps/ecash-herald/src/events.ts
372 ↗(On Diff #51103)

this is the existing behavior. activeStakers stays undefined, and we simply do not include the msg lines that depend on activeStakers.

We do not want to return if we miss the price API call or the active stakers API call. We still send the msg, just not with the info we fail to get.

we "should" be testing the parent function, handleUtcMidnight, but it's not trivial to mock a days worth of blockTxs. You can see this is the same logic used in getDailySummary.ts, which can be tested with ts-node scripts/getDailySummary

emack accepted this revision.Tue, Nov 26, 13:32
This revision is now accepted and ready to land.Tue, Nov 26, 13:32
Closed by commit rABC9a35304c1008: [ecash-herald] Remember to pass the staking info param for daily summary msg (authored by bytesofman). · Explain WhyTue, Nov 26, 14:07
This revision was automatically updated to reflect the committed changes.
bytesofman marked an inline comment as done.
bytesofman added a commit: rABC9a35304c1008: [ecash-herald] Remember to pass the staking info param for daily summary msg.

Revision Contents
Changeset List

PathSize
apps/
ecash-herald/
src/
14 lines

Diff 51109

View Options

apps/ecash-herald/src/events.ts

Loading...