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

Pay enhancements: remove legacy, add maxfee and description params #5122

Merged
merged 8 commits into from
Apr 4, 2022
6 changes: 5 additions & 1 deletion contrib/pyln-client/pyln/client/lightning.py
Original file line number Diff line number Diff line change
Expand Up @@ -997,7 +997,8 @@ def newaddr(self, addresstype=None):

def pay(self, bolt11, msatoshi=None, label=None, riskfactor=None,
maxfeepercent=None, retry_for=None,
maxdelay=None, exemptfee=None, exclude=None):
maxdelay=None, exemptfee=None, localofferid=None, exclude=None,
maxfee=None, description=None):
"""
Send payment specified by {bolt11} with {msatoshi}
(ignored if {bolt11} has an amount), optional {label}
Expand All @@ -1012,7 +1013,10 @@ def pay(self, bolt11, msatoshi=None, label=None, riskfactor=None,
"retry_for": retry_for,
"maxdelay": maxdelay,
"exemptfee": exemptfee,
"localofferid": localofferid,
"exclude": exclude,
"maxfee": maxfee,
"description": description,
}
return self.call("pay", payload)

Expand Down
3 changes: 2 additions & 1 deletion doc/lightning-listpays.7.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ On success, an object containing **pays** is returned. It is an array of object
- **destination** (pubkey, optional): the final destination of the payment if known
- **label** (string, optional): the label, if given to sendpay
- **bolt11** (string, optional): the bolt11 string (if pay supplied one)
- **description** (string, optional): the description matching the bolt11 description hash (if pay supplied one)
- **bolt12** (string, optional): the bolt12 string (if supplied for pay: **experimental-offers** only).

If **status** is "pending" or "complete":
Expand Down Expand Up @@ -56,4 +57,4 @@ RESOURCES

Main web site: <https://github.com/ElementsProject/lightning>

[comment]: # ( SHA256STAMP:6ffbb1273de04f356cf79dab9a988ab030eee3317cb22e10d12d1c672249fc67)
[comment]: # ( SHA256STAMP:3c158259410ff8eb81669e26eca9ee53017002d739f89e7f0e2fd8e61edb8a14)
3 changes: 2 additions & 1 deletion doc/lightning-listsendpays.7.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ On success, an object containing **payments** is returned. It is an array of ob
- **destination** (pubkey, optional): the final destination of the payment if known
- **label** (string, optional): the label, if given to sendpay
- **bolt11** (string, optional): the bolt11 string (if pay supplied one)
- **description** (string, optional): the description matching the bolt11 description hash (if pay supplied one)
- **bolt12** (string, optional): the bolt12 string (if supplied for pay: **experimental-offers** only).

If **status** is "complete":
Expand All @@ -60,4 +61,4 @@ RESOURCES

Main web site: <https://github.com/ElementsProject/lightning>

[comment]: # ( SHA256STAMP:b03c2f306bafb1919f0933ebc695657bd691591484ddcb39b1e8706335593cd2)
[comment]: # ( SHA256STAMP:eaa0b4c6309d45bc2a72baf44288f1faa75d7f6ff2e8bf6d03be53747fe82c84)
20 changes: 18 additions & 2 deletions doc/lightning-pay.7.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ SYNOPSIS
--------

**pay** *bolt11* [*msatoshi*] [*label*] [*riskfactor*]
[*maxfeepercent*] [*retry\_for*] [*maxdelay*] [*exemptfee*]
[*exclude*]
[*maxfeepercent*] [*retry_for*] [*maxdelay*] [*exemptfee*]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[*maxfeepercent*] [*retry_for*] [*maxdelay*] [*exemptfee*]
[*maxfeepercent*] [*retry\_for*] [*maxdelay*] [*exemptfee*]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we've been eliminating those _

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh ok is the opposite!

[*localofferid*] [*exclude*] [*maxfee*] [*description*]

DESCRIPTION
-----------
Expand All @@ -32,6 +32,22 @@ leveraged by forwarding nodes. Setting `exemptfee` allows the
`maxfeepercent` check to be skipped on fees that are smaller than
`exemptfee` (default: 5000 millisatoshi).

`localofferid` is used by offers to link a payment attempt to a local
`send_invoice` offer created by lightningd-offerout(7). This ensures
that we only make a single payment for an offer, and that the offer is
marked `used` once paid.

*maxfee* overrides both *maxfeepercent* and *exemptfee* defaults (and
if you specify *maxfee* you cannot specify either of those), and
creates an absolute limit on what fee we will pay. This allows you to
implement your own heuristics rather than the primitive ones used
here.

*description* is only required for bolt11 invoices which do not
contain a description themselves, but contain a description hash.
*description* is then checked against the hash inside the invoice
before it will be paid.
Comment on lines +46 to +49
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requirement is incredibly restrictive: it limits us to invoices with description_hash where the description itself is representable as a raw JSON value. Anything like a PDF, which is something that people have been asking for, is essentially impossible to do with this, unless we allows a hex-encoded value to be provided instead, which makes this cumbersome to use. I know I voted in favor of requiring the description but this version is too limiting imho

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the limits of json!

But we can add descriptionhex later for this case?

Copy link
Contributor Author

@rustyrussell rustyrussell Apr 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at this, we need a non-trivial amount of re-engineering to add this. We cannot handle bolt11 descriptions which are not basically valid strings right now, and changing that would be an API change everywhere we print description.

So I'm leaving that for next release.


The response will occur when the payment fails or succeeds. Once a
payment has succeeded, calls to **pay** with the same *bolt11* will
succeed immediately.
Expand Down
3 changes: 2 additions & 1 deletion doc/lightning-sendpay.7.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ SYNOPSIS
--------

**sendpay** *route* *payment\_hash* [*label*] [*msatoshi*]
[*bolt11*] [*payment_secret*] [*partid*] [*localofferid*] [*groupid*] [*payment_metadata*]
[*bolt11*] [*payment_secret*] [*partid*] [*localofferid*] [*groupid*]
[*payment_metadata*] [*description*]
Comment on lines +8 to +9
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[*bolt11*] [*payment_secret*] [*partid*] [*localofferid*] [*groupid*]
[*payment_metadata*] [*description*]
[*bolt11*] [*payment\_secret*] [*partid*] [*localofferid*] [*groupid*]
[*payment\_metadata*] [*description*]

this is not a new change, at this point, I'm not really sure that \ is required!


DESCRIPTION
-----------
Expand Down
7 changes: 7 additions & 0 deletions doc/schemas/listpays.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@
"type": "string",
"description": "the bolt11 string (if pay supplied one)"
},
"description": {
"type": "string",
"description": "the description matching the bolt11 description hash (if pay supplied one)"
},
"bolt12": {
"type": "string",
"description": "the bolt12 string (if supplied for pay: **experimental-offers** only)."
Expand Down Expand Up @@ -78,6 +82,7 @@
"created_at": {},
"label": {},
"bolt11": {},
"description": {},
"bolt12": {},
"preimage": {},
"number_of_parts": {},
Expand Down Expand Up @@ -115,6 +120,7 @@
"created_at": {},
"label": {},
"bolt11": {},
"description": {},
"bolt12": {},
"amount_msat": {},
"amount_sent_msat": {},
Expand Down Expand Up @@ -152,6 +158,7 @@
"created_at": {},
"label": {},
"bolt11": {},
"description": {},
"bolt12": {},
"amount_sent_msat": {},
"erroronion": {
Expand Down
7 changes: 7 additions & 0 deletions doc/schemas/listsendpays.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@
"type": "string",
"description": "the bolt11 string (if pay supplied one)"
},
"description": {
"type": "string",
"description": "the description matching the bolt11 description hash (if pay supplied one)"
},
"bolt12": {
"type": "string",
"description": "the bolt12 string (if supplied for pay: **experimental-offers** only)."
Expand Down Expand Up @@ -108,6 +112,7 @@
"amount_sent_msat": {},
"label": {},
"bolt11": {},
"description": {},
"bolt12": {},
"payment_preimage": {
"type": "secret",
Expand Down Expand Up @@ -146,6 +151,7 @@
"amount_sent_msat": {},
"label": {},
"bolt11": {},
"description": {},
"bolt12": {},
"erroronion": {
"type": "hex",
Expand Down Expand Up @@ -182,6 +188,7 @@
"amount_sent_msat": {},
"label": {},
"bolt11": {},
"description": {},
"bolt12": {}
}
}
Expand Down
6 changes: 6 additions & 0 deletions doc/schemas/pay.request.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,12 @@
}
]
}
},
"maxfee": {
"type": "msat"
},
"description": {
"type": "string"
}
}
}
21 changes: 16 additions & 5 deletions lightningd/pay.c
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,8 @@ void json_add_payment_fields(struct json_stream *response,
else
json_add_string(response, "bolt11", t->invstring);
}
if (t->description)
json_add_string(response, "description", t->description);

if (t->failonion)
json_add_hex(response, "erroronion", t->failonion,
Expand Down Expand Up @@ -857,6 +859,7 @@ send_payment_core(struct lightningd *ld,
struct amount_msat total_msat,
const char *label TAKES,
const char *invstring TAKES,
const char *description TAKES,
const struct onionpacket *packet,
const struct node_id *destination,
struct node_id *route_nodes TAKES,
Expand Down Expand Up @@ -1100,6 +1103,10 @@ send_payment_core(struct lightningd *ld,
payment->invstring = tal_strdup(payment, invstring);
else
payment->invstring = NULL;
if (description != NULL)
payment->description = tal_strdup(payment, description);
else
payment->description = NULL;
payment->local_offer_id = tal_dup_or_null(payment, struct sha256,
local_offer_id);

Expand All @@ -1121,6 +1128,7 @@ send_payment(struct lightningd *ld,
struct amount_msat total_msat,
const char *label TAKES,
const char *invstring TAKES,
const char *description TAKES,
const struct sha256 *local_offer_id,
const struct secret *payment_secret,
const u8 *payment_metadata)
Expand Down Expand Up @@ -1194,7 +1202,7 @@ send_payment(struct lightningd *ld,
packet = create_onionpacket(tmpctx, path, ROUTING_INFO_SIZE, &path_secrets);
return send_payment_core(ld, cmd, rhash, partid, group, &route[0],
msat, total_msat,
label, invstring,
label, invstring, description,
packet, &ids[n_hops - 1], ids,
channels, path_secrets, local_offer_id);
}
Expand Down Expand Up @@ -1265,7 +1273,7 @@ static struct command_result *json_sendonion(struct command *cmd,
struct route_hop *first_hop;
struct sha256 *payment_hash;
struct lightningd *ld = cmd->ld;
const char *label, *invstring;
const char *label, *invstring, *description;
struct node_id *destination;
struct secret *path_secrets;
struct amount_msat *msat;
Expand All @@ -1285,6 +1293,7 @@ static struct command_result *json_sendonion(struct command *cmd,
p_opt("destination", param_node_id, &destination),
p_opt("localofferid", param_sha256, &local_offer_id),
p_opt("groupid", param_u64, &group),
p_opt("description", param_string, &description),
NULL))
return command_param_failed();

Expand All @@ -1306,7 +1315,8 @@ static struct command_result *json_sendonion(struct command *cmd,

return send_payment_core(ld, cmd, payment_hash, *partid, *group,
first_hop, *msat, AMOUNT_MSAT(0),
label, invstring, packet, destination, NULL, NULL,
label, invstring, description,
packet, destination, NULL, NULL,
path_secrets, local_offer_id);
}

Expand Down Expand Up @@ -1420,7 +1430,7 @@ static struct command_result *json_sendpay(struct command *cmd,
struct sha256 *rhash;
struct route_hop *route;
struct amount_msat *msat;
const char *invstring, *label;
const char *invstring, *label, *description;
u64 *partid, *group;
struct secret *payment_secret;
struct sha256 *local_offer_id;
Expand All @@ -1439,6 +1449,7 @@ static struct command_result *json_sendpay(struct command *cmd,
p_opt("localofferid", param_sha256, &local_offer_id),
p_opt("groupid", param_u64, &group),
p_opt("payment_metadata", param_bin_from_hex, &payment_metadata),
p_opt("description", param_string, &description),
NULL))
return command_param_failed();

Expand Down Expand Up @@ -1488,7 +1499,7 @@ static struct command_result *json_sendpay(struct command *cmd,
route,
final_amount,
msat ? *msat : final_amount,
label, invstring, local_offer_id,
label, invstring, description, local_offer_id,
payment_secret, payment_metadata);
}

Expand Down
2 changes: 2 additions & 0 deletions plugins/keysend.c
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,8 @@ static struct command_result *json_keysend(struct command *cmd, const char *buf,
p->min_final_cltv_expiry = 22;
p->features = NULL;
p->invstring = NULL;
/* Don't try to use invstring to hand to sendonion! */
p->invstring_used = true;
p->why = "Initial attempt";
p->constraints.cltv_budget = *maxdelay;
p->deadline = timeabs_add(time_now(), time_from_sec(*retryfor));
Expand Down
19 changes: 12 additions & 7 deletions plugins/libplugin-pay.c
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,10 @@ struct payment *payment_new(tal_t *ctx, struct command *cmd,
p->failreason = NULL;
p->getroute->riskfactorppm = 10000000;
p->abort = false;
p->invstring_used = false;
p->route = NULL;
p->temp_exclusion = NULL;
p->failroute_retry = false;
p->invstring = NULL;
p->routetxt = NULL;
p->max_htlcs = UINT32_MAX;
p->aborterror = NULL;
Expand Down Expand Up @@ -93,6 +93,8 @@ struct payment *payment_new(tal_t *ctx, struct command *cmd,
p->local_id = parent->local_id;
p->local_offer_id = parent->local_offer_id;
p->groupid = parent->groupid;
p->invstring = parent->invstring;
p->description = parent->description;
} else {
assert(cmd != NULL);
p->partid = 0;
Expand All @@ -101,6 +103,7 @@ struct payment *payment_new(tal_t *ctx, struct command *cmd,
p->channel_hints = tal_arr(p, struct channel_hint, 0);
p->excluded_nodes = tal_arr(p, struct node_id, 0);
p->id = next_id++;
p->description = NULL;
/* Caller must set this. */
p->local_id = NULL;
p->local_offer_id = NULL;
Expand Down Expand Up @@ -1535,6 +1538,7 @@ static struct command_result *payment_createonion_success(struct command *cmd,
struct out_req *req;
struct route_hop *first = &p->route[0];
struct secret *secrets;
struct payment *root = payment_root(p);

p->createonion_response = json_to_createonion_response(p, buffer, toks);

Expand Down Expand Up @@ -1564,10 +1568,16 @@ static struct command_result *payment_createonion_success(struct command *cmd,
if (p->label)
json_add_string(req->js, "label", p->label);

if (p->invstring)
if (!root->invstring_used) {
/* FIXME: rename parameter to invstring */
json_add_string(req->js, "bolt11", p->invstring);

if (p->description)
json_add_string(req->js, "description", p->description);

root->invstring_used = true;
}

if (p->destination)
json_add_node_id(req->js, "destination", p->destination);

Expand Down Expand Up @@ -3555,11 +3565,6 @@ static void presplit_cb(struct presplit_mod_data *d, struct payment *p)
struct payment *c =
payment_new(p, NULL, p, p->modifiers);

/* Annotate the subpayments with the bolt11 string,
* they'll be used when aggregating the payments
* again. */
c->invstring = tal_strdup(c, p->invstring);

/* Get ~ target, but don't exceed amt */
c->amount = fuzzed_near(target, amt);

Expand Down
7 changes: 7 additions & 0 deletions plugins/libplugin-pay.h
Original file line number Diff line number Diff line change
Expand Up @@ -255,10 +255,17 @@ struct payment {
* true. Set only on the root payment. */
bool abort;

/* We only set invstring/description on one of our sendpays per group,
* so we track when we've done that. */
bool invstring_used;

/* Serialized bolt11/12 string, kept attachd to the root so we can filter
* by the invoice. */
const char *invstring;

/* Description, usually set if bolt11 has only description_hash */
const char *description;

/* If this is paying a local offer, this is the one (sendpay ensures we
* don't pay twice for single-use offers) */
struct sha256 *local_offer_id;
Expand Down
Loading