-
Notifications
You must be signed in to change notification settings - Fork 893
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
rs: Backwards and Forwards compatibility story for cln-rpc and cln-grpc #5988
Comments
Let's see if I can visualize the proposal a bit. AdditionThis is the transition for a required property in the schema. In gantt
title Adding a new property to a schema
dateFormat YYYY-MM-DD
Pre-addition :a1, 2000-01-01, 10d
Addition :a2, after a1 , 20d
Post-Addition :a3, after a2, 10d
section Optionality
Option<> :b1, after a1, 20d
Required :after b1, 10d
RemovalThe following shows how we remove a required property from a schema. Pre-removal the property is required, as soon as we mark it as deprecated we mark it as optional, and once it gets removed from the schema we no longer parse it. gantt
title Removing a property from the schema
dateFormat YYYY-MM-DD
Pre-removal :a1, 2000-01-01, 10d
Removal :a2, after a1, 20d
Post-removal :a3, after a2, 10d
section Optionality
Required :b1, 2000-01-01, 10d
Optional<> :b2, after b1, 20d
Don't bother with the dates, I chose a gantt chart just to be able to show the swimlanes, and how parts match up, the real intervals are likely 2 releases / 6 months in reality. |
You should be able to get most of this from the "added" and "deprecated" options (which now all take a version number)? |
Correct, that's the plan. I just want to make sure that a) this idea is sound and minimizes changes to users of the library while the schema evolves, and b) we maintain as much compatibility as possible across library versions, wwithout sacrificing the type-based guarantees. Specifically for languages such as Rust a expecting a field and it isn't there (can't be represented as I haven't found an issue with this so far, except maybe that adding or removing a field now causes 2 changes: add as optional followed by add mandatory, and the inverse for removals. That means library users will have to sprinkle case statements throughout their code if they use these fields, and have to adapt their code, but they'd likely have to do that anyway, and I prefer causing trouble at compile-time rather than at runtime (which would be the case if we just didn't care and let the parsers crash). |
cln-rpc
bindings are generated directly from the JSON schemas indoc/schemas
and should therefore always be up-to-date with the version currently being developed (and match the release if we cut acln-rpc
release in sync with the release).This means however that there is a tight coupling between the CLN version and any client/app/plugin built using
cln-rpc
and to a lesser extentcln-grpc
. For example when the schema adds a newrequired
field, the correspondingcln-rpc
code will expect that field to be present and populated, since there is no defaultnull
value that could be used (Option<>
should be used instead. The same goes for a required field getting dropped in a later version. In either of these cases, havingcln-rpc
expect a value that is then not present causes parsing of the entire JSON document to fail, and not being able to recover.For this purpose I'd like to introduce
inferred optional
fields, i.e., fields that are mandatory in the schema, but due to them having been recently introduced or removed they are to be treated as optional, so we remain compatible with either the old version pre-removal or newer version post-addition. This can leverage theadded
anddeprecated
annotations we recently started adding and enforcing on schema changes.The idea is to explicitly have
cln-rpc
and others support a range of releases (in line with the deprecation cycle). If a field is required, but has been added or deprecated in the range of supported versions, we infer that the field may not be present before the release that introduced it, or may have been removed after the release that enacted the deprecation, hence we need to make it an inferred optional. Incln-rpc
that'd be translated into anOption<>
, fixing the parsing. Sadly that means that updatingcln-rpc
in the client/app/plugin can cause fields to flip-flop between required and optional, but that's a compile-time concern, rather than a runtime concern as is the case for the current crates.This issue is mainly intended to brainstorm this idea, and if we don't find a hole in the logic, I'll start implementing it in
msggen
.The text was updated successfully, but these errors were encountered: