-
Notifications
You must be signed in to change notification settings - Fork 883
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
msggen: add routes to decode and decodepay #7257
Conversation
80bfcbe
to
e97721c
Compare
cln-rpc/src/primitives.rs
Outdated
#[serde(rename = "id")] | ||
pub pubkey: PublicKey, | ||
#[serde(rename = "scid")] | ||
pub short_channel_id: ShortChannelId, | ||
#[serde(rename = "feebase")] | ||
pub fee_base_msat: Amount, | ||
#[serde(rename = "feeprop")] | ||
pub fee_proportional_millionths: u32, | ||
#[serde(rename = "expirydelta")] | ||
pub cltv_expiry_delta: u16, |
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.
Feels like this'd introduce a lot of headaches, such as using one name from these renames in another struct? I'd like to keep the names as in line with the docs as possible. And in this case the need to rename appears to come from a desire to reuse the Routehop
struct in another location, where these different names are used?
Let's rather create a separate one, and keep them separate, and then start a discussion about renaming them in the schemas themselves, so that all interfacaes remain in sync.
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.
Ok i can make them separate, but what about the inconsistency in keysend schema and the original RouteHop primitive?:
keysend schema:
"id",
"scid",
"feebase",
"feeprop",
"expirydelta"
but then in primitives.proto we have:
id = 1;
short_channel_id = 2;
feebase = 3;
feeprop = 4;
expirydelta = 5;
for comparison decode/decodepay schema:
"pubkey",
"short_channel_id",
"fee_base_msat",
"fee_proportional_millionths",
"cltv_expiry_delta"
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.
Oh boy, did I go against my own comment again there? I'm sorry, I should not have done that, and we probably should be undoing that pointless name change too 🤦 You can probably guess that this was a learning experience for me too ^^
e97721c
to
6c50206
Compare
done, found some more inconsistencies in pb.rs |
f23032d
to
8a57feb
Compare
8a57feb
to
f937061
Compare
rebased |
Changelog-None
f937061
to
5b7836c
Compare
The goal was to have the routes be a reusable object. So i had to edit the primitives files, do some overrides and changes to msggen.
Fixes: #7186