Page MenuHomePhabricator
Differential D18909

[e.cash] Pre-consensus countdown banner edit
ClosedPublic
Actions

Authored by johnkuney on Fri, Nov 7, 17:28.

Details

Reviewers
bytesofman
Group Reviewers
Restricted Project
Commits
rABCd6efa3681be9: [e.cash] Pre-consensus countdown banner edit
Summary

Was requested to add some more granularity to the countdown bar. ie watching the actual block count until its live

Previous behavior > Display countdown text and timer until after the end date, and then display upgrade complete text

New behavior > Display countdown text and timer until after the end date, then start polling an avalanche api that has the blocks remaining
until the feature is officially live (when it reaches -1), then display the its live text

Also added a grace period of 6 hours after the target date to skip the block count and just go straight to live message as it should be live by then and we dont want to be flashing different states days after the upgrade

Added some tests for this behavior

Test Plan

preview and check, npm test

Diff Detail

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

Event Timeline

johnkuney created this revision.Fri, Nov 7, 17:28
Herald added a reviewer: Restricted Project. · View Herald TranscriptFri, Nov 7, 17:28
Harbormaster completed remote builds in B35017: Diff 56522.Fri, Nov 7, 17:28
johnkuney updated this revision to Diff 56523.Fri, Nov 7, 17:29

add headers

Harbormaster completed remote builds in B35018: Diff 56523.Fri, Nov 7, 17:30
johnkuney retitled this revision from [e.cash] Countdown banner edit to [e.cash] Pre-consensus countdown banner edit.Fri, Nov 7, 17:38
johnkuney edited the summary of this revision. (Show Details)
johnkuney updated this revision to Diff 56524.Fri, Nov 7, 17:53
johnkuney edited the summary of this revision. (Show Details)

add skip polling grace period

Harbormaster completed remote builds in B35019: Diff 56524.Fri, Nov 7, 17:53
johnkuney edited the summary of this revision. (Show Details)Fri, Nov 7, 17:57
johnkuney updated this revision to Diff 56527.Fri, Nov 7, 19:17

add test

Harbormaster completed remote builds in B35021: Diff 56527.Fri, Nov 7, 19:17
johnkuney published this revision for review.Fri, Nov 7, 19:18
bytesofman added a subscriber: bytesofman.Fri, Nov 7, 21:57

@bot preview-e.cash

teamcity awarded a token.Fri, Nov 7, 21:57

Build Bitcoin ABC Diffs / Diff Testing (preview-e.cash) passed.
Preview is available at http://51.91.14.25:41818 for the next 60 minutes.

bytesofman requested changes to this revision.Fri, Nov 7, 22:14

overall...questionable decision to use an API for this at all.

it's not too difficult to implement chronik-client in a nextjs app, stick it in context, have it subscribe to block msgs, poll for the chaintip on page load and use the chain tip and its timestamp to set initial state.

advantage is the websocket is to-the-second accurate. Lose the server API though chronik is ofc it's own API.

but could just use this, poll more frequently ... looks like an improvement vs current countdown.

web/e.cash/app/components/Atoms/UpgradeCountdown.tsx
116 ↗(On Diff #56527)

seems like a pretty long interval. for so much engineering into getting the countdown, this is basically guaranteed to be off by a few minutes.

imo ... even the API itself is probably overkill. Better way to do this would be subscribe to the blocks websocket

then, on finalized block, check if you are before or after the deadline

if you are after, start setting the state

if you are 6 after, trigger the countdown

I can see how this would be a bit more complicated to implement, esp as you need some way to set the state "on load" -- tho this could be done with this API endpoint.

for now, probably good enough to just poll every 10s instead of every 5 min.

web/e.cash/app/components/Atoms/__tests__/UpgradeCountdown.test.tsx
145–147 ↗(On Diff #56527)

AI really loves to test that errors are logged

imo kinda dumb, should not do this

can lose all the mocks and tests that we console.error what is expected

This revision now requires changes to proceed.Fri, Nov 7, 22:14
johnkuney updated this revision to Diff 56532.Sat, Nov 8, 02:30

adjust tests, change api polling to 10s

Harbormaster completed remote builds in B35024: Diff 56532.Sat, Nov 8, 02:30
bytesofman requested changes to this revision.Sat, Nov 8, 17:53

fix is good, test file still seems to have some AI copypasta

i patched the diff and removed this stuff, tests pass and so does arc lint

can you do the same or do you see errors?

web/e.cash/app/components/Atoms/__tests__/UpgradeCountdown.test.tsx
5 ↗(On Diff #56532)

shouldn't need this

6 ↗(On Diff #56532)

should not need to import this

8 ↗(On Diff #56532)

should not need to import these

12 ↗(On Diff #56532)

not sure why this is here

This revision now requires changes to proceed.Sat, Nov 8, 17:53
johnkuney updated this revision to Diff 56555.Mon, Nov 10, 16:20

clean up test file

Harbormaster completed remote builds in B35037: Diff 56555.Mon, Nov 10, 16:20
johnkuney added a comment.Mon, Nov 10, 16:21

cool, removed those, test and lint pass

bytesofman accepted this revision.Mon, Nov 10, 17:19
This revision is now accepted and ready to land.Mon, Nov 10, 17:19
Closed by commit rABCd6efa3681be9: [e.cash] Pre-consensus countdown banner edit (authored by johnkuney). · Explain WhyMon, Nov 10, 17:36
This revision was automatically updated to reflect the committed changes.
johnkuney added a commit: rABCd6efa3681be9: [e.cash] Pre-consensus countdown banner edit.

Revision Contents
Changeset List

PathSize
web/
e.cash/
app/
components/
Atoms/
80 lines
__tests__/
151 lines
2 lines
5 lines
2 lines

Diff 56557

View Options

web/e.cash/app/components/Atoms/UpgradeCountdown.tsx

Loading...
View Options

web/e.cash/app/components/Atoms/__tests__/UpgradeCountdown.test.tsx

Loading...
View Options

web/e.cash/jest.config.js

Loading...
View Options

web/e.cash/jest.setup.ts

Loading...
View Options

web/e.cash/next.config.ts

Loading...