Page MenuHomePhabricator
Differential D10912

[Cashtab] Token burn functionality
ClosedPublic
Actions

Authored by emack on Jan 27 2022, 14:01.

Details

Reviewers
bytesofman
Group Reviewers
Restricted Project
Commits
rABC1dcce06d1e8f: [Cashtab] Token burn functionality
Summary

Adding a new feature in the Token Details screen to allow the wallet holder to burn specific amount of eTokens from their address via the TokenType1.generateBurnOpReturn() api call.

  • new burnEtoken() function created in useBCH.js to burn eTokens via the generateBurnOpReturn() api
  • sendToken.js updated with the burn UI
  • added useBCH.js unit tests to test parsing of eToken burning transactions and burning of invalid eToken IDs
  • Burn notification added to Notifications.js

Manifest: T2189

Test Plan

npm start

Frontend

  • navigate to the eToken info screen and click the Burn button without inputting the burn amount and verify the burn amount defaults to 1
  • attempt to input 0 or below, or above the eToken balance in the wallet and verify validation error is displayed
  • click on the max button and ensure the entirety of the token balance is inserted into the burn amount input
  • test with a valid burn amount and ensure the burn confirmation dialogue shows the correct eToken burn amount and eToken ticker
  • ensure clicking confirm on the burn confirmation dialogue with an incorrect phrase results in validation error
  • ensure a correct burn confirmation phrase results in the completion of the burn transaction
  • verify burn option is not displayed on a fresh wallet with no eTokens since the token details screen is not accessible.
  • verify cross browser compatibility across Chrome and Firefox

Backend

  • test a burn amount of minimum (> 0) value and ensure the user's eToken balance decrements accordingly
  • test burn amount which is within the token's decimal amount and ensure transaction is successful
  • test burn amount which exceeds the token's decimal amount and ensure transaction is still successful, with the token balance updated to a figure that is rounded to the token's designated decimal point
  • ensure the Total Burned figure is a correct tally of all previous burn transactions up to this point
  • test burn amount which is the entire eToken balance and stay on that same eToken screen, and ensure the user is redirected to the Wallet screen and the eToken is no longer displayed in the eTokens list
  • test the customized dust error message by reducing XEC balance to below dust
  • ensure burn function works for eTokens which existed in the wallet prior to this diff
  • ensure burn function works for newly created eTokens

Regression

  • ensure a normal send eToken transaction completes successfully
  • ensure a normal create eToken transaction completes successfully and is displayed correctly in the eTokens list

Extension/Mobile

  • Verify send-token and burn confirmation dialogues are displayed correctly in reduced dimensions

Diff Detail

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

Event Timeline

emack created this revision.Jan 27 2022, 14:01
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 27 2022, 14:01
emack requested review of this revision.Jan 27 2022, 14:01
Harbormaster completed remote builds in B18031: Diff 32008.Jan 27 2022, 14:04
emack edited the summary of this revision. (Show Details)Jan 27 2022, 14:07
emack edited the test plan for this revision. (Show Details)
bytesofman requested changes to this revision.Feb 1 2022, 20:36

image.png (87×131 px, 2 KB)

  1. Add a "max" button

image.png (166×513 px, 15 KB)

  1. Match the UX of deleting a wallet, i.e. require the user to manually type "Burn etokenTicker"

Here's the "delete wallet" UX for ref. Can use this same code, check out Configure.js

image.png (258×505 px, 11 KB)

web/cashtab/src/components/Wallet/TokenList.js
10 ↗(On Diff #32008)

While this is related to the diff, it should be it's own diff.

Please back this out, create a new task "Hide tokens with zero balance", and submit that diff on its own.

Tokens with zero balance are already hidden, with the exception of tokens that have a minting baton -- what cases did you see where this feature was necessary?

This revision now requires changes to proceed.Feb 1 2022, 20:36
emack edited the summary of this revision. (Show Details)Feb 3 2022, 09:59
emack edited the test plan for this revision. (Show Details)
emack updated this revision to Diff 32164.Feb 3 2022, 10:49
  • aligned burn confirmaion modal UX to the delete wallet logic
  • added Max button which inputs the token.balance and triggers the burn amount validation
  • updated burn amount validation check in validation.js along with unit tests
  • backed out the 'hide tokens with zero balance' logic. I realised I was attempting to burn NFTs which had the minting baton which then displayed the token even with 0 balance.
Harbormaster completed remote builds in B18130: Diff 32164.Feb 3 2022, 10:59
bytesofman requested changes to this revision.Feb 3 2022, 23:13
bytesofman added inline comments.
web/cashtab/src/hooks/useBCH.js
1311 ↗(On Diff #32164)

use currency.tokenExplorerUrl , as it actually shows that this tx is a token burn tx

Going forward, it probably makes sense to use explorer.be.cash everywhere and discontinue the process of having 2 different explorers. I'll make a task for this.

This revision now requires changes to proceed.Feb 3 2022, 23:13
emack updated this revision to Diff 32184.Feb 4 2022, 02:43
emack marked an inline comment as done.
  • updated token burn confirmation link to be.cash explorer
Harbormaster completed remote builds in B18146: Diff 32184.Feb 4 2022, 02:47
emack mentioned this in D11011: [Cashtab] Upgrade tx history to recognize eToken burn transactions.Feb 8 2022, 13:35
emack updated this revision to Diff 32406.Feb 16 2022, 13:20

Rebased to new UI

Harbormaster completed remote builds in B18287: Diff 32406.Feb 16 2022, 13:23
bytesofman requested changes to this revision.Feb 24 2022, 00:34
bytesofman added inline comments.
web/cashtab/src/components/Send/SendToken.js
651 ↗(On Diff #32406)

use the new TokenIcon component

web/cashtab/src/hooks/useBCH.js
1246 ↗(On Diff #32406)

Create a task to adjust this after 11062 lands

web/cashtab/src/utils/validation.js
262 ↗(On Diff #32406)

tokenBurnAmount and maxAmount are both BigNumbers, right?

If so, please use the appropriate compare methods.

tokenBurnAmount.gt(0) && tokenBurnAmount.lte(maxAmount)

263 ↗(On Diff #32406)

Don't think we need this 1 if using BigNumber methods

This revision now requires changes to proceed.Feb 24 2022, 00:34
emack updated this revision to Diff 32539.Feb 26 2022, 07:11
emack marked 3 inline comments as done.
  • Utilized the new TokenIcon component in SendToken.js
  • updated validation logic
  • T2274 created to separately update the burnEtoken fee calculation logic
Harbormaster completed remote builds in B18369: Diff 32539.Feb 26 2022, 07:15
bytesofman requested changes to this revision.Mar 1 2022, 22:18
bytesofman added inline comments.
web/cashtab/src/hooks/useBCH.js
1226 ↗(On Diff #32539)

Here we add slpBurnObj

1235 ↗(On Diff #32539)

Here we are investigating its outputs key -- what does slpBurnObj look like? Does it just "play nice" with transactionBuilder ?

This revision now requires changes to proceed.Mar 1 2022, 22:18
emack updated this revision to Diff 32612.Mar 7 2022, 02:26
  • Your hunch was correct - the slpBurnObj.outputs block is actually redundant because the generateBurnOpReturn API call returns the OP_RETURN hex for the burn transaction output, which does not contain any 'outputs' key, hence this diff has been skipping this block by default.
  • It 'played nicely' with transaction builder because burn transactions in general don't need a separate output to transfer the change tokens back to the burn wallet, since it's merely achieved by specifying an output token balance that is subtracted of the burn amount.
  • I have however added a null check on slpBurnObj to throw error if for whatever reason generateBurnOpReturn craps out due to invalid input.
Harbormaster completed remote builds in B18417: Diff 32612.Mar 7 2022, 02:29
bytesofman requested changes to this revision.Mar 7 2022, 19:39
bytesofman added inline comments.
web/cashtab/src/hooks/useBCH.js
1243 ↗(On Diff #32612)

The output amount here seems predictable

  • slpBurnObj is 1 output
  • change is another output

so, it's okay to use 2 instead of 5 -- and also remove the "this may not be totally accurate" comment.

This revision now requires changes to proceed.Mar 7 2022, 19:39
emack updated this revision to Diff 32662.Mar 11 2022, 10:59

Updated fee calculation logic

Harbormaster completed remote builds in B18454: Diff 32662.Mar 11 2022, 11:03
bytesofman requested changes to this revision.Mar 14 2022, 21:37

Please rebase to latest master & see inline comments

web/cashtab/src/hooks/useBCH.js
1232 ↗(On Diff #32662)

Do we need this output? Currently, this means we have 3 transaction outputs

1 - the SLP obj op_return data
2 - a dust tx (to ourself)
3 - a change tx (also to ourself)

...seems like we can just do the change tx

1247 ↗(On Diff #32662)

In this case, remainder = originalAmount - txFee

This revision now requires changes to proceed.Mar 14 2022, 21:37
emack marked 2 inline comments as done.Mar 15 2022, 13:20

tested removal of the etoken dust tx and adjustment of the remainder calculation logic, however this creates a consistent issue in that the xec change is stuck as the token dust.

e.g.

consider this tx to burn 50 x TKN etokens, and the explorer shows the burn tx but no change tx
https://explorer.be.cash/tx/446d1012f23dfa8e9d04484fd16601ec98d23462a1a184ecde4ebeb8dbd1ab73

if you check the address overview
https://explorer.be.cash/address/ecash:qpatql05s9jfavnu0tv6lkjjk25n6tmj9gkpyrlwu8

notice the 95 XEC stuck as the token dust.

image.png (124×757 px, 12 KB)

bytesofman added a comment.Mar 15 2022, 16:07

Okay -- so looks like the token dust output is necessary if this is not a 'burn all' transaction.

I think it's worth adding this logic. "if you are not burning all the token utxos, add a token dust transaction output"

emack added a comment.Mar 19 2022, 00:27

After applying the conditional block:

image.png (199×622 px, 71 KB)

As discussed on telegram, we're leaving this dust output in unchanged.
No revision needed for this feedback.

In D10912#250429, @bytesofman wrote:

Okay -- so looks like the token dust output is necessary if this is not a 'burn all' transaction.

I think it's worth adding this logic. "if you are not burning all the token utxos, add a token dust transaction output"

bytesofman added inline comments.Mar 21 2022, 18:28
web/cashtab/src/hooks/useBCH.js
1242 ↗(On Diff #32662)

2 outputs and 1 op_return output --> 3 outputs

1243 ↗(On Diff #32662)

lose the 1.1 fudge factor

emack updated this revision to Diff 32876.Mar 22 2022, 01:29
emack marked 2 inline comments as done.

updated burn fee logic

Harbormaster completed remote builds in B18586: Diff 32876.Mar 22 2022, 01:32
bytesofman accepted this revision.Mar 22 2022, 13:10
This revision is now accepted and ready to land.Mar 22 2022, 13:10
aimal added a subscriber: aimal.Mar 22 2022, 15:27
Closed by commit rABC1dcce06d1e8f: [Cashtab] Token burn functionality (authored by emack). · Explain WhyMar 22 2022, 23:32
This revision was automatically updated to reflect the committed changes.
emack added a commit: rABC1dcce06d1e8f: [Cashtab] Token burn functionality.

Revision Contents
Changeset List

PathSize
web/
cashtab/
src/
components/
Common/
4 lines
18 lines
Configure/
__tests__/
__snapshots__/
Home/
__tests__/
__snapshots__/
Receive/
__tests__/
__snapshots__/
Send/
197 lines
__tests__/
__snapshots__/
Tokens/
__tests__/
__snapshots__/
hooks/
__mocks__/
18 lines
70 lines
__tests__/
34 lines
157 lines
utils/
__tests__/
33 lines
12 lines

Diff 32904

View Options

web/cashtab/src/components/Common/Atoms.js

Loading...
View Options

web/cashtab/src/components/Common/Notifications.js

Loading...
View Options

web/cashtab/src/components/Configure/__tests__/__snapshots__/Configure.test.js.snap

Loading...
View Options

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

Loading...
View Options

web/cashtab/src/components/Receive/__tests__/__snapshots__/Receive.test.js.snap

Loading...
View Options

web/cashtab/src/components/Send/SendToken.js

Loading...
View Options

web/cashtab/src/components/Send/__tests__/__snapshots__/Send.test.js.snap

Loading...
View Options

web/cashtab/src/components/Send/__tests__/__snapshots__/SendToken.test.js.snap

Loading...
View Options

web/cashtab/src/components/Tokens/__tests__/__snapshots__/CreateTokenForm.test.js.snap

Loading...
View Options

web/cashtab/src/components/Tokens/__tests__/__snapshots__/Tokens.test.js.snap

Loading...
View Options

web/cashtab/src/hooks/__mocks__/mockParsedTxs.js

Loading...
View Options

web/cashtab/src/hooks/__mocks__/mockTxDataWithPassthrough.js

Loading...
View Options

web/cashtab/src/hooks/__tests__/useBCH.test.js

Loading...
View Options

web/cashtab/src/hooks/useBCH.js

Loading...
View Options

web/cashtab/src/utils/__tests__/validation.test.js

Loading...
View Options

web/cashtab/src/utils/validation.js

Loading...