-
Notifications
You must be signed in to change notification settings - Fork 26
Add serde serializers for Bytes and Enums in protos #122
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
base: main
Are you sure you want to change the base?
Add serde serializers for Bytes and Enums in protos #122
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
f02251f to
c45e341
Compare
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
tnull
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, not the biggest fan of this approach, as it will add a lot of additional boilerplate here, that then surely will get stale/out-of-sync at some point. Do you see a way to avoid that? Could we maybe reuse some of the Server types rather than replicating them?
|
Okay found a much better way to do this. Only downside is we have the double |
c45e341 to
8c5c493
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, love that it's much less invasive! One question.
Also, clippy CI is sad it seems
ldk-server-protos/src/serde_utils.rs
Outdated
| S: Serializer, | ||
| { | ||
| let name = match PaymentDirection::from_i32(*value) { | ||
| Some(PaymentDirection::Inbound) => PaymentDirection::Inbound.as_str_name(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make use of the stringify macro instead of having to match each type/variant individually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't use the actual stringify macro but was able to create my own that makes this easier
Adds serializers for bytevectors and enums defined in our proto files. This makes it so we can print out something like `"status": "SUCCEEDED"` instead of `"status": 1`.
8c5c493 to
af16148
Compare
Adds serializers for bytevectors and enums defined in our proto files.
This makes it so we can print out something like
"status": "SUCCEEDED"instead of"status": 1.before:
after: