Skip to content
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.

Sign up for GitHub

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

Jump to bottom

cln-plugin: Save "configuration" from "init" method #5279

Merged
merged 4 commits into from
Jun 22, 2022
Merged

cln-plugin: Save "configuration" from "init" method #5279

merged 4 commits into from
Jun 22, 2022

Conversation

justinmoon
Copy link
Contributor

@justinmoon justinmoon commented May 20, 2022 •
edited
Loading

cln-plugin throws away the "configuration" field from the "init" message.

@justinmoon
Copy link
Contributor Author

justinmoon commented May 20, 2022 •
edited
Loading

Added a unittest as separate commit to make sure the "init" message (used this example) can be parsed. Not sure if this is helpful or not.

@justinmoon justinmoon marked this pull request as ready for review May 20, 2022 23:48
@justinmoon justinmoon requested a review from cdecker as a code owner May 20, 2022 23:48
@justinmoon justinmoon mentioned this pull request May 20, 2022
Closed
justinmoon
justinmoon commented May 20, 2022
View reviewed changes
plugins/src/lib.rs Outdated
pub struct Configuration {
#[serde(rename = "lightning-dir")]
pub lightning_dir: String,
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we keep this struct, what else should go in it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the docs https://lightning.readthedocs.io/PLUGINS.html#the-init-method and if you want to do a copy and paste https://github.com/laanwj/rust-clightning-rpc/blob/92912c8a5318624b2c9684676a0f697b4a38432e/plugin/src/commands/types.rs#L25-L51

vincenzopalazzo
vincenzopalazzo requested changes May 21, 2022
View reviewed changes
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall LGTM, just take a decision if we want use an HasMap or a Struct for the configuration

plugins/src/lib.rs Show resolved Hide resolved
@cdecker
Copy link
Member

cdecker commented May 21, 2022

overall LGTM, just take a decision if we want use an HasMap or a Struct for the configuration

As you mentioned above making these things we have control over as strongly typed as possible would be a good idea. So I'd vote in favor of the Configuration struct 👍

@justinmoon justinmoon force-pushed the cln_plugin_config branch 4 times, most recently from a3f8c1c to 01549e5 Compare May 21, 2022 15:27
@justinmoon
Copy link
Contributor Author

Added the rest of the fields to Configuration mostly copying from rust-clightning-rpc as suggested by @vincenzopalazzo. I prefer the strongly typed struct too.

@vincenzopalazzo
Copy link
Collaborator

It is missing the Changelog in the body commit, maybe Changelog-Added: cln_plugin: take into consideration the cln configuration from init msg can be a good description?

@justinmoon
Copy link
Contributor Author

Added changelog entry @vincenzopalazzo

@rustyrussell
Copy link
Contributor

BTW I've invited you to project as a reader. This should mean CI runs for you...

vincenzopalazzo
vincenzopalazzo requested changes May 31, 2022
View reviewed changes
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a small comment, and LGTM.

With the next commit, we will see with the CI will be happy!

plugins/src/messages.rs Outdated Show resolved Hide resolved
@justinmoon
cln-plugin: Configuration struct
eb9e7f5 
Represents the "configuration" part of the "init" message during
plugin initialization.

Changelog-Added: cln_plugin: persist cln configuration from init msg
@justinmoon
Copy link
Contributor Author

justinmoon commented Jun 21, 2022 •
edited
Loading

The tests seem to be failing because this match is failing with my Configuration struct. But AFAIKT my unittest does exactly the same match and it works.

In my unittest I'm using exactly this JSON and it passes. Why is it failing in the python test in CI?

It does seem that the example JSON in the link above is different from what the CI test is sending. For example, the always_use_proxy value, for example, is missing in CI.

@cdecker
cln-plugin: Make the proxy-related configuration Option<>
9e912f8 
These are only populated if a proxy was specified, see
lightningd/plugin.c:1855, so we were getting upset when we expected
them and they weren't set.
@cdecker
Copy link
Member

cdecker commented Jun 22, 2022

ACK 9e912f8

@cdecker cdecker merged commit e6442d7 into ElementsProject:master Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vincenzopalazzo vincenzopalazzo vincenzopalazzo requested changes

@cdecker cdecker Awaiting requested review from cdecker cdecker is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@justinmoon @cdecker @vincenzopalazzo @rustyrussell