Page MenuHomePhabricator
Differential D15553

[Cashtab] Calculate max send amount in appropriate way using updated ecash-coinselect
ClosedPublic
Actions

Authored by bytesofman on Feb 26 2024, 18:14.

Details

Reviewers
emack
Group Reviewers
Restricted Project
Commits
rABCf958feda21dc: [Cashtab] Calculate max send amount in appropriate way using updated ecash…
Summary

We are using a legacy function to calculate the fee here that is underestimating the actual max send amount.

Using an updated helper function from ecash-coinselect, calculate the max send amount appropriately (including consideration for a cashtab msg, if present).

Test Plan

npm test

Diff Detail

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

Event Timeline

bytesofman created this revision.Feb 26 2024, 18:14
Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 26 2024, 18:14
Harbormaster completed remote builds in B27433: Diff 45630.Feb 26 2024, 18:21
bytesofman updated this revision to Diff 45634.Feb 26 2024, 18:25

use rc from ecash-coinselect diff

Harbormaster completed remote builds in B27437: Diff 45634.Feb 26 2024, 18:30
bytesofman updated this revision to Diff 45650.Feb 27 2024, 00:24

rebase

bytesofman updated this revision to Diff 45651.Feb 27 2024, 00:27

back out plock changes

bytesofman updated this revision to Diff 45652.Feb 27 2024, 00:29

back out other unrelated changes

bytesofman updated this revision to Diff 45654.Feb 27 2024, 00:46

add updated sendxec tests (will rebase), add integration test

Harbormaster completed remote builds in B27455: Diff 45654.Feb 27 2024, 00:46
bytesofman updated this revision to Diff 45655.Feb 27 2024, 00:46

lint

bytesofman updated this revision to Diff 45657.Feb 27 2024, 00:56

rebase

bytesofman edited the summary of this revision. (Show Details)Feb 27 2024, 00:57
bytesofman added a parent revision: D15556: [Cashtab] Standardize and improve SendXec.test.js.
Harbormaster completed remote builds in B27451: Diff 45650.Feb 27 2024, 01:01
Harbormaster completed remote builds in B27452: Diff 45651.
teamcity added a comment.Feb 27 2024, 01:01
Build Bitcoin ABC Diffs / Diff Testing (ecash-coinselect) failed.

Tail of the build log:

/work/modules/ecash-coinselect /work/abc-ci-builds/ecash-coinselect
npm ERR! code EUSAGE
npm ERR! 
npm ERR! `npm ci` can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with `npm install` before continuing.
npm ERR! 
npm ERR! Missing: ecash-coinselect@2.2.0 from lock file
npm ERR! 
npm ERR! Clean install a project
npm ERR! 
npm ERR! Usage:
npm ERR! npm ci
npm ERR! 
npm ERR! Options:
npm ERR! [--install-strategy <hoisted|nested|shallow|linked>] [--legacy-bundling]
npm ERR! [--global-style] [--omit <dev|optional|peer> [--omit <dev|optional|peer> ...]]
npm ERR! [--include <prod|dev|optional|peer> [--include <prod|dev|optional|peer> ...]]
npm ERR! [--strict-peer-deps] [--foreground-scripts] [--ignore-scripts] [--no-audit]
npm ERR! [--no-bin-links] [--no-fund] [--dry-run]
npm ERR! [-w|--workspace <workspace-name> [-w|--workspace <workspace-name> ...]]
npm ERR! [-ws|--workspaces] [--include-workspace-root] [--install-links]
npm ERR! 
npm ERR! aliases: clean-install, ic, install-clean, isntall-clean
npm ERR! 
npm ERR! Run "npm help ci" for more info

npm ERR! A complete log of this run can be found in: /root/.npm/_logs/2024-02-27T01_01_42_041Z-debug-0.log
Build ecash-coinselect failed with exit code 1
bytesofman added a comment.Feb 27 2024, 01:04
In D15553#350890, @teamcity wrote:
Build Bitcoin ABC Diffs / Diff Testing (ecash-coinselect) failed.

Tail of the build log:

/work/modules/ecash-coinselect /work/abc-ci-builds/ecash-coinselect
npm ERR! code EUSAGE
npm ERR! 
npm ERR! `npm ci` can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with `npm install` before continuing.
npm ERR! 
npm ERR! Missing: ecash-coinselect@2.2.0 from lock file
npm ERR! 
npm ERR! Clean install a project
npm ERR! 
npm ERR! Usage:
npm ERR! npm ci
npm ERR! 
npm ERR! Options:
npm ERR! [--install-strategy <hoisted|nested|shallow|linked>] [--legacy-bundling]
npm ERR! [--global-style] [--omit <dev|optional|peer> [--omit <dev|optional|peer> ...]]
npm ERR! [--include <prod|dev|optional|peer> [--include <prod|dev|optional|peer> ...]]
npm ERR! [--strict-peer-deps] [--foreground-scripts] [--ignore-scripts] [--no-audit]
npm ERR! [--no-bin-links] [--no-fund] [--dry-run]
npm ERR! [-w|--workspace <workspace-name> [-w|--workspace <workspace-name> ...]]
npm ERR! [-ws|--workspaces] [--include-workspace-root] [--install-links]
npm ERR! 
npm ERR! aliases: clean-install, ic, install-clean, isntall-clean
npm ERR! 
npm ERR! Run "npm help ci" for more info

