Skip to content

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions sim-cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ futures = "0.3.30"
console-subscriber = { version = "0.4.0", optional = true}
tokio-util = { version = "0.7.13", features = ["rt"] }
openssl = { version = "0.10", features = ["vendored"] }
regex = "1.11.1"

[features]
dev = ["console-subscriber"]
55 changes: 53 additions & 2 deletions sim-cli/src/parsing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 = ".";
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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()
Copy link
Contributor

Choose a reason for hiding this comment

The 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.",
Copy link
Contributor

Choose a reason for hiding this comment

The 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
},
};

// 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 {
Expand Down Expand Up @@ -297,6 +347,7 @@ async fn validate_activities(
};

validated_activities.push(ActivityDefinition {
name: Some(name),
source,
destination,
interval_secs: act.interval_secs,
Expand Down
6 changes: 6 additions & 0 deletions simln-lib/src/defined_activity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ use tokio::time::Duration;

#[derive(Clone)]
pub struct DefinedPaymentActivity {
#[allow(dead_code)]
name: Option<String>,
Copy link
Contributor

Choose a reason for hiding this comment

The 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 Option?

Copy link
Author

@Onyekachukwu-Nweke Onyekachukwu-Nweke Apr 26, 2025

Choose a reason for hiding this comment

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

Yeah, and also another reason for the Option is for it not to break the system for backward compatibility

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that this should be optional in ActivityParser, but DefinedPaymentActivity is an internal struct unrelated to parsing user values? It can be an Option when we parse it and required when we use it interanally.

Also shouldn't have #[allow(dead_code)] here?

destination: NodeInfo,
start: Option<Duration>,
count: Option<u64>,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
22 changes: 17 additions & 5 deletions simln-lib/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

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

Having something be an Option then unwraping it later effectively means that it isn't actually an Option?

If somebody were using sim-ln as a library, this would make their code panic if they didn't provide a name which is seemingly optional based on our public API.


log::info!(
"Starting activity producer for {}: {}.",
"[{}] Starting activity producer for {}: {}.",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: log name elsewhere as well?

name,
source,
executor.payment_generator
);
Expand Down Expand Up @@ -1571,8 +1579,9 @@ mod tests {
let missing_nodes = test_utils::create_nodes(1, 100_000);
let missing_node = missing_nodes.first().unwrap().0.clone();
let dest_node = nodes[0].clone();
let activity_name = None;

let activity = test_utils::create_activity(missing_node, dest_node, 1000);
let activity = test_utils::create_activity(activity_name, missing_node, dest_node, 1000);
let simulation = test_utils::create_simulation(clients, vec![activity]);
let result = simulation.validate_activity().await;

Expand All @@ -1588,8 +1597,9 @@ mod tests {
let (nodes, clients) = LightningTestNodeBuilder::new(1).build_full();
let dest_nodes = test_utils::create_nodes(1, 100_000);
let dest_node = dest_nodes.first().unwrap().0.clone();
let activity_name = None;

let activity = test_utils::create_activity(nodes[0].clone(), dest_node, 1000);
let activity = test_utils::create_activity(activity_name, nodes[0].clone(), dest_node, 1000);
let simulation = test_utils::create_simulation(clients, vec![activity]);
let result = simulation.validate_activity().await;

Expand All @@ -1605,9 +1615,10 @@ mod tests {
let (nodes, clients) = LightningTestNodeBuilder::new(1).build_full();
let dest_nodes = test_utils::create_nodes(1, 100_000);
let mut dest_node = dest_nodes.first().unwrap().0.clone();
let activity_name = None;
dest_node.features.set_keysend_optional();

let activity = test_utils::create_activity(nodes[0].clone(), dest_node, 1000);
let activity = test_utils::create_activity(activity_name, nodes[0].clone(), dest_node, 1000);
let simulation = test_utils::create_simulation(clients, vec![activity]);
let result = simulation.validate_activity().await;

Expand All @@ -1619,8 +1630,9 @@ mod tests {
#[tokio::test]
async fn test_validate_zero_amount_no_valid() {
let (nodes, clients) = LightningTestNodeBuilder::new(2).build_full();
let activity_name = None;

let activity = test_utils::create_activity(nodes[0].clone(), nodes[1].clone(), 0);
let activity = test_utils::create_activity(activity_name, nodes[0].clone(), nodes[1].clone(), 0);
let simulation = test_utils::create_simulation(clients, vec![activity]);
let result = simulation.validate_activity().await;

Expand Down
2 changes: 2 additions & 0 deletions simln-lib/src/test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,11 +207,13 @@ pub fn create_simulation(
)
}
pub fn create_activity(
name: Option<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Unclear to me what value this commit is adding?

source: NodeInfo,
destination: NodeInfo,
amount_msat: u64,
) -> ActivityDefinition {
ActivityDefinition {
name,
source,
destination,
start_secs: None,
Expand Down
Loading