Page MenuHomePhabricator

[ecash-lib] Add validation to confirm user input param feePerKb is an integer
ClosedPublic

Authored by bytesofman on May 21 2024, 18:40.

Details

Summary

The user cannot be guaranteed to use an integer value for feePerKb. Many legacy apps use satoshis per byte as a fee rate, and may implement eCash lib by converting existing fee rates to feePerKb.

For example, the default fee rate in Cashtab is 2.01 satoshis per byte. However, in JS, 1000*2.01 !== 2010. We get a hard to decipher error from eCash lib if it is implemented with this kind of value. With this diff, a more helpful error is thrown.

Since we cannot control whether or not developers choose integer values for their fee rate, need to handle the case.

Test Plan

CI

Diff Detail

Event Timeline

bytesofman added inline comments.
modules/ecash-lib/tests/txBuilder.test.ts
437 ↗(On Diff #47863)

this is what JS gives for 2.01*1000

This is the wrong approach imo, ecash-lib should accept floating points or use sat/kB instead

tobias_ruck added a subscriber: tobias_ruck.

NAK this approach

modules/ecash-lib/tests/txBuilder.test.ts
437 ↗(On Diff #47863)

You shouldn't use floats here anyway, you should use BigDecimal or so to convert the 2.01 fee to 2010 sats/kB, or use sats/kB in CashTab directly.

This revision now requires changes to proceed.May 21 2024, 20:32

validate feePerKb to be an integer

bytesofman retitled this revision from [ecash-lib] Support non-integer values for tx fees to [ecash-lib] Add validation to confirm user input param feePerKb is an integer.May 21 2024, 22:41
bytesofman edited the summary of this revision. (Show Details)
tobias_ruck added inline comments.
modules/ecash-lib/tests/txBuilder.test.ts
560 ↗(On Diff #47872)

No need for a complex fraction here imo

This revision now requires changes to proceed.May 22 2024, 06:48
bytesofman marked an inline comment as done.

follow suggested value

modules/ecash-lib/tests/txBuilder.test.ts
560 ↗(On Diff #47872)

What is the use case for a value of less than 1000? Could add validation for this as well.

tobias_ruck added inline comments.
modules/ecash-lib/tests/txBuilder.test.ts
560 ↗(On Diff #47886)

I think the test is clearer this way

This revision now requires changes to proceed.May 22 2024, 16:35
bytesofman marked an inline comment as done.

do not specify dust error in test that is not looking for it

This revision is now accepted and ready to land.May 22 2024, 19:00