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 #231

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

YusukeShimizu
Copy link
Contributor

@YusukeShimizu YusukeShimizu commented Aug 31, 2023

This is for #134.
This change is the serialization part for peerswapd and the cln-plugin.
I use the peerswaprpc proto file for shared serialization.

The following changes will be made.

  • differences in the struct's json tags.
  • short channel id and state for PeerSwapPeerChannel is converted to channel id and active.

We should take advantage of the peerswaprpc proto file and use the generated stubs for the responses of the cln-plugin.

Part of the response message is changed.
Since short channel id needs to be changed to channel id due to the use of peerswaprpc for serialization.
@wtogami
Copy link
Contributor

wtogami commented Aug 31, 2023

I didn't expect anyone to work on this ticket. This is very late right before a release. Are you sure this change has no impact on how things currently behave?

@nepet nepet linked an issue Aug 31, 2023 that may be closed by this pull request
@YusukeShimizu
Copy link
Contributor Author

No, this change may affect cln plugin behavior.

@wtogami
Copy link
Contributor

wtogami commented Sep 1, 2023

  • We want to do a stable release soon so we don't want to change anything big in the short-term.
  • This will result in API changes.
  • The premium feature will require API changes.
  • So it's best to merge this along with other API changes so other applications don't need to change more than once.

Copy link
Contributor

@nepet nepet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @YusukeShimizu,

You definitely got the idea what #134 is about and your execution so far is exactly what I was hoping for.

This PR gives us the opportunity to clean up some technical dept and serialization overhead and allows us to revisit the API and how it is accessed by humans. I expect most comments and discussions to be about field names and values while you streamline the API.

All of that being said, as we are close to a release, we might want to wait postpone merging this PR until the transition to the peerswaprc messages is fully completed. I do not expect the API to change that much, if at all in the meantime.

Cheers!

return nil, err
}
peerSwapPeerChannels = append(peerSwapPeerChannels, &peerswaprpc.PeerSwapPeerChannel{
ChannelId: scid.ToUint64(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a scid it is more intuitive and "human readable". Do you think it makes sense to update the proto message

message PeerSwapPeerChannel {
uint64 channel_id = 1;
uint64 local_balance = 2;
uint64 remote_balance = 3;
bool active = 5;
}
to take a scid (string) instead of a channel id?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a concern that replacing the channel id with a short channel id would be a breaking change and have a impact.
I think there will be a no problem if both channel id and short channel id are set, so I just add a short channel id.
de30b18

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Internally we use the scid so there should no be a problem here. Changing from channel_id to scid would "break" the api that is just consumed by peerswapcli and maybe RTL so far. Vice versa for a change from scid to channel_id on the core-lightning plugin. We either way have to go with on change on the user facing api 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this may have caused a little confusion, so I will state it again.

I considered the following two way.
I personally believe that 1 is the way that will have the least impact, which do you think better?

  1. Add string short_channel_id = 6;.
    The current change(de30b18) does this.
message PeerSwapPeerChannel {
    uint64 channel_id = 1;
    uint64 local_balance = 2;
    uint64 remote_balance = 3;
    bool active = 5;
    string short_channel_id = 6;
}
  1. Update uint64 uint channel_id = 1; to string short_channel_id = 1;
message PeerSwapPeerChannel {
    string short_channel_id = 1;
    uint64 local_balance = 2;
    uint64 remote_balance = 3;
    bool active = 5;
}

A short channel id is more intuitive and "human readable".
Added short channel id for rpc, and set the value both short channel id and channel id for cln.
@YusukeShimizu
Copy link
Contributor Author

All of that being said, as we are close to a release, we might want to wait postpone merging this PR until the transition to the peerswaprc messages is fully completed. I do not expect the API to change that much, if at all in the meantime.

I understand the above.
I will keep the PR in draft status until merge is available.
Please let me know if there is a proper way to operate it.

@YusukeShimizu YusukeShimizu mentioned this pull request Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use the peerswaprpc proto file for shared serialization
3 participants