Add Api impl for ListForwardedPayments.

[G8XSU]

Based on #35

[jkczyz]

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

[G8XSU]

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]

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.

[G8XSU]

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.

[jkczyz]

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.

[G8XSU]

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.

[jkczyz]

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.

[G8XSU]

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

[jkczyz]

Right, inside call is accurate. I misspoke earlier.

[jkczyz]

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.

[G8XSU]

Checkout current diff with the Context being created in call

[jkczyz]

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]

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

[jkczyz]

LGTM. Please squash

[G8XSU]

Squashed fixups !