Use Infallible for the unconstructable default custom message type #1094
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TheBlueMatt
force-pushed
the
2021-09-type-explicit-bounds
branch
from
bc4d8b9 to
0f031a1
Compare
Codecov Report
@@ Coverage Diff @@
## main #1094 +/- ##
==========================================
- Coverage 90.73% 90.71% -0.02%
==========================================
Files 65 65
Lines 34315 34318 +3
==========================================
- Hits 31136 31132 -4
- Misses 3179 3186 +7
Continue to review full report at Codecov.
|
When we landed custom messages, we used the empty tuple for the custom message type for `IgnoringMessageHandler`. This was fine, except that we also implemented `Writeable` to panic when writing a `()`. Later, we added support for anchor output construction in CommitmentTransaction, signified by setting a field to `Some(())`, which is serialized as-is. This causes us to panic when writing a `CommitmentTransaction` with `opt_anchors` set. Note that we never set it inside of LDK, but downstream users may. Instead, we implement `Writeable` to write nothing for `()` and use `core::convert::Infallible` for the default custom message type as it is, appropriately, unconstructable. This also makes it easier to implement various things in bindings, as we can always assume `Infallible`-conversion logic is unreachable.
TheBlueMatt
force-pushed
the
2021-09-type-explicit-bounds
branch
from
0f031a1 to
e82318d
Compare
jkczyz
approved these changes
Sep 22, 2021
TheBlueMatt
force-pushed
the
2021-09-type-explicit-bounds
branch
from
ce8237a to
401d035
Compare
|
Oops, realized I should push one more commit to clean up generic bounds. |
jkczyz
approved these changes
Sep 23, 2021
Associated types in C bindings is somewhat of a misnomer - we concretize each trait to a single struct. Thus, different trait implementations must still have the same type, which defeats the point of associated types. In this particular case, however, we can reasonably special-case the `Infallible` type, as an instance of it existing implies something has gone horribly wrong. In order to help our bindings code figure out how to do so when referencing a parent trait's associated type, we specify the explicit type in the implementation method signature.
|
Heh, sorry, pushed one last commit and now the bindings are fully auto-generating - this one is a bit of a hack to help the bindings generator, but its a trivial change and we really need to ship 0.0.101 ASAP, so I figure just land it. |
jkczyz
approved these changes
Sep 23, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When we landed custom messages, we used the empty tuple for the
custom message type for
IgnoringMessageHandler. This was fine,except that we also implemented
Writeableto panic when writinga
(). Later, we added support for anchor output construction inCommitmentTransaction, signified by setting a field to
Some(()),which is serialized as-is.
This causes us to panic when writing a
CommitmentTransactionwith
opt_anchorsset. Note that we never set it inside of LDK,but downstream users may.
Instead, we implement
Writeableto write nothing for()and usecore::convert::Infalliblefor the default custom message type asit is, appropriately, unconstructable.
This also makes it easier to implement various things in bindings,
as we can always assume
Infallible-conversion logic isunreachable.