Add Initial Protobuf generation setup and initial protos for v1 api's.

[G8XSU]

• Add Initial Protobuf generation setup and initial protos for v1 api's.

Api specification included in this PR:

• OnchainReceive
• OnchainSend
• Bolt11Receive
• Bolt11Send
• Bolt12Receive
• Bolt12Send
• OpenChannel
• CloseChannel (includes force-close)
• ListChannels

[G8XSU]

@tnull

[tnull]

Ah, seems there was a prost version mismatch in protos vs. server, now corrected in #3.

[tnull]

Had a first look, added a few comments.

I really think we should align at least the initial version of the API with LDK Node's API and copy over the LDK Node docs. This way, we'd be able to take advantage of the effort we already put in over there, in particular properly documenting potential caveats, etc.

[tnull]

Needs a minor rebase now.

[G8XSU]

I really think we should align at least the initial version of the API with LDK Node's API and copy over the LDK Node docs.

Most of it is that and we align with apis in ldk-node/ldk and want to refer to docs from ldk-node or ldk for details.
So I choose to provide a brief description, and mostly refer to ldk-node/ldk docs. Because it would be infeasible to cover all the cases again at this api level.

Note that it is much easier to change apis for a library than it is for a service, we have lot of flexibility in both ldk/ldk-node to change/merge api's but we can't do that easily in ldk-node-server.

Hence I kept most of the api as same but made some minor simplifications after referring to both lnd and clightning api.
I want to avoid making a separate api operation for everything, it can easily balloon up into an unmanageable set from a service perspective.

I think it makes sense to combine CloseChannel & ForceCloseChannel into 1 api.
Send and SendAll into 1 api.
CreateInvoiceWithAmount and CreateInvoice into 1 api.

[tnull]

Most of it is that and we align with apis in ldk-node/ldk and want to refer to docs from ldk-node or ldk for details.
So I choose to provide a brief description, and mostly refer to ldk-node/ldk docs. Because it would be infeasible to cover all the cases again at this api level.

But if we really align the API for the first version, we could literally copy over the docs, do some minor modifications and be done with it, no? So it would save us a bunch of work re-writing and re-reviewing all the API docs?

Note that it is much easier to change apis for a library than it is for a service, we have lot of flexibility in both ldk/ldk-node to change/merge api's but we can't do that easily in ldk-node-server.

Could you expand on why you think this is the case? How is a library materially different here?

Hence I kept most of the api as same but made some minor simplifications after referring to both lnd and clightning api.

I'm really really sceptical of this approach. Each implementation has their own idiosyncrasies and IMO we should strive to create the best API design we can come up with, and shouldn't unnecessarily restrict us by trying to match LND and/or CLN which have different technical scope and limitations. If you accommodate all the limitations of LDK (Node), CLN, and LND in the API design, you end up with the least common denominator, which is likely not a great outcome.

I want to avoid making a separate api operation for everything, it can easily balloon up into an unmanageable set from a service perspective.

Well, we share the same design goals in LDK Node. Meaning, if we think we can find a more succinct API design that accommodates our users needs, the changes should also/first happen in LDK Node itself, not only in the server part.

I think it makes sense to combine CloseChannel & ForceCloseChannel into 1 api.
Send and SendAll into 1 api.
CreateInvoiceWithAmount and CreateInvoice into 1 api.

No, there are good reasons why we kept them separate API methods in the first place, and in some cases they used to be combined methods and we refactored them to be separate. For close channel, you really want to avoid API misuse by have the user call two clearly distinct endpoints, as force closures are much more costly.

For send/send_all, they currently have very distinct meaning w.r.t. balance handling (send_all draining all available inputs), but we're just about to introduce a way to honor the reserves in send_all (cf. lightningdevkit/ldk-node#345), which users repeatedly requested. I'm not sure you'll be easily be able to accommodate these behavioral details if you munch them together.

