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

Use the peerswaprpc proto file for shared serialization #134

Open
nepet opened this issue Sep 21, 2022 · 6 comments · May be fixed by #231
Open

Use the peerswaprpc proto file for shared serialization #134

nepet opened this issue Sep 21, 2022 · 6 comments · May be fixed by #231
Labels
good first issue Good for newcomers

Comments

@nepet
Copy link
Contributor

nepet commented Sep 21, 2022

Currently we don't have a shared serialization for peerswapd and the cln-plugin.
We should take advantage of the peerswaprpc proto file and use the generated stubs for the responses of the cln-plugin.

@nepet nepet added the good first issue Good for newcomers label Sep 21, 2022
@NonsoAmadi10
Copy link

Hello @nepet Can I take up this issue?

@nepet
Copy link
Contributor Author

nepet commented Mar 22, 2023

Hey @NonsoAmadi10, welcome aboard!

This issue is not worked out really well and I am certain that I was a bit quick with the good first issue label.
The broader idea is to have a common source for the responses of peerswap rpc/grpc methods.

  • Core-Lightning uses json-rpcv2 that is currently handled by glightning.
  • Lnd uses grpc.

We currently maintain two different data models. The grpc stubs are generated from the peerswaprpc.proto file while the json responses for core-lightning are defined in the clightning dir. It would be nice to have a common model for lnd and core-lightning responses as they already have diverted in the past.

One idea was to use the messages that are generated from peerswap.proto for both implementations. Some core-lightning response messages do already use the generated stubs. Look out for peerswaprpc in the clightning/clightning_commands.go file.

Any thoughts?

@NonsoAmadi10
Copy link

Using the generated messages from the peerswap.proto file for both implementations could be a good solution, as it would provide a common source of truth for response messages. But won't it require significant refactoring in both Core-Lightning and Lnd to adopt the new message definitions? One possible approach to minimize the impact of such a change is to gradually introduce the new messages into both components in a backwards-compatible way, while maintaining the existing response models for backwards compatibility. This could involve mapping the new messages to the old models.

I am not quite conversant with proto buffers but I won't mind working on it if I get to understand clearly where all this pieces fit in. My answer is based on the context you defined above. I hope I made sense

@nepet
Copy link
Contributor Author

nepet commented Mar 29, 2023

Lnd already uses the generated stubs from the peerswaprpc.proto, so there we are already good. For cln, a good first step might be to compare and find differences in the response messages, if there are any (ideally there should be none).
@ShahanaFarooqui (Ride The Lightning) might be the biggest consumer of our api right now. Maybe she can help point to the differences?
If she is okay with the changes that are imposed by using the peerswaprpc.proto as a SSOT for our response messages we should not care too much about compatibility layers right now as the software is in quite an early stage.

Did you join our discord server? Most of the peerswap operators are members of our discord server and we could ask who would be affected by any changes before we make them.

Cheers!

@ShahanaFarooqui
Copy link

@nepet @NonsoAmadi10 I already have to adjust some of the response signatures for CLN as they are outdated. I wouldn't mind to update some more responses. So I am not worried about backward compatibility right now. Rather it will be helpful when we will implement peerswap UI on LND.

@YusukeShimizu
Copy link
Contributor

YusukeShimizu commented Aug 30, 2023

Hello @nepet

I am considering a policy of using messages generated from peerswaprpc for both implementations.
I think there will probably be differences in the struct's json tags.
For example, GetAddressResponse.Address will be changed from lbtc_address to address,omitempty.
Is this acceptable?

Also, it looks like the short channel id and state for PeerSwapPeerChannel needs to be converted to channel id and active.
The existing behavior appears to be that the short channel id is set, though it is named channel id.

I have created a draft #231 to show you my thoughts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants