Page MenuHomePhabricator

[Cashtab] Add ecash-agora lib and helper methods
ClosedPublic

Authored by bytesofman on Jul 17 2024, 22:28.

Details

Reviewers
emack
Group Reviewers
Restricted Project
Commits
rABC6964bcd2d10f: [Cashtab] Add ecash-agora lib and helper methods
Summary

Add (unimplemented) ecash-agora methods to support NFT trading in Cashtab. See D16328 for implementation.

It makes sense to split these helper methods off in their own diff.

  • Easier to review function and tests in isolation (can reference D16328 for intended implementation)
  • We can test that ecash-agora does not break Cashtab deployment before we launch the feature

Why we need the getTokenOfferMaps method

Whenever a user goes to the NFT listings page, we need to query chronik for the most recent listings. However, Cashtab does not necessarily support every agora listing. This process may be simplified in the future by a chronik plugin. However we can handle this process client-side.

Cashtab needs to take all indexed agora offers and

  • Throw out any that are already spent (i.e. sold or canceled), as we do not render these for now (going forward, mb we want to show things like "most valuable sale this week")
  • Throw out any that are not explicitly supported by Cashtab, i.e. SLP1 NFT sales with valid SLP 1 NFT outputs
  • Organize valid offers into two categories, myListings (which the user can cancel) and offeredListings (which the user can buy).
Test Plan

npm test, CI tests pass (updated dep)

For CI deployment, docker build -f cashtab.Dockerfile -t cashtab_local . builds

Diff Detail

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

Event Timeline

bytesofman edited the summary of this revision. (Show Details)
bytesofman edited the summary of this revision. (Show Details)
emack requested changes to this revision.Jul 18 2024, 09:53
emack added a subscriber: emack.
emack added inline comments.
cashtab/src/agora/__tests__/index.test.js
28–33 ↗(On Diff #48712)

what's the significance of using the BASE_ prefix in var names? Will there be an alternate layer of token ID, seller hash, price...etc?

462 ↗(On Diff #48712)

All of the above tests results in either a fresh new Map or a Map of size 1. Add a test for getTokenOfferMaps where you're expecting an offer to be appended to an existing non-empty Map for myListings and offeredListings so you know the function isn't inadvertently overwriting the existing Map entries.

cashtab/src/agora/index.js
23 ↗(On Diff #48712)

missing publicKey

39 ↗(On Diff #48712)

if publicKey is undefined you'll get a potential false negative from isEqualTypedArray further down

60 ↗(On Diff #48712)

I assume this payload will be added to a README somewhere in its own diff later on?

120 ↗(On Diff #48712)

don't let Tobias see this

This revision now requires changes to proceed.Jul 18 2024, 09:53
bytesofman marked 5 inline comments as done.

throw error in getTokenOfferMaps if called without a publicKey

cashtab/src/agora/__tests__/index.test.js
28–33 ↗(On Diff #48712)

In building up a complicated mock type, there are often cases where we want to test "the same thing, but with one key different."

In this case, we want to test a valid agora tx, which we call BASE_AGORA_TX. All of the "default" keys here, I am also prefixing with "BASE_"

This is not any kind of official convention, it's just something I started doing for chronik-client tests and have continued here.

That way, if we want to test "agora tx that is kind of like the base one but a little different," we can do

{...BASE_AGORA_TX, someKey: 'some value that is not the base value'}

This is the logic I am using in composing the tests. So, generally, there may be alt values ... but usually they are on-off and do not get their own variable.

It's a little more complicated for these tests because we can't just store an agora tx as an object, since it includes special types. We have to recreate the Script for each test. This is why the tests here are all built in this file instead of pulled from vectors.

462 ↗(On Diff #48712)

For now, the function is expected to overwrite existing Map entries. myListings and offeredListings are never cached, they are state variables that depend on the current lokadId tx history for agora txs.

They are re-calculated every time the NFT page is loaded (or every time the tx history changes, see NFT market diff).

Potential optimization to cache these but would be handled in a later diff. Tricky to handle though. Lokad Id tx history won't change, but an "old" lokad id tx can change -- i.e. it is unspent, but becomes spent. So you can't just cache the lokad history. Need to check something on every page load or have a chronik plugin that indexes only the unspent offers.

cashtab/src/agora/index.js
60 ↗(On Diff #48712)

Probably not. This is pretty technical dev documentation. Only people who would want or need to know are devs.

For devs, this is already documented (well enough) by the integration tests in ecash-agora.

Cashtab users don't need to worry about this.

120 ↗(On Diff #48712)

😐

This revision is now accepted and ready to land.Jul 20 2024, 14:41