For receive/receive_variable_amount you could consider making the amount optional in a single receive method, but you might want to think through which fields will need to be added in the future. The _for_hash fields also? JIT channel receive? Or will these then be separate methods?

[G8XSU]

But if we really align the API for the first version, we could literally copy over the docs, do some minor modifications and be done with it, no? So it would save us a bunch of work re-writing and re-reviewing all the API docs?

Most of it is just that and we refer to docs from ldk-node/ldk.

Could you expand on why you think this is the case? How is a library materially different here?

In case of a service, api's could be getting used by multiple components deployed and managed separately by different team etc in case of large scale deployment, and upgrade/downgrade becomes challenging considering backward compatibility.
In case of a library, api is generally deployed within a single unit and we have compile time errors.

I understand you might have reasons to keep them as separate api's in ldk_node, but at the same time i don't want to introduce too many api's where client is left guessing which one to use for what and whether an api exists for some specific requirement.

Not so long ago, a client asked for yet another method for creating invoice with all params,
and cited:

In an ideal world, we will have a builder method and will create invoice using that, and we can do validations.
We should try to move in that direction and this is a step towards that.

Overall I would like to minimize api surface now if i can rather than wait for ldk-node. Whether we want to do similar changes in ldk-node or not is a separate topic. At no point I want to combine api's which could cause confusion but note that these set of api's are similar to what bdk, lnd and cln provide, I don't see big downsides here.

For example just receive has : receive, receive_with_varaible_amount, receive_with_hash, receive_variable_amount_with_hash, receive_with_jit_channel, receive_variable_amount_via_jit_channel as apis.

if it makes sense to semantically combine some of them, we could make some grouping like
"(receive, receive_with_varaible_amount), (receive_with_hash, receive_varaible_amount_with_hash) and (receive_with_jit_channel, receive_variable_amount_via_jit_channel)" combined.

Lets discuss it offline in meeting, so that we can converge faster.

[tnull]

The APIs look good, just some comments.

Will do a (final?) closer look at the docs next.

Do you mind cleaning up the commit history, grouping them in feature and fixup commits?

[tnull]

It seems we also need error response types for all of these *Requests, no?

[jkczyz]

FWIW, the best practices I've seen for protocol buffers is to use a single error response. https://cloud.google.com/apis/design/errors

[G8XSU]

Yes will introduce an error type at once for all api's probably.
It needs some more thinking, hence i left it out as a separate task for now.

[tnull]

Yeah, while I think using a single error response might be most common, in Rust it might be more convenient to have separate types or at least enum variants? So we may then want to have a single response type with the numerical error code, but then actually convert it (by implementing From both ways?) to a well-defined error type that we use most places in Rust?

[G8XSU]

we may then want to have a single response type with the numerical error code,

Yes, I envision something like that. Let us have this discussion once I am working on error handling, this PR isn't directly related to this. Will discuss some approaches before the error-handling part.

[tnull]

Alright, makes sense!

Do we want a tracking issue listing the planned upcoming PRs? Might make it easier to follow what will be added at which stage, but maybe also no big deal?

[tnull]

LGTM, I think.

To make some progress here and keep iterating I won't get into any doc nits for now, but I think we should proofread and make them more uniform in a single go once all of the MVP APIs have been added.

Could you interleave the fixups and squash them into their respective feature commits?

[tnull]

Going to land this.

It would be nice if you could provide a diff if you make additional changes during squashing. Diff wasn't huge in this case, but makes reviewing the changes easier.

[jkczyz]

Going to land this.

It would be nice if you could provide a diff if you make additional changes during squashing. Diff wasn't huge in this case, but makes reviewing the changes easier.

FYI, the Github "Compare" link next to the forced push item should give you a diff. It mostly works lol.

[tnull]

FYI, the Github "Compare" link next to the forced push item should give you a diff. It mostly works lol.

Thanks, I'm aware of that link, but especially for repeated force-pushes GitHub likes to forget the diffs, so often it's not really reliable..