Add Api impl for ListForwardedPayments. by G8XSU · Pull Request #32 · lightningdevkit/ldk-server

G8XSU · 2024-12-07T00:04:36Z

Based on #35

jkczyz · 2024-12-09T18:33:11Z

+ outbound_amount_forwarded_msat.unwrap_or(0), total_fee_earned_msat.unwrap_or(0), prev_channel_id, next_channel_id
+ );
+
+ let forwarded_payment = forwarded_payment_to_proto(payment_forwarded_event);


Do we need to use the proto version here? Seems it would better not to to the translation and allocate a vec.

Main reason for doing this is so that this data is easily accessible outside of LDK if need be.
We have APIs that are protobuf-defined, and eventually, we will have events that are protobuf-defined, along with some data that makes sense to be easily accessible outside of LDK in protobuf format. (However, this doesn't apply to all types of data that we store.)
For example, when a user has a Postgres backend for storage with read replicas, it might make sense for them to query data directly from the database-read-replica using a simple utility for batch queries and analytics.
This becomes even more important as LDK-server itself isn't horizontally scalable, and users might want to offload simple but large read queries to another system.

jkczyz · 2024-12-09T18:41:20Z

+pub(crate) struct Context {
+	pub(crate) node: Arc<Node>,
+}


Could you remind me why PaginatedKvStore can't be in NodeService? Is it just to save passing two parameters to the relevant handlers? If this is only need for one handler, then I'm not sure if we should bother adding the wrapper.

Yes, it is to avoid passing two parameters; otherwise, every time there is a new parameter, we need a signature change for all handlers.
Currently, the way the code is structured, we have some common handler code in handle_request, and it is enforcing every handler to have the same signature.
I believe it is okay to have the same signature for every handler and only have these common context members exposed to them; some future additions could include logger, metrics etc.

Right, that makes sense. Alternatively, you could have everything take Arc<NodeService> instead. Though we may need a wrapper on that to implement Service<Request<Incoming>> since we can't implement that trait on Arc<NodeService> directly. Then you could remove the Arc from Node and have the event handler access it through an Arc. I'm more inclined towards that than introducing a Context wrapper, unless I'm missing something.

Not sure if i completely understand this approach but it seems very complicated.
Why not go with something simpler such as service-context? what is the advantage of the other approach over it?
Currently, the whole reason for NodeService to exist is to implement Service<Request<Incoming>>, which in turn calls api-handlers. Giving api-handlers access to nodeService seems like a circular dependency.

Hmm... seems less complicated if we don't add another struct. I don't see it as a circular dependency. The only reason why these functions aren't methods on NodeService is to get around the fact that Service uses Future instead of being an async trait. Using Arc<NodeService> is just like a &self parameter that works with async.
The wrapper is only needed because both Service and Arc don't belong to this crate, so we need another type to implement Service on. But that is only needed locally.

I agree with that, we can create context inside call method.
Will use call instead of handle_request so that we don't need a signature change (for new context members) for all these methods : https://github.com/lightningdevkit/ldk-server/blob/main/ldk-server/src/service.rs#L53-L76

Right, inside call is accurate. I misspoke earlier.

Err... actually you need to create Context in handle_request to pass it to call, IIUC, since you need both the Arc<Node> and Arc<PaginatedKVStore> from NodeService to create Context.

Checkout current diff with the Context being created in call

Yeah, that looks good. Seems I was just mixing up the names for some reason. 😛

tnull

LGTM from my side I think, mod pending discussions.

G8XSU · 2024-12-13T21:47:17Z

Squashed current fixups since it would have been difficult to do context refactoring.

jkczyz

LGTM. Please squash

It will become easier to support multiple members in Context. In future commits, we will add PaginatedKvStore to Context.

G8XSU · 2024-12-13T22:13:15Z

Squashed fixups !

Add Api impl for ListForwardedPayments.

G8XSU · 2024-12-07T00:04:36Z

Based on #35

jkczyz · 2024-12-09T18:33:11Z

+ outbound_amount_forwarded_msat.unwrap_or(0), total_fee_earned_msat.unwrap_or(0), prev_channel_id, next_channel_id
+ );
+
+ let forwarded_payment = forwarded_payment_to_proto(payment_forwarded_event);


Do we need to use the proto version here? Seems it would better not to to the translation and allocate a vec.

