Page MenuHomePhabricator

[Cashtab][Alias] Sync pricing on new blocks
ClosedPublic

Authored by emack on Nov 2 2023, 14:31.

Details

Reviewers
bytesofman
Fabien
Group Reviewers
Restricted Project
Commits
rABCcfa2e804dff0: [Cashtab][Alias] Sync pricing on new blocks
Summary

This diff listens for 'BlockConnected' events, which then retrieves the latest alias registration fees from alias-server and checks whether prices have changed or not based on the length of the pricing array, before rendering as a localized price preview in Alias.js.

At this point in time the chronik-client module is still being upgraded to work with the in-node chronik indexer, therefore it is not efficient to be making a blockheight comparison between server and cashtab arrays. This is needed due to the potential for multiple pricing tiers in the pricing array returned from alias-server, where the latest entry could have a startHeight into the future.

As a temporary mitigation it is assumed initial go live pricing will always be a single entry array. Once chronik-client has been upgraded, this price parsing logic will be updated to use the new websocket for blockheight comparisons. The intention is to eventually reverse loop through aliasPrices.prices and parse for the latest array entry that has a startHeight within the chain's tipHeight.

Test Plan
  • npm test
  • Enable alias flag in config
  • npm start
  • Navigate to Alias.js, enter a valid alias and verify the pricing preview being displayed per keystroke
  • Enter an invalid alias and ensure the "Please enter an alias (lowercase a-z, 0-9) between 1 and 21 bytes" error is displayed
  • Manually add an additional entry to the aliasPrices.prices array inside Alias.js (e.g. aliasPrices.prices.push({foo: 'asdfa'});) so that the length would be > 1, and verify the "Alias registration is temporarily unavailable, please check again later" error is displayed and both the Check Alias and Register Alias buttons are disabled
  • Manually empty the aliasPrices.prices array inside Alias.js (e.g. aliasPrices.prices = [];) so that the length would be 0, and verify the same "Alias registration is temporarily unavailable, please check again later" error is displayed and both the Check Alias and Register Alias buttons are disabled
  • Temporarily reintate previous debug log statements in processChronikWsMsg(), open dev console, wait for a new block to be found and observe the "New block found, refreshing alias prices" message in console with the latest prices displayed matching what's on the alias-server prices endpoint

image.png (128×344 px, 12 KB)

  • Regression test alias component features that calls upon the /address and /alias endpoints (e.g. check the registered downdown, register a new alias, attempt to re-register and observe pending dialog) to ensure the changes to queryAliasServer in this diff did not introduce new bugs.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
