Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow both BTC and sat amounts for single send / CJ #441

Merged
merged 1 commit into from
Nov 11, 2019
Merged

Allow both BTC and sat amounts for single send / CJ #441

merged 1 commit into from
Nov 11, 2019

Conversation

kristapsk
Copy link
Member

This allows no to specify both BTC and sat amounts for sendpayment.py and in joinmarket-qt.py for single joins / sends. If number contains a dot, it is treated as a BTC amount, if it's integer - as sats. In addition "btc" or "sat" suffixes (case insensitive) can be specified (not in qt, because input box is number-only there; would like to see some btc/sat dropdown after amount there, but that would require rewrite of getSettingsWidgets(), didn't want to do that in this PR).

In addition, in various places where we display amounts, now both BTC and sat amounts are printed.

In future the same thing could be done with joinmarket.cfg too.

@chris-belcher
Copy link
Contributor

Are you not concerned that someone might accidentally send too much or too little? (by 8 orders of magnitude, obviously it would fail most of the time) Perhaps Electrum's solution is better of having one configuration for the unit and that unit is then used everywhere.

@kristapsk
Copy link
Member Author

Not really, as both sendpayment and qt asks for confirmation before sending, where it now displays both BTC and sat amounts.

But probably "." check could be removed and instead ask to always specify "btc" suffix for BTC amounts. That's the amount syntax for c-lightning commands:

       amount is the amount in satoshis taken from the internal wallet to  fund  the
       channel.  The  string  all  can  be  used  to specify all available funds (or
       16777215 satoshi if more is available). Otherwise, it is  in  satoshi  preci-
       sion;  it can be a whole number, a whole number ending in sat, a whole number
       ending in 000msat, or a number with 1 to 8 decimal places ending in btc.

@chris-belcher
Copy link
Contributor

OK fair enough.

utACK.

@chris-belcher
Copy link
Contributor

tACK b2e4308

Tested by running wallet-tool history, send sending transactions with sendpayment and joinmarket-qt, using both sats and btc.

chris-belcher added a commit that referenced this pull request Nov 11, 2019
b2e4308 Allow both BTC and sat amounts for single send / CJ (Kristaps Kaupe)

Tree-SHA512: be9d728831a97ce89fa28efd88f919e3e4be454b4b8e0d2ab165b22a60c02c5c1bf62a5f36a690f88da17e41f2beef624775a6fbdb710868f97c130fc2917f57
@chris-belcher chris-belcher merged commit b2e4308 into JoinMarket-Org:master Nov 11, 2019
@kristapsk kristapsk deleted the amount-btc-sat branch November 14, 2019 02:21
@AdamISZ
Copy link
Member

AdamISZ commented Nov 18, 2019

We need to consider schedules carefully here.
Both for the little used (but should be, more) feature of custom schedules in sendpayment, but also tumbler.
There is a weird parsing of amounts as floats implying tumbler percentages in schedules. I'm looking into this a bit now due to a bug related to #447 (I think the author's a bit off in their diagnosis but the code as-is is definitely not right due to an earlier PR, not this one).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants