-
Notifications
You must be signed in to change notification settings - Fork 37
simln-lib/feature: Optional name for activity descriptions #253
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
a3f4495
b99ac02
6f8d5c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,13 +8,14 @@ use simln_lib::{ | |
ActivityDefinition, Amount, Interval, LightningError, LightningNode, NodeId, NodeInfo, | ||
Simulation, SimulationCfg, WriteResults, | ||
}; | ||
use std::collections::HashMap; | ||
use std::collections::{HashMap, HashSet}; | ||
use std::fs; | ||
use std::ops::AsyncFn; | ||
use std::path::PathBuf; | ||
use std::sync::Arc; | ||
use tokio::sync::Mutex; | ||
use tokio_util::task::TaskTracker; | ||
use regex::Regex; | ||
|
||
/// The default directory where the simulation files are stored and where the results will be written to. | ||
pub const DEFAULT_DATA_DIR: &str = "."; | ||
|
@@ -100,6 +101,9 @@ enum NodeConnection { | |
/// [NodeId], which enables the use of public keys and aliases in the simulation description. | ||
#[derive(Debug, Clone, Serialize, Deserialize)] | ||
struct ActivityParser { | ||
/// Optional identifier for this activity. | ||
#[serde(default)] | ||
pub name: Option<String>, | ||
/// The source of the payment. | ||
#[serde(with = "serializers::serde_node_id")] | ||
pub source: NodeId, | ||
|
@@ -259,9 +263,55 @@ async fn validate_activities( | |
get_node_info: impl AsyncFn(&PublicKey) -> Result<NodeInfo, LightningError>, | ||
) -> Result<Vec<ActivityDefinition>, LightningError> { | ||
let mut validated_activities = vec![]; | ||
let mut activity_names = HashSet::new(); | ||
|
||
// Make all the activities identifiable by PK internally | ||
for act in activity.into_iter() { | ||
for (index, act) in activity.into_iter().enumerate() { | ||
// Generate a default name if one is not provided | ||
let name = match &act.name { | ||
Some(name) => { | ||
// Disallow empty names | ||
if name.is_empty() { | ||
return Err(LightningError::ValidationError( | ||
"activity name cannot be empty".to_string() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: let's make this a little more helpful? "activity name cannot be an empty string, either remove name entirely or provide a string value" |
||
)); | ||
} | ||
|
||
// Disallow users from using the reserved "Activity-x" format | ||
let reserved_pattern = Regex::new(r"^Activity-\d+$").unwrap(); | ||
if reserved_pattern.is_match(name) { | ||
return Err(LightningError::ValidationError(format!( | ||
"'{}' uses a reserved name format. 'Activity-{{number}}' is reserved for system use.", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. likewise - slightly more helpful error names, add a prompt to pick another name here |
||
name | ||
))); | ||
} | ||
|
||
// Check for duplicate names | ||
if !activity_names.insert(name.clone()) { | ||
return Err(LightningError::ValidationError(format!( | ||
"duplicate activity name: {}", | ||
name | ||
))); | ||
} | ||
name.clone() | ||
}, | ||
None => { | ||
// Generate a unique system name | ||
let mut counter = index; | ||
let mut unique_name; | ||
|
||
loop { | ||
unique_name = format!("Activity-{}", counter); | ||
if activity_names.insert(unique_name.clone()) { | ||
break; | ||
} | ||
counter += 1; | ||
} | ||
|
||
unique_name | ||
}, | ||
carlaKC marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}; | ||
|
||
// We can only map aliases to nodes we control, so if either the source or destination alias | ||
// is not in alias_node_map, we fail | ||
let source = if let Some(source) = match &act.source { | ||
|
@@ -297,6 +347,7 @@ async fn validate_activities( | |
}; | ||
|
||
validated_activities.push(ActivityDefinition { | ||
name: Some(name), | ||
source, | ||
destination, | ||
interval_secs: act.interval_secs, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,8 @@ use tokio::time::Duration; | |
|
||
#[derive(Clone)] | ||
pub struct DefinedPaymentActivity { | ||
#[allow(dead_code)] | ||
name: Option<String>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're going to always fill this in if the end user provides it, so we want this to be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, and also another reason for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that this should be optional in Also shouldn't have |
||
destination: NodeInfo, | ||
start: Option<Duration>, | ||
count: Option<u64>, | ||
|
@@ -16,13 +18,15 @@ pub struct DefinedPaymentActivity { | |
|
||
impl DefinedPaymentActivity { | ||
pub fn new( | ||
name: Option<String>, | ||
destination: NodeInfo, | ||
start: Option<Duration>, | ||
count: Option<u64>, | ||
wait: ValueOrRange<u16>, | ||
amount: ValueOrRange<u64>, | ||
) -> Self { | ||
DefinedPaymentActivity { | ||
name, | ||
destination, | ||
start, | ||
count, | ||
|
@@ -86,13 +90,15 @@ mod tests { | |
|
||
#[test] | ||
fn test_defined_activity_generator() { | ||
let name: String = "test_generator".to_string(); | ||
let node = create_nodes(1, 100000); | ||
let node = &node.first().unwrap().0; | ||
|
||
let source = get_random_keypair(); | ||
let payment_amt = 50; | ||
|
||
let generator = DefinedPaymentActivity::new( | ||
Option::from(name), | ||
node.clone(), | ||
None, | ||
None, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -164,6 +164,8 @@ pub type Interval = ValueOrRange<u16>; | |
/// This is constructed during activity validation and passed along to the [Simulation]. | ||
#[derive(Debug, Clone)] | ||
pub struct ActivityDefinition { | ||
/// Optional identifier for this activity. | ||
pub name: Option<String>, | ||
/// The source of the payment. | ||
pub source: NodeInfo, | ||
/// The destination of the payment. | ||
|
@@ -500,6 +502,7 @@ pub struct WriteResults { | |
/// ExecutorKit contains the components required to spin up an activity configured by the user, to be used to | ||
/// spin up the appropriate producers and consumers for the activity. | ||
struct ExecutorKit { | ||
name: Option<String>, | ||
source_info: NodeInfo, | ||
/// We use an arc mutex here because some implementations of the trait will be very expensive to clone. | ||
/// See [NetworkGraphView] for details. | ||
|
@@ -806,6 +809,7 @@ impl Simulation { | |
if !self.activity.is_empty() { | ||
for description in self.activity.iter() { | ||
let activity_generator = DefinedPaymentActivity::new( | ||
description.name.clone(), | ||
description.destination.clone(), | ||
description | ||
.start_secs | ||
|
@@ -816,6 +820,7 @@ impl Simulation { | |
); | ||
|
||
generators.push(ExecutorKit { | ||
name: description.name.clone(), | ||
source_info: description.source.clone(), | ||
// Defined activities have very simple generators, so the traits required are implemented on | ||
// a single struct which we just cheaply clone. | ||
|
@@ -874,6 +879,7 @@ impl Simulation { | |
|
||
for (node_info, capacity) in active_nodes.values() { | ||
generators.push(ExecutorKit { | ||
name: None, | ||
source_info: node_info.clone(), | ||
network_generator: network_generator.clone(), | ||
payment_generator: Box::new( | ||
|
@@ -958,9 +964,11 @@ impl Simulation { | |
let pe_sender = sender.clone(); | ||
tasks.spawn(async move { | ||
let source = executor.source_info.clone(); | ||
let name = executor.name.as_deref().unwrap(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having something be an If somebody were using sim-ln as a library, this would make their code panic if they didn't provide a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the |
||
|
||
log::info!( | ||
"Starting activity producer for {}: {}.", | ||
"[{}] Starting activity producer for {}: {}.", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: log name elsewhere as well? |
||
name, | ||
source, | ||
executor.payment_generator | ||
); | ||
|
Uh oh!
There was an error while loading. Please reload this page.