sim-lib: Allows amount and interval ranges in manual activity definition #153

sr-gi · 2023-10-25T19:19:51Z

Values in activities can now be fixed values (e.g. 1000) or ranges (e.g. [500, 1000]).

If a range is provided, the value will be sampled uniformly at random from it.

Close #43

carlaKC · 2023-10-26T18:31:07Z

I think that we should get some refactoring in before we make this type of change.

produce_events and produce_random events should really just be one function that takes a NetworkGenerator and PaymentGenerator. I've started on that refactor, and we're going to have some significant churn if we do this piece first. Since this isn't very high prio I'd like to hold off till that's in if it's okay.

sr-gi · 2023-10-26T18:47:02Z

Sure, no rush on my end

sr-gi · 2024-03-27T16:20:09Z

@carlaKC I saw you tagged #43 for the V3 release. I'm happy to rework this if you find it useful

carlaKC · 2024-03-27T17:08:04Z

@carlaKC I saw you tagged #43 for the V3 release. I'm happy to rework this if you find it useful

That would be great!

sr-gi · 2024-03-28T12:07:10Z

sim-lib/src/defined_activity.rs

@@ -37,7 +39,11 @@ impl DestinationGenerator for DefinedPaymentActivity {

 impl PaymentGenerator for DefinedPaymentActivity {
    fn next_payment_wait(&self) -> Duration {
-        self.wait
+        let wait = Duration::from_secs(self.wait.value() as u64);


This is the only bit I'm not super happy about, given we'll be building a Duration object every time we call next_payment_wait, even for fixed wait payments. However, changing this would imply changing the generic for ValueOrRange, and I think potentially more code bolilerplate.

I don't think this is too bad, but I'm open to change it if you find it is.

We could have a RangePaymentActivity impl of PaymentGenerator which deals exclusively with the range case?

We can stil parse everything as-is here, but then create either RangePaymentActivity or DefinedPaymentActivity when we switch over from ActiviyParser -> ActivityDefinition. Might cause a little bit of trait object agony in main, but we should be mostly okay in lib 🤞

That might also be quite boiler-platey, but could be worth considering!

I don't know if that would be even more complex.

We have two fields that can be either a range or a value (but only one of them has the initialization issue), how do we deal with the combination of these?

DefinedPaymentActivity would need to have wait: Duration, amount: ValueOrRange<u64> while RangePaymentActivity would have wait: ValueOrRange<u16>, amount: ValueOrRange<u64>. That's really counterintuitive. The alternative would be to only allow either both or none to be ranges, but that's limiting the functionality.

None of the solutions I've been trying to come up with have been satisfying for one reason or another (nor even having a two-argument generic, since deserialization becomes a mess). I've been checking the constructor of Duration and, honestly, it doesn't look that bad (specially considering that we are constructing with nanos=0): https://doc.rust-lang.org/src/core/time.rs.html#199-207

So I would bite the bullet with this one, but I'm happy to be convinced of an alternative solution if you could find something I'm missing

Managed to completely miss this comment - sorry!!

DefinedPaymentActivity would need to have wait: Duration, amount: ValueOrRange while RangePaymentActivity would have wait: ValueOrRange, amount: ValueOrRange. That's really counterintuitive. The alternative would be to only allow either both or none to be ranges, but that's limiting the functionality.

Good point, didn't think about the inbetween case where we only want a range for one of them! Agreed, let's just go ahead w/ what we have here.

np, I'll update it with the most recent comments

carlaKC

Thanks for picking this one back up!

sim-lib/src/defined_activity.rs

carlaKC · 2024-03-29T15:54:36Z

sim-lib/src/defined_activity.rs

@@ -37,7 +39,11 @@ impl DestinationGenerator for DefinedPaymentActivity {

 impl PaymentGenerator for DefinedPaymentActivity {
    fn next_payment_wait(&self) -> Duration {
-        self.wait
+        let wait = Duration::from_secs(self.wait.value() as u64);


We could have a RangePaymentActivity impl of PaymentGenerator which deals exclusively with the range case?

We can stil parse everything as-is here, but then create either RangePaymentActivity or DefinedPaymentActivity when we switch over from ActiviyParser -> ActivityDefinition. Might cause a little bit of trait object agony in main, but we should be mostly okay in lib 🤞

That might also be quite boiler-platey, but could be worth considering!

bjohnson5

@sr-gi Hello! I am a new contributor trying to learn my way around sim-ln. Helping out with reviews right now and @carlaKC mentioned this one would be good to look at. I tested out using ranges and it works great, this will be a very useful feature. Left a few small comments.

sim-lib/src/lib.rs

sim-lib/src/serializers.rs

sim-lib/src/lib.rs

sr-gi · 2024-05-07T01:22:33Z

CI failure is unrelated

carlaKC · 2024-05-15T18:10:47Z

CI failure is unrelated

Fixed in #186. Also is this ready for a look?

Amounts and intervals in activities can now be fixed values (e.g. 1000) or ranges (e.g. [500, 1000]). If a range is provided, the value will be sampled uniformly at random from it.

sr-gi · 2024-05-16T15:39:40Z

Rebased to make CI green again.

This should be review-ready.

carlaKC

tACK, such a nice feature to add 😻

carlaKC · 2024-05-20T15:27:24Z

sim-lib/src/defined_activity.rs

    use super::DefinedPaymentActivity;
+    use super::*;


nit: can remove DefinedPaymentActivity if we're using *

sr-gi force-pushed the 20231025_payment_range branch from cc19a99 to ec8670c Compare November 2, 2023 18:59

carlaKC mentioned this pull request Nov 17, 2023

refactor: Unify Dispatch of Random and Defined Activity #160

Merged

sr-gi force-pushed the 20231025_payment_range branch from ec8670c to fe7bef6 Compare March 28, 2024 11:42

sr-gi commented Mar 28, 2024

View reviewed changes

sr-gi requested a review from carlaKC March 28, 2024 12:07

carlaKC reviewed Mar 29, 2024

View reviewed changes

sr-gi force-pushed the 20231025_payment_range branch from fe7bef6 to 3cb1443 Compare April 5, 2024 10:14

bjohnson5 reviewed Apr 15, 2024

View reviewed changes

sim-lib/src/lib.rs Show resolved Hide resolved

sim-lib/src/serializers.rs Show resolved Hide resolved

sim-lib/src/lib.rs Outdated Show resolved Hide resolved

sr-gi force-pushed the 20231025_payment_range branch from 3cb1443 to a6c40a4 Compare May 6, 2024 21:05

sim-lib: Allows amount and interval ranges in manual activity definition

f1a3275

Amounts and intervals in activities can now be fixed values (e.g. 1000) or ranges (e.g. [500, 1000]). If a range is provided, the value will be sampled uniformly at random from it.

sr-gi force-pushed the 20231025_payment_range branch from a6c40a4 to f1a3275 Compare May 16, 2024 15:35

carlaKC approved these changes May 20, 2024

View reviewed changes

carlaKC requested a review from bjohnson5 May 20, 2024 15:28

bjohnson5 approved these changes May 20, 2024

View reviewed changes

carlaKC merged commit a9aba5d into bitcoin-dev-project:main May 20, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sim-lib: Allows amount and interval ranges in manual activity definition #153

sim-lib: Allows amount and interval ranges in manual activity definition #153

sr-gi commented Oct 25, 2023

carlaKC commented Oct 26, 2023

sr-gi commented Oct 26, 2023

sr-gi commented Mar 27, 2024

carlaKC commented Mar 27, 2024

sr-gi Mar 28, 2024 •

edited

Loading

carlaKC Mar 29, 2024

sr-gi Apr 5, 2024

carlaKC May 6, 2024

sr-gi May 6, 2024

carlaKC left a comment

carlaKC Mar 29, 2024

bjohnson5 left a comment

sr-gi commented May 7, 2024

carlaKC commented May 15, 2024 •

edited

Loading

sr-gi commented May 16, 2024

carlaKC left a comment

carlaKC May 20, 2024

sim-lib: Allows amount and interval ranges in manual activity definition #153

sim-lib: Allows amount and interval ranges in manual activity definition #153

Conversation

sr-gi commented Oct 25, 2023

carlaKC commented Oct 26, 2023

sr-gi commented Oct 26, 2023

sr-gi commented Mar 27, 2024

carlaKC commented Mar 27, 2024

sr-gi Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

carlaKC Mar 29, 2024

Choose a reason for hiding this comment

sr-gi Apr 5, 2024

Choose a reason for hiding this comment

carlaKC May 6, 2024

Choose a reason for hiding this comment

sr-gi May 6, 2024

Choose a reason for hiding this comment

carlaKC left a comment

Choose a reason for hiding this comment

carlaKC Mar 29, 2024

Choose a reason for hiding this comment

bjohnson5 left a comment

Choose a reason for hiding this comment

sr-gi commented May 7, 2024

carlaKC commented May 15, 2024 • edited Loading

sr-gi commented May 16, 2024

carlaKC left a comment

Choose a reason for hiding this comment

carlaKC May 20, 2024

Choose a reason for hiding this comment

sr-gi Mar 28, 2024 •

edited

Loading

carlaKC commented May 15, 2024 •

edited

Loading