Page MenuHomePhabricator

[Cashtab] Support NFT trading
ClosedPublic

Authored by bytesofman on Jun 12 2024, 20:39.

Details

Reviewers
emack
Group Reviewers
Restricted Project
Commits
rABCc1722a1f56f6: [Cashtab] Support NFT trading
Summary

Implement NFT trading with ecash-agora

Supporting functions for this diff have been isolated in other diffs. What remains, though, is still a large diff --- because NFT functionality is interdependent (i.e. ability to list NFTs, browse NFTs, and cancel listed NFTs are all imo MVP-critical).

Integration tests have been added to support expected behavior and this diff has been tested.

After landing this diff, additional NFT improvements are high priority:
Paginate quantity of NFTs loaded and displayed less critical with ecash-agora plugin and by-collection organization

  • Organize listed NFTs by collection
  • Index ABC chronik server for lokad and switch Cashtab back to ABC chronik server
  • Minimize XEC required for a listing TX
  • Ability to manage utxo in the event only 1 of 2 of the required ad listing txs goes through
  • Parse NFT actions in tx history

I do not think we need these in the MVP.

Guide for reviewer

This is one diff, but it can still be reviewed in compartmentalized parts.

  1. Adding a new "Listed NFTs" screen to App.js (icon, menu option, nav tests)
  2. Style changes (new input component for NFT list price, modify modal to expand/collapse with transition to confirm NFT actions)
  3. Functionality of new NFTs screen (manage your listings, browse other listings, cancel listing, buy listing)
  4. Functionality added to existing Token screen (list your NFT, price input fiat or XEC. Note this function requires two transactions which are executed with one click.

This diff is available for testing at https://cashtab-local-dev.netlify.app/ (make sure version is v2.41.6)

Test Plan

npm test

Diff Detail

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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

remove debug logs, add test to confirm wallet change behavior for NFT screen, organize TODO(s)

bytesofman edited the summary of this revision. (Show Details)

nav test for new screen

bytesofman edited the summary of this revision. (Show Details)
bytesofman edited the summary of this revision. (Show Details)
tobias_ruck added inline comments.
cashtab/src/config/chronik.js
9 ↗(On Diff #48842)

what's going on here?

cashtab/src/config/chronik.js
9 ↗(On Diff #48842)

Might not land it like this.

Right now, ABC has a chronik server and Cashtab defaults to this.

pay2stay is the backup

But, the ABC server is not lokadid indexed yet. So we need to use pay2stay to support NFT trading.

emack added inline comments.
cashtab/src/config/chronik.js
9 ↗(On Diff #48842)

It should be a seamless transition to the lokad indexed https://chronik-native.fabien.cash right since nothings deprecated.

bytesofman added inline comments.
cashtab/src/config/chronik.js
9 ↗(On Diff #48842)

while this server would probably work for Cashtab, it's a dev server and not at the same specs as the prod server chronik-native1.fabien.cash

emack requested changes to this revision.Jul 26 2024, 15:27

On a semi related note, not for this diff, but if you try to use a non-jpg/png file (e.g. webp) for the NFT collection it crashes the app (blank white screen), with the console spitting out TypeError: Cannot read properties of undefined (reading 'size').

cashtab/src/components/App/App.js
468 ↗(On Diff #48842)

This is where the UX gets a bit confusing navigating to the NFT component and not actually seeing the wallet's unlited NFTs shown here. As part of MVP I think you should either:
a) distinguish NFTs from SLP1 tokens in the eTokens component;
b) move the NFTs to the NFT component; or
c) make it clear the NFT component is actually a marketplace (i.e. Trade NFTs)

cashtab/src/components/Common/NftListingActions.js
138–142 ↗(On Diff #48842)

This should be a Red button to alert the user this is a final click before finalizing the action. It would apply to both cancellation of listing and buying of an NFT so don't need to change the conditional rendering logic here.
The 'Never mind' should be labelled as 'Cancel' for consistency.

cashtab/src/components/Etokens/Token/index.js
910 ↗(On Diff #48842)

use appConfig.derivationPath

cashtab/src/components/Nfts/index.js
69 ↗(On Diff #48842)

IIUC, both myListings and offeredListings are only ever stored as state variables? If so, every load of this nfts component will always trigger getAllTxHistoryByLokadId which traverses through the entire agora lokad history which will cause scalability issues later on. Since myListings and offeredListings are only state vars you'll always get that delay every time you navigate to this page while it retrieves every single lokad history page from chronik, which is already 1.5 seconds with 3 NFTs list and half a dozen associated lokad history txs.

I would suggest storing listedNfts const into localforage and then preload it in useEffect(), and THEN kick off the above to refresh the listings in the background, similar how cashtab tx history loads the last cached history and then updates to the latest.

107 ↗(On Diff #48842)

If you go down the route of directly using chronik response numPages prop to derive frontend pagination pages then keep in mind the scenario where one chronik page might have txs which aren't meant for frontend listings display (e.g. cancel action, buy actions) and you end up with some pages with only 1-2 NFT listings whilst other pages contain 25 NFT listings. On the other hand if you intend to retrieve all lokad history pages and then paginate from within the app then just keep in mind the increasing processing time.

122 ↗(On Diff #48842)

Needs a loading spinner of some sort here. The underlying lokad id lookup call will increase loading time very quickly once this launches.

268 ↗(On Diff #48842)

Presumably this will always be shown upon app startup right? I haven't seen this even on new wallet on a fresh browser. I just see an empty render for 1-2 seconds and then the listing below shows up.

This revision now requires changes to proceed.Jul 26 2024, 15:27
bytesofman added inline comments.
cashtab/src/components/App/App.js
468 ↗(On Diff #48842)

For now, going with the (easiest) c

"Listed NFTs"

the UX problem of organizing and presenting all of the user's NFTs in one place is big enough to warrant its own diff, if not a handful of diffs.

cashtab/src/components/Common/NftListingActions.js
138–142 ↗(On Diff #48842)

imo the buttons are ok. we have a confirmation step. mb nicer to have an even more "this is serious" button color, but I don't think we need it to launch.

primary buttons are already "final decision" buttons elsewhere in the app.

Updated the "Never Mind" to "Cancel" for the "buy" decision ... but leaving it for the "cancel listing" decision. too ambiguous to cancel the decision to cancel...does this cancel?

cashtab/src/components/Nfts/index.js
69 ↗(On Diff #48842)

this is better, but I don't want to overengineer.

The "right way" to do this is to use a plugin to better handle the original query. Instead of

  • ask for all the LOKAD ID txs
  • parse them all

we should

  • ask for only the LOKAD txs you want (i.e. unsold NFTs)
  • parse them all

The current approach is not scalable and may need to be patched / improved before we have better chronik indexing ready. But I think it's good enough to launch and see what happens. This diff is large/complex enough as it is, trying to bundle caching would be too much complexity.

107 ↗(On Diff #48842)

good point, will need to see how this shakes out in the wild. as above, the right solution is probably with customized plugin for chronik.

268 ↗(On Diff #48842)

I added a spinner for when we are actually waiting for API calls

bytesofman marked 2 inline comments as done.

better screen name, spinner for loading on NFT screen, use const instead of 1899

cashtab/src/components/Nfts/index.js
72 ↗(On Diff #48894)

D16544 is now passing, I think we should at least have an instance up with the Agora plugin enabled so we don't have to use this unscalable option—if it becomes popular over night it could get very slow

Accepted subject to a decision on plugin use vs current implementation

This revision is now accepted and ready to land.Jul 30 2024, 06:00

will look at getting the chronik plugins supported first

implementing upgraded ecash-agora

This revision is now accepted and ready to land.Sep 9 2024, 23:35
cashtab/src/config/chronik.js
9 ↗(On Diff #49547)

Maybe upgrade one other instance to have the LOKAD ID index + agora plugin enabled

cashtab/src/wallet/useWallet.js
228 ↗(On Diff #49547)

is this kept intentionally?

bytesofman marked an inline comment as done.

Still need to clean up debug logs, run some tests. This updated version has been deployed to the test site.

cashtab/src/wallet/useWallet.js
228 ↗(On Diff #49547)

no -- I will mark this "changes planned," I still have some more clean-up to do here.

Also need to implement the new agora tx building functions -- but mb that can wait until this is in prod. that is more an optimization, while the migration to agora plugin queries gets us over the lokad query scaling issue.

bytesofman marked an inline comment as done.

more robust handling of cached token info, do not display BrowseCollection modals if offers have not loaded yet

This revision is now accepted and ready to land.Sep 10 2024, 20:44

remove unneeded lokad mocks from app test, improve comments, remove debug comments

Big diff but I think this is enough to get into prod.

The biggest thing that could go here instead of being added later would be using the new ecash-agora transaction builder methods, with fuelInputs, instead of the current manual-ish approach.

At this point though, getting user data is I think more important, and it's already pretty easy to get lost with the changes going on here.

Cashtab also needs to implement Typescript. With ecash-agora, ecash-lib and chronik-client all implementing typescript and supporting a wide array of complex types, it's getting more complicated to implement this stuff in Cashtab without typescript.

add test and improve logic for showing chronik query error when we cannot get the info we need to load the page

This revision was automatically updated to reflect the committed changes.