Main reason for doing this is so that this data is easily accessible outside of LDK if need be.
We have APIs that are protobuf-defined, and eventually, we will have events that are protobuf-defined, along with some data that makes sense to be easily accessible outside of LDK in protobuf format. (However, this doesn't apply to all types of data that we store.)
For example, when a user has a Postgres backend for storage with read replicas, it might make sense for them to query data directly from the database-read-replica using a simple utility for batch queries and analytics.
This becomes even more important as LDK-server itself isn't horizontally scalable, and users might want to offload simple but large read queries to another system.

jkczyz · 2024-12-09T18:41:20Z

+pub(crate) struct Context {
+	pub(crate) node: Arc<Node>,
+}


Could you remind me why PaginatedKvStore can't be in NodeService? Is it just to save passing two parameters to the relevant handlers? If this is only need for one handler, then I'm not sure if we should bother adding the wrapper.

Yes, it is to avoid passing two parameters; otherwise, every time there is a new parameter, we need a signature change for all handlers.
Currently, the way the code is structured, we have some common handler code in handle_request, and it is enforcing every handler to have the same signature.
I believe it is okay to have the same signature for every handler and only have these common context members exposed to them; some future additions could include logger, metrics etc.

Right, that makes sense. Alternatively, you could have everything take Arc<NodeService> instead. Though we may need a wrapper on that to implement Service<Request<Incoming>> since we can't implement that trait on Arc<NodeService> directly. Then you could remove the Arc from Node and have the event handler access it through an Arc. I'm more inclined towards that than introducing a Context wrapper, unless I'm missing something.

Not sure if i completely understand this approach but it seems very complicated.
Why not go with something simpler such as service-context? what is the advantage of the other approach over it?
Currently, the whole reason for NodeService to exist is to implement Service<Request<Incoming>>, which in turn calls api-handlers. Giving api-handlers access to nodeService seems like a circular dependency.

Hmm... seems less complicated if we don't add another struct. I don't see it as a circular dependency. The only reason why these functions aren't methods on NodeService is to get around the fact that Service uses Future instead of being an async trait. Using Arc<NodeService> is just like a &self parameter that works with async.
The wrapper is only needed because both Service and Arc don't belong to this crate, so we need another type to implement Service on. But that is only needed locally.

I agree with that, we can create context inside call method.
Will use call instead of handle_request so that we don't need a signature change (for new context members) for all these methods : https://github.com/lightningdevkit/ldk-server/blob/main/ldk-server/src/service.rs#L53-L76

Right, inside call is accurate. I misspoke earlier.

Err... actually you need to create Context in handle_request to pass it to call, IIUC, since you need both the Arc<Node> and Arc<PaginatedKVStore> from NodeService to create Context.

Checkout current diff with the Context being created in call

Yeah, that looks good. Seems I was just mixing up the names for some reason. 😛

tnull

LGTM from my side I think, mod pending discussions.

G8XSU · 2024-12-13T21:47:17Z

Squashed current fixups since it would have been difficult to do context refactoring.

jkczyz

LGTM. Please squash

It will become easier to support multiple members in Context. In future commits, we will add PaginatedKvStore to Context.

G8XSU · 2024-12-13T22:13:15Z

Squashed fixups !

Add Api impl for ListForwardedPayments.

G8XSU · 2024-12-07T00:04:36Z

Based on #35

jkczyz · 2024-12-09T18:33:11Z

+ outbound_amount_forwarded_msat.unwrap_or(0), total_fee_earned_msat.unwrap_or(0), prev_channel_id, next_channel_id
+ );
+
+ let forwarded_payment = forwarded_payment_to_proto(payment_forwarded_event);


Do we need to use the proto version here? Seems it would better not to to the translation and allocate a vec.

Main reason for doing this is so that this data is easily accessible outside of LDK if need be.
We have APIs that are protobuf-defined, and eventually, we will have events that are protobuf-defined, along with some data that makes sense to be easily accessible outside of LDK in protobuf format. (However, this doesn't apply to all types of data that we store.)
For example, when a user has a Postgres backend for storage with read replicas, it might make sense for them to query data directly from the database-read-replica using a simple utility for batch queries and analytics.
This becomes even more important as LDK-server itself isn't horizontally scalable, and users might want to offload simple but large read queries to another system.

jkczyz · 2024-12-09T18:41:20Z

+pub(crate) struct Context {
+	pub(crate) node: Arc<Node>,
+}


Could you remind me why PaginatedKvStore can't be in NodeService? Is it just to save passing two parameters to the relevant handlers? If this is only need for one handler, then I'm not sure if we should bother adding the wrapper.

Yes, it is to avoid passing two parameters; otherwise, every time there is a new parameter, we need a signature change for all handlers.
Currently, the way the code is structured, we have some common handler code in handle_request, and it is enforcing every handler to have the same signature.
I believe it is okay to have the same signature for every handler and only have these common context members exposed to them; some future additions could include logger, metrics etc.

Right, that makes sense. Alternatively, you could have everything take Arc<NodeService> instead. Though we may need a wrapper on that to implement Service<Request<Incoming>> since we can't implement that trait on Arc<NodeService> directly. Then you could remove the Arc from Node and have the event handler access it through an Arc. I'm more inclined towards that than introducing a Context wrapper, unless I'm missing something.

Not sure if i completely understand this approach but it seems very complicated.
Why not go with something simpler such as service-context? what is the advantage of the other approach over it?
Currently, the whole reason for NodeService to exist is to implement Service<Request<Incoming>>, which in turn calls api-handlers. Giving api-handlers access to nodeService seems like a circular dependency.

Hmm... seems less complicated if we don't add another struct. I don't see it as a circular dependency. The only reason why these functions aren't methods on NodeService is to get around the fact that Service uses Future instead of being an async trait. Using Arc<NodeService> is just like a &self parameter that works with async.
The wrapper is only needed because both Service and Arc don't belong to this crate, so we need another type to implement Service on. But that is only needed locally.

I agree with that, we can create context inside call method.
Will use call instead of handle_request so that we don't need a signature change (for new context members) for all these methods : https://github.com/lightningdevkit/ldk-server/blob/main/ldk-server/src/service.rs#L53-L76

Right, inside call is accurate. I misspoke earlier.

Err... actually you need to create Context in handle_request to pass it to call, IIUC, since you need both the Arc<Node> and Arc<PaginatedKVStore> from NodeService to create Context.

Checkout current diff with the Context being created in call

Yeah, that looks good. Seems I was just mixing up the names for some reason. 😛

tnull

LGTM from my side I think, mod pending discussions.

G8XSU · 2024-12-13T21:47:17Z

Squashed current fixups since it would have been difficult to do context refactoring.

jkczyz

LGTM. Please squash

It will become easier to support multiple members in Context. In future commits, we will add PaginatedKvStore to Context.

G8XSU · 2024-12-13T22:13:15Z

Squashed fixups !

Add Api impl for ListForwardedPayments.

G8XSU · 2024-12-07T00:04:36Z

Based on #35

jkczyz · 2024-12-09T18:33:11Z

+ outbound_amount_forwarded_msat.unwrap_or(0), total_fee_earned_msat.unwrap_or(0), prev_channel_id, next_channel_id
+ );
+
+ let forwarded_payment = forwarded_payment_to_proto(payment_forwarded_event);


Do we need to use the proto version here? Seems it would better not to to the translation and allocate a vec.

Main reason for doing this is so that this data is easily accessible outside of LDK if need be.
We have APIs that are protobuf-defined, and eventually, we will have events that are protobuf-defined, along with some data that makes sense to be easily accessible outside of LDK in protobuf format. (However, this doesn't apply to all types of data that we store.)
For example, when a user has a Postgres backend for storage with read replicas, it might make sense for them to query data directly from the database-read-replica using a simple utility for batch queries and analytics.
This becomes even more important as LDK-server itself isn't horizontally scalable, and users might want to offload simple but large read queries to another system.

jkczyz · 2024-12-09T18:41:20Z

+pub(crate) struct Context {
+	pub(crate) node: Arc<Node>,
+}


Could you remind me why PaginatedKvStore can't be in NodeService? Is it just to save passing two parameters to the relevant handlers? If this is only need for one handler, then I'm not sure if we should bother adding the wrapper.

Yes, it is to avoid passing two parameters; otherwise, every time there is a new parameter, we need a signature change for all handlers.
Currently, the way the code is structured, we have some common handler code in handle_request, and it is enforcing every handler to have the same signature.
I believe it is okay to have the same signature for every handler and only have these common context members exposed to them; some future additions could include logger, metrics etc.

Right, that makes sense. Alternatively, you could have everything take Arc<NodeService> instead. Though we may need a wrapper on that to implement Service<Request<Incoming>> since we can't implement that trait on Arc<NodeService> directly. Then you could remove the Arc from Node and have the event handler access it through an Arc. I'm more inclined towards that than introducing a Context wrapper, unless I'm missing something.

Not sure if i completely understand this approach but it seems very complicated.
Why not go with something simpler such as service-context? what is the advantage of the other approach over it?
Currently, the whole reason for NodeService to exist is to implement Service<Request<Incoming>>, which in turn calls api-handlers. Giving api-handlers access to nodeService seems like a circular dependency.

Hmm... seems less complicated if we don't add another struct. I don't see it as a circular dependency. The only reason why these functions aren't methods on NodeService is to get around the fact that Service uses Future instead of being an async trait. Using Arc<NodeService> is just like a &self parameter that works with async.
The wrapper is only needed because both Service and Arc don't belong to this crate, so we need another type to implement Service on. But that is only needed locally.

I agree with that, we can create context inside call method.
Will use call instead of handle_request so that we don't need a signature change (for new context members) for all these methods : https://github.com/lightningdevkit/ldk-server/blob/main/ldk-server/src/service.rs#L53-L76

Right, inside call is accurate. I misspoke earlier.

Err... actually you need to create Context in handle_request to pass it to call, IIUC, since you need both the Arc<Node> and Arc<PaginatedKVStore> from NodeService to create Context.

Checkout current diff with the Context being created in call

Yeah, that looks good. Seems I was just mixing up the names for some reason. 😛

tnull

LGTM from my side I think, mod pending discussions.

G8XSU · 2024-12-13T21:47:17Z

Squashed current fixups since it would have been difficult to do context refactoring.

jkczyz

LGTM. Please squash

It will become easier to support multiple members in Context. In future commits, we will add PaginatedKvStore to Context.

G8XSU · 2024-12-13T22:13:15Z

Squashed fixups !

Add Api impl for ListForwardedPayments.

G8XSU · 2024-12-07T00:04:36Z

Based on #35

jkczyz · 2024-12-09T18:33:11Z

+ outbound_amount_forwarded_msat.unwrap_or(0), total_fee_earned_msat.unwrap_or(0), prev_channel_id, next_channel_id
+ );
+
+ let forwarded_payment = forwarded_payment_to_proto(payment_forwarded_event);


Do we need to use the proto version here? Seems it would better not to to the translation and allocate a vec.

Main reason for doing this is so that this data is easily accessible outside of LDK if need be.
We have APIs that are protobuf-defined, and eventually, we will have events that are protobuf-defined, along with some data that makes sense to be easily accessible outside of LDK in protobuf format. (However, this doesn't apply to all types of data that we store.)
For example, when a user has a Postgres backend for storage with read replicas, it might make sense for them to query data directly from the database-read-replica using a simple utility for batch queries and analytics.
This becomes even more important as LDK-server itself isn't horizontally scalable, and users might want to offload simple but large read queries to another system.

jkczyz · 2024-12-09T18:41:20Z

+pub(crate) struct Context {
+	pub(crate) node: Arc<Node>,
+}


Could you remind me why PaginatedKvStore can't be in NodeService? Is it just to save passing two parameters to the relevant handlers? If this is only need for one handler, then I'm not sure if we should bother adding the wrapper.

Yes, it is to avoid passing two parameters; otherwise, every time there is a new parameter, we need a signature change for all handlers.
Currently, the way the code is structured, we have some common handler code in handle_request, and it is enforcing every handler to have the same signature.
I believe it is okay to have the same signature for every handler and only have these common context members exposed to them; some future additions could include logger, metrics etc.

Right, that makes sense. Alternatively, you could have everything take Arc<NodeService> instead. Though we may need a wrapper on that to implement Service<Request<Incoming>> since we can't implement that trait on Arc<NodeService> directly. Then you could remove the Arc from Node and have the event handler access it through an Arc. I'm more inclined towards that than introducing a Context wrapper, unless I'm missing something.

Not sure if i completely understand this approach but it seems very complicated.
Why not go with something simpler such as service-context? what is the advantage of the other approach over it?
Currently, the whole reason for NodeService to exist is to implement Service<Request<Incoming>>, which in turn calls api-handlers. Giving api-handlers access to nodeService seems like a circular dependency.

Hmm... seems less complicated if we don't add another struct. I don't see it as a circular dependency. The only reason why these functions aren't methods on NodeService is to get around the fact that Service uses Future instead of being an async trait. Using Arc<NodeService> is just like a &self parameter that works with async.
The wrapper is only needed because both Service and Arc don't belong to this crate, so we need another type to implement Service on. But that is only needed locally.

I agree with that, we can create context inside call method.
Will use call instead of handle_request so that we don't need a signature change (for new context members) for all these methods : https://github.com/lightningdevkit/ldk-server/blob/main/ldk-server/src/service.rs#L53-L76

Right, inside call is accurate. I misspoke earlier.

Err... actually you need to create Context in handle_request to pass it to call, IIUC, since you need both the Arc<Node> and Arc<PaginatedKVStore> from NodeService to create Context.

Checkout current diff with the Context being created in call

Yeah, that looks good. Seems I was just mixing up the names for some reason. 😛

tnull

LGTM from my side I think, mod pending discussions.

G8XSU · 2024-12-13T21:47:17Z

Squashed current fixups since it would have been difficult to do context refactoring.

jkczyz

LGTM. Please squash

It will become easier to support multiple members in Context. In future commits, we will add PaginatedKvStore to Context.

G8XSU · 2024-12-13T22:13:15Z

Squashed fixups !

Add Api impl for ListForwardedPayments.

G8XSU · 2024-12-07T00:04:36Z

Based on #35

jkczyz · 2024-12-09T18:33:11Z

+ outbound_amount_forwarded_msat.unwrap_or(0), total_fee_earned_msat.unwrap_or(0), prev_channel_id, next_channel_id
+ );
+
+ let forwarded_payment = forwarded_payment_to_proto(payment_forwarded_event);


