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.

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

cln-plugin: Add dynamic configs and a callback for changes #7293

Merged

Conversation

daywalker90
Copy link
Contributor

I want this in cln-plugin: #7289

So this is my take for cln-plugin on it. The formatting changes are because @cdecker is not using rustfmt like a good boy and i noticed it too late, sorry. Some things get a bit weird here and there but overall it works!

I wanted existing projects to not have to do any changes if they were using static options (so all of them up to now), that's why i added the dynamic switch as a function, instead of an extra argument to the new_* functions.

Let me know what you think.

@cdecker
Copy link
Member

cdecker commented May 8, 2024

That's me alright, bad boy for life 🤣
Will review ASAP

Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

Very nice change, just a minor suggestion to make dynamic() return a copy so we can just chain the invocations as if it were a Builder.

ACK 952da34

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

cdecker commented May 8, 2024

ACK ac263e1

@daywalker90
Copy link
Contributor Author

btw CLN printing broken logs if the plugin does not like the new option value and returns an Err() as a Result is a bit much imo:

**BROKEN** jsonrpc#70: Invalid setconfig summars-sort-by response from plugin: {"error":"Not a valid column name: `1`. Must be one of: GRAPH_SATS, OUT_SATS, IN_SATS, SCID, MAX_HTLC, FLAG, PRIVATE, OFFLINE, BASE, PPM, ALIAS, PEER_ID, UPTIME, HTLCS, STATE","id":"-c:setconfig#18/cln:setconfig#51","jsonrpc":"2.0"}

should be UNUSUAL, no?

@daywalker90
Copy link
Contributor Author

and on the console you only get a generic error instead of the one from the plugin:

{
   "code": -1,
   "message": "Malformed setvalue summars-sort-by plugin return"
}

You have to then look at the broken log entry to get the error reported by the plugin. This could be improved!

@daywalker90
Copy link
Contributor Author

So after the meeting and a private coding session with rusty it turned out that it works as it should with the python framework. So this is a skill issue on my part or an actual issue with the rust framework.
Closest i came to a well formed error:

pub async fn setconfig_callback(
    plugin: Plugin<PluginState>,
    args: serde_json::Value,
) -> Result<serde_json::Value, Error> {
      return Err(anyhow!(json!(RpcError {
          code: Some(-1),
          message: "bad thing".to_string(),
          data: None
      })));
**BROKEN** jsonrpc#68: Invalid setconfig clnrod-blockmode response from plugin: {"error":"{\"code\":-1,\"data\":null,\"message\":\"bad thing\"}","id":"pytest:setconfig#4/cln:setconfig#45","jsonrpc":"2.0"}

Changelog-Added: cln-plugin: Add dynamic configs and a callback for changes
@daywalker90
Copy link
Contributor Author

Ok , so cln-plugin was escaping RpcError's too much. Looking at the io log of an error coming from a plugin:

{"error":"{\"code\":-1,\"data\":null,\"message\":\"could not parse BlockMode from test\"}","id":"pytest:setconfig#4/cln:setconfig#45","jsonrpc":"2.0"}

I've added a commit where error messages get the proper (non-) escaping string. With that we get:

{"error":{"code":-1,"data":null,"message":"could not parse BlockMode from test"},"id":"pytest:setconfig#4/cln:setconfig#45","jsonrpc":"2.0"}

@cdecker cdecker merged commit abd1b5f into ElementsProject:master May 15, 2024
35 checks passed
@daywalker90 daywalker90 deleted the cln-plugin-dynamic-options branch May 15, 2024 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants