Page MenuHomePhabricator

[Cashtab] Add function to generate targetOutputs for multisend tx from user input
ClosedPublic

Authored by bytesofman on Dec 12 2023, 21:32.

Details

Summary

Diff required to implement new send functions. Convert user input into desired targetOutputs for new ecash-coinselect based tx generation fn.

Test Plan

npm test

Diff Detail

Repository
rABC Bitcoin ABC
Branch
cashtab-getMultisendTargetOutputs
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 25978
Build 51530: Build Diffcashtab-tests
Build 51529: arc lint + arc unit

Event Timeline

Fabien requested changes to this revision.Dec 13 2023, 10:13
Fabien added a subscriber: Fabien.
Fabien added inline comments.
cashtab/src/transactions/__tests__/index.test.js
54 ↗(On Diff #43592)

copy pasta

cashtab/src/transactions/index.js
122 ↗(On Diff #43592)

Don't split twice, it's expensive

127 ↗(On Diff #43592)

There is a lot of sanitizing that is missing here:

  • You should trim the address and amount (+test)
  • If the amount is expected to be a float (missing test), the decimal point can be a comma depending on the user locale ? This will conflict with the CSV comma separator
  • There is no satoshi rounding nor decimal check, so your function will accept 550.442 as an amount
  • There might be a buffer overflow vulnerability, if I set the amount to be close to the max js integer the multiply will overflow.
This revision now requires changes to proceed.Dec 13 2023, 10:13
bytesofman marked 2 inline comments as done.
bytesofman added inline comments.
cashtab/src/transactions/index.js
127 ↗(On Diff #43592)

You should trim the address and amount (+test)

Cashtab rejects any input with extra spaces, this will fail address validation in isValidMultiSendUserInput. Arguably we should change to accept the spaces and trim them, will address in a separate diff.

If the amount is expected to be a float (missing test), the decimal point can be a comma depending on the user locale ? This will conflict with the CSV comma separator

good point + existing issue in multisend validation. Will address and rebase this diff on that fix.

There is no satoshi rounding nor decimal check, so your function will accept 550.442 as an amount

This is handled in isValidMultiSendUserInput

There might be a buffer overflow vulnerability, if I set the amount to be close to the max js integer the multiply will overflow.

Existing issue, will patch and rebase this diff on the patch. Currently "handled" by impossibility of user having more than XEC supply worth of satoshis to spend.

cashtab/src/transactions/index.js
127 ↗(On Diff #43592)

still need to trim()

Improve validation, support extra spaces, add unit tests to confirm supported cases and errors

Please add another test for extra comma outside of the float, like ecash:qzj5zu6fgg8v2we82gh76xnrk9njcreglum9ffspnr,200.12,foobar, unless it's already tested somewhere else.

This revision is now accepted and ready to land.Dec 18 2023, 08:51