npm ERR! A complete log of this run can be found in: /root/.npm/_logs/2024-02-27T01_01_42_041Z-debug-0.log
Build ecash-coinselect failed with exit code 1

failure related to pulling ecash-coinselect changes out of this diff, handled in already landed separate diff

Harbormaster completed remote builds in B27453: Diff 45652.Feb 27 2024, 01:06
Harbormaster completed remote builds in B27458: Diff 45657.
Harbormaster completed remote builds in B27456: Diff 45655.Feb 27 2024, 01:08
bytesofman published this revision for review.Feb 27 2024, 01:08
emack requested changes to this revision.Feb 27 2024, 07:30
emack added a subscriber: emack.
emack added inline comments.
cashtab/src/components/Send/SendXec.js
681 ↗(On Diff #45657)

need to use opReturnMsg.trim() !== '' here otherwise a message full of spaces will bypass this

cashtab/src/components/Send/__tests__/SendXec.test.js
1088 ↗(On Diff #45657)

you are adding a cashtab msg though?

This revision now requires changes to proceed.Feb 27 2024, 07:30
bytesofman updated this revision to Diff 45662.Feb 27 2024, 12:08

fix test title, confirm msg of empty spaces is passed

Harbormaster completed remote builds in B27460: Diff 45662.Feb 27 2024, 12:08
bytesofman marked 2 inline comments as done.Feb 27 2024, 12:09
bytesofman added inline comments.
cashtab/src/components/Send/SendXec.js
681 ↗(On Diff #45657)

not seeing this behavior in the current app, which uses this same check to decide whether or not to push a target output for a cashtab msg

else if (opReturnMsg !== false && opReturnMsg !== '') {
            // If not an airdrop msg, add opReturnMsg as a Cashtab Msg, if it exists
            targetOutputs.push(getCashtabMsgTargetOutput(opReturnMsg));
        }

perhaps it could be improved in both spots, but that should be its own diff.

here's a msg i made by just adding spaces to the message output: https://explorer.e.cash/tx/bb1d0bd6e875278fed70e3c23f4da769282866b2c27a6fed7295ff6d9213448e

updated the added test here to confirm this

cashtab/src/components/Send/__tests__/SendXec.test.js
1088 ↗(On Diff #45657)

lol right but I wasn't when I first wrote this title

good catch, updated

bytesofman updated this revision to Diff 45663.Feb 27 2024, 12:09
bytesofman marked 2 inline comments as done.

remove focused test

bytesofman updated this revision to Diff 45665.Feb 27 2024, 12:12

rebase, bump version

bytesofman edited the summary of this revision. (Show Details)Feb 27 2024, 12:14
bytesofman removed a parent revision: D15556: [Cashtab] Standardize and improve SendXec.test.js.
Harbormaster completed remote builds in B27461: Diff 45663.Feb 27 2024, 12:21
Harbormaster completed remote builds in B27462: Diff 45665.Feb 27 2024, 12:25
emack accepted this revision.Feb 27 2024, 20:18
This revision is now accepted and ready to land.Feb 27 2024, 20:18
Closed by commit rABCf958feda21dc: [Cashtab] Calculate max send amount in appropriate way using updated ecash… (authored by bytesofman). · Explain WhyFeb 27 2024, 20:47
This revision was automatically updated to reflect the committed changes.
bytesofman added a commit: rABCf958feda21dc: [Cashtab] Calculate max send amount in appropriate way using updated ecash….

Revision Contents
Changeset List

PathSize
cashtab/
12 lines
4 lines
src/
components/
Send/
58 lines
__tests__/
77 lines
utils/
__tests__/
31 lines
40 lines

Diff 45680

View Options

cashtab/package-lock.json

Loading...
View Options

cashtab/package.json

Loading...
View Options

cashtab/src/components/Send/SendXec.js

Loading...
View Options

cashtab/src/components/Send/__tests__/SendXec.test.js

Loading...
View Options

cashtab/src/utils/__tests__/cashMethods.test.js

Loading...
View Options

cashtab/src/utils/cashMethods.js

Loading...