Details

Reviewers

Group Reviewers

Restricted Project

Commits

rABC9d3e439afb0a: [Cashtab] Create standalone component for Etokens

Summary

T3364

The legacy access point for the etokens screen has always been a bit unintuitive so this diff decouples it from the Home component and establishes it as a standalone component accessed from the bottom navigation menu. This will also make it easier to update eToken related UI in the future without getting tied up amongst TxHistory code.

The removal of the original Etokens tab on the Home component meant there is no reason for the active tab switching menu (since it's just Tx History now by itself), hence removing the tab altogether in Home.js.

This is updated for the extension as well.

Note: We should announce this adjustment via twitter once landed as it may not be obvious to users.

Separate future diffs will enhance the Send component to incorporate the sending of etokens on the same page.

Test Plan

npm test
npm start
Click on the new Etokens component, pick an existing token in the wallet and verify it sends successfully to a designated ecash address. Then ensure the specific token balances are updated on the sending and receiving wallets
Switch to another wallet, and visit both the Home and the new Etokens components
Create a new token via the new Etokens component and ensure it is successfully minted to the active wallet
Create a fresh new unfunded wallet and ensure a) the onboarding message and styling on Home.js remains unchanged and b) navigating to the new Etokens component will result in the "Tokens sent to your eToken address will appear here" message being rendered
Fund this newly created wallet and ensure the onboarding message is no longer rendered on the Home.js screen
Create a new token on this newly created wallet and ensure the onboarding token message is no longer rendered on the Etokens.js screen
Switch between 3-4 wallets and ensure the eTokens list accurately reflects the tokens held in the active wallet
npm run extension and refresh the local browser instance
Ensure the new Etokens component is present in the extension and the original location in Home.js has been deprecated
Publish this to a staging site (e.g. netlify) and ensure the Etokens icon on the bottom menu is of adequate size for fat fingers on mobile devices
Open a fresh new incognito browser session and visit http://localhost:3000/#/etokens and ensure the wallet creation screen is rendered.
Create a new wallet in this new incognito browser session and ensure http://localhost:3000/#/etokens now takes user to the etokens screen.

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

emack created this revision.Dec 16 2023, 02:43

Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 16 2023, 02:43

emack requested review of this revision.Dec 16 2023, 02:43

Harbormaster completed remote builds in B25982: Diff 43646.Dec 16 2023, 02:46

emack edited the summary of this revision. (Show Details)Dec 16 2023, 02:46

emack edited the test plan for this revision. (Show Details)Dec 16 2023, 02:48

bytesofman requested changes to this revision.Dec 19 2023, 20:00

bytesofman added inline comments.

cashtab/src/components/Common/CustomIcons.js
216 ↗	(On Diff #43646)	is `AppstoreAddOutlined` the best icon for this? Doesn't seem super intuitive. looking through antd options, I didn't really see a better one. Not a blocker but something to keep in mind. I think it's come up a few times "we should have an eToken logo" which...we should -- but need some effort / design on that for it to happen.
cashtab/src/components/Etokens/Etokens.js
41 ↗	(On Diff #43646)	the whole `withWalletPresent` seems overkill. We don't want to have an onboarding msg on all kinds of screens if there is no wallet. Instead, for this situation, all screens should fwd to an onboarding screen. potentially this is a deeper Cashtab issue that should be fixed elsewhere as well.

This revision now requires changes to proceed.Dec 19 2023, 20:00

emack edited the test plan for this revision. (Show Details)Dec 21 2023, 08:43

Removed withWalletPresent and will now redirect to the default wallet screen i.e. onboarding/wallet creation

cashtab/src/components/Common/CustomIcons.js
216 ↗	(On Diff #43646)	Created as T3380

Harbormaster completed remote builds in B26038: Diff 43734.Dec 21 2023, 13:28

Build Bitcoin ABC Diffs / Diff Testing (cashtab-tests) failed.

Failed tests logs:

====== CashTab Unit Tests: Improved Cashtab transaction broadcasting function sendXec: Sending below dust threshold ======
Error: expect(received).rejects.toThrow(expected)

Expected substring: "Transaction must send more than dust threshold of 546 satoshis"
Received message:   "Transaction output amount must be at least the dust threshold of 546 satoshis"

      58 |     const utxos = wallet.state.nonSlpUtxos;
      59 |
    > 60 |     let { inputs, outputs } = coinSelect(utxos, targetOutputs, feeRate);
         |                                         ^
      61 |
      62 |     // Initialize TransactionBuilder
      63 |     let txBuilder = utxolib.bitgo.createTransactionBuilderForNetwork(

      at coinSelect (node_modules/ecash-coinselect/src/coinSelect.js:49:15)
      at sendXec (src/transactions/index.js:60:41)
      at Object.<anonymous> (src/transactions/__tests__/index.test.js:47:24)
    at Object.toThrow (/work/cashtab/node_modules/jest-circus/node_modules/expect/build/index.js:285:22)
    at Object.<anonymous> (/work/cashtab/src/transactions/__tests__/index.test.js:48:23)
    at Promise.then.completed (/work/cashtab/node_modules/jest-circus/build/utils.js:391:28)
    at new Promise (<anonymous>)
    at callAsyncCircusFn (/work/cashtab/node_modules/jest-circus/build/utils.js:316:10)
    at _callCircusTest (/work/cashtab/node_modules/jest-circus/build/run.js:218:40)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at _runTest (/work/cashtab/node_modules/jest-circus/build/run.js:155:3)
    at _runTestsForDescribeBlock (/work/cashtab/node_modules/jest-circus/build/run.js:66:9)
    at _runTestsForDescribeBlock (/work/cashtab/node_modules/jest-circus/build/run.js:60:9)
    at run (/work/cashtab/node_modules/jest-circus/build/run.js:25:3)
    at runAndTransformResultsToJestFormat (/work/cashtab/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:170:21)
    at jestAdapter (/work/cashtab/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:82:19)
    at runTestInternal (/work/cashtab/node_modules/jest-runner/build/runTest.js:389:16)
    at runTest (/work/cashtab/node_modules/jest-runner/build/runTest.js:475:34)
    at Object.worker (/work/cashtab/node_modules/jest-runner/build/testWorker.js:133:12)

Each failure log is accessible here:
CashTab Unit Tests: Improved Cashtab transaction broadcasting function sendXec: Sending below dust threshold

rebase to master

Harbormaster completed remote builds in B26039: Diff 43735.Dec 21 2023, 14:12

emack added inline comments.Dec 21 2023, 14:12

cashtab/src/transactions/fixtures/vectors.js
216 ↗	(On Diff #43735)	strange... this is what's in master at the moment so it shouldn't be showing up as a change - see https://github.com/Bitcoin-ABC/bitcoin-abc/blob/2087527d4ba2636af9f27e0abd707ee046491c71/modules/ecash-coinselect/src/coinSelect.js#L50

emack added inline comments.Dec 21 2023, 14:14

cashtab/src/transactions/fixtures/vectors.js
216 ↗	(On Diff #43735)	and https://github.com/Bitcoin-ABC/bitcoin-abc/blob/2087527d4ba2636af9f27e0abd707ee046491c71/cashtab/src/transactions/fixtures/vectors.js#L216

Kronkmeister added a subscriber: Kronkmeister.Dec 21 2023, 16:40

bytesofman added inline comments.Dec 21 2023, 21:30

cashtab/src/components/Etokens/Etokens.js
59 ↗	(On Diff #43734)	why do we test `previousWallet` here? the component will still not render correctly without `wallet` or if `wallet === false` ... unclear why we are ok with previousWallet overriding this.

I get this error trying to arc patch -- mb this branch has 2 commits on it?

 INFO  Base commit is not in local repository; trying to fetch.
Created and checked out branch arcpatch-D14998.


    This diff is against commit 371448ef6c24f080685f0b9a1ff4ce411746417b, but
    the commit is nowhere in the working copy. Try to apply it against the
    current working copy state? (f72321734870f49febcb8517f2df2cd72a939a0f)
    [Y/n]

you can test with git rebase -i master ...if so, can squash them into 1 to fix.

This revision now requires changes to proceed.Dec 21 2023, 21:31

rebased/merged to D14990 (landed earlier today) where the extension app.js is removed altogether and extension specific logic is conditionally executed based on env var
dropped the lingering commit via interactive rebase that was causing the unit test error
removed redundant previousWallet checks

Harbormaster completed remote builds in B26054: Diff 43758.Dec 22 2023, 04:01

rebase

Harbormaster completed remote builds in B26055: Diff 43759.Dec 22 2023, 04:16

lots of revisions so far on this diff. however -- unlucky timing with the required rebases and change in snapshot philosophy.

this is a good change and we should get it in.

cashtab/src/components/Etokens/__tests__/Etokens.test.js
1 ↗	(On Diff #43759)	Unless this is testing some functionality of the component, do not include a snapshot test -- esp not boilerplate. Some approaches that would add value: Expect <LoadingCtn> to render if `loading` Expect balance header to render if balance is not zero Expect zero balance header to render if balance is zero Expect redirect if `!wallet` If these are impractical, then do not include a snapshot file, and include manual integration tests in the test plan for this diff.

This revision now requires changes to proceed.Dec 22 2023, 23:03

Removed snapshot in alignment with recent change in snapshot testing philosphy. Test plan reflects integration testing coverage.

Harbormaster completed remote builds in B26068: Diff 43777.Dec 24 2023, 11:29

bytesofman accepted this revision.Dec 27 2023, 19:51

This revision is now accepted and ready to land.Dec 27 2023, 19:51

Will land this once I coordinate the tweet release with the comms team.

ok. this is a complication for every other diff that lands since it impacts a widely shared component. just keeping this floating around, always waiting on a rebase -- not great practice for repo management and continuous improvement of Cashtab.

please land as soon as possible. I don't think a higher level of comms coordination than getting a tweet out is worth delaying more than a day.

Closed by commit rABC9d3e439afb0a: [Cashtab] Create standalone component for Etokens (authored by emack). · Explain WhyDec 29 2023, 22:08

This revision was automatically updated to reflect the committed changes.

emack added a commit: rABC9d3e439afb0a: [Cashtab] Create standalone component for Etokens.

[Cashtab] Create standalone component for Etokens
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 43798

cashtab/src/components/App.js

cashtab/src/components/Common/CustomIcons.js

cashtab/src/components/Etokens/Etokens.js

cashtab/src/components/Home/Home.js

cashtab/src/components/Home/tests/snapshots/Home.test.js.snap

[Cashtab] Create standalone component for EtokensClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 43798

cashtab/src/components/App.js

cashtab/src/components/Common/CustomIcons.js

cashtab/src/components/Etokens/Etokens.js

cashtab/src/components/Home/Home.js

cashtab/src/components/Home/__tests__/__snapshots__/Home.test.js.snap

[Cashtab] Create standalone component for Etokens
ClosedPublic
Actions

Revision Contents
Changeset List

cashtab/src/components/Home/tests/snapshots/Home.test.js.snap