Add a ManagerOptions type to allow for customizable options #102

sosaucily · 2023-03-29T12:34:50Z

Added a ManagerOptions type with defaults, to let the constants of the DLCManager be configurable.

Open to design ideas!

Tibo-lg

Thanks, looks good, my main concern is the added dependency otherwise mainly nits.

dlc-manager/Cargo.toml

dlc-manager/src/manager.rs

Tibo-lg · 2023-03-30T11:32:45Z

dlc-manager/src/manager.rs

+            self.options.refund_delay,
+            self.options.refund_delay * 2,


I guess you might as well have two options rather than forcing the *2 to be more flexible? You'd probably need to check that max > min though.

Sure, i'll make that change. More flexibility is always good.

@Tibo-lg What is the intention of having a minRefundDelay and a max? What does it do to have those bounds, rather than just 1 value and making the refund after that time?

I created a type called RefundDelayWindow. Tell me what you think.

Tibo-lg · 2023-03-30T11:33:47Z

dlc-manager/src/manager.rs

+            self.options.cet_nsequence,
+            self.options.cet_nsequence * 2,


So I set about trying to make this change in the same way I did for refund_delay, but it became a huge rats nest. I created a struct called CETnSequenceWindow, but I couldn't import it from dlc-manager in dlc-messages because of a circular dependency. Then I tried just using a cet_nsequence_min and cet_nsequence_max, and handing those all the way down, but that also became really messy because that sometimes it expects just the "min" value, and it gets really confusing.

What do you think?

I'm not sure I understand why you need to use that in dlc-messages?

Ah, I think I was trying to pass the struct with the min/max values too far down , when I could have just been passing the u32.

Anyway, do you think it would be good to have a CETnSequenceWindow object with a min / max values, or just leave it as a u32 with default 288?

As mentioned above I feel like not having a separate object is simpler as in I think the max value is only required in few places.

dlc-manager/src/manager.rs

Tibo-lg · 2023-03-30T11:36:11Z

dlc-manager/tests/channel_execution_tests.rs

        )
        .unwrap(),
    ));

+    let cet_nsequence: u32 = ManagerOptions::default().cet_nsequence;


Seeing this I feel it might be better to preserve the const values (maybe with a DEFAULT prefix)

I'm not sure what you mean here. I see that cet_nsequence used to be a CONST, and now because I'm grabbing this off the struct I can only store it as a let. Can you give me an example? I'm not sure what you mean by using the DEFAULT prefix. Thanks

What I meant was that you could keep the const and use them to initialize the default values of the struct, and make them accessible so that someone using the default values doesn't have to initialize a struct for nothing just to access the default values.

Ah, I understand now. Yes, updated

…nd_delay to a struct with a min and max

Tibo-lg · 2023-04-24T02:07:05Z

dlc-manager/tests/manager_execution_tests.rs

@@ -502,6 +502,7 @@ fn manager_execution_test(test_params: TestParams, path: TestPath) {
            alice_oracles,
            Arc::clone(&mock_time),
            Arc::clone(&electrs),
+            Some(ManagerOptions::default()),


You might as well pass None

…m:dlc-link/rust-dlc into feat/allow-customizable-num-confirmations

sosaucily · 2023-04-24T23:47:43Z

OK @Tibo-lg I think it's ready for review again. I didn't make a Window object for CETnSequence, but I left the window struct for the refund times. Can you elaborate for me why there is a window for refund, rather than just a single value?

Tibo-lg

Thinking about this more, I feel it might actually be best to have three values for both cet_nsequence and refund. The reason is because you need:

one that you use when you are offering a contract
a min and a max value for when you receive an offer, to reject contracts that have values outside of the range you define. For example, if the refund value is too short, your counterparty might be able to use the refund before you have time to close the contract (and for the nsequence they might be able to cheat by using an old state without you having enough time to react to publish the punishment transaction).

Tibo-lg · 2023-04-25T03:35:32Z

dlc-manager/src/manager.rs

@@ -37,16 +37,54 @@ use std::collections::HashMap;
 use std::ops::Deref;
 use std::string::ToString;

-/// The number of confirmations required before moving the the confirmed state.
+/// The number of btc confirmations required before moving the DLC to the confirmed state.


Why change this comment, I'm not sure "btc confirmations" makes so much sense.

Oh I think github copilot did that, I can remove it.

Tibo-lg · 2023-04-25T03:36:17Z

dlc-manager/src/manager.rs

+
+/// The min and max value in seconds until a DLC will be refunded.
+#[derive(Clone, Copy)]
+pub struct RefundDelayWindow {


Looking through the code I'm not sure this is really useful, in some places you end up passing that when only one of the value is required.

Tibo-lg · 2023-04-25T03:38:39Z

dlc-manager/src/manager.rs

+            self.options.cet_nsequence,
+            self.options.cet_nsequence * 2,


As mentioned above I feel like not having a separate object is simpler as in I think the max value is only required in few places.

Tibo-lg · 2023-04-25T03:41:19Z

dlc-manager/tests/channel_execution_tests.rs

        )
        .unwrap(),
    ));

