From 2e0e815a764d7d0508eed4d1477dfbef5443f036 Mon Sep 17 00:00:00 2001 From: Erik De Smedt Date: Thu, 1 Feb 2024 10:42:28 +0100 Subject: [PATCH 1/6] cln_rpc: Store ConfigOptions in HashMap We used to store all `ConfigOptions` in a `Vec`. I decided to use a `HashMap` isntead because it will be easier to implement dynamic options later on. --- plugins/src/lib.rs | 38 +++++++++++++++----------------------- 1 file changed, 15 insertions(+), 23 deletions(-) diff --git a/plugins/src/lib.rs b/plugins/src/lib.rs index 67c6567f8d04..ef8ecaed14e3 100644 --- a/plugins/src/lib.rs +++ b/plugins/src/lib.rs @@ -5,7 +5,7 @@ use futures::sink::SinkExt; use tokio::io::{AsyncReadExt, AsyncWriteExt}; extern crate log; use log::trace; -use messages::{Configuration, NotificationTopic, FeatureBits}; +use messages::{Configuration, FeatureBits, NotificationTopic}; use options::ConfigOption; use std::collections::HashMap; use std::future::Future; @@ -43,11 +43,11 @@ where output: Option, hooks: HashMap>, - options: Vec, + options: HashMap, rpcmethods: HashMap>, subscriptions: HashMap>, notifications: Vec, - custommessages : Vec, + custommessages: Vec, featurebits: FeatureBits, dynamic: bool, // Do we want the plugin framework to automatically register a logging handler? @@ -65,7 +65,7 @@ where init_id: serde_json::Value, input: FramedRead, output: Arc>>, - options: Vec, + options: HashMap, configuration: Configuration, rpcmethods: HashMap>, hooks: HashMap>, @@ -99,7 +99,7 @@ where /// The state gets cloned for each request state: S, /// "options" field of "init" message sent by cln - options: Vec, + options: HashMap, /// "configuration" field of "init" message sent by cln configuration: Configuration, /// A signal that allows us to wait on the plugin's shutdown. @@ -120,18 +120,18 @@ where output: Some(output), hooks: HashMap::new(), subscriptions: HashMap::new(), - options: vec![], + options: HashMap::new(), rpcmethods: HashMap::new(), notifications: vec![], featurebits: FeatureBits::default(), dynamic: false, - custommessages : vec![], + custommessages: vec![], logging: true, } } pub fn option(mut self, opt: options::ConfigOption) -> Builder { - self.options.push(opt); + self.options.insert(opt.name().to_string(), opt); self } @@ -245,7 +245,7 @@ where /// Tells lightningd explicitly to allow custommmessages of the provided /// type - pub fn custommessages(mut self, custommessages : Vec) -> Self { + pub fn custommessages(mut self, custommessages: Vec) -> Self { self.custommessages = custommessages; self } @@ -369,7 +369,7 @@ where .collect(); messages::GetManifestResponse { - options: self.options.clone(), + options: self.options.values().cloned().collect(), subscriptions: self.subscriptions.keys().map(|s| s.clone()).collect(), hooks: self.hooks.keys().map(|s| s.clone()).collect(), rpcmethods, @@ -377,7 +377,7 @@ where featurebits: self.featurebits.clone(), dynamic: self.dynamic, nonnumericids: true, - custommessages : self.custommessages.clone() + custommessages: self.custommessages.clone(), } } @@ -387,7 +387,7 @@ where // Match up the ConfigOptions and fill in their values if we // have a matching entry. - for opt in self.options.iter_mut() { + for (_name, opt) in self.options.iter_mut() { let val = call.options.get(opt.name()); opt.value = match (&opt, &opt.default(), &val) { (_, OValue::String(_), Some(JValue::String(s))) => Some(OValue::String(s.clone())), @@ -506,11 +506,7 @@ where S: Clone + Send, { pub fn option(&self, name: &str) -> Option { - self.options - .iter() - .filter(|o| o.name() == name) - .next() - .map(|co| co.value.clone().unwrap_or(co.default().clone())) + self.options.get(name).and_then(|x| x.value.clone()) } } @@ -595,11 +591,7 @@ where } pub fn option(&self, name: &str) -> Option { - self.options - .iter() - .filter(|o| o.name() == name) - .next() - .map(|co| co.value.clone().unwrap_or(co.default().clone())) + self.options.get(name).and_then(|x| x.value.clone()) } /// return the cln configuration send to the @@ -740,7 +732,7 @@ where S: Clone + Send, { pub fn options(&self) -> Vec { - self.options.clone() + self.options.values().cloned().collect() } pub fn configuration(&self) -> Configuration { self.configuration.clone() From 4793cd6398dc4fb7c05cfdc8e83e3f0d3924e5fd Mon Sep 17 00:00:00 2001 From: Erik De Smedt Date: Thu, 1 Feb 2024 15:36:45 +0100 Subject: [PATCH 2/6] cln_rpc: Refactor `ConfigOption` and `Value` Breaking changes here. This improves the semantics of `ConfigOption` and `options::Value` drastically. We've been using `options::Value` for 2 purposes 1. To specify the type and default value of an option 2. Coummunicate how a user configured an option We fall here in the pit-fall of being poor at both purposes. I've edited the code to ensure - `options::Value` -> To read or specify the value of an option - `option::ValueType` -> To specify the type of an option **Configure an option** Let's create an string-typed option create an option named `"required-string-opt"` the developer has to use the following code. ```rust let option = ConfigOption::new("required-string", Value::OptString, "description"); ``` The semantics of `OptString` might falsely suggest that it is optional to specify the config. However, we use `OptString` to say we are not providing a default-value. After this commit we can use instead ```rust let option = ConfigOption::new_str_no_default("required-string", "description"); ``` For reading a configured value the `option::Value` is somewhat cumbersome. The old version of had 6 different types of value ```rust let value = plugin.option("required-string"); match value { String(s) => {}, // User has configured string value or default Integer(i) => {}, // User has configured int value or default Boolean(b) => {}, // User has configured bool value or default OptString => {}, // User has not configured value and no default OptInteger = {}, // User has not configured value and no default OptBOolean => {}, // User has not configured value and no default } ``` This has been changed to ```rust let value = plugin.option("required-string"); match value { Some(String(s)) => {}, Some(Integer(i)) => {}, Some(Boolean(b)) => {}, None => {}, } ``` --- plugins/examples/cln-plugin-startup.rs | 19 +- plugins/grpc-plugin/src/main.rs | 6 +- plugins/src/lib.rs | 37 ++-- plugins/src/messages.rs | 6 +- plugins/src/options.rs | 239 +++++++++++++++++++------ 5 files changed, 216 insertions(+), 91 deletions(-) diff --git a/plugins/examples/cln-plugin-startup.rs b/plugins/examples/cln-plugin-startup.rs index 13f93589194e..48144ceee8e0 100644 --- a/plugins/examples/cln-plugin-startup.rs +++ b/plugins/examples/cln-plugin-startup.rs @@ -12,16 +12,15 @@ async fn main() -> Result<(), anyhow::Error> { let state = (); if let Some(plugin) = Builder::new(tokio::io::stdin(), tokio::io::stdout()) - .option(options::ConfigOption::new( - "test-option", - options::Value::Integer(42), - "a test-option with default 42", - )) - .option(options::ConfigOption::new( - "opt-option", - options::Value::OptInteger, - "An optional option", - )) + .option( + options::ConfigOption::new_i64_with_default( + "test-option", + 42, + "a test-option with default 42", + ) + .build(), + ) + .option(options::ConfigOption::new_opt_i64("opt-option", "An optional option").build()) .rpcmethod("testmethod", "This is a test", testmethod) .rpcmethod( "testoptions", diff --git a/plugins/grpc-plugin/src/main.rs b/plugins/grpc-plugin/src/main.rs index 782752c22166..a60b7000b602 100644 --- a/plugins/grpc-plugin/src/main.rs +++ b/plugins/grpc-plugin/src/main.rs @@ -22,11 +22,11 @@ async fn main() -> Result<()> { let directory = std::env::current_dir()?; let plugin = match Builder::new(tokio::io::stdin(), tokio::io::stdout()) - .option(options::ConfigOption::new( + .option(options::ConfigOption::new_i64_with_default( "grpc-port", - options::Value::Integer(-1), + -1, "Which port should the grpc plugin listen for incoming connections?", - )) + ).build()) .configure() .await? { diff --git a/plugins/src/lib.rs b/plugins/src/lib.rs index ef8ecaed14e3..c57860e721e6 100644 --- a/plugins/src/lib.rs +++ b/plugins/src/lib.rs @@ -6,7 +6,7 @@ use tokio::io::{AsyncReadExt, AsyncWriteExt}; extern crate log; use log::trace; use messages::{Configuration, FeatureBits, NotificationTopic}; -use options::ConfigOption; +use options::UntypedConfigOption; use std::collections::HashMap; use std::future::Future; use std::pin::Pin; @@ -43,7 +43,7 @@ where output: Option, hooks: HashMap>, - options: HashMap, + options: HashMap, rpcmethods: HashMap>, subscriptions: HashMap>, notifications: Vec, @@ -65,7 +65,7 @@ where init_id: serde_json::Value, input: FramedRead, output: Arc>>, - options: HashMap, + options: HashMap, configuration: Configuration, rpcmethods: HashMap>, hooks: HashMap>, @@ -99,7 +99,7 @@ where /// The state gets cloned for each request state: S, /// "options" field of "init" message sent by cln - options: HashMap, + options: HashMap, /// "configuration" field of "init" message sent by cln configuration: Configuration, /// A signal that allows us to wait on the plugin's shutdown. @@ -130,7 +130,7 @@ where } } - pub fn option(mut self, opt: options::ConfigOption) -> Builder { + pub fn option(mut self, opt: options::UntypedConfigOption) -> Builder { self.options.insert(opt.name().to_string(), opt); self } @@ -389,23 +389,20 @@ where // have a matching entry. for (_name, opt) in self.options.iter_mut() { let val = call.options.get(opt.name()); - opt.value = match (&opt, &opt.default(), &val) { - (_, OValue::String(_), Some(JValue::String(s))) => Some(OValue::String(s.clone())), - (_, OValue::OptString, Some(JValue::String(s))) => Some(OValue::String(s.clone())), - (_, OValue::OptString, None) => None, - (_, OValue::Integer(_), Some(JValue::Number(s))) => { - Some(OValue::Integer(s.as_i64().unwrap())) + opt.value = match (&opt.value, &opt.default(), &val) { + (_, Some(OValue::String(_)), Some(JValue::String(s))) => { + Some(OValue::String(s.clone())) } - (_, OValue::OptInteger, Some(JValue::Number(s))) => { + (_, None, Some(JValue::String(s))) => Some(OValue::String(s.clone())), + (_, None, None) => None, + + (_, Some(OValue::Integer(_)), Some(JValue::Number(s))) => { Some(OValue::Integer(s.as_i64().unwrap())) } - (_, OValue::OptInteger, None) => None, - - (_, OValue::Boolean(_), Some(JValue::Bool(s))) => Some(OValue::Boolean(*s)), - (_, OValue::OptBoolean, Some(JValue::Bool(s))) => Some(OValue::Boolean(*s)), - (_, OValue::OptBoolean, None) => None, - + (_, None, Some(JValue::Number(s))) => Some(OValue::Integer(s.as_i64().unwrap())), + (_, Some(OValue::Boolean(_)), Some(JValue::Bool(s))) => Some(OValue::Boolean(*s)), + (_, None, Some(JValue::Bool(s))) => Some(OValue::Boolean(*s)), (o, _, _) => panic!("Type mismatch for option {:?}", o), } } @@ -591,7 +588,7 @@ where } pub fn option(&self, name: &str) -> Option { - self.options.get(name).and_then(|x| x.value.clone()) + self.options.get(name).and_then(|c| c.value.clone()) } /// return the cln configuration send to the @@ -731,7 +728,7 @@ impl Plugin where S: Clone + Send, { - pub fn options(&self) -> Vec { + pub fn options(&self) -> Vec { self.options.values().cloned().collect() } pub fn configuration(&self) -> Configuration { diff --git a/plugins/src/messages.rs b/plugins/src/messages.rs index 8a1bbae956e8..75402bb21959 100644 --- a/plugins/src/messages.rs +++ b/plugins/src/messages.rs @@ -1,4 +1,4 @@ -use crate::options::ConfigOption; +use crate::options::UntypedConfigOption; use serde::de::{self, Deserializer}; use serde::{Deserialize, Serialize}; use serde_json::Value; @@ -171,7 +171,7 @@ impl NotificationTopic { #[derive(Serialize, Default, Debug)] pub(crate) struct GetManifestResponse { - pub(crate) options: Vec, + pub(crate) options: Vec, pub(crate) rpcmethods: Vec, pub(crate) subscriptions: Vec, pub(crate) notifications: Vec, @@ -180,7 +180,7 @@ pub(crate) struct GetManifestResponse { pub(crate) featurebits: FeatureBits, pub(crate) nonnumericids: bool, #[serde(skip_serializing_if = "Vec::is_empty")] - pub(crate) custommessages : Vec + pub(crate) custommessages: Vec, } #[derive(Serialize, Default, Debug, Clone)] diff --git a/plugins/src/options.rs b/plugins/src/options.rs index 026fea00fa33..35f22a71bb87 100644 --- a/plugins/src/options.rs +++ b/plugins/src/options.rs @@ -1,14 +1,34 @@ +use anyhow::Result; use serde::ser::{SerializeStruct, Serializer}; use serde::Serialize; +// Marker trait for possible values of options +pub trait OptionType {} + +impl OptionType for String {} +impl OptionType for i64 {} +impl OptionType for bool {} +impl OptionType for Option {} +impl OptionType for Option {} +impl OptionType for Option {} + +#[derive(Clone, Debug, Serialize)] +pub enum ValueType { + #[serde(rename = "string")] + String, + #[serde(rename = "int")] + Integer, + #[serde(rename = "bool")] + Boolean, + #[serde(rename = "flag")] + Flag, +} + #[derive(Clone, Debug)] pub enum Value { String(String), Integer(i64), Boolean(bool), - OptString, - OptInteger, - OptBoolean, } impl Value { @@ -24,8 +44,9 @@ impl Value { /// otherwise. pub fn as_str(&self) -> Option<&str> { match self { - Value::String(s) => Some(s), - _ => None, + Value::String(s) => Some(&s), + Value::Integer(_) => None, + Value::Boolean(_) => None, } } @@ -63,34 +84,142 @@ impl Value { _ => None, } } +} - /// Return `true` if the option is not None and `false` otherwise. - pub fn is_some(&self) -> bool { - match self { - Value::String(_) => false, - Value::Integer(_) => false, - Value::Boolean(_) => false, - Value::OptString => true, - Value::OptInteger => true, - Value::OptBoolean => true, +#[derive(Clone, Debug)] +pub struct ConfigOption { + name: String, + default: Option, + value_type: ValueType, + description: String, +} + +impl ConfigOption { + pub fn build(&self) -> UntypedConfigOption { + UntypedConfigOption { + name: self.name.clone(), + value_type: self.value_type.clone(), + default: self.default.as_ref().map(|s| Value::String(s.clone())), + description: self.description.clone(), + value: None, + } + } + + pub fn new_str_with_default, S2: AsRef, S3: AsRef>( + name: S1, + default: S2, + description: S3, + ) -> Self { + Self { + name: name.as_ref().to_string(), + default: Some(default.as_ref().to_string()), + value_type: ValueType::String, + description: description.as_ref().to_string(), + } + } +} + +impl ConfigOption { + pub fn build(&self) -> UntypedConfigOption { + UntypedConfigOption { + name: self.name.clone(), + value_type: self.value_type.clone(), + default: self.default.map(|i| Value::Integer(i)), + description: self.description.clone(), + value: None, + } + } + + pub fn new_i64_with_default, C: AsRef>( + name: A, + default: i64, + description: C, + ) -> Self { + Self { + name: name.as_ref().to_string(), + default: Some(default), + value_type: ValueType::Integer, + description: description.as_ref().to_string(), + } + } +} + +impl ConfigOption> { + pub fn new_opt_i64, S2: AsRef>(name: S1, description: S2) -> Self { + Self { + name: name.as_ref().to_string(), + default: None, + value_type: ValueType::Integer, + description: description.as_ref().to_string(), + } + } + + pub fn build(&self) -> UntypedConfigOption { + UntypedConfigOption { + name: self.name.clone(), + value_type: self.value_type.clone(), + default: None, + description: self.description.clone(), + value: None, + } + } +} + +impl ConfigOption { + pub fn build(&self) -> UntypedConfigOption { + let default = match self.value_type { + ValueType::Flag => Some(Value::Boolean(false)), + ValueType::Boolean => self.default.map(|b| Value::Boolean(b)), + _ => panic!("Failed to build type"), + }; + + UntypedConfigOption { + name: self.name.clone(), + value_type: self.value_type.clone(), + default, + description: self.description.clone(), + value: None, + } + } + + pub fn new_bool_with_default, S2: AsRef>( + name: S1, + default: bool, + description: S2, + ) -> Self { + Self { + name: name.as_ref().to_string(), + description: description.as_ref().to_string(), + default: Some(default), + value_type: ValueType::Boolean, + } + } + + pub fn new_flag, S2: AsRef>(name: S1, description: S2) -> Self { + Self { + name: name.as_ref().to_string(), + description: description.as_ref().to_string(), + default: Some(false), + value_type: ValueType::Flag, } } } /// An stringly typed option that is passed to #[derive(Clone, Debug)] -pub struct ConfigOption { +pub struct UntypedConfigOption { name: String, + pub(crate) value_type: ValueType, pub(crate) value: Option, - default: Value, + default: Option, description: String, } -impl ConfigOption { +impl UntypedConfigOption { pub fn name(&self) -> &str { &self.name } - pub fn default(&self) -> &Value { + pub fn default(&self) -> &Option { &self.default } } @@ -98,7 +227,7 @@ impl ConfigOption { // When we serialize we don't add the value. This is because we only // ever serialize when we pass the option back to lightningd during // the getmanifest call. -impl Serialize for ConfigOption { +impl Serialize for UntypedConfigOption { fn serialize(&self, serializer: S) -> Result where S: Serializer, @@ -106,52 +235,37 @@ impl Serialize for ConfigOption { let mut s = serializer.serialize_struct("ConfigOption", 4)?; s.serialize_field("name", &self.name)?; match &self.default { - Value::String(ss) => { - s.serialize_field("type", "string")?; + Some(Value::String(ss)) => { s.serialize_field("default", ss)?; } - Value::Integer(i) => { - s.serialize_field("type", "int")?; + Some(Value::Integer(i)) => { s.serialize_field("default", i)?; } - Value::Boolean(b) => { - s.serialize_field("type", "bool")?; - s.serialize_field("default", b)?; - } - Value::OptString => { - s.serialize_field("type", "string")?; - } - Value::OptInteger => { - s.serialize_field("type", "int")?; - } - Value::OptBoolean => { - s.serialize_field("type", "bool")?; + Some(Value::Boolean(b)) => { + match self.value_type { + ValueType::Boolean => s.serialize_field("default", b)?, + ValueType::Flag => {} + _ => {} // This should never happen + } } + _ => {} } - + s.serialize_field("type", &self.value_type)?; s.serialize_field("description", &self.description)?; s.end() } } -impl ConfigOption { - pub fn new(name: &str, default: Value, description: &str) -> Self { - Self { - name: name.to_string(), - default, - description: description.to_string(), - value: None, - } - } - pub fn value(&self) -> Value { - match &self.value { - None => self.default.clone(), - Some(v) => v.clone(), - } +impl ConfigOption +where + V: OptionType, +{ + pub fn name(&self) -> &str { + &self.name } - pub fn description(&self) -> String { - self.description.clone() + pub fn description(&self) -> &str { + &self.description } } @@ -163,7 +277,7 @@ mod test { fn test_option_serialize() { let tests = vec![ ( - ConfigOption::new("name", Value::String("default".to_string()), "description"), + ConfigOption::new_str_with_default("name", "default", "description").build(), json!({ "name": "name", "description":"description", @@ -172,7 +286,7 @@ mod test { }), ), ( - ConfigOption::new("name", Value::Integer(42), "description"), + ConfigOption::new_i64_with_default("name", 42, "description").build(), json!({ "name": "name", "description":"description", @@ -181,7 +295,7 @@ mod test { }), ), ( - ConfigOption::new("name", Value::Boolean(true), "description"), + ConfigOption::new_bool_with_default("name", true, "description").build(), json!({ "name": "name", "description":"description", @@ -189,6 +303,14 @@ mod test { "type": "bool", }), ), + ( + ConfigOption::new_flag("name", "description").build(), + json!({ + "name" : "name", + "description": "description", + "type" : "flag" + }), + ), ]; for (input, expected) in tests.iter() { @@ -196,4 +318,11 @@ mod test { assert_eq!(&res, expected); } } + + #[test] + fn test_type_serialize() { + assert_eq!(json!(ValueType::Integer), json!("int")); + + assert_eq!(json!(ValueType::Flag), json!("flag")); + } } From 100f1bfc2818d4e09075dcf9e6a63c33e4f22556 Mon Sep 17 00:00:00 2001 From: Erik De Smedt Date: Fri, 2 Feb 2024 09:08:50 +0100 Subject: [PATCH 3/6] Allow `ConfigOption` to be specified as const This is the first part of two commits that attempts to simplify the API that is used to retrieve configuration values. Previously, we encouraged the following pattern to build a plugin. ```rust let configured_plugin = Builder::new( tokio::io::stdin(), tokio::io::stdout()) .option(ConfigOption::new_i64_with_default("some-option", 0, "Description")) .configure(); let value = configured_plugion.option("some-option"); match value { Some(Integer(i)) => {}, // Config provided None => {}, // No config set _ => {}, // This should never happened } ``` This commit helps to move to the following pattern ```rust const SOME_OPTION : ConfigOption = ConfigOption::new_i64_with_default( "some-option", "description"); async fn main() -> Result<()> { let plugin = Builder::new(tokio::io::stdin(), tokio::io::stdoout()) .option(SOME_OPTION) .configure() .await?; let value : i64 = plugin.option(SOME_OPTION)?; } ``` --- plugins/examples/cln-plugin-startup.rs | 18 +-- plugins/grpc-plugin/src/main.rs | 2 +- plugins/src/lib.rs | 7 +- plugins/src/options.rs | 187 +++++++++++++++---------- 4 files changed, 131 insertions(+), 83 deletions(-) diff --git a/plugins/examples/cln-plugin-startup.rs b/plugins/examples/cln-plugin-startup.rs index 48144ceee8e0..c876ba2e457d 100644 --- a/plugins/examples/cln-plugin-startup.rs +++ b/plugins/examples/cln-plugin-startup.rs @@ -12,15 +12,15 @@ async fn main() -> Result<(), anyhow::Error> { let state = (); if let Some(plugin) = Builder::new(tokio::io::stdin(), tokio::io::stdout()) - .option( - options::ConfigOption::new_i64_with_default( - "test-option", - 42, - "a test-option with default 42", - ) - .build(), - ) - .option(options::ConfigOption::new_opt_i64("opt-option", "An optional option").build()) + .option(options::ConfigOption::new_i64_with_default( + "test-option", + 42, + "a test-option with default 42", + )) + .option(options::ConfigOption::new_i64_no_default( + "opt-option", + "An optional option", + )) .rpcmethod("testmethod", "This is a test", testmethod) .rpcmethod( "testoptions", diff --git a/plugins/grpc-plugin/src/main.rs b/plugins/grpc-plugin/src/main.rs index a60b7000b602..aa2a829b04cc 100644 --- a/plugins/grpc-plugin/src/main.rs +++ b/plugins/grpc-plugin/src/main.rs @@ -26,7 +26,7 @@ async fn main() -> Result<()> { "grpc-port", -1, "Which port should the grpc plugin listen for incoming connections?", - ).build()) + )) .configure() .await? { diff --git a/plugins/src/lib.rs b/plugins/src/lib.rs index c57860e721e6..ce2ed1e3c025 100644 --- a/plugins/src/lib.rs +++ b/plugins/src/lib.rs @@ -130,8 +130,11 @@ where } } - pub fn option(mut self, opt: options::UntypedConfigOption) -> Builder { - self.options.insert(opt.name().to_string(), opt); + pub fn option( + mut self, + opt: options::ConfigOption, + ) -> Builder { + self.options.insert(opt.name().to_string(), opt.build()); self } diff --git a/plugins/src/options.rs b/plugins/src/options.rs index 35f22a71bb87..465aa18f679f 100644 --- a/plugins/src/options.rs +++ b/plugins/src/options.rs @@ -3,14 +3,53 @@ use serde::ser::{SerializeStruct, Serializer}; use serde::Serialize; // Marker trait for possible values of options -pub trait OptionType {} +pub trait OptionType { + fn convert_default(value: Option<&Self>) -> Option; +} + +impl OptionType for &str { + fn convert_default(value: Option<&Self>) -> Option { + value.map(|s| Value::String(s.to_string())) + } +} + +impl OptionType for String { + fn convert_default(value: Option<&Self>) -> Option { + value.map(|s| Value::String(s.clone())) + } +} +impl OptionType for i64 { + fn convert_default(value: Option<&Self>) -> Option { + value.map(|i| Value::Integer(*i)) + } +} +impl OptionType for bool { + fn convert_default(value: Option<&Self>) -> Option { + value.map(|b| Value::Boolean(*b)) + } +} -impl OptionType for String {} -impl OptionType for i64 {} -impl OptionType for bool {} -impl OptionType for Option {} -impl OptionType for Option {} -impl OptionType for Option {} +impl OptionType for Option { + fn convert_default(_value: Option<&Self>) -> Option { + None + } +} + +impl OptionType for Option<&str> { + fn convert_default(_value: Option<&Self>) -> Option { + None + } +} +impl OptionType for Option { + fn convert_default(_value: Option<&Self>) -> Option { + None + } +} +impl OptionType for Option { + fn convert_default(_value: Option<&Self>) -> Option { + None + } +} #[derive(Clone, Debug, Serialize)] pub enum ValueType { @@ -87,118 +126,106 @@ impl Value { } #[derive(Clone, Debug)] -pub struct ConfigOption { - name: String, +pub struct ConfigOption<'a, V: OptionType> { + name: &'a str, default: Option, value_type: ValueType, - description: String, + description: &'a str, } -impl ConfigOption { +impl ConfigOption<'_, V> { pub fn build(&self) -> UntypedConfigOption { UntypedConfigOption { - name: self.name.clone(), + name: self.name.to_string(), value_type: self.value_type.clone(), - default: self.default.as_ref().map(|s| Value::String(s.clone())), - description: self.description.clone(), + default: OptionType::convert_default(self.default.as_ref()), value: None, + description: self.description.to_string(), } } +} - pub fn new_str_with_default, S2: AsRef, S3: AsRef>( - name: S1, - default: S2, - description: S3, +impl ConfigOption<'_, &'static str> { + pub const fn new_str_with_default( + name: &'static str, + default: &'static str, + description: &'static str, ) -> Self { Self { - name: name.as_ref().to_string(), - default: Some(default.as_ref().to_string()), + name: name, + default: Some(default), value_type: ValueType::String, - description: description.as_ref().to_string(), + description: description, } } } -impl ConfigOption { - pub fn build(&self) -> UntypedConfigOption { - UntypedConfigOption { - name: self.name.clone(), - value_type: self.value_type.clone(), - default: self.default.map(|i| Value::Integer(i)), - description: self.description.clone(), - value: None, +impl ConfigOption<'_, Option<&str>> { + pub const fn new_str_no_default(name: &'static str, description: &'static str) -> Self { + Self { + name, + default: None, + value_type: ValueType::String, + description, } } +} - pub fn new_i64_with_default, C: AsRef>( - name: A, +impl ConfigOption<'_, i64> { + pub const fn new_i64_with_default( + name: &'static str, default: i64, - description: C, + description: &'static str, ) -> Self { Self { - name: name.as_ref().to_string(), + name: name, default: Some(default), value_type: ValueType::Integer, - description: description.as_ref().to_string(), + description: description, } } } -impl ConfigOption> { - pub fn new_opt_i64, S2: AsRef>(name: S1, description: S2) -> Self { +impl ConfigOption<'_, Option> { + pub const fn new_i64_no_default(name: &'static str, description: &'static str) -> Self { Self { - name: name.as_ref().to_string(), + name: name, default: None, value_type: ValueType::Integer, - description: description.as_ref().to_string(), + description: description, } } +} - pub fn build(&self) -> UntypedConfigOption { - UntypedConfigOption { - name: self.name.clone(), - value_type: self.value_type.clone(), +impl ConfigOption<'_, Option> { + pub const fn new_bool_no_default(name: &'static str, description: &'static str) -> Self { + Self { + name, + description, default: None, - description: self.description.clone(), - value: None, + value_type: ValueType::Boolean, } } } -impl ConfigOption { - pub fn build(&self) -> UntypedConfigOption { - let default = match self.value_type { - ValueType::Flag => Some(Value::Boolean(false)), - ValueType::Boolean => self.default.map(|b| Value::Boolean(b)), - _ => panic!("Failed to build type"), - }; - - UntypedConfigOption { - name: self.name.clone(), - value_type: self.value_type.clone(), - default, - description: self.description.clone(), - value: None, - } - } - - pub fn new_bool_with_default, S2: AsRef>( - name: S1, +impl ConfigOption<'_, bool> { + pub const fn new_bool_with_default( + name: &'static str, default: bool, - description: S2, + description: &'static str, ) -> Self { Self { - name: name.as_ref().to_string(), - description: description.as_ref().to_string(), + name, + description, default: Some(default), value_type: ValueType::Boolean, } } - pub fn new_flag, S2: AsRef>(name: S1, description: S2) -> Self { + pub const fn new_flag(name: &'static str, description: &'static str) -> Self { Self { - name: name.as_ref().to_string(), - description: description.as_ref().to_string(), + name, + description, default: Some(false), value_type: ValueType::Flag, } @@ -256,7 +283,7 @@ impl Serialize for UntypedConfigOption { } } -impl ConfigOption +impl ConfigOption<'_, V> where V: OptionType, { @@ -319,10 +346,28 @@ mod test { } } + #[test] + fn const_config_option() { + const _: ConfigOption = ConfigOption::new_flag("flag-option", "A flag option"); + const _: ConfigOption = + ConfigOption::new_bool_with_default("bool-option", false, "A boolean option"); + const _: ConfigOption> = + ConfigOption::new_bool_no_default("bool-option", "A boolean option"); + + const _: ConfigOption> = + ConfigOption::new_i64_no_default("integer-option", "A flag option"); + const _: ConfigOption = + ConfigOption::new_i64_with_default("integer-option", 12, "A flag option"); + + const _: ConfigOption> = + ConfigOption::new_str_no_default("integer-option", "A flag option"); + const _: ConfigOption<&str> = + ConfigOption::new_str_with_default("integer-option", "erik", "A flag option"); + } + #[test] fn test_type_serialize() { assert_eq!(json!(ValueType::Integer), json!("int")); - assert_eq!(json!(ValueType::Flag), json!("flag")); } } From 0f6c791c5e936fcdfa353983b4f28ae73ca036e5 Mon Sep 17 00:00:00 2001 From: Erik De Smedt Date: Fri, 2 Feb 2024 13:10:51 +0100 Subject: [PATCH 4/6] cln_plugin: Request value as rust primitive In the old version requesting the config-value of an option was a little bit tricky. Let's say we want to have a plugin which uses a default port of 1234. ```rust let value = plugin.option("plugin-port"); match value { Some(Value::Integer(_)) => {}, Some(Value::String(_)) => {}, // Can never happen Some(Value::Boolean(_)) => {}, // Can never happen None => {}, // Can never happen } ``` Many users of the `cln_plugin` crate are overly cautious and handle all these error scenario's. Which is completely unneeded. Core Lightning will complain if you put a `String` where an `Integer` is expected and will never send the value to the plug-in. This change makes the API much more ergonomical and actually motivates some of the changes in previous commits. ``` const MY_OPTION : ConfigOption = ConfigOption::new_i64_with_default( "plugin-port', 1235, "Description"); let value : Result = plugin.option(MY_OPTION); ``` The result will provide a proper error-message. It is also safe to `unwrap` the result because it will only be triggered if the user neglected to provide the option to the `Builder`. --- plugins/examples/cln-plugin-startup.rs | 2 +- plugins/grpc-plugin/src/main.rs | 26 +- plugins/src/lib.rs | 75 ++++-- plugins/src/options.rs | 321 +++++++++++++++++-------- 4 files changed, 288 insertions(+), 136 deletions(-) diff --git a/plugins/examples/cln-plugin-startup.rs b/plugins/examples/cln-plugin-startup.rs index c876ba2e457d..4b82c5361afd 100644 --- a/plugins/examples/cln-plugin-startup.rs +++ b/plugins/examples/cln-plugin-startup.rs @@ -47,7 +47,7 @@ async fn main() -> Result<(), anyhow::Error> { async fn testoptions(p: Plugin<()>, _v: serde_json::Value) -> Result { Ok(json!({ - "opt-option": format!("{:?}", p.option("opt-option").unwrap()) + "opt-option": format!("{:?}", p.option_str("opt-option").unwrap()) })) } diff --git a/plugins/grpc-plugin/src/main.rs b/plugins/grpc-plugin/src/main.rs index aa2a829b04cc..27eca08a711f 100644 --- a/plugins/grpc-plugin/src/main.rs +++ b/plugins/grpc-plugin/src/main.rs @@ -1,4 +1,4 @@ -use anyhow::{anyhow, Context, Result}; +use anyhow::{Context, Result}; use cln_grpc::pb::node_server::NodeServer; use cln_plugin::{options, Builder}; use log::{debug, warn}; @@ -14,6 +14,10 @@ struct PluginState { ca_cert: Vec, } +const OPTION_GRPC_PORT : options::IntegerConfigOption = options::ConfigOption::new_i64_no_default( + "grpc-port", + "Which port should the grpc plugin listen for incoming connections?"); + #[tokio::main(flavor = "current_thread")] async fn main() -> Result<()> { debug!("Starting grpc plugin"); @@ -22,11 +26,7 @@ async fn main() -> Result<()> { let directory = std::env::current_dir()?; let plugin = match Builder::new(tokio::io::stdin(), tokio::io::stdout()) - .option(options::ConfigOption::new_i64_with_default( - "grpc-port", - -1, - "Which port should the grpc plugin listen for incoming connections?", - )) + .option(OPTION_GRPC_PORT) .configure() .await? { @@ -34,17 +34,15 @@ async fn main() -> Result<()> { None => return Ok(()), }; - let bind_port = match plugin.option("grpc-port") { - Some(options::Value::Integer(-1)) => { - log::info!("`grpc-port` option is not configured, exiting."); + let bind_port = match plugin.option(&OPTION_GRPC_PORT).unwrap() { + Some(port) => port, + None => { + log::info!("'grpc-port' options i not configured. exiting."); plugin - .disable("`grpc-port` option is not configured.") + .disable("Missing 'grpc-port' option") .await?; - return Ok(()); + return Ok(()) } - Some(options::Value::Integer(i)) => i, - None => return Err(anyhow!("Missing 'grpc-port' option")), - Some(o) => return Err(anyhow!("grpc-port is not a valid integer: {:?}", o)), }; let (identity, ca_cert) = tls::init(&directory)?; diff --git a/plugins/src/lib.rs b/plugins/src/lib.rs index ce2ed1e3c025..1dd19a4acc00 100644 --- a/plugins/src/lib.rs +++ b/plugins/src/lib.rs @@ -1,12 +1,12 @@ use crate::codec::{JsonCodec, JsonRpcCodec}; pub use anyhow::anyhow; -use anyhow::Context; +use anyhow::{Context, Result}; use futures::sink::SinkExt; use tokio::io::{AsyncReadExt, AsyncWriteExt}; extern crate log; use log::trace; use messages::{Configuration, FeatureBits, NotificationTopic}; -use options::UntypedConfigOption; +use options::{OptionType, UntypedConfigOption}; use std::collections::HashMap; use std::future::Future; use std::pin::Pin; @@ -44,6 +44,7 @@ where hooks: HashMap>, options: HashMap, + option_values: HashMap>, rpcmethods: HashMap>, subscriptions: HashMap>, notifications: Vec, @@ -66,6 +67,7 @@ where input: FramedRead, output: Arc>>, options: HashMap, + option_values: HashMap>, configuration: Configuration, rpcmethods: HashMap>, hooks: HashMap>, @@ -100,6 +102,7 @@ where state: S, /// "options" field of "init" message sent by cln options: HashMap, + option_values: HashMap>, /// "configuration" field of "init" message sent by cln configuration: Configuration, /// A signal that allows us to wait on the plugin's shutdown. @@ -121,6 +124,9 @@ where hooks: HashMap::new(), subscriptions: HashMap::new(), options: HashMap::new(), + // Should not be configured by user. + // This values are set when parsing the init-call + option_values: HashMap::new(), rpcmethods: HashMap::new(), notifications: vec![], featurebits: FeatureBits::default(), @@ -334,6 +340,7 @@ where notifications: self.notifications, subscriptions, options: self.options, + option_values: self.option_values, configuration, hooks: HashMap::new(), })) @@ -390,26 +397,21 @@ where // Match up the ConfigOptions and fill in their values if we // have a matching entry. - for (_name, opt) in self.options.iter_mut() { - let val = call.options.get(opt.name()); - - opt.value = match (&opt.value, &opt.default(), &val) { - (_, Some(OValue::String(_)), Some(JValue::String(s))) => { - Some(OValue::String(s.clone())) - } - (_, None, Some(JValue::String(s))) => Some(OValue::String(s.clone())), - (_, None, None) => None, - - (_, Some(OValue::Integer(_)), Some(JValue::Number(s))) => { - Some(OValue::Integer(s.as_i64().unwrap())) - } - (_, None, Some(JValue::Number(s))) => Some(OValue::Integer(s.as_i64().unwrap())), - (_, Some(OValue::Boolean(_)), Some(JValue::Bool(s))) => Some(OValue::Boolean(*s)), - (_, None, Some(JValue::Bool(s))) => Some(OValue::Boolean(*s)), - (o, _, _) => panic!("Type mismatch for option {:?}", o), - } + for (name, option) in self.options.iter() { + let json_value = call.options.get(name); + let default_value = option.default(); + + let option_value: Option = match (json_value, default_value) { + (None, None) => None, + (None, Some(default)) => Some(default.clone()), + (Some(JValue::String(s)), _) => Some(OValue::String(s.to_string())), + (Some(JValue::Number(i)), _) => Some(OValue::Integer(i.as_i64().unwrap())), + (Some(JValue::Bool(b)), _) => Some(OValue::Boolean(*b)), + _ => panic!("Type mismatch for option {}", name), + }; + + self.option_values.insert(name.to_string(), option_value); } - Ok(call.configuration) } } @@ -505,8 +507,19 @@ impl Plugin where S: Clone + Send, { - pub fn option(&self, name: &str) -> Option { - self.options.get(name).and_then(|x| x.value.clone()) + pub fn option_str(&self, name: &str) -> Result> { + self.option_values + .get(name) + .ok_or(anyhow!("No option named {}", name)) + .map(|c| c.clone()) + } + + pub fn option( + &self, + config_option: &options::ConfigOption, + ) -> Result { + let value = self.option_str(config_option.name())?; + Ok(OV::from_value(&value)) } } @@ -529,6 +542,7 @@ where let plugin = Plugin { state, options: self.options, + option_values: self.option_values, configuration: self.configuration, wait_handle, sender, @@ -590,8 +604,19 @@ where Ok(()) } - pub fn option(&self, name: &str) -> Option { - self.options.get(name).and_then(|c| c.value.clone()) + pub fn option_str(&self, name: &str) -> Result> { + self.option_values + .get(name) + .ok_or(anyhow!("No option named '{}'", name)) + .map(|c| c.clone()) + } + + pub fn option( + &self, + config_option: &options::ConfigOption, + ) -> Result { + let value = self.option_str(config_option.name())?; + Ok(OV::from_value(&value)) } /// return the cln configuration send to the diff --git a/plugins/src/options.rs b/plugins/src/options.rs index 465aa18f679f..8f45439f59e5 100644 --- a/plugins/src/options.rs +++ b/plugins/src/options.rs @@ -1,54 +1,185 @@ -use anyhow::Result; -use serde::ser::{SerializeStruct, Serializer}; +use serde::ser::Serializer; use serde::Serialize; -// Marker trait for possible values of options +pub mod config_type { + pub struct Integer; + pub struct DefaultInteger; + pub struct String; + pub struct DefaultString; + pub struct Boolean; + pub struct DefaultBoolean; + pub struct Flag; +} + +pub type IntegerConfigOption<'a> = ConfigOption<'a, config_type::Integer>; +pub type StringConfigOption<'a> = ConfigOption<'a, config_type::String>; +pub type BooleanConfigOption<'a> = ConfigOption<'a, config_type::Boolean>; + +pub type DefaultIntegerConfigOption<'a> = ConfigOption<'a, config_type::DefaultInteger>; +pub type DefaultStringConfigOption<'a> = ConfigOption<'a, config_type::DefaultString>; +pub type DefaultBooleanConfigOption<'a> = ConfigOption<'a, config_type::DefaultBoolean>; +/// Config value is represented as a flag +pub type FlagConfigOption<'a> = ConfigOption<'a, config_type::Flag>; + + pub trait OptionType { - fn convert_default(value: Option<&Self>) -> Option; + type OutputValue; + type DefaultValue; + + fn convert_default(value: &Self::DefaultValue) -> Option; + + fn from_value(value: &Option) -> Self::OutputValue; + + fn get_value_type() -> ValueType; } -impl OptionType for &str { - fn convert_default(value: Option<&Self>) -> Option { - value.map(|s| Value::String(s.to_string())) +impl OptionType for config_type::DefaultString { + type OutputValue = String; + type DefaultValue = &'static str; + + fn convert_default(value: &Self::DefaultValue) -> Option { + Some(Value::String(value.to_string())) } -} -impl OptionType for String { - fn convert_default(value: Option<&Self>) -> Option { - value.map(|s| Value::String(s.clone())) + fn from_value(value: &Option) -> Self::OutputValue { + match value { + Some(Value::String(s)) => s.to_string(), + _ => panic!("Type mismatch. Expected string but found {:?}", value), + } + } + + fn get_value_type() -> ValueType { + ValueType::String } } -impl OptionType for i64 { - fn convert_default(value: Option<&Self>) -> Option { - value.map(|i| Value::Integer(*i)) + +impl OptionType for config_type::DefaultInteger { + type OutputValue = i64; + type DefaultValue = i64; + + fn convert_default(value: &Self::DefaultValue) -> Option { + Some(Value::Integer(*value)) + } + + fn from_value(value: &Option) -> i64 { + match value { + Some(Value::Integer(i)) => *i, + _ => panic!("Type mismatch. Expected Integer but found {:?}", value), + } + } + + fn get_value_type() -> ValueType { + ValueType::Integer } } -impl OptionType for bool { - fn convert_default(value: Option<&Self>) -> Option { - value.map(|b| Value::Boolean(*b)) + +impl OptionType for config_type::DefaultBoolean { + type OutputValue = bool; + type DefaultValue = bool; + + fn convert_default(value: &bool) -> Option { + Some(Value::Boolean(*value)) + } + fn from_value(value: &Option) -> bool { + match value { + Some(Value::Boolean(b)) => *b, + _ => panic!("Type mismatch. Expected Boolean but found {:?}", value), + } + } + + fn get_value_type() -> ValueType { + ValueType::Boolean } } -impl OptionType for Option { - fn convert_default(_value: Option<&Self>) -> Option { - None +impl OptionType for config_type::Flag { + type OutputValue = bool; + type DefaultValue = (); + + fn convert_default(_value: &()) -> Option { + Some(Value::Boolean(false)) + } + + fn from_value(value: &Option) -> bool { + match value { + Some(Value::Boolean(b)) => *b, + _ => panic!("Type mismatch. Expected Boolean but found {:?}", value), + } + } + + fn get_value_type() -> ValueType { + ValueType::Flag } } -impl OptionType for Option<&str> { - fn convert_default(_value: Option<&Self>) -> Option { +impl OptionType for config_type::String { + type OutputValue = Option; + type DefaultValue = (); + + fn convert_default(_value: &()) -> Option { None } + + fn from_value(value: &Option) -> Option { + match value { + Some(Value::String(s)) => Some(s.to_string()), + None => None, + _ => panic!( + "Type mismatch. Expected Option but found {:?}", + value + ), + } + } + + fn get_value_type() -> ValueType { + ValueType::String + } } -impl OptionType for Option { - fn convert_default(_value: Option<&Self>) -> Option { + +impl OptionType for config_type::Integer { + type OutputValue = Option; + type DefaultValue = (); + + fn convert_default(_value: &()) -> Option { None } + + fn from_value(value: &Option) -> Self::OutputValue { + match value { + Some(Value::Integer(i)) => Some(*i), + None => None, + _ => panic!( + "Type mismatch. Expected Option but found {:?}", + value + ), + } + } + + fn get_value_type() -> ValueType { + ValueType::Integer + } } -impl OptionType for Option { - fn convert_default(_value: Option<&Self>) -> Option { +impl OptionType for config_type::Boolean { + type OutputValue = Option; + type DefaultValue = (); + + fn convert_default(_value: &()) -> Option { None } + fn from_value(value: &Option) -> Self::OutputValue { + match value { + Some(Value::Boolean(b)) => Some(*b), + None => None, + _ => panic!( + "Type mismatch. Expected Option but found {:?}", + value + ), + } + } + + fn get_value_type() -> ValueType { + ValueType::Boolean + } } #[derive(Clone, Debug, Serialize)] @@ -70,6 +201,19 @@ pub enum Value { Boolean(bool), } +impl Serialize for Value { + fn serialize(&self, serializer: S) -> std::prelude::v1::Result + where + S: Serializer, + { + match self { + Value::String(s) => serializer.serialize_str(s), + Value::Integer(i) => serializer.serialize_i64(*i), + Value::Boolean(b) => serializer.serialize_bool(*b), + } + } +} + impl Value { /// Returns true if the `Value` is a String. Returns false otherwise. /// @@ -127,25 +271,27 @@ impl Value { #[derive(Clone, Debug)] pub struct ConfigOption<'a, V: OptionType> { - name: &'a str, - default: Option, - value_type: ValueType, - description: &'a str, + /// The name of the `ConfigOption`. + pub name: &'a str, + /// The default value of the `ConfigOption` + pub default: V::DefaultValue, + pub description: &'a str, + pub deprecated: bool, } impl ConfigOption<'_, V> { pub fn build(&self) -> UntypedConfigOption { UntypedConfigOption { name: self.name.to_string(), - value_type: self.value_type.clone(), - default: OptionType::convert_default(self.default.as_ref()), - value: None, + value_type: V::get_value_type(), + default: ::convert_default(&self.default), description: self.description.to_string(), + deprecated: self.deprecated, } } } -impl ConfigOption<'_, &'static str> { +impl DefaultStringConfigOption<'_> { pub const fn new_str_with_default( name: &'static str, default: &'static str, @@ -153,25 +299,25 @@ impl ConfigOption<'_, &'static str> { ) -> Self { Self { name: name, - default: Some(default), - value_type: ValueType::String, + default: default, description: description, + deprecated: false, } } } -impl ConfigOption<'_, Option<&str>> { +impl StringConfigOption<'_> { pub const fn new_str_no_default(name: &'static str, description: &'static str) -> Self { Self { name, - default: None, - value_type: ValueType::String, - description, + default: (), + description : description, + deprecated: false, } } } -impl ConfigOption<'_, i64> { +impl DefaultIntegerConfigOption<'_> { pub const fn new_i64_with_default( name: &'static str, default: i64, @@ -179,36 +325,36 @@ impl ConfigOption<'_, i64> { ) -> Self { Self { name: name, - default: Some(default), - value_type: ValueType::Integer, + default: default, description: description, + deprecated: false, } } } -impl ConfigOption<'_, Option> { +impl IntegerConfigOption<'_> { pub const fn new_i64_no_default(name: &'static str, description: &'static str) -> Self { Self { name: name, - default: None, - value_type: ValueType::Integer, + default: (), description: description, + deprecated: false, } } } -impl ConfigOption<'_, Option> { +impl BooleanConfigOption<'_> { pub const fn new_bool_no_default(name: &'static str, description: &'static str) -> Self { Self { name, description, - default: None, - value_type: ValueType::Boolean, + default: (), + deprecated: false, } } } -impl ConfigOption<'_, bool> { +impl DefaultBooleanConfigOption<'_> { pub const fn new_bool_with_default( name: &'static str, default: bool, @@ -217,29 +363,38 @@ impl ConfigOption<'_, bool> { Self { name, description, - default: Some(default), - value_type: ValueType::Boolean, + default: default, + deprecated: false, } } +} +impl FlagConfigOption<'_> { pub const fn new_flag(name: &'static str, description: &'static str) -> Self { Self { name, description, - default: Some(false), - value_type: ValueType::Flag, + default: (), + deprecated: false, } } } +fn is_false(b: &bool) -> bool { + *b == false +} + /// An stringly typed option that is passed to -#[derive(Clone, Debug)] +#[derive(Clone, Debug, Serialize)] pub struct UntypedConfigOption { name: String, + #[serde(rename = "type")] pub(crate) value_type: ValueType, - pub(crate) value: Option, + #[serde(skip_serializing_if = "Option::is_none")] default: Option, description: String, + #[serde(skip_serializing_if = "is_false")] + deprecated: bool, } impl UntypedConfigOption { @@ -251,38 +406,6 @@ impl UntypedConfigOption { } } -// When we serialize we don't add the value. This is because we only -// ever serialize when we pass the option back to lightningd during -// the getmanifest call. -impl Serialize for UntypedConfigOption { - fn serialize(&self, serializer: S) -> Result - where - S: Serializer, - { - let mut s = serializer.serialize_struct("ConfigOption", 4)?; - s.serialize_field("name", &self.name)?; - match &self.default { - Some(Value::String(ss)) => { - s.serialize_field("default", ss)?; - } - Some(Value::Integer(i)) => { - s.serialize_field("default", i)?; - } - Some(Value::Boolean(b)) => { - match self.value_type { - ValueType::Boolean => s.serialize_field("default", b)?, - ValueType::Flag => {} - _ => {} // This should never happen - } - } - _ => {} - } - s.serialize_field("type", &self.value_type)?; - s.serialize_field("description", &self.description)?; - s.end() - } -} - impl ConfigOption<'_, V> where V: OptionType, @@ -298,6 +421,7 @@ where #[cfg(test)] mod test { + use super::*; #[test] @@ -335,7 +459,8 @@ mod test { json!({ "name" : "name", "description": "description", - "type" : "flag" + "type" : "flag", + "default" : false }), ), ]; @@ -348,20 +473,24 @@ mod test { #[test] fn const_config_option() { - const _: ConfigOption = ConfigOption::new_flag("flag-option", "A flag option"); - const _: ConfigOption = + // The main goal of this test is to test compilation + + // Initiate every type as a const + const _: FlagConfigOption = + ConfigOption::new_flag("flag-option", "A flag option"); + const _: DefaultBooleanConfigOption = ConfigOption::new_bool_with_default("bool-option", false, "A boolean option"); - const _: ConfigOption> = + const _: BooleanConfigOption = ConfigOption::new_bool_no_default("bool-option", "A boolean option"); - const _: ConfigOption> = + const _: IntegerConfigOption = ConfigOption::new_i64_no_default("integer-option", "A flag option"); - const _: ConfigOption = + const _: DefaultIntegerConfigOption = ConfigOption::new_i64_with_default("integer-option", 12, "A flag option"); - const _: ConfigOption> = + const _: StringConfigOption = ConfigOption::new_str_no_default("integer-option", "A flag option"); - const _: ConfigOption<&str> = + const _: DefaultStringConfigOption = ConfigOption::new_str_with_default("integer-option", "erik", "A flag option"); } From 6aba17b50adec50a6cffc40b58807f378d1f0e06 Mon Sep 17 00:00:00 2001 From: Erik De Smedt Date: Tue, 6 Feb 2024 19:47:20 +0100 Subject: [PATCH 5/6] cln_plugin: Add documentation --- plugins/src/options.rs | 140 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 139 insertions(+), 1 deletion(-) diff --git a/plugins/src/options.rs b/plugins/src/options.rs index 8f45439f59e5..cb976798df18 100644 --- a/plugins/src/options.rs +++ b/plugins/src/options.rs @@ -1,3 +1,135 @@ +//! This module contains all logic related to `ConfigOption`'s that can be +//! set in Core Lightning. The [Core Lightning documentation](https://docs.corelightning.org/reference/lightningd-config) +//! describes how the user can specify configuration. This can be done using +//! a command-line argument or by specifying the value in the `config`-file. +//! +//! ## A simple example +//! +//! A config option can either be specified using helper-methods or explicitly. +//! +//! ```no_run +//! use anyhow::Result; +//! +//! use cln_plugin::ConfiguredPlugin; +//! use cln_plugin::Builder; +//! use cln_plugin::options::{StringConfigOption, DefaultStringConfigOption}; +//! +//! const STRING_OPTION : StringConfigOption = +//! StringConfigOption::new_str_no_default( +//! "string-option", +//! "A config option of type string with no default" +//! ); +//! +//! const DEFAULT_STRING_OPTION : DefaultStringConfigOption = +//! DefaultStringConfigOption::new_str_with_default( +//! "string-option", +//! "bitcoin", +//! "A config option which uses 'bitcoin when as a default" +//! ); +//! +//! #[tokio::main] +//! async fn main() -> Result<()>{ +//! let configured_plugin = Builder::new(tokio::io::stdin(), tokio::io::stdout()) +//! .option(STRING_OPTION) +//! .option(DEFAULT_STRING_OPTION) +//! .configure() +//! .await?; +//! +//! let configured_plugin :ConfiguredPlugin<(),_,_> = match configured_plugin { +//! Some(plugin) => plugin, +//! None => return Ok(()) // Core Lightning was started with --help +//! }; +//! +//! // Note the types here. +//! // In `string_option` the developer did not specify a default and `None` +//! // will be returned if the user doesn't specify a configuration. +//! // +//! // In `default_string_option` the developer set a default-value. +//! // If the user doesn't specify a configuration the `String` `"bitcoin"` +//! // will be returned. +//! let string_option : Option = configured_plugin +//! .option(&STRING_OPTION) +//! .expect("Failed to configure option"); +//! let default_string_option : String = configured_plugin +//! .option(&DEFAULT_STRING_OPTION) +//! .expect("Failed to configure option"); +//! +//! // You can start the plugin here +//! // ... +//! +//! Ok(()) +//! } +//! +//! ``` +//! +//! ## Explicit initialization +//! +//! A `ConfigOption` can be initialized explicitly or using one of the helper methods. +//! The two code-samples below are equivalent. The explicit version is more verbose +//! but allows specifying additional information. +//! +//! ``` +//! use cln_plugin::options::{StringConfigOption}; +//! +//! const STRING_OPTION : StringConfigOption = StringConfigOption { +//! name : "string-option", +//! default : (), // We provide no default here +//! description : "A config option of type string that takes no default", +//! deprecated : false, // Option is not deprecated +//! }; +//! ``` +//! +//! ``` +//! use cln_plugin::options::{StringConfigOption}; +//! // This code is equivalent +//! const STRING_OPTION_EQ : StringConfigOption = StringConfigOption::new_str_no_default( +//! "string-option-eq", +//! "A config option of type string that takes no default" +//! ); +//! ``` +//! +//! ## Required options +//! +//! In some cases you want to require the user to specify a value. +//! This can be achieved using [`crate::ConfiguredPlugin::disable`]. +//! +//! ```no_run +//! use anyhow::Result; +//! +//! use cln_plugin::ConfiguredPlugin; +//! use cln_plugin::Builder; +//! use cln_plugin::options::{IntegerConfigOption}; +//! +//! const WEBPORTAL_PORT : IntegerConfigOption = IntegerConfigOption::new_i64_no_default( +//! "webportal-port", +//! "The port on which the web-portal will be exposed" +//! ); +//! +//! #[tokio::main] +//! async fn main() -> Result<()> { +//! let configured_plugin = Builder::new(tokio::io::stdin(), tokio::io::stdout()) +//! .option(WEBPORTAL_PORT) +//! .configure() +//! .await?; +//! +//! let configured_plugin :ConfiguredPlugin<(),_,_> = match configured_plugin { +//! Some(plugin) => plugin, +//! None => return Ok(()) // Core Lightning was started with --help +//! }; +//! +//! let webportal_port : i64 = match(configured_plugin.option(&WEBPORTAL_PORT)?) { +//! Some(port) => port, +//! None => { +//! return configured_plugin.disable("No value specified for webportal-port").await +//! } +//! }; +//! +//! // Start the plugin here +//! //.. +//! +//! Ok(()) +//! } +//! ``` use serde::ser::Serializer; use serde::Serialize; @@ -11,12 +143,18 @@ pub mod config_type { pub struct Flag; } + +/// Config values are represented as an i64. No default is used pub type IntegerConfigOption<'a> = ConfigOption<'a, config_type::Integer>; +/// Config values are represented as a String. No default is used. pub type StringConfigOption<'a> = ConfigOption<'a, config_type::String>; +/// Config values are represented as a boolean. No default is used. pub type BooleanConfigOption<'a> = ConfigOption<'a, config_type::Boolean>; - +/// Config values are repsentedas an i64. A default is used pub type DefaultIntegerConfigOption<'a> = ConfigOption<'a, config_type::DefaultInteger>; +/// Config values are repsentedas an String. A default is used pub type DefaultStringConfigOption<'a> = ConfigOption<'a, config_type::DefaultString>; +/// Config values are repsentedas an bool. A default is used pub type DefaultBooleanConfigOption<'a> = ConfigOption<'a, config_type::DefaultBoolean>; /// Config value is represented as a flag pub type FlagConfigOption<'a> = ConfigOption<'a, config_type::Flag>; From f57beb293dbf96800f3650247e44e3306db32180 Mon Sep 17 00:00:00 2001 From: Erik De Smedt Date: Tue, 6 Feb 2024 10:52:50 +0100 Subject: [PATCH 6/6] `cln_plugin` : Test default values for ConfigOptions --- plugins/examples/cln-plugin-startup.rs | 30 ++++++++++++++++---------- plugins/src/options.rs | 5 +---- tests/test_cln_rs.py | 10 +++++---- 3 files changed, 26 insertions(+), 19 deletions(-) diff --git a/plugins/examples/cln-plugin-startup.rs b/plugins/examples/cln-plugin-startup.rs index 4b82c5361afd..81a001773e62 100644 --- a/plugins/examples/cln-plugin-startup.rs +++ b/plugins/examples/cln-plugin-startup.rs @@ -2,25 +2,28 @@ //! plugins using the Rust API against Core Lightning. #[macro_use] extern crate serde_json; -use cln_plugin::{messages, options, Builder, Error, Plugin}; +use cln_plugin::options::{DefaultIntegerConfigOption, IntegerConfigOption}; +use cln_plugin::{messages, Builder, Error, Plugin}; use tokio; const TEST_NOTIF_TAG: &str = "test_custom_notification"; +const TEST_OPTION: DefaultIntegerConfigOption = DefaultIntegerConfigOption::new_i64_with_default( + "test-option", + 42, + "a test-option with default 42", +); + +const TEST_OPTION_NO_DEFAULT: IntegerConfigOption = + IntegerConfigOption::new_i64_no_default("opt-option", "An option without a default"); + #[tokio::main] async fn main() -> Result<(), anyhow::Error> { let state = (); if let Some(plugin) = Builder::new(tokio::io::stdin(), tokio::io::stdout()) - .option(options::ConfigOption::new_i64_with_default( - "test-option", - 42, - "a test-option with default 42", - )) - .option(options::ConfigOption::new_i64_no_default( - "opt-option", - "An optional option", - )) + .option(TEST_OPTION) + .option(TEST_OPTION_NO_DEFAULT) .rpcmethod("testmethod", "This is a test", testmethod) .rpcmethod( "testoptions", @@ -46,8 +49,13 @@ async fn main() -> Result<(), anyhow::Error> { } async fn testoptions(p: Plugin<()>, _v: serde_json::Value) -> Result { + let test_option = p.option(&TEST_OPTION)?; + let test_option_no_default = p + .option(&TEST_OPTION_NO_DEFAULT)?; + Ok(json!({ - "opt-option": format!("{:?}", p.option_str("opt-option").unwrap()) + "test-option": test_option, + "opt-option" : test_option_no_default })) } diff --git a/plugins/src/options.rs b/plugins/src/options.rs index cb976798df18..843ee6e95021 100644 --- a/plugins/src/options.rs +++ b/plugins/src/options.rs @@ -143,7 +143,6 @@ pub mod config_type { pub struct Flag; } - /// Config values are represented as an i64. No default is used pub type IntegerConfigOption<'a> = ConfigOption<'a, config_type::Integer>; /// Config values are represented as a String. No default is used. @@ -159,7 +158,6 @@ pub type DefaultBooleanConfigOption<'a> = ConfigOption<'a, config_type::DefaultB /// Config value is represented as a flag pub type FlagConfigOption<'a> = ConfigOption<'a, config_type::Flag>; - pub trait OptionType { type OutputValue; type DefaultValue; @@ -614,8 +612,7 @@ mod test { // The main goal of this test is to test compilation // Initiate every type as a const - const _: FlagConfigOption = - ConfigOption::new_flag("flag-option", "A flag option"); + const _: FlagConfigOption = ConfigOption::new_flag("flag-option", "A flag option"); const _: DefaultBooleanConfigOption = ConfigOption::new_bool_with_default("bool-option", false, "A boolean option"); const _: BooleanConfigOption = diff --git a/tests/test_cln_rs.py b/tests/test_cln_rs.py index eda1d9f61f35..de97ec717223 100644 --- a/tests/test_cln_rs.py +++ b/tests/test_cln_rs.py @@ -67,18 +67,20 @@ def test_plugin_start(node_factory): l1.daemon.wait_for_log(r'Got a connect notification') -def test_plugin_optional_opts(node_factory): +def test_plugin_options_handle_defaults(node_factory): """Start a minimal plugin and ensure it is well-behaved """ bin_path = Path.cwd() / "target" / RUST_PROFILE / "examples" / "cln-plugin-startup" - l1 = node_factory.get_node(options={"plugin": str(bin_path), 'opt-option': 31337}) + l1 = node_factory.get_node(options={"plugin": str(bin_path), 'opt-option': 31337, "test-option": 31338}) opts = l1.rpc.testoptions() - print(opts) + assert opts["opt-option"] == 31337 + assert opts["test-option"] == 31338 # Do not set any value, should be None now l1 = node_factory.get_node(options={"plugin": str(bin_path)}) opts = l1.rpc.testoptions() - print(opts) + assert opts["opt-option"] is None, "opt-option has no default" + assert opts["test-option"] == 42, "test-option has a default of 42" def test_grpc_connect(node_factory):