[Cashtab] Send multi-recipient XEC transaction
AbandonedPublic
Actions

Authored by emack on Nov 13 2021, 11:21.

Details

Reviewers

bytesofman

Group Reviewers

Restricted Project

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.
See https://explorer.be.cash/tx/534a360fb1759291f1ab4a94c7fa4b45fbdac1a3df1f14ef7ce2afcfad8f9355 for mainnet execution

T1954

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

Branch

sendmanytx

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 17278
Build 34384: Build Diff	cashtab-tests
Build 34383: arc lint + arc unit

Event Timeline

emack created this revision.Nov 13 2021, 11:21

Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 13 2021, 11:21

emack requested review of this revision.Nov 13 2021, 11:21

Harbormaster completed remote builds in B17251: Diff 30864.Nov 13 2021, 11:26

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 Dust error notification is displayed

I'm not seeing this, e.g.

Looks great. Well thought out implementation from user perspective.

web/cashtab/src/components/Common/Ticker.js
136 ↗	(On Diff #30864)	Add some unit tests for this new function
139 ↗	(On Diff #30864)	I think Cashtab consistently uses `let` or `const` every time. Unless there is a specific need, please keep to this convention. In this case, should be `const` as the input should not change.
139 ↗	(On Diff #30864)	Might need to do a type or validation test on `addressArray` to make sure it has a `length` property. Function needs to handle bad input, can then do this case in unit tests.
141 ↗	(On Diff #30864)	`let` for `var`
web/cashtab/src/components/Send/Send.js
206 ↗	(On Diff #30864)	Might as well call it XEC now, even though the old comment is still saying BCHA
207 ↗	(On Diff #30864)	XEC
208 ↗	(On Diff #30864)	Let's make a new event, say "SendToMany" In general, I've been disappointed with the performance of this event tracking in GA. It seems to be undercounting. I'll put up a task for that too, separate thing.
413 ↗	(On Diff #30864)	See how validation is handled for other functions in `utils/validation.js` and copy that approach. This approach lets you build unit tests for validation.
423 ↗	(On Diff #30864)	Is this used? Remove Confusing to have this with `const {value, name}` above

This revision now requires changes to proceed.Nov 15 2021, 23:44

emack edited the test plan for this revision. (Show Details)Nov 16 2021, 12:46

Updates to reflect feedback:

validation logic in handleMultiAddressChange() function in send.js is moved to the newly created isValidSendToMany() function in validation.js to check both address and send amount
toLegacyArray() in Ticker.js adjusted to handle bad addressArray input as well as upstream in Submit() function by moving the toLegacyArray call inside the try/catch block.
Added SendToMany GA event
New unit tests added to Ticker.test.js and Validation.test.js
The 'less than minimum issue' raised above was being validated in the backend upon clicking Send but I've now adjusted it to be caught in the front end before sending.
Various syntax adjustments
Additional manual test added to Test Plan around ensuring valid eToken addresses fail front end validation

Harbormaster completed remote builds in B17260: Diff 30876.Nov 16 2021, 13:01

User story bugs

First

I enter valid send-to-many input
I add a line break at the end
I see an error msg `` that is hard to parse, my input looks valid

Second

I enter valid send to many input and send a tx

ecash:qphlhe78677sz227k83hrh542qeehh8el5lcjwk72y, 20
ecash:qphpmfj0qn7znklqhrfn5dq7qh36l3vxav9up3h67g, 30
ecash:qp89xgjhcqdnzzemts0aj378nfe2mhu9yvxj9nhgg6, 40
ecash:qplkmuz3rx480u6vc4xgc0qxnza42p0e7vll6p90wr, 50

I switch back to single mode
I see an 'address invalid' error because the field is populated with ecash:qphlhe78677sz227k83hrh542qeehh8el5lcjwk72y, 20ecash:qphpmfj0qn7znklqhrfn5dq7qh36l3vxav9up3h67g, 30ecash:qp89xgjhcqdnzzemts0aj378nfe2mhu9yvxj9nhgg6, 40ecash:qplkmuz3rx480u6vc4xgc0qxnza42p0e7vll6p90wr, 50

I get how this is probably better than losing the multi-address input if the user inadvertently clicks back. So I don't think this should hold up the diff, but please make a task to correct this behavior going forward.

web/cashtab/src/components/Common/__tests__/Ticker.test.js
6 ↗	(On Diff #30876)	Please also include at least one unit test where a large array is successfully converted
web/cashtab/src/components/Send/Send.js
197 ↗	(On Diff #30876)	Better code readability if everything in this `if () {}` block were in its own function, say, `oneToManyXECSend()` Then the `submit` function would be more readable as "if it's a 1 to many, send 1 to many, otherwise send normal" As a side point, I think would want to replace `is1toManyXECSend` variable name with `isOneToManyXECSend`; cashtab doesn't have any other variables with a number in the name.
259 ↗	(On Diff #30876)	See above, this else block should call a function called something like `oneToOneXECSend`
web/cashtab/src/hooks/useBCH.js
1027 ↗	(On Diff #30876)	This should have at least the same unit tests as `sendBch`
1027 ↗	(On Diff #30876)	Thinking more about this (see below comment on utxo selection) -- probably want this to be in the same function as `sendBch` The functions are very similar and already seeing an error in trying to duplicate them Please modify the `sendBch` function to be called `sendXec`, and have it accept `isOneToMany` as a parameter. Then put the different output in an `if (isOneToMany)` block.
1052 ↗	(On Diff #30876)	See how this logic works in `sendBch` In its current state, this loop is adding every single utxo as an input utxo. We only want to add as many as we need.

This revision now requires changes to proceed.Nov 17 2021, 20:36

fixed user story bug #1 by adding additional validation logic specific to catching line breaks in the input field
multi-recipient input persisting issue when switching back to single recipient mode raised as a separate task (T1965)
New unit tests added to Ticker.test.js to test a standard sized (~10 addresses) and large (110 addresses) sized addressArray input is successfully converted, as well as throwing error on an invalid array
Combined one to many and one to one XEC send logic into the single sendXec() function (formerly sendBch) in useBCH.js
Updated all unit tests in useBCH.test.js to reference the new sendXec() function, with one additional unit test to cover the one to many XEC send transaction

Harbormaster completed remote builds in B17278: Diff 30906.Nov 18 2021, 11:35

bytesofman requested changes to this revision.Nov 25 2021, 21:59

bytesofman added inline comments.

web/cashtab/src/components/Send/Send.js
217	Change the order of these params so that they are at the end and can just be excluded when sendOneToMany is true
471	Change `1` to `One`, i.e. `handleOneToManyXecSend`
web/cashtab/src/utils/validation.js
161	Follow format of other functions in `validation.js` -- if it is valid, then the function should return `true`