+    let cet_nsequence: u32 = dlc_manager::manager::CET_NSEQUENCE;


If you just use default value it feels like most of the changes in this file are unnecessary?

I'm not sure I really understand what you mean by the 3 values of refund time and nsequence, and I'm not sure how they would be handled in the code.

If you don't mind, would it be OK if I scale this PR back to only be accepting the NUM_CONFIRMATIONS as a configurable variable? That's the only thing I actually wanted, and I feel like I'm starting to bite off more than I can chew.

Or feel free to jump in and make commits on this PR if you'd like.

add a ManagerOptions type

c0e3dfb

Tibo-lg requested changes Mar 30, 2023

View reviewed changes

sosaucily added 4 commits April 4, 2023 14:25

Remove Derivative package and use standard Default trait; change refu…

caa0a40

…nd_delay to a struct with a min and max

removed unused import

57af634

derive copy/clone rather than writing it

891b986

switch refund_delay back to u32 to solve bug

6d7fc60

Tibo-lg reviewed Apr 24, 2023

View reviewed changes

sosaucily added 3 commits April 24, 2023 16:14

Merge branch 'feat/allow-customizable-num-confirmations' of github.co…

80a21fe

…m:dlc-link/rust-dlc into feat/allow-customizable-num-confirmations

add back in consts; use none instead of Some(default)

9c393db

Merge branch 'master' into feat/allow-customizable-num-confirmations

bb8361f

Tibo-lg reviewed Apr 25, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a ManagerOptions type to allow for customizable options #102

Add a ManagerOptions type to allow for customizable options #102

sosaucily commented Mar 29, 2023 •

edited

Loading

Tibo-lg left a comment

Tibo-lg Mar 30, 2023

sosaucily Apr 4, 2023

sosaucily Apr 4, 2023

Tibo-lg Mar 30, 2023

sosaucily Apr 4, 2023

Tibo-lg Apr 24, 2023

sosaucily Apr 24, 2023

Tibo-lg Apr 25, 2023

Tibo-lg Mar 30, 2023

sosaucily Apr 4, 2023

Tibo-lg Apr 24, 2023

sosaucily Apr 24, 2023

Tibo-lg Apr 24, 2023

sosaucily Apr 24, 2023

sosaucily commented Apr 24, 2023

Tibo-lg left a comment

Tibo-lg Apr 25, 2023

sosaucily Apr 27, 2023

Tibo-lg Apr 25, 2023

Tibo-lg Apr 25, 2023

Tibo-lg Apr 25, 2023

sosaucily Apr 27, 2023

sosaucily Apr 27, 2023

Add a ManagerOptions type to allow for customizable options #102

Are you sure you want to change the base?

Add a ManagerOptions type to allow for customizable options #102

Conversation

sosaucily commented Mar 29, 2023 • edited Loading

Tibo-lg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sosaucily commented Apr 24, 2023

Tibo-lg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sosaucily commented Mar 29, 2023 •

edited

Loading