Page MenuHomePhabricator

[Cashtab] [Chronik] [Tx Gen] Parse XEC send value
ClosedPublic

Authored by emack on Jun 13 2022, 11:05.

Details

Reviewers
bytesofman
Group Reviewers
Restricted Project
Commits
rABC6c5a3dd84f6a: [Cashtab] [Chronik] [Tx Gen] Parse XEC send value
Summary

As per T2447, this is part of a series of diffs to refactor the transaction generation process.

This diff is a standalone utility function which parses parameters for both one to one and one to many send XEC transactions, aggregates the total send value (in case of one to many) and then checks whether the final send value is above dust.

This diff does not change any existing functionality within the codebase. Once all the utility functions are accepted, then there'll be a final set of diffs to refactor the existing sendXec() function which is getting quite large and cumbersome to maintain.

Test Plan

npm start
npm test
observe the successful execution of new useBCH.test.js unit tests

Diff Detail

Repository
rABC Bitcoin ABC
Branch
selectUtxos
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 19439
Build 38611: Build Diffcashtab-tests
Build 38610: arc lint + arc unit

Event Timeline

emack requested review of this revision.Jun 13 2022, 11:05

Could you add a pseudocode description of how you would like the final sendXec() function to work, including each component function and a short description of its inputs and outputs? We have a skeleton in T2447 but I think this should really be its own task. We might come back and edit it a bit as these diffs land and we see more about the best way to get this done.

I'd like to see the context more fleshed out before approving this initial step.

This revision now requires changes to proceed.Jun 13 2022, 22:13
emack requested review of this revision.Jun 14 2022, 11:41

I'd like to see the context more fleshed out before approving this initial step.

See T2503 for overarching context on this refactor initiative

bytesofman added inline comments.
web/cashtab/src/hooks/__tests__/useBCH.test.js
695 ↗(On Diff #33988)

We should be able to capture the specific error msg in the unit tests, as we'll want to throw a notification with the specific error msg in the app (not just console.log it)

700 ↗(On Diff #33988)

See above

705 ↗(On Diff #33988)

Return a specific error msg

710 ↗(On Diff #33988)

Return a specific error msg

web/cashtab/src/hooks/useBCH.js
1471 ↗(On Diff #33988)

Situation here where we will have to check for a null return value after calling this function. Also, Cashtab doesn't know what caused this error -- only a user reading the console.log statement would be able to figure it out.

Whatever error causes sendXec() to fail should be thrown in a notification. So, we should be capturing the specific errors thrown in this function for the unit test failures.

This revision now requires changes to proceed.Jun 14 2022, 22:47
emack marked 5 inline comments as done.

Throwing specific errors and verified via unit tests

bytesofman added inline comments.
web/cashtab/src/hooks/__tests__/useBCH.test.js
678 ↗(On Diff #34029)

The parameter bchValue, used to pass the single send XEC amount in Send.js, is a string. It's okay if this function also works taking a number input, but the unit test should match the app -- i.e. parseXecSendValue(false, '550', null)...

This revision now requires changes to proceed.Jun 15 2022, 23:39
emack marked an inline comment as done.

Changed single XEC input from int to str

image.png (105×150 px, 6 KB)

image.png (353×1 px, 40 KB)

Pulling down this diff, there is a REMOVE.husky folder.

Make sure that the end result of this diff shows no changes to the husky files.

This revision now requires changes to proceed.Jun 17 2022, 17:01

Removed redundant husky folder

Updated husky pre-commit

Make sure that the end result of this diff shows no changes to the husky files.

Done - ready for final review

bytesofman added inline comments.
web/cashtab/src/hooks/useBCH.js
1455 ↗(On Diff #34050)

dust test can be applied only once to final value, instead of applying it two times to value in both parts of the if...else block

This revision now requires changes to proceed.Jun 20 2022, 17:27
emack marked an inline comment as done.

Moved dust validation to outside of the if else block

This revision is now accepted and ready to land.Jun 21 2022, 14:00
bytesofman added inline comments.
web/cashtab/src/hooks/useBCH.js
1794 ↗(On Diff #34065)

Move the function to cashMethods.js

This revision now requires changes to proceed.Jun 21 2022, 14:43

Migrated function to cashMethods

This revision is now accepted and ready to land.Jun 22 2022, 18:23