pricesLookup
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 25679
Build 50938: Build Diffcashtab-tests
Build 50937: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
bytesofman requested changes to this revision.Nov 2 2023, 19:30
bytesofman added inline comments.
cashtab/src/hooks/useWallet.js
875 ↗(On Diff #42920)

Seems a bit much to be setting this every block to something at is not expected to change frequently.

Can compare the received prices with what is set and, if they are the same, don't set anything.

Per spec, any change to alias pricing needs a new startHeight. So, can just check that aliasPrices.length === aliasPricesResp.length ... if this is true, the prices haven't changed, bail on the function.

Even with this tho -- still feels overkill to be checking something every block -- every cashtab user, everywhere, always checking something that is going to be constant unless bitcoin abc (so, us) changes it. It is more robust, and I get how it would be bad practice to just hardcode it in two places. Maybe change the check interval to some fraction of the minimum block notice for a price change, so it isn't every block? just on startup and then, say, every 10 blocks.

For Cashtab purposes, we only need to update them the block before they change. So, 90 blocks before is still pretty good.

This revision now requires changes to proceed.Nov 2 2023, 19:30

Updated processChronikWsMsg() to refresh pricing every 90 blocks rather than every block

When you run this diff for the first time it will refresh anyway even if the pricing height is not divisible by 90, because the aliasPricing state var starts off as null on app load and needs to be populated. After this, it will pick up a new block and then skip the refresh if the height is not divisible by 90.

If the prices array length between the server and cashtab matches then it will also skip the refresh as it would indicate no new pricing tiers have been published.

image.png (214×339 px, 30 KB)

Unit tests for processChronikWsMsg() updated for the following permutations:

  1. Alias prices refreshed with default null aliasPricing and prices height divisible by 90
  2. Alias prices refreshed with default null aliasPricing and prices height not divisible by 90
  3. Alias prices refreshed with existing aliasPricing and prices height divisible by 90
  4. Alias prices NOT refreshed with existing aliasPricing and prices height not divisible by 90
  5. Alias prices NOT refreshed with existing aliasPricing and prices height divisible by 90 and prices array length unchanged
  6. Alias prices NOT refreshed with existing aliasPricing and prices height not divisible by 90 and prices array length unchanged
Fabien requested changes to this revision.Nov 3 2023, 13:40
Fabien added a subscriber: Fabien.
Fabien added inline comments.
cashtab/src/hooks/useWallet.js
865 ↗(On Diff #42925)

I must have missed something here...

I'm OK with the comment from @bytesofman, you don't need to query for every block since the prices endpoint will tell you the list of changes and there is a minimal notice delay when a new change is added. Updating every 90 blocks (why this value ???) is probably fine if you don't have another startHeight already announced (like it is now), and when you have one you can check it's more than 90 blocks ahead and keep going.

But what's the point of all of this if you're querying the server every block ? This completely defeats the purpose of this optimization ?

This revision now requires changes to proceed.Nov 3 2023, 13:40

I must have missed something here

image.png (263×446 px, 165 KB)

ok, you didn't miss anything, the above approach doesn't work. Any block height related optimization will incur an API call to get the latest blockheight as a minimum.

Discussed alternate approaches with Joey, including a direct cross-monorepo reference to the alias prices config in alias-server repo directly, similar to how some apps in the monorepo are directly importing the mock-chronik-client module via relative path.

However React has a very strict restriction on importing modules from outside of its /src/ path.
There are two ways of getting around it:

  1. Disable certain elements of webpack config (not recommended as we would lose other protections)
  2. Symlink (not recommended, as it's for importing entire npm packages and we don't want to import the whole alias-server into Cashtab)
  3. Simply duplicating the prices constant in Cashtab config (not recommended for obvious reasons)

In addition, for approaches 1 & 2 above to work we would need to somehow modify Cashtab's teamcity trigger to build and deploy if alias-server alias constants change, which all up is a significant chunk of engineering for a low ROI impact.

Long story short, we concluded your original suggestion of polling prices on each new block is the way to go and updated diff accordingly.

The unit test permutations are now:

  • processChronikWsMsg() refreshes alias prices when aliasPrices is null
  • processChronikWsMsg() refreshes alias prices when aliasPrices exists, server and cashtab prices array length do not match
  • processChronikWsMsg() DOES NOT refresh alias prices when aliasPrices exists, server and cashtab prices array length do match

Console log samples:

  • When new block found and aliasPrices in Cashtab is yet to be populated

image.png (163×324 px, 17 KB)

  • When new block found and aliasPrices in Cashtab matches the latest response from alias-server

image.png (158×315 px, 19 KB)

Any block height related optimization will incur an API call to get the latest blockheight as a minimum.

Not with websockets ?

Any block height related optimization will incur an API call to get the latest blockheight as a minimum.

Not with websockets ?

Nay, the BlockConnected websocket message only contains the message type and block hash. Usually you'd pass the block hash to the chronik client's block() API to get more info but that itself is an API call.

The other websocket messages don't return height either.

Any block height related optimization will incur an API call to get the latest blockheight as a minimum.

Not with websockets ?

Nay, the BlockConnected websocket message only contains the message type and block hash. Usually you'd pass the block hash to the chronik client's block() API to get more info but that itself is an API call.

The other websocket messages don't return height either.

Damned I just checked, it's a feature of the built-in chronik version, not the NNG one...

bytesofman requested changes to this revision.Nov 6 2023, 13:59
bytesofman added inline comments.
cashtab/src/hooks/useWallet.js
75 ↗(On Diff #42948)

So this only becomes not-null after a block has been found and the user has Cashtab open?

For now then this is totally independent of the Alias.js screen and how it gets price info.

The Alias screen should pull in aliasPrices as a context variable. If they are null, API request to set them. If not, use the context var.

If this is going to be a separate diff, summary should be updated. But imo appropriate to do this here so we know we're not engineering this state var in a way where it's not readily useful.

863 ↗(On Diff #42948)

useful for testing but won't want this in prod.

872 ↗(On Diff #42948)

expand this comment. it's not necessarily obvious to someone uninvolved in alias dev that any price change requires a length change to the object.

cashtab/src/utils/aliasUtils.js
63 ↗(On Diff #42948)

why is this changed here?

This revision now requires changes to proceed.Nov 6 2023, 13:59
emack marked 4 inline comments as done.

Alias.js now pulls aliasPrices as a context variable and triggers a regresh if none exist.

cashtab/src/utils/aliasUtils.js
63 ↗(On Diff #42948)

Prices endpoint has no params and if applied to the original state of queryAliasServer it will error out because it basically becomes /prices/null.

bytesofman requested changes to this revision.Nov 7 2023, 14:23
bytesofman added inline comments.
cashtab/src/components/Alias/Alias.js
102 ↗(On Diff #42966)

ok i see. didn't realize aliasPrices is distinct from our current price source, which comes from pinging the server's /alias/<alias> endpoint.

In this case, go ahead and implement the displayed pricing change on keystroke in this diff.

Otherwise, we just have the object and it is not really clear how it is going to be used. For example, maybe it is best to condition aliasPrices in useWallet.js, maybe the object could be a more useful shape on the server-side, etc -- this is the kind of stuff that could come up by actually implementing aliasPrices` in a use case here. So, we should be implementing it in this diff.

152 ↗(On Diff #42966)

let's get away from this generalized notation (!aliasPrices) to mean "aliasPrices is not null"

In this case, aliasPrices is initialized as null

so, if (aliasPrices === null) {}...

can aliasPrices ever be false?

cashtab/src/utils/aliasUtils.js
60–67 ↗(On Diff #42966)

Don't test undefined for an optional param. Give it a default value.

This revision now requires changes to proceed.Nov 7 2023, 14:23
emack marked 3 inline comments as done.

Added real time localized pricing displays per alias input, improved not null checks, added default value for aliasPAram in queryAliasServer args.
LocalString formatting applied to pricing since it's likely to eventually be a large number in prod.

image.png (414×563 px, 42 KB)

bytesofman requested changes to this revision.Nov 8 2023, 22:17
bytesofman added inline comments.
cashtab/src/components/Alias/Alias.js
122–123 ↗(On Diff #42983)

no state fields that are derivatives of other state fields

573–582 ↗(On Diff #42983)
This revision now requires changes to proceed.Nov 8 2023, 22:17
emack marked 2 inline comments as done.

Removed aliasFee state far and updated component to access directly from the aliasPrices object

bytesofman added inline comments.
cashtab/src/components/Alias/Alias.js
583 ↗(On Diff #43009)

would be nice to also show the price in fiat here, but that can be another diff

cashtab/src/hooks/useWallet.js
878 ↗(On Diff #43009)

remove

882–885 ↗(On Diff #43009)

remove

emack marked 3 inline comments as done.

Removed debug logs

Fabien requested changes to this revision.Nov 9 2023, 10:12

This is just wrong. You're not using the correct price.

This revision now requires changes to proceed.Nov 9 2023, 10:12

Per tg chat, disabling alias registration if the pricing array from API has more than one entry. I've kept the Alias component itself still accessible as the user should still be able to check their purchased aliases, which is separate from pricing matters.

image.png (347×653 px, 46 KB)

Fabien requested changes to this revision.Nov 19 2023, 19:33
Fabien added inline comments.
cashtab/src/components/Alias/Alias.js
576 ↗(On Diff #43163)

You also need to check for length > 0 or the later array access will be out of bounds

579 ↗(On Diff #43163)

The error message is quite cryptic. You deliberately disabled the feature because there is an unsupported condition.
Suggestion: "Alias registration is temporary unavailable, please check again later."

594 ↗(On Diff #43163)

Just prices[0], as per your above verification

This revision now requires changes to proceed.Nov 19 2023, 19:33
emack marked 3 inline comments as done.

Updated array length checks, error message and price element index.

image.png (357×587 px, 43 KB)

Fabien requested changes to this revision.Nov 20 2023, 08:16

Please update your summary and test plan to match the last diff revision

cashtab/src/components/Alias/Alias.js
572

if the length is 0 you want to display the same error message as below, the check is misplaced

This revision now requires changes to proceed.Nov 20 2023, 08:16
emack edited the test plan for this revision. (Show Details)
emack marked an inline comment as done.
emack edited the summary of this revision. (Show Details)

Updated for pricing array length 0 check, diff summary and test plan.

Fabien requested changes to this revision.Nov 21 2023, 13:49

wait for a new block to be found and observe the "New block found, refreshing alias prices" message in console

I think this message has been removed ?

This revision now requires changes to proceed.Nov 21 2023, 13:49
emack requested review of this revision.Nov 21 2023, 20:24

wait for a new block to be found and observe the "New block found, refreshing alias prices" message in console

I think this message has been removed ?

It has but this is why it says "Temporarily reintate previous debug log statements..." beforehand because it is still a material change in this diff and should remain as a test, albeit requiring a manual log insertion to verify.

This revision is now accepted and ready to land.Nov 21 2023, 21:33
This revision was automatically updated to reflect the committed changes.