Implement first set of Api's #5
Conversation
|
This needs a rebase now. |
G8XSU
changed the title
| node: Arc<Node>, request: OnchainSendRequest, | ||
| ) -> Result<OnchainSendResponse, ldk_node::NodeError> { | ||
| let address = Address::from_str(&request.address) | ||
| .map_err(|_| ldk_node::NodeError::InvalidAddress)? |
There was a problem hiding this comment.
Should we import NodeError to avoid the prefix?
There was a problem hiding this comment.
I kept it to make it explicit when there are other error types floating around, specifically ldk-server specific.
But I dont mind it either way.
| ) -> Result<OnchainSendResponse, ldk_node::NodeError> { | ||
| let address = Address::from_str(&request.address) | ||
| .map_err(|_| ldk_node::NodeError::InvalidAddress)? | ||
| .require_network(node.config().network) |
There was a problem hiding this comment.
I wonder if it would be worth retrieving the Config once and then giving a &Config to the handler methods rather than always calling in? Would at least avoid cloning it every time.
There was a problem hiding this comment.
Not all api's need it, and there could be future such instances where a subset of api's need some field.
I think we shouldn't add it to common handler for now.
|
|
||
| pub(crate) fn handle_onchain_send_request( | ||
| node: Arc<Node>, request: OnchainSendRequest, | ||
| ) -> Result<OnchainSendResponse, ldk_node::NodeError> { |
There was a problem hiding this comment.
Might be fine for the intial step, but we might need to introduce a separate error type that wraps the NodeError, i.e., will be a superset?
There was a problem hiding this comment.
Yeah currently it is only for initial step, will probably spend more time on error handling when we have it in api interface.
server/src/api/open_channel.rs
Outdated
| request.announce_channel, | ||
| )?; | ||
| let response = | ||
| OpenChannelResponse { user_channel_id: user_channel_id.0.to_be_bytes().to_vec() }; |
There was a problem hiding this comment.
I wonder if it would be worth down the line defining some kind of de/ser traits to make sure the encoding from these types into their respective field types is always consistent and we can't accidentally, e.g., encode as little-endian in some places?
| pub(crate) fn handle_close_channel_request( | ||
| node: Arc<Node>, request: CloseChannelRequest, | ||
| ) -> Result<CloseChannelResponse, ldk_node::NodeError> { | ||
| //TODO: Should this be string? |
There was a problem hiding this comment.
IMO, since PaymentId is its dedicated proto type, UserChannelId/ChannelId, etc. should probably also follow the same pattern?
There was a problem hiding this comment.
@tnull @jkczyz
The main problem is user interaction with these byte identifiers, for which I could use some ideas. Essentially, IDs such as ChannelId, UserChannelId, and PaymentId are not human-readable in byte form and cannot be easily input or output as bytes.
Using bytes is fine for programmatic access, but for CLI or while interacting with a lightning node through a UI, this isn't really feasible. We have a similar problem for the logs of these IDs as well. (See: lightningdevkit/rust-lightning#3306)
In my opinion, we need a standardized way of interacting with these in a human-friendly manner, not just for debug logs. For this, I was considering whether we can use a hex representation string in the interface. This could be just for the CLI/UI or as part of the main API interface.
There was a problem hiding this comment.
Mhh, I don't have a strong opinion, but I do think it should be uniform, at least across all *Id types.
There was a problem hiding this comment.
I thought more about this,
I don't think adding separate types for *Id helps from API perspective. It just introduces further nesting of types.
It might make sense in ldk-node api to introduce special types where you can have type checking but not in an api.
{
payment_id :PaymentId{data:[..]},
user_channel_id: UserChannelId{data:[..]},
channel_id: ChannelId{data:[..]},
}
compared to:
{
payment_id :[..],
user_channel_id: [..],
channel_id: [..],
}
Bolt11SendResponse { payment_id: Some(PaymentId { data: payment_id.0.to_vec() }) }
compared to
Bolt11SendResponse { payment_id: payment_id.0.to_vec() }
So to make it uniform, I am thinking of removing PaymentId type itself.
There was a problem hiding this comment.
So to make it uniform, I am thinking of removing PaymentId type itself.
Alright, fine by me as long as it's uniform and ~predictable by the user/dev so they don't have to look up every single detail in the docs when using the API. And, at this stage, we could still easily change anything if we find a reason why we need separate types in the future.
Feel free to add the commit dropping PaymentId here before we land this.
There was a problem hiding this comment.
Yes added a commit for it.
Squashed and rebased.
|
@tnull Is there any additional feedback on this? |
There was a problem hiding this comment.
@tnull Is there any additional feedback on this?
Not really, but we should probably come to a conclusion on #5 (comment) and #5 (comment) before we move on?
Besides that, feel free to interleave and squash the fixups into their respective commits.
| use crate::api::onchain_send::*; | ||
| use crate::api::open_channel::*; | ||
| use crate::api::bolt11_receive::handle_bolt11_receive_request; | ||
| use crate::api::bolt11_receive::BOLT11_RECEIVE_PATH; |
There was a problem hiding this comment.
nit: Could group the imports from each module, but doesn't matter too much.
|
Squashed. |
|
Squashed and rebased. |
Adds implementation for:
Based on #2