Do we need to use the proto version here? Seems it would better not to to the translation and allocate a vec.

Main reason for doing this is so that this data is easily accessible outside of LDK if need be.
We have APIs that are protobuf-defined, and eventually, we will have events that are protobuf-defined, along with some data that makes sense to be easily accessible outside of LDK in protobuf format. (However, this doesn't apply to all types of data that we store.)
For example, when a user has a Postgres backend for storage with read replicas, it might make sense for them to query data directly from the database-read-replica using a simple utility for batch queries and analytics.
This becomes even more important as LDK-server itself isn't horizontally scalable, and users might want to offload simple but large read queries to another system.

jkczyz · 2024-12-09T18:41:20Z

+pub(crate) struct Context {
+	pub(crate) node: Arc<Node>,
+}


Could you remind me why PaginatedKvStore can't be in NodeService? Is it just to save passing two parameters to the relevant handlers? If this is only need for one handler, then I'm not sure if we should bother adding the wrapper.

Yes, it is to avoid passing two parameters; otherwise, every time there is a new parameter, we need a signature change for all handlers.
Currently, the way the code is structured, we have some common handler code in handle_request, and it is enforcing every handler to have the same signature.
I believe it is okay to have the same signature for every handler and only have these common context members exposed to them; some future additions could include logger, metrics etc.

Right, that makes sense. Alternatively, you could have everything take Arc<NodeService> instead. Though we may need a wrapper on that to implement Service<Request<Incoming>> since we can't implement that trait on Arc<NodeService> directly. Then you could remove the Arc from Node and have the event handler access it through an Arc. I'm more inclined towards that than introducing a Context wrapper, unless I'm missing something.

Not sure if i completely understand this approach but it seems very complicated.
Why not go with something simpler such as service-context? what is the advantage of the other approach over it?
Currently, the whole reason for NodeService to exist is to implement Service<Request<Incoming>>, which in turn calls api-handlers. Giving api-handlers access to nodeService seems like a circular dependency.

Hmm... seems less complicated if we don't add another struct. I don't see it as a circular dependency. The only reason why these functions aren't methods on NodeService is to get around the fact that Service uses Future instead of being an async trait. Using Arc<NodeService> is just like a &self parameter that works with async.
The wrapper is only needed because both Service and Arc don't belong to this crate, so we need another type to implement Service on. But that is only needed locally.

I agree with that, we can create context inside call method.
Will use call instead of handle_request so that we don't need a signature change (for new context members) for all these methods : https://github.com/lightningdevkit/ldk-server/blob/main/ldk-server/src/service.rs#L53-L76

Right, inside call is accurate. I misspoke earlier.

Err... actually you need to create Context in handle_request to pass it to call, IIUC, since you need both the Arc<Node> and Arc<PaginatedKVStore> from NodeService to create Context.

Checkout current diff with the Context being created in call

Yeah, that looks good. Seems I was just mixing up the names for some reason. 😛

tnull

LGTM from my side I think, mod pending discussions.

G8XSU · 2024-12-13T21:47:17Z

Squashed current fixups since it would have been difficult to do context refactoring.

jkczyz

LGTM. Please squash

It will become easier to support multiple members in Context. In future commits, we will add PaginatedKvStore to Context.

G8XSU · 2024-12-13T22:13:15Z

Squashed fixups !

Add Api impl for ListForwardedPayments.

G8XSU · 2024-12-07T00:04:36Z

Based on #35

jkczyz · 2024-12-09T18:33:11Z

+ outbound_amount_forwarded_msat.unwrap_or(0), total_fee_earned_msat.unwrap_or(0), prev_channel_id, next_channel_id
+ );
+
+ let forwarded_payment = forwarded_payment_to_proto(payment_forwarded_event);


Do we need to use the proto version here? Seems it would better not to to the translation and allocate a vec.

Main reason for doing this is so that this data is easily accessible outside of LDK if need be.
We have APIs that are protobuf-defined, and eventually, we will have events that are protobuf-defined, along with some data that makes sense to be easily accessible outside of LDK in protobuf format. (However, this doesn't apply to all types of data that we store.)
For example, when a user has a Postgres backend for storage with read replicas, it might make sense for them to query data directly from the database-read-replica using a simple utility for batch queries and analytics.
This becomes even more important as LDK-server itself isn't horizontally scalable, and users might want to offload simple but large read queries to another system.

jkczyz · 2024-12-09T18:41:20Z

+pub(crate) struct Context {
+	pub(crate) node: Arc<Node>,
+}


Could you remind me why PaginatedKvStore can't be in NodeService? Is it just to save passing two parameters to the relevant handlers? If this is only need for one handler, then I'm not sure if we should bother adding the wrapper.

Yes, it is to avoid passing two parameters; otherwise, every time there is a new parameter, we need a signature change for all handlers.
Currently, the way the code is structured, we have some common handler code in handle_request, and it is enforcing every handler to have the same signature.
I believe it is okay to have the same signature for every handler and only have these common context members exposed to them; some future additions could include logger, metrics etc.

Right, that makes sense. Alternatively, you could have everything take Arc<NodeService> instead. Though we may need a wrapper on that to implement Service<Request<Incoming>> since we can't implement that trait on Arc<NodeService> directly. Then you could remove the Arc from Node and have the event handler access it through an Arc. I'm more inclined towards that than introducing a Context wrapper, unless I'm missing something.

Not sure if i completely understand this approach but it seems very complicated.
Why not go with something simpler such as service-context? what is the advantage of the other approach over it?
Currently, the whole reason for NodeService to exist is to implement Service<Request<Incoming>>, which in turn calls api-handlers. Giving api-handlers access to nodeService seems like a circular dependency.

Hmm... seems less complicated if we don't add another struct. I don't see it as a circular dependency. The only reason why these functions aren't methods on NodeService is to get around the fact that Service uses Future instead of being an async trait. Using Arc<NodeService> is just like a &self parameter that works with async.
The wrapper is only needed because both Service and Arc don't belong to this crate, so we need another type to implement Service on. But that is only needed locally.

I agree with that, we can create context inside call method.
Will use call instead of handle_request so that we don't need a signature change (for new context members) for all these methods : https://github.com/lightningdevkit/ldk-server/blob/main/ldk-server/src/service.rs#L53-L76

Right, inside call is accurate. I misspoke earlier.

Err... actually you need to create Context in handle_request to pass it to call, IIUC, since you need both the Arc<Node> and Arc<PaginatedKVStore> from NodeService to create Context.

Checkout current diff with the Context being created in call

Yeah, that looks good. Seems I was just mixing up the names for some reason. 😛

tnull

LGTM from my side I think, mod pending discussions.

G8XSU · 2024-12-13T21:47:17Z

Squashed current fixups since it would have been difficult to do context refactoring.

jkczyz

LGTM. Please squash

It will become easier to support multiple members in Context. In future commits, we will add PaginatedKvStore to Context.

G8XSU · 2024-12-13T22:13:15Z

Squashed fixups !

Add Api impl for ListForwardedPayments.

G8XSU · 2024-12-07T00:04:36Z

Based on #35

jkczyz · 2024-12-09T18:33:11Z

+ outbound_amount_forwarded_msat.unwrap_or(0), total_fee_earned_msat.unwrap_or(0), prev_channel_id, next_channel_id
+ );
+
+ let forwarded_payment = forwarded_payment_to_proto(payment_forwarded_event);


Do we need to use the proto version here? Seems it would better not to to the translation and allocate a vec.

Main reason for doing this is so that this data is easily accessible outside of LDK if need be.
We have APIs that are protobuf-defined, and eventually, we will have events that are protobuf-defined, along with some data that makes sense to be easily accessible outside of LDK in protobuf format. (However, this doesn't apply to all types of data that we store.)
For example, when a user has a Postgres backend for storage with read replicas, it might make sense for them to query data directly from the database-read-replica using a simple utility for batch queries and analytics.
This becomes even more important as LDK-server itself isn't horizontally scalable, and users might want to offload simple but large read queries to another system.

jkczyz · 2024-12-09T18:41:20Z

+pub(crate) struct Context {
+	pub(crate) node: Arc<Node>,
+}


Could you remind me why PaginatedKvStore can't be in NodeService? Is it just to save passing two parameters to the relevant handlers? If this is only need for one handler, then I'm not sure if we should bother adding the wrapper.

Yes, it is to avoid passing two parameters; otherwise, every time there is a new parameter, we need a signature change for all handlers.
Currently, the way the code is structured, we have some common handler code in handle_request, and it is enforcing every handler to have the same signature.
I believe it is okay to have the same signature for every handler and only have these common context members exposed to them; some future additions could include logger, metrics etc.

Right, that makes sense. Alternatively, you could have everything take Arc<NodeService> instead. Though we may need a wrapper on that to implement Service<Request<Incoming>> since we can't implement that trait on Arc<NodeService> directly. Then you could remove the Arc from Node and have the event handler access it through an Arc. I'm more inclined towards that than introducing a Context wrapper, unless I'm missing something.

Not sure if i completely understand this approach but it seems very complicated.
Why not go with something simpler such as service-context? what is the advantage of the other approach over it?
Currently, the whole reason for NodeService to exist is to implement Service<Request<Incoming>>, which in turn calls api-handlers. Giving api-handlers access to nodeService seems like a circular dependency.

Hmm... seems less complicated if we don't add another struct. I don't see it as a circular dependency. The only reason why these functions aren't methods on NodeService is to get around the fact that Service uses Future instead of being an async trait. Using Arc<NodeService> is just like a &self parameter that works with async.
The wrapper is only needed because both Service and Arc don't belong to this crate, so we need another type to implement Service on. But that is only needed locally.

I agree with that, we can create context inside call method.
Will use call instead of handle_request so that we don't need a signature change (for new context members) for all these methods : https://github.com/lightningdevkit/ldk-server/blob/main/ldk-server/src/service.rs#L53-L76

Right, inside call is accurate. I misspoke earlier.

Err... actually you need to create Context in handle_request to pass it to call, IIUC, since you need both the Arc<Node> and Arc<PaginatedKVStore> from NodeService to create Context.

Checkout current diff with the Context being created in call

Yeah, that looks good. Seems I was just mixing up the names for some reason. 😛

tnull

LGTM from my side I think, mod pending discussions.

G8XSU · 2024-12-13T21:47:17Z

Squashed current fixups since it would have been difficult to do context refactoring.

jkczyz

LGTM. Please squash

It will become easier to support multiple members in Context. In future commits, we will add PaginatedKvStore to Context.

G8XSU · 2024-12-13T22:13:15Z

Squashed fixups !

Add Api impl for ListForwardedPayments.

G8XSU requested a review from jkczyz December 7, 2024 00:04

jkczyz reviewed Dec 9, 2024
View reviewed changes

G8XSU force-pushed the forwarded-payments branch from 5ef7584 to 15964eaCompare December 11, 2024 23:44

G8XSU marked this pull request as ready for review December 11, 2024 23:44

tnull reviewed Dec 12, 2024
View reviewed changes

G8XSU requested review from jkczyz and tnull December 12, 2024 22:55

G8XSU mentioned this pull request Dec 12, 2024
Ldk-Server Milestone 3 #11
Open
9 tasks

tnull reviewed Dec 13, 2024
View reviewed changes

Comment threadldk-server/src/api/list_forwarded_payments.rs

G8XSU force-pushed the forwarded-payments branch from 5f6c91f to f73a54cCompare December 13, 2024 21:46

jkczyz reviewed Dec 13, 2024
View reviewed changes

Comment threadldk-server/src/api/list_forwarded_payments.rs

G8XSU requested a review from jkczyz December 13, 2024 22:03

jkczyz reviewed Dec 13, 2024
View reviewed changes

G8XSU added 5 commits December 13, 2024 14:12

Move ldk-node dependency to github commit version.
ae1deb1

Add proto definition for ListForwardedPayments API.
25c0e70

Start storing ForwardedPayments using PaginatedKVStore.
b16dc89

Move Arc<Node> to Context.
2d39edc
It will become easier to support multiple members in Context. In future commits, we will add PaginatedKvStore to Context.

Add Api impl for ListForwardedPayments.
5f34539

G8XSU force-pushed the forwarded-payments branch from ad9f221 to 5f34539Compare December 13, 2024 22:13

jkczyz approved these changes Dec 13, 2024
View reviewed changes

G8XSU merged commit f28b20d into lightningdevkit:mainDec 13, 2024

rsafier pushed a commit to rsafier/ldk-server that referenced this pull request Apr 20, 2026
Merge pull requestlightningdevkit#32from G8XSU/forwarded-payments
69fdd2c
Add Api impl for ListForwardedPayments.

G8XSU requested a review from jkczyz December 7, 2024 00:04

jkczyz reviewed Dec 9, 2024
View reviewed changes

G8XSU force-pushed the forwarded-payments branch from 5ef7584 to 15964eaCompare December 11, 2024 23:44

G8XSU marked this pull request as ready for review December 11, 2024 23:44

tnull reviewed Dec 12, 2024
View reviewed changes

G8XSU requested review from jkczyz and tnull December 12, 2024 22:55

G8XSU mentioned this pull request Dec 12, 2024
Ldk-Server Milestone 3 #11
Open
9 tasks

tnull reviewed Dec 13, 2024
View reviewed changes

Comment threadldk-server/src/api/list_forwarded_payments.rs

G8XSU force-pushed the forwarded-payments branch from 5f6c91f to f73a54cCompare December 13, 2024 21:46

jkczyz reviewed Dec 13, 2024
View reviewed changes

Comment threadldk-server/src/api/list_forwarded_payments.rs

G8XSU requested a review from jkczyz December 13, 2024 22:03

jkczyz reviewed Dec 13, 2024
View reviewed changes

G8XSU added 5 commits December 13, 2024 14:12

Move ldk-node dependency to github commit version.
ae1deb1

Add proto definition for ListForwardedPayments API.
25c0e70

Start storing ForwardedPayments using PaginatedKVStore.
b16dc89

Move Arc<Node> to Context.
2d39edc
It will become easier to support multiple members in Context. In future commits, we will add PaginatedKvStore to Context.

Add Api impl for ListForwardedPayments.
5f34539

G8XSU force-pushed the forwarded-payments branch from ad9f221 to 5f34539Compare December 13, 2024 22:13

jkczyz approved these changes Dec 13, 2024
View reviewed changes

G8XSU merged commit f28b20d into lightningdevkit:mainDec 13, 2024

rsafier pushed a commit to rsafier/ldk-server that referenced this pull request Apr 20, 2026
Merge pull requestlightningdevkit#32from G8XSU/forwarded-payments
69fdd2c
Add Api impl for ListForwardedPayments.

G8XSU requested a review from jkczyz December 7, 2024 00:04

jkczyz reviewed Dec 9, 2024
View reviewed changes

G8XSU force-pushed the forwarded-payments branch from 5ef7584 to 15964eaCompare December 11, 2024 23:44

G8XSU marked this pull request as ready for review December 11, 2024 23:44

tnull reviewed Dec 12, 2024
View reviewed changes

G8XSU requested review from jkczyz and tnull December 12, 2024 22:55

G8XSU mentioned this pull request Dec 12, 2024
Ldk-Server Milestone 3 #11
Open
9 tasks

tnull reviewed Dec 13, 2024
View reviewed changes

Comment threadldk-server/src/api/list_forwarded_payments.rs

G8XSU force-pushed the forwarded-payments branch from 5f6c91f to f73a54cCompare December 13, 2024 21:46

jkczyz reviewed Dec 13, 2024
View reviewed changes

Comment threadldk-server/src/api/list_forwarded_payments.rs

G8XSU requested a review from jkczyz December 13, 2024 22:03

jkczyz reviewed Dec 13, 2024
View reviewed changes

G8XSU added 5 commits December 13, 2024 14:12

Move ldk-node dependency to github commit version.
ae1deb1

Add proto definition for ListForwardedPayments API.
25c0e70

Start storing ForwardedPayments using PaginatedKVStore.
b16dc89

Move Arc<Node> to Context.
2d39edc
It will become easier to support multiple members in Context. In future commits, we will add PaginatedKvStore to Context.

Add Api impl for ListForwardedPayments.
5f34539

G8XSU force-pushed the forwarded-payments branch from ad9f221 to 5f34539Compare December 13, 2024 22:13

jkczyz approved these changes Dec 13, 2024
View reviewed changes

G8XSU merged commit f28b20d into lightningdevkit:mainDec 13, 2024

rsafier pushed a commit to rsafier/ldk-server that referenced this pull request Apr 20, 2026
Merge pull requestlightningdevkit#32from G8XSU/forwarded-payments
69fdd2c
Add Api impl for ListForwardedPayments.

G8XSU requested a review from jkczyz December 7, 2024 00:04

jkczyz reviewed Dec 9, 2024
View reviewed changes

G8XSU force-pushed the forwarded-payments branch from 5ef7584 to 15964eaCompare December 11, 2024 23:44

G8XSU marked this pull request as ready for review December 11, 2024 23:44

tnull reviewed Dec 12, 2024
View reviewed changes

G8XSU requested review from jkczyz and tnull December 12, 2024 22:55

G8XSU mentioned this pull request Dec 12, 2024
Ldk-Server Milestone 3 #11
Open
9 tasks

tnull reviewed Dec 13, 2024
View reviewed changes

Comment threadldk-server/src/api/list_forwarded_payments.rs

G8XSU force-pushed the forwarded-payments branch from 5f6c91f to f73a54cCompare December 13, 2024 21:46

jkczyz reviewed Dec 13, 2024
View reviewed changes

Comment threadldk-server/src/api/list_forwarded_payments.rs

G8XSU requested a review from jkczyz December 13, 2024 22:03

jkczyz reviewed Dec 13, 2024
View reviewed changes

G8XSU added 5 commits December 13, 2024 14:12

Move ldk-node dependency to github commit version.
ae1deb1

Add proto definition for ListForwardedPayments API.
25c0e70

Start storing ForwardedPayments using PaginatedKVStore.
b16dc89

Move Arc<Node> to Context.
2d39edc
It will become easier to support multiple members in Context. In future commits, we will add PaginatedKvStore to Context.

Add Api impl for ListForwardedPayments.
5f34539

G8XSU force-pushed the forwarded-payments branch from ad9f221 to 5f34539Compare December 13, 2024 22:13

jkczyz approved these changes Dec 13, 2024
View reviewed changes

G8XSU merged commit f28b20d into lightningdevkit:mainDec 13, 2024

rsafier pushed a commit to rsafier/ldk-server that referenced this pull request Apr 20, 2026
Merge pull requestlightningdevkit#32from G8XSU/forwarded-payments
69fdd2c
Add Api impl for ListForwardedPayments.

G8XSU requested a review from jkczyz December 7, 2024 00:04

jkczyz reviewed Dec 9, 2024
View reviewed changes

G8XSU force-pushed the forwarded-payments branch from 5ef7584 to 15964eaCompare December 11, 2024 23:44

G8XSU marked this pull request as ready for review December 11, 2024 23:44

tnull reviewed Dec 12, 2024
View reviewed changes

G8XSU requested review from jkczyz and tnull December 12, 2024 22:55

G8XSU mentioned this pull request Dec 12, 2024
Ldk-Server Milestone 3 #11
Open
9 tasks

tnull reviewed Dec 13, 2024
View reviewed changes

Comment threadldk-server/src/api/list_forwarded_payments.rs

G8XSU force-pushed the forwarded-payments branch from 5f6c91f to f73a54cCompare December 13, 2024 21:46

jkczyz reviewed Dec 13, 2024
View reviewed changes

Comment threadldk-server/src/api/list_forwarded_payments.rs

G8XSU requested a review from jkczyz December 13, 2024 22:03

jkczyz reviewed Dec 13, 2024
View reviewed changes

G8XSU added 5 commits December 13, 2024 14:12

Move ldk-node dependency to github commit version.
ae1deb1

Add proto definition for ListForwardedPayments API.
25c0e70

Start storing ForwardedPayments using PaginatedKVStore.
b16dc89

Move Arc<Node> to Context.
2d39edc
It will become easier to support multiple members in Context. In future commits, we will add PaginatedKvStore to Context.

Add Api impl for ListForwardedPayments.
5f34539

G8XSU force-pushed the forwarded-payments branch from ad9f221 to 5f34539Compare December 13, 2024 22:13

jkczyz approved these changes Dec 13, 2024
View reviewed changes

G8XSU merged commit f28b20d into lightningdevkit:mainDec 13, 2024

rsafier pushed a commit to rsafier/ldk-server that referenced this pull request Apr 20, 2026
Merge pull requestlightningdevkit#32from G8XSU/forwarded-payments
69fdd2c
Add Api impl for ListForwardedPayments.

G8XSU requested a review from jkczyz December 7, 2024 00:04

jkczyz reviewed Dec 9, 2024
View reviewed changes

G8XSU force-pushed the forwarded-payments branch from 5ef7584 to 15964eaCompare December 11, 2024 23:44

G8XSU marked this pull request as ready for review December 11, 2024 23:44

tnull reviewed Dec 12, 2024
View reviewed changes

G8XSU requested review from jkczyz and tnull December 12, 2024 22:55

G8XSU mentioned this pull request Dec 12, 2024
Ldk-Server Milestone 3 #11
Open
9 tasks

tnull reviewed Dec 13, 2024
View reviewed changes

Comment threadldk-server/src/api/list_forwarded_payments.rs

G8XSU force-pushed the forwarded-payments branch from 5f6c91f to f73a54cCompare December 13, 2024 21:46

jkczyz reviewed Dec 13, 2024
View reviewed changes

Comment threadldk-server/src/api/list_forwarded_payments.rs

G8XSU requested a review from jkczyz December 13, 2024 22:03

jkczyz reviewed Dec 13, 2024
View reviewed changes

G8XSU added 5 commits December 13, 2024 14:12

Move ldk-node dependency to github commit version.
ae1deb1

Add proto definition for ListForwardedPayments API.
25c0e70

Start storing ForwardedPayments using PaginatedKVStore.
b16dc89

Move Arc<Node> to Context.
2d39edc
It will become easier to support multiple members in Context. In future commits, we will add PaginatedKvStore to Context.

Add Api impl for ListForwardedPayments.
5f34539

G8XSU force-pushed the forwarded-payments branch from ad9f221 to 5f34539Compare December 13, 2024 22:13

jkczyz approved these changes Dec 13, 2024
View reviewed changes

G8XSU merged commit f28b20d into lightningdevkit:mainDec 13, 2024

rsafier pushed a commit to rsafier/ldk-server that referenced this pull request Apr 20, 2026
Merge pull requestlightningdevkit#32from G8XSU/forwarded-payments
69fdd2c
Add Api impl for ListForwardedPayments.

G8XSU requested a review from jkczyz December 7, 2024 00:04

jkczyz reviewed Dec 9, 2024
View reviewed changes

G8XSU force-pushed the forwarded-payments branch from 5ef7584 to 15964eaCompare December 11, 2024 23:44

G8XSU marked this pull request as ready for review December 11, 2024 23:44

tnull reviewed Dec 12, 2024
View reviewed changes

G8XSU requested review from jkczyz and tnull December 12, 2024 22:55

G8XSU mentioned this pull request Dec 12, 2024
Ldk-Server Milestone 3 #11
Open
9 tasks

tnull reviewed Dec 13, 2024
View reviewed changes

Comment threadldk-server/src/api/list_forwarded_payments.rs

G8XSU force-pushed the forwarded-payments branch from 5f6c91f to f73a54cCompare December 13, 2024 21:46

jkczyz reviewed Dec 13, 2024
View reviewed changes

Comment threadldk-server/src/api/list_forwarded_payments.rs

G8XSU requested a review from jkczyz December 13, 2024 22:03

jkczyz reviewed Dec 13, 2024
View reviewed changes

G8XSU added 5 commits December 13, 2024 14:12

Move ldk-node dependency to github commit version.
ae1deb1

Add proto definition for ListForwardedPayments API.
25c0e70

Start storing ForwardedPayments using PaginatedKVStore.
b16dc89

Move Arc<Node> to Context.
2d39edc
It will become easier to support multiple members in Context. In future commits, we will add PaginatedKvStore to Context.

Add Api impl for ListForwardedPayments.
5f34539

G8XSU force-pushed the forwarded-payments branch from ad9f221 to 5f34539Compare December 13, 2024 22:13

jkczyz approved these changes Dec 13, 2024
View reviewed changes

G8XSU merged commit f28b20d into lightningdevkit:mainDec 13, 2024

rsafier pushed a commit to rsafier/ldk-server that referenced this pull request Apr 20, 2026
Merge pull requestlightningdevkit#32from G8XSU/forwarded-payments
69fdd2c
Add Api impl for ListForwardedPayments.

G8XSU requested a review from jkczyz December 7, 2024 00:04

jkczyz reviewed Dec 9, 2024
View reviewed changes

G8XSU force-pushed the forwarded-payments branch from 5ef7584 to 15964eaCompare December 11, 2024 23:44

G8XSU marked this pull request as ready for review December 11, 2024 23:44

tnull reviewed Dec 12, 2024
View reviewed changes

G8XSU requested review from jkczyz and tnull December 12, 2024 22:55

G8XSU mentioned this pull request Dec 12, 2024
Ldk-Server Milestone 3 #11
Open
9 tasks

tnull reviewed Dec 13, 2024
View reviewed changes

Comment threadldk-server/src/api/list_forwarded_payments.rs

G8XSU force-pushed the forwarded-payments branch from 5f6c91f to f73a54cCompare December 13, 2024 21:46

jkczyz reviewed Dec 13, 2024
View reviewed changes

Comment threadldk-server/src/api/list_forwarded_payments.rs

G8XSU requested a review from jkczyz December 13, 2024 22:03

jkczyz reviewed Dec 13, 2024
View reviewed changes

G8XSU added 5 commits December 13, 2024 14:12

Move ldk-node dependency to github commit version.
ae1deb1

Add proto definition for ListForwardedPayments API.
25c0e70

Start storing ForwardedPayments using PaginatedKVStore.
b16dc89

Move Arc<Node> to Context.
2d39edc
It will become easier to support multiple members in Context. In future commits, we will add PaginatedKvStore to Context.

Add Api impl for ListForwardedPayments.
5f34539

G8XSU force-pushed the forwarded-payments branch from ad9f221 to 5f34539Compare December 13, 2024 22:13

jkczyz approved these changes Dec 13, 2024
View reviewed changes

G8XSU merged commit f28b20d into lightningdevkit:mainDec 13, 2024

rsafier pushed a commit to rsafier/ldk-server that referenced this pull request Apr 20, 2026
Merge pull requestlightningdevkit#32from G8XSU/forwarded-payments
69fdd2c
Add Api impl for ListForwardedPayments.

Uh oh!

Conversation

G8XSU commented Dec 7, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

G8XSUDec 13, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkczyzDec 13, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

G8XSU commented Dec 13, 2024

Uh oh!

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

G8XSU commented Dec 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

G8XSU commented Dec 7, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

G8XSUDec 13, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

G8XSU commented Dec 7, 2024•
edited
Loading

G8XSUDec 13, 2024•
edited
Loading

jkczyzDec 13, 2024•
edited
Loading

G8XSU commented Dec 7, 2024•
edited
Loading

G8XSUDec 13, 2024•
edited
Loading

jkczyzDec 13, 2024•
edited
Loading