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

Amounts Part II: Amount API change #2360

Merged
merged 11 commits into from Feb 21, 2019

Conversation

Projects
None yet
3 participants
@rustyrussell
Copy link
Contributor

commented Feb 17, 2019

This changes all the JSON APIs to reduce satoshi/millisatoshi confusion. It's built on the groundwork from Part I in #2353. Part III converts all the internal fields to 'struct amount_sat' and 'amount_msat;.

  1. Amount parameters now accept raw integers as before, or msat/sat/btc-suffixed strings of restricted forms, see documentation.
  2. Every output amount field has new _msat or _sat equivalent which are strings with the value with 'msat' or 'sat' appended.
  3. The python library turns these into Satoshi and Millisatoshi classes, which you can also use to hand to all those amount parameters.

After much flip-flopping:

  1. I didn't deprecate any old fields. That could come later.
  2. I didn't deprecate non-suffixed integers. That could also come later.
  3. I left _msat and _sat redundantly appended to field names so that users don't have to write generic parsers: they know 'amount_msat' will always have value "NNNmsat".

@rustyrussell rustyrussell requested review from cdecker, niftynei, wythe and ZmnSCPxj Feb 17, 2019

@rustyrussell rustyrussell changed the title Amounts Part II: Amount API change WIP: Amounts Part II: Amount API change Feb 17, 2019

@rustyrussell rustyrussell force-pushed the rustyrussell:amount-apichange branch 2 times, most recently from 94de516 to 9063334 Feb 17, 2019

@rustyrussell rustyrussell changed the title WIP: Amounts Part II: Amount API change Amounts Part II: Amount API change Feb 18, 2019

@cdecker cdecker added this to the v0.7 milestone Feb 19, 2019

@rustyrussell rustyrussell force-pushed the rustyrussell:amount-apichange branch 2 times, most recently from 61997bb to b94788d Feb 20, 2019

json_add_u64(response, "msatoshi", *b11->msatoshi);
if (b11->msat)
json_add_amount_msat(response, *b11->msat,
"msatoshi", "amount_msat");

This comment has been minimized.

Copy link
@niftynei

niftynei Feb 20, 2019

Collaborator

petition to rename "amount_msat" as "formatted_msat" or "formatted_value" or something to denote the 'formatted' i.e. string nature of the field value -- amount imo connotes an int or float type field.

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Feb 21, 2019

Author Contributor

The problem is that we may eventually deprecate the old raw-numeric fields. So it does need some semantic :(

return command_fail(tx->cmd, FUND_OUTPUT_IS_DUST,
"Output %"PRIu64" satoshis would be dust",
amount);
if (amount.satoshis < get_chainparams(wtx->cmd->ld)->dust_limit) {

This comment has been minimized.

Copy link
@niftynei

niftynei Feb 20, 2019

Collaborator

👍

if (amount <= tx->amount) {
tx->change = 0;
/* tx->amount is max permissible */
if (amount_sat_less(amount, tx->amount)) {

This comment has been minimized.

Copy link
@niftynei

niftynei Feb 20, 2019

Collaborator

i think you want less_eq here?

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Feb 21, 2019

Author Contributor

Good catch! Effect should be the same, but it was not a deliberate change!

@@ -32,7 +32,7 @@ json_add_route_hop(struct json_stream *r, char const *n,
json_add_short_channel_id(r, "channel",
&h->channel_id);
json_add_num(r, "direction", h->direction);
json_add_u64(r, "msatoshi", h->amount);
json_add_amount_msat(r, h->amount, "msatoshi", "amount_msat");

This comment has been minimized.

Copy link
@niftynei

niftynei Feb 20, 2019

Collaborator

petition to rename "amount_msat" ... same as other comment.

json_to_u64(buf, json_get_member(buf, chan, "satoshis"), &sats);
if (sats * 1000 < pc->msatoshi)
json_to_sat(buf, json_get_member(buf, chan, "satoshis"), &sats);
if (sats.satoshis * 1000 < pc->msatoshi)

This comment has been minimized.

Copy link
@niftynei

niftynei Feb 20, 2019

Collaborator

should we use a conversion or comparison functions rather than multiplying by 1000?

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Feb 21, 2019

Author Contributor

Yes; see part III. This is the minimum shims to get the JSON API converted.

@rustyrussell rustyrussell force-pushed the rustyrussell:amount-apichange branch from b94788d to 7ca68d7 Feb 21, 2019

rustyrussell added some commits Feb 21, 2019

common/json_tok: add param_msat / param_sat.
The current param_sat accepts "any": rename and move that to invoice.c
where it's called.  We rename it to param_msat_or_any and invoice.c
is our first (trivial) amount_msat user.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
common: json_to_msat and json_to_sat helpers.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

@rustyrussell rustyrussell force-pushed the rustyrussell:amount-apichange branch from 7ca68d7 to 9589a6f Feb 21, 2019

@rustyrussell

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

Rebase to fix conflicts with master (fairly minor), and correct error @niftynei found; didn't change amount_msat but better names welcome...

@niftynei

This comment has been minimized.

Copy link
Collaborator

commented Feb 21, 2019

ACK 9589a6f

rustyrussell added some commits Feb 21, 2019

lightningd: add json_add_amount_msat and json_add_amount_sat helpers.
These create two fields, one old one which is purely numeric,
and a modern on with a suffix, eg "msatoshi" and "amount_msat".

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
common/bolt11: use struct amount_msat
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
lightningd/json: make wallet_tx functions take amount_sat.
Using param_tok is generally deprecated, as it doesn't give any sanity checking
for the JSON 'check' command.  So make param_wtx usable directly, and
also make it have a struct amount_sat.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
sendpay: allow 'amount_msat'
We're about to add 'amount_msat' to getroute, but it's common to feed
'getroute' back into 'sendpay', so sendpay should allow it.

If both are specified, make sure they're the same!

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
lightningd and routing: use struct amount_msat.
We use it in route_hop, and paper over it in the JSON APIs.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
wallet: use amount_msat / amount_sat.
We change struct utxo to use amount_sat, and paper over the JSON APIs.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
listpeers: add all the alternate "msat" and "sat" fields for channels.
These are undocumented, unfortunately, but at least that means I don't
have to update the docs!

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
CHANGELOG, documentation: update changelog to reflect suffix changes.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
pylightning: handle msat fields in JSON more appropriately.
Little point having users handle the postfixes manually, this
translates them, and also allows Millisatoshi to be used wherever an
'int' would be previously.

There are also helpers to create the formatting in a way c-lightning's
JSONRPC will accept.

All standard arithmetic operations with integers work.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

@rustyrussell rustyrussell force-pushed the rustyrussell:amount-apichange branch from 9589a6f to d61c1a8 Feb 21, 2019

@rustyrussell rustyrussell merged commit cc95a56 into ElementsProject:master Feb 21, 2019

1 of 2 checks passed

ackbot Need at least 1 ACKs
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.