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

listpays #2382

Merged
merged 9 commits into from Feb 23, 2019

Conversation

Projects
None yet
2 participants
@rustyrussell
Copy link
Contributor

rustyrussell commented Feb 22, 2019

Also cleans up our badly-misunderstood 'description' parameter for sendpay/pay to do what people (including me!) thought it did.

@cdecker Q on what you want to do with compat in pylightning: I didn't, but we can.

@rustyrussell rustyrussell requested a review from cdecker as a code owner Feb 22, 2019

@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/listpays branch 2 times, most recently from e827a19 to a72782c Feb 22, 2019

@rustyrussell rustyrussell added this to the v0.7 milestone Feb 22, 2019

@@ -464,18 +464,16 @@ def waitsendpay(self, payment_hash, timeout=None):
}
return self.call("waitsendpay", payload)

def pay(self, bolt11, msatoshi=None, description=None, riskfactor=None):
def pay(self, bolt11, msatoshi=None, label=None, riskfactor=None):

This comment has been minimized.

@cdecker

cdecker Feb 22, 2019

Member

I'd propose to just add both label and description and then check that only one is set when constructing the payload (assigning that one to label). We can also warn about description being phased out and remove it once we remove the first wave of deprecated APIs.

This comment has been minimized.

@rustyrussell

rustyrussell Feb 22, 2019

Author Contributor

Well, if we accept both, lightningd already does that check?

This comment has been minimized.

@cdecker

cdecker Feb 22, 2019

Member

Right 👍

return command_success(cmd, ret);
}

static struct command_result *handle_listpays(struct command *cmd,

This comment has been minimized.

@cdecker

cdecker Feb 22, 2019

Member

So far we had the convention of calling these json_<command_name> which I found very useful for grepping them.

This comment has been minimized.

@rustyrussell

rustyrussell Feb 22, 2019

Author Contributor

Yeah, good idea.

"Unexpected non-array result from listpayments");

ret = tal_fmt(cmd, "{ 'pays': [");
json_for_each_arr(i, t, arr) {

This comment has been minimized.

@cdecker

cdecker Feb 22, 2019

Member

Am I correct in thinking that this loop will eventually coalesce all parts of a multi-part payment?

This comment has been minimized.

@rustyrussell

rustyrussell Feb 22, 2019

Author Contributor

Yes.

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Feb 22, 2019

ACK a72782c

@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/listpays branch 2 times, most recently from fb83e61 to bda197d Feb 23, 2019

rustyrussell added some commits Feb 23, 2019

sendpay: provide 'bolt11' parameter.
Without this, there's no proof of payment, since it is the signed invoice
that make the receipt valid.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
sendpay: rename 'description' to 'label'.
This field was used by `pay` to hold the bolt11 description if the bolt11
string used `h` to hash the description (which nobody ever did).  If the
`h` field wasn't present, it could contain anything, as it wasn't checked.

It's really useful to have a label for payments (eg. '1 Cuban'), but adding
yet-another option would be painful, so we simply rename 'description'
to 'label' except inside the db.

This means we need to do some tricky parameter parsing to handle array
and keyword JSON arguments, but only until we remove the old name.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
plugin/pay: feed bolt11 string through to sendpay.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
libplugin: add deprecated_apis flag for plugins to access.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/listpays branch 2 times, most recently from 539ff28 to e8c20e9 Feb 23, 2019

rustyrussell added some commits Feb 23, 2019

plugin/pay: rename 'description' to 'label', deprecate 'description'.
This is the same deprecation, but one level up.  For the moment, we
still support invoices with a `h` field (where description will be
necessary) but that will be removed once this option is removed.

Note that I just changed pylightning without backwards compatibility,
since the field was unlikely to be used, but we could do something
more complex here?

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
doc: update listpayments man page to match current semantics.
It didn't show the optional parameters, and the output fields were
obsolete.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
listpays: new command to summarize pay commands.
This is to future-proof against multi-part-payments: the low-level commands
will start returning multiple results once we have that, so prepare
transition plan now.

Closes: #2372
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
listsendpays: updated version of listpayments.
New name is less confusing, and most people should be transitioning to
listpays rather than this anyway.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
plugins/pay: use json_ prefix for json command handlers.
That makes them intuitive to find with TAGS or grep.

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

@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/listpays branch from e8c20e9 to f516d94 Feb 23, 2019

@rustyrussell rustyrussell merged commit b996173 into ElementsProject:master Feb 23, 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.