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

Channel fund hook #2650

Merged
merged 5 commits into from May 21, 2019

Conversation

Projects
None yet
5 participants
@rustyrussell
Copy link
Contributor

commented May 20, 2019

Somehow I dropped this: I had a PR which I decided to split into pieces, and never resubmitted. I was reviewing the milestones for 0.7.1 when I noticed.

Closes: #2483

rustyrussell added some commits May 20, 2019

tools/generate-wire.py: handle optional variable-length fields.
We generated code which didn't compile (we never had one before though).

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
openingd: check with lightningd when we receive an offer.
Instead of lightningd telling us when it's ready, we ask it.
This also provides an opportunity to have a plugin hook at this point.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
json: rename json_add_amount_sat to json_add_amount_sat_compat.
New fields don't have to be spelled out twice.

The raw version are called _only, so we don't miss a call
accidentally.  We can rename them when we finally deprecated old
fields.

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

@rustyrussell rustyrussell added this to the 0.7.1 milestone May 20, 2019

@rustyrussell rustyrussell requested a review from m-schmoock May 20, 2019

@rustyrussell rustyrussell requested a review from cdecker as a code owner May 20, 2019

@ZmnSCPxj
Copy link
Collaborator

left a comment

Minor nit in function arguments.

const char *msatfieldname)
void json_add_amount_msat_compat(struct json_stream *result,
struct amount_msat msat,
const char *rawfieldname,

This comment has been minimized.

Copy link
@ZmnSCPxj

ZmnSCPxj May 20, 2019

Collaborator

Other json_add_* functions have field names before the value being printed.
Mildly suggest, to follow other functions.

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell May 20, 2019

Author Contributor

I'd agree, but it's a lot of churn for a function we're going to phase out, so I think it's a bit gratuitous for now.

@m-schmoock
Copy link
Collaborator

left a comment

I like really it. As ususal, well structured, testcase, doc..
I didn't finish my implementation it as I noticed you already worked on this last month (#2576 (comment))....

Here my remarks:

  • If possible add the remote routing fees to the payload
  • msat/sat fieldname/parameter name confusions everywhere, maybe I misunderstood the _compat function?
struct amount_msat max_htlc_value_in_flight_msat;
struct amount_sat channel_reserve_satoshis;
struct amount_msat htlc_minimum_msat;
u32 feerate_per_kw;

This comment has been minimized.

Copy link
@m-schmoock

m-schmoock May 20, 2019

Collaborator

Do we already know remote routing fees (base, ppm) here? If yes, we should add it to payload so plugin knows...

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell May 20, 2019

Author Contributor

No, we don't ever get told those directly; we learn them like everyone else, when/if it advertizes the channel.

"max_htlc_value_in_flight_msat": "18446744073709551615msat",
"channel_reserve_satoshis": "1000000msat",
"htlc_minimum_msat": "0msat",
"feerate_per_kw": 7500

This comment has been minimized.

Copy link
@m-schmoock

m-schmoock May 20, 2019

Collaborator

missing comma end of line :-)

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell May 20, 2019

Author Contributor

Yes, and there's a trailing one on the last line, which isn't valid JSON either. Fixed!

void json_add_amount_sat_compat(struct json_stream *result UNNEEDED,
struct amount_sat sat UNNEEDED,
const char *rawfieldname UNNEEDED,
const char *msatfieldname)

This comment has been minimized.

Copy link
@m-schmoock

m-schmoock May 20, 2019

Collaborator

Since you are already touching this line, rename msatfieldname to satfieldname

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell May 20, 2019

Author Contributor

No, these are always msat fields, since we unified on that as standard.

void json_add_amount_sat_compat(struct json_stream *result UNNEEDED,
struct amount_sat sat UNNEEDED,
const char *rawfieldname UNNEEDED,
const char *msatfieldname)

This comment has been minimized.

Copy link
@m-schmoock

m-schmoock May 20, 2019

Collaborator

same here, rename to satfieldname

json_add_amount_sat(response, utxos[i]->amount,
"value", "amount_msat");
json_add_amount_sat_compat(response, utxos[i]->amount,
"value", "amount_msat");

This comment has been minimized.

Copy link
@m-schmoock

m-schmoock May 20, 2019

Collaborator

Maybe I'm off here, but isn't that supposte to be called amount_sat?

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell May 20, 2019

Author Contributor

No... we always print amounts as msat now, to avoid confusion.

void json_add_amount_sat_compat(struct json_stream *result,
struct amount_sat sat,
const char *rawfieldname,
const char *msatfieldname)

This comment has been minimized.

Copy link
@m-schmoock

m-schmoock May 20, 2019

Collaborator

Should this be satfieldname?

}

void json_add_amount_sat_only(struct json_stream *result,
const char *msatfieldname,

This comment has been minimized.

Copy link
@m-schmoock

m-schmoock May 20, 2019

Collaborator

Here too?

"our_amount_msat");
json_add_amount_sat_compat(response, c->funding,
"channel_total_sat",
"amount_msat");

This comment has been minimized.

Copy link
@m-schmoock

m-schmoock May 20, 2019

Collaborator

... same here in line 512 and 515, since this msat/sat is mixed up everywhere, maybe its just me??


@plugin.hook('openchannel')
def on_openchannel(openchannel, plugin):
print("{} VARS".format(len(openchannel.keys())))

This comment has been minimized.

Copy link
@cdecker

cdecker May 20, 2019

Member

You could just include from pprint import pprint which recursively prettyprints whatever you pass to it.

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell May 20, 2019

Author Contributor

Nice idea!

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell May 20, 2019

Author Contributor

Doesn't work as well due to undefined ordering, however, and the fact that it prints "{" and "}":

b"2019-05-20T22:47:55.079Z plugin-reject_odd_funding_amounts.py {'channel_flags': 1,"
b"2019-05-20T22:47:55.079Z plugin-reject_odd_funding_amounts.py  'channel_reserve_satoshis': '1000000msat',"
b"2019-05-20T22:47:55.079Z plugin-reject_odd_funding_amounts.py  'dust_limit_satoshis': '546000msat',"
b"2019-05-20T22:47:55.079Z plugin-reject_odd_funding_amounts.py  'feerate_per_kw': 7500,"

rustyrussell added some commits May 20, 2019

openingd: add openchannel hook.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/channel-fund-hook branch from 54b583d to 2abc7e7 May 20, 2019

@niftynei

This comment has been minimized.

Copy link
Collaborator

commented May 21, 2019

ACK 2abc7e7

@niftynei niftynei merged commit e5b5f1d into ElementsProject:master May 21, 2019

2 checks passed

ackbot PR ack'd by niftynei
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.