cln-plugin: Allow user to set featurebits #6881
Conversation
|
This commit is good to go. The CI failed because of a flake? |
|
Yes, looking into the failures they all look like flakes. I'll restart them, and during the next days I will add another flake-disable PR, based on the flakiness tracking. |
plugins/src/lib.rs
Outdated
| @@ -345,6 +358,7 @@ where | |||
| hooks: self.hooks.keys().map(|s| s.clone()).collect(), | |||
| rpcmethods, | |||
| notifications: self.notifications.clone(), | |||
| featurebits: self.featurebits.clone().to_message(), | |||
There was a problem hiding this comment.
Is this clone really needed? Can't call the to_message() method that returns a serializable struct (same struct as the "builder" but serde annotated) without cloning the builder struct?
I guess the "builder" is just for separation of concerns, as it contains the exact same fields as the message type?
There was a problem hiding this comment.
I see your point here.
The FeatureBitBuilder is indeed for separation of concerns and was a bit overkill. I made it back into one type.
The problem here is not in the conversion of to_message(). The core problem is that the Builder must outlive the GetManifestResponse.
We could add a lifetime to GetManifestResponse and borrow the FeatureBits instead.
However, I don't think it's worth the engineering-time to save a few allocations at plugin-start.
ErikDeSmedt
force-pushed
the
cln-plugin-set-featurebits
branch
3 times, most recently
from
6f40a8d
to
b222a74
Compare
There was a problem hiding this comment.
@ErikDeSmedt I left one small comment regarding naming convention, but that's not important and we're splitting hairs at this point.
If you're happy ith the current state I'll merge it, or if you'd like to do a quick fixup I'll wait a couple of days 👍
ACK b222a74
|
@cdecker : I agree that I've renamed it in a separate commit |
ErikDeSmedt
force-pushed
the
cln-plugin-set-featurebits
branch
from
32294fb
to
4390311
Compare
cdecker
force-pushed
the
cln-plugin-set-featurebits
branch
from
4390311
to
1719122
Compare
cdecker
force-pushed
the
cln-plugin-set-featurebits
branch
from
1719122
to
9b4e68d
Compare
No description provided.