Page MenuHomePhabricator

[Cashtab] Send multi-recipient XEC transaction
ClosedPublic

Authored by emack on Dec 1 2021, 11:47.

Details

Reviewers
bytesofman
Group Reviewers
Restricted Project
Commits
rABC97eea03b145c: [Cashtab] Send multi-recipient XEC transaction
Summary
  • new isOnetoManyXECSend state added to Send.js to conditionally render address input field based on whether the user is in single recipient or multi-recipient mode.
  • When in multi-recipient mode, new <TextArea> is rendered for users to input multiple recipients with formatting instructions shown in placeholder text.
  • submit() function in Send.js updated to reflect whether the isOnetoManyXECSend modal is true or false. If true (multi recipient), it calls a separate one to many version of the original submit() logic which iterates through the <TextArea> inputs and converts it into an array and validates each line.
  • new toLegacyArray() function added to Ticker.js which traverses the array of recipient addresses and converts them to bch cash addresses due to bch-api constraint
  • sendBch() in useBCH.js now updated to sendXec() along with new parameters which can build both one to one and one to many multi recipient transactions.
Test Plan
  • npm start
  • navigate to the Send screen and send a single XEC transction to ensure no regression
  • click on 'Switch to multiple recipients' and enter in 3 valid recipient addresses and XEC send value separated by comma.
  • click on 'Switch to multiple recipients' and enter in 3 valid recipient addresses and XEC send value separated by comma WITH an OP_RETURN message
  • click on send and ensure the block explorer shows a transaction with the exact multi outputs and the total of those 3 values matches the cashtab Tx history entry
  • click on 'Switch to multiple recipients' and enter in 3 valid recipient addresses and XEC send value, PLUS a 4th invalid address, and ensure this is picked up by validation
  • click on 'Switch to multiple recipients' and enter in 3 valid recipient addresses and XEC send value, PLUS a 4th valid address but less than minimum transfer value (e.g. 3 XEC) and ensure a 5.5 XEC minimum error is displayed
  • click on 'Switch to multiple recipients' and enter in 3 valid recipient addresses and XEC send value, PLUS a 4th valid eToken address and ensure an eToken error is displayed
  • click on 'Switch to multiple recipients' and enter in no input or several spaces and ensure the 'Empty rows must be removed' field validation is displayed
  • click on 'Switch to multiple recipients' and enter in multiple valid addresses and valid send values without clicking Send yet. Switch back to single recipient then switch back to multiple recipients. Ensure the inputs persisted between switches. Press Send and ensure successful transaction
  • click on 'Switch to multiple recipients' and then switch back to single recipient and ensure a single recipient send XEC transaction is sent successfully
  • npm run extension
  • test the above in web extension mode
  • verify cross browser compatibility in firefox

Diff Detail

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

Event Timeline

emack requested review of this revision.Dec 1 2021, 11:48
bytesofman requested changes to this revision.Dec 2 2021, 00:29
bytesofman added inline comments.
web/cashtab/src/components/Common/EnhancedInputs.js
285 ↗(On Diff #31203)

Let's call this DestinationAddressMulti

The original naming convention for the other form item is terrible, should also be changed. Created a task for that.

web/cashtab/src/components/Common/Ticker.js
224 ↗(On Diff #31203)

Split the array once and then assign the other variables.

let addressValueArr = addressArray[i].split(',');
let address = addressValueArr[0]
let value = addressValueArr[1]
web/cashtab/src/components/Send/Send.js
191 ↗(On Diff #31203)

Sorry about this one....but.....now that we have sendXec as one function, we no longer need two separate functions for oneToManyXECSend() and oneToOneXECSend()

Instead, a single combined send function should accept the isOneToManyXECSend state variable as a parameter -- then use this parameter for the isolated different workflows (calling sendXec with different parameters, logging separate event in Google Analytics)

355 ↗(On Diff #31203)

Refactor so there is only one function here.

This revision now requires changes to proceed.Dec 2 2021, 00:29
emack marked 4 inline comments as done.
  • optimised array splitting in Ticker.js' toLegacyArray() function
  • refactored the submit() and send() functions in Send.js
  • FormItemWithoutQrCodeAddon in EnhancedInputs.js component changed to DestinationAddressMulti
bytesofman requested changes to this revision.Dec 2 2021, 23:52
bytesofman added inline comments.
web/cashtab/src/components/Send/Send.js
232 ↗(On Diff #31225)

This catch {} block looks identical for both oneToMany === true and oneToMany === false

Please create a new function handleSendXecError(oneToManyFlag) that holds this code and is called in both catch {} blocks..

In the function, the event sent to errorNotification() should depend on oneToMany. If true, errorNotification(e, message, 'Sending XEC one to many'), if false, errorNotification(e, message, 'Sending XEC')

355 ↗(On Diff #31225)

Delete this submit() {} function and just use the above send() function. Use isOneToManyXECSend as the boolean checked inside the send function.

This revision now requires changes to proceed.Dec 2 2021, 23:52
emack marked 2 inline comments as done.
  • Moved error handling for single and multi recipient XEC sends into a separate handleSendXecError() function.
  • Removed submit() function and adjusted frontend to call send() directly.
bytesofman requested changes to this revision.Dec 3 2021, 22:47
bytesofman added inline comments.
web/cashtab/src/components/Send/Send.js
345 ↗(On Diff #31245)

Pass the parameter here as well. If intent is for param to show as not true because it is not passed and therefore undefined, it's okay to just use false -- but in this case might as well stay consistent with the flag.

just a readability quibble

This revision now requires changes to proceed.Dec 3 2021, 22:47
emack marked an inline comment as done.

Rebased to latest master and updated the multi send flag in send.js

bytesofman requested changes to this revision.Dec 5 2021, 17:45

May not need to fix in this diff, but there is an annoying UX issue. User story:

  1. Switch to multi-recipient mode and enter valid data
  2. Switch back to single. No validation error appears.
  3. Click "Send" --> validation error and send fails
  4. Switch back to multi-recipient mode --> input is saved and is now valid, but is shown as invalid with validation error, 'Send" button is disabled

Fix I think is to do a validation check every time the user toggles modes. Please correct or create a task.

web/cashtab/src/hooks/__tests__/useBCH.test.js
208 ↗(On Diff #31264)

Remove

web/cashtab/src/hooks/useBCH.js
987 ↗(On Diff #31264)

Use BigNumber here instead of Number to be consistent with rest of function --- we want to always use BigNumber when dealing with XEC or token amounts.

Might also need it if user is sending exceptionally large amounts

992 ↗(On Diff #31264)

Should already be a BigNumber here -- let me know if there was an important reason to do it the other way (keeping it as Number for all the addition, then converting at the end).

This revision now requires changes to proceed.Dec 5 2021, 17:45
emack marked 3 inline comments as done.

Adjusted with BigNumber usage. A new task will be created to look at the edge case above.

bytesofman requested changes to this revision.Dec 6 2021, 14:22
bytesofman added inline comments.
web/cashtab/src/components/Common/Ticker.js
232 ↗(On Diff #31281)

This is just being converted back to a string on line 255, then back again to BigNumber in useBCH.js

This should be a string until math operations are performed on it in useBCH.js, so no need to implement BigNumber in this function --- the conversion on L987 in useBCH.js is sufficient

This revision now requires changes to proceed.Dec 6 2021, 14:22
emack marked an inline comment as done.

Removed redundant BigNumber call

This revision is now accepted and ready to land.Dec 7 2021, 15:17