Page MenuHomePhabricator

[Cashtab] Token burn functionality
ClosedPublic

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
Branch
etokenburn
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 18417
Build 36631: Build Diffcashtab-tests
Build 36630: arc lint + arc unit

Event Timeline

emack requested review of this revision.Jan 27 2022, 14:01
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 test plan for this revision. (Show Details)
  • 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.
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 marked an inline comment as done.
  • updated token burn confirmation link to be.cash explorer
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 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
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
  • 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.
bytesofman requested changes to this revision.Mar 7 2022, 19:39
bytesofman added inline comments.
web/cashtab/src/hooks/useBCH.js
1243

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

Updated fee calculation logic

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)

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"

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.

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"

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 marked 2 inline comments as done.

updated burn fee logic

This revision is now accepted and ready to land.Mar 22 2022, 13:10
This revision was automatically updated to reflect the committed changes.