Skip to content
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.

Sign up for GitHub

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

Add a ManagerOptions type to allow for customizable options #102

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
+133 −61
Open

Add a ManagerOptions type to allow for customizable options #102

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c0e3dfb
add a ManagerOptions type
sosaucily Mar 29, 2023
caa0a40
Remove Derivative package and use standard Default trait; change refu…
sosaucily Apr 4, 2023
57af634
removed unused import
sosaucily Apr 4, 2023
891b986
derive copy/clone rather than writing it
sosaucily Apr 4, 2023
6d7fc60
switch refund_delay back to u32 to solve bug
sosaucily Apr 17, 2023
80a21fe
Merge branch 'feat/allow-customizable-num-confirmations' of github.co…
sosaucily Apr 24, 2023
9c393db
add back in consts; use none instead of Some(default)
sosaucily Apr 24, 2023
bb8361f
Merge branch 'master' into feat/allow-customizable-num-confirmations
sosaucily Apr 24, 2023
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 dlc-manager/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use-serde = ["serde", "dlc/use-serde", "dlc-messages/serde", "dlc-trie/use-serde
[dependencies]
async-trait = "0.1.50"
bitcoin = {version = "0.29.2"}
derivative = {version = "2.2.0"}
sosaucily marked this conversation as resolved.
Show resolved Hide resolved
dlc = {version = "0.4.0", path = "../dlc"}
dlc-messages = {version = "0.4.0", path = "../dlc-messages"}
dlc-trie = {version = "0.4.0", path = "../dlc-trie"}
Expand Down
8 changes: 6 additions & 2 deletions dlc-manager/src/channel/offered_channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,11 @@ pub struct OfferedChannel {
}

impl OfferedChannel {
pub(crate) fn get_offer_channel_msg(&self, offered_contract: &OfferedContract) -> OfferChannel {
pub(crate) fn get_offer_channel_msg(
&self,
offered_contract: &OfferedContract,
cet_nsequence: u32,
) -> OfferChannel {
let party_points = &self.party_points;
OfferChannel {
protocol_version: crate::conversion_utils::PROTOCOL_VERSION,
Expand Down Expand Up @@ -72,7 +76,7 @@ impl OfferedChannel {
refund_locktime: offered_contract.refund_locktime,
fee_rate_per_vb: offered_contract.fee_rate_per_vb,
fund_output_serial_id: offered_contract.fund_output_serial_id,
cet_nsequence: crate::manager::CET_NSEQUENCE,
cet_nsequence,
}
}

Expand Down
3 changes: 2 additions & 1 deletion dlc-manager/src/channel_updater.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1490,6 +1490,7 @@ pub fn offer_collaborative_close<C: Signing, S: Deref, T: Deref>(
counter_payout: u64,
signer: &S,
time: &T,
peer_timeout: u64,
) -> Result<(CollaborativeCloseOffer, Transaction), Error>
where
S::Target: Signer,
Expand Down Expand Up @@ -1535,7 +1536,7 @@ where
counter_payout,
offer_signature: close_signature,
close_tx: close_tx.clone(),
timeout: time.unix_time_now() + super::manager::PEER_TIMEOUT,
timeout: time.unix_time_now() + peer_timeout,
};
std::mem::swap(&mut state, &mut signed_channel.state);
signed_channel.roll_back_state = Some(state);
Expand Down
97 changes: 59 additions & 38 deletions dlc-manager/src/manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use crate::Signer;
use crate::{ChannelId, ContractId};
use bitcoin::Address;
use bitcoin::Transaction;
use derivative::Derivative;
use dlc_messages::channel::{
AcceptChannel, CollaborativeCloseOffer, OfferChannel, Reject, RenewAccept, RenewConfirm,
RenewFinalize, RenewOffer, SettleAccept, SettleConfirm, SettleFinalize, SettleOffer,
Expand All @@ -37,15 +38,23 @@ use std::collections::HashMap;
use std::ops::Deref;
use std::string::ToString;

/// The number of confirmations required before moving the the confirmed state.
pub const NB_CONFIRMATIONS: u32 = 6;
/// The delay to set the refund value to.
pub const REFUND_DELAY: u32 = 86400 * 7;
/// The nSequence value used for CETs in DLC channels
pub const CET_NSEQUENCE: u32 = 288;
/// Timeout in seconds when waiting for a peer's reply, after which a DLC channel
/// is forced closed.
pub const PEER_TIMEOUT: u64 = 3600;
/// The options used to configure the DLC manager.
#[derive(Derivative)]
#[derivative(Default)]
pub struct ManagerOptions {
sosaucily marked this conversation as resolved.
Show resolved Hide resolved
/// The number of btc confirmations required before moving the DLC to the confirmed state.
#[derivative(Default(value = "6"))]
pub nb_confirmations: u32,
/// The delay to set the refund value to.
#[derivative(Default(value = "86400 * 7"))]
pub refund_delay: u32,
/// The nSequence value used for CETs in DLC channels
#[derivative(Default(value = "288"))]
pub cet_nsequence: u32,
/// Timeout in seconds when waiting for a peer's reply, after which a DLC channel
#[derivative(Default(value = "3600"))]
pub peer_timeout: u64,
}

type ClosableContractInfo<'a> = Option<(
&'a ContractInfo,
Expand All @@ -71,6 +80,7 @@ where
chain_monitor: ChainMonitor,
time: T,
fee_estimator: F,
options: ManagerOptions,
}

macro_rules! get_object_in_state {
Expand Down Expand Up @@ -175,6 +185,7 @@ where
oracles: HashMap<XOnlyPublicKey, O>,
time: T,
fee_estimator: F,
options: ManagerOptions,
sosaucily marked this conversation as resolved.
Show resolved Hide resolved
) -> Result<Self, Error> {
let init_height = blockchain.get_blockchain_height()?;
Ok(Manager {
Expand All @@ -185,6 +196,7 @@ where
oracles,
time,
fee_estimator,
options,
chain_monitor: ChainMonitor::new(init_height),
})
}
Expand Down Expand Up @@ -284,7 +296,7 @@ where
&self.secp,
contract_input,
oracle_announcements,
REFUND_DELAY,
self.options.refund_delay,
&counter_party,
&self.wallet,
&self.blockchain,
Expand Down Expand Up @@ -344,7 +356,11 @@ where
offered_message: &OfferDlc,
counter_party: PublicKey,
) -> Result<(), Error> {
offered_message.validate(&self.secp, REFUND_DELAY, REFUND_DELAY * 2)?;
offered_message.validate(
&self.secp,
self.options.refund_delay,
self.options.refund_delay * 2,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

)?;
let contract: OfferedContract =
OfferedContract::try_from_offer_dlc(offered_message, counter_party)?;
contract.validate()?;
Expand Down Expand Up @@ -474,7 +490,7 @@ where
let confirmations = self.blockchain.get_transaction_confirmations(
&contract.accepted_contract.dlc_transactions.fund.txid(),
)?;
if confirmations >= NB_CONFIRMATIONS {
if confirmations >= self.options.nb_confirmations {
self.store
.update_contract(&Contract::Confirmed(contract.clone()))?;
}
Expand Down Expand Up @@ -604,7 +620,7 @@ where
let confirmations = self
.blockchain
.get_transaction_confirmations(&broadcasted_txid)?;
if confirmations >= NB_CONFIRMATIONS {
if confirmations >= self.options.nb_confirmations {
let closed_contract = ClosedContract {
attestations: contract.attestations.clone(),
signed_cet: Some(contract.signed_cet.clone()),
Expand Down Expand Up @@ -655,7 +671,7 @@ where
};

return Ok(Contract::PreClosed(preclosed_contract));
} else if confirmations < NB_CONFIRMATIONS {
} else if confirmations < self.options.nb_confirmations {
let preclosed_contract = PreClosedContract {
signed_contract: contract.clone(),
attestations: Some(attestations),
Expand Down Expand Up @@ -733,14 +749,15 @@ where
contract_input,
&counter_party,
&oracle_announcements,
CET_NSEQUENCE,
REFUND_DELAY,
self.options.cet_nsequence,
self.options.refund_delay,
&self.wallet,
&self.blockchain,
&self.time,
)?;

let msg = offered_channel.get_offer_channel_msg(&offered_contract);
let msg =
offered_channel.get_offer_channel_msg(&offered_contract, self.options.cet_nsequence);

self.store.upsert_channel(
Channel::Offered(offered_channel),
Expand Down Expand Up @@ -821,7 +838,7 @@ where
&self.secp,
&mut signed_channel,
counter_payout,
PEER_TIMEOUT,
self.options.peer_timeout,
&self.wallet,
&self.time,
)?;
Expand All @@ -846,9 +863,9 @@ where
let msg = crate::channel_updater::settle_channel_accept(
&self.secp,
&mut signed_channel,
CET_NSEQUENCE,
self.options.cet_nsequence,
0,
PEER_TIMEOUT,
self.options.peer_timeout,
&self.wallet,
&self.time,
)?;
Expand Down Expand Up @@ -885,9 +902,9 @@ where
contract_input,
oracle_announcements,
counter_payout,
REFUND_DELAY,
PEER_TIMEOUT,
CET_NSEQUENCE,
self.options.refund_delay,
self.options.peer_timeout,
self.options.cet_nsequence,
&self.wallet,
&self.time,
)?;
Expand Down Expand Up @@ -926,8 +943,8 @@ where
&self.secp,
&mut signed_channel,
&offered_contract,
CET_NSEQUENCE,
PEER_TIMEOUT,
self.options.cet_nsequence,
self.options.peer_timeout,
&self.wallet,
&self.time,
)?;
Expand Down Expand Up @@ -1014,6 +1031,7 @@ where
counter_payout,
&self.wallet,
&self.time,
self.options.peer_timeout,
)?;

self.chain_monitor.add_tx(
Expand Down Expand Up @@ -1107,7 +1125,7 @@ where
if self
.blockchain
.get_transaction_confirmations(&buffer_tx.txid())?
> CET_NSEQUENCE
> self.options.cet_nsequence
{
let confirmed_contract =
get_contract_in_state!(self, &contract_id, Confirmed, None as Option<PublicKey>)?;
Expand All @@ -1131,10 +1149,10 @@ where
) -> Result<(), Error> {
offer_channel.validate(
&self.secp,
REFUND_DELAY,
REFUND_DELAY * 2,
CET_NSEQUENCE,
CET_NSEQUENCE * 2,
self.options.refund_delay,
self.options.refund_delay * 2,
self.options.cet_nsequence,
self.options.cet_nsequence * 2,
Comment on lines +1184 to +1185
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

)?;

let (channel, contract) = OfferedChannel::from_offer_channel(offer_channel, counter_party)?;
Expand Down Expand Up @@ -1182,7 +1200,7 @@ where
&offered_contract,
accept_channel,
//TODO(tibo): this should be parameterizable.
CET_NSEQUENCE,
self.options.cet_nsequence,
&self.wallet,
);

Expand Down Expand Up @@ -1334,9 +1352,9 @@ where
&self.secp,
&mut signed_channel,
settle_accept,
CET_NSEQUENCE,
self.options.cet_nsequence,
0,
PEER_TIMEOUT,
self.options.peer_timeout,
&self.wallet,
&self.time,
)?;
Expand Down Expand Up @@ -1536,8 +1554,8 @@ where
renew_accept,
&mut signed_channel,
&offered_contract,
CET_NSEQUENCE,
PEER_TIMEOUT,
self.options.cet_nsequence,
self.options.peer_timeout,
&self.wallet,
&self.time,
)?;
Expand Down Expand Up @@ -1790,7 +1808,7 @@ where
crate::channel_updater::on_collaborative_close_offer(
&mut signed_channel,
close_offer,
PEER_TIMEOUT,
self.options.peer_timeout,
&self.time,
)?;

Expand Down Expand Up @@ -2020,7 +2038,7 @@ where
&counter_revocation_sk,
&tx,
&self.wallet.get_new_address()?,
CET_NSEQUENCE,
self.options.cet_nsequence,
0,
fee_rate_per_vb,
is_offer,
Expand Down Expand Up @@ -2189,7 +2207,7 @@ where
mod test {
use dlc_messages::Message;
use mocks::{
dlc_manager::{manager::Manager, Oracle},
dlc_manager::{manager::Manager, manager::ManagerOptions, Oracle},
memory_storage_provider::MemoryStorage,
mock_blockchain::MockBlockchain,
mock_oracle_provider::MockOracle,
Expand All @@ -2199,6 +2217,8 @@ mod test {
use secp256k1_zkp::PublicKey;
use std::{collections::HashMap, rc::Rc};

// use super::ManagerOptions;

sosaucily marked this conversation as resolved.
Show resolved Hide resolved
type TestManager = Manager<
Rc<MockWallet>,
Rc<MockBlockchain>,
Expand Down Expand Up @@ -2229,6 +2249,7 @@ mod test {
oracles,
time,
blockchain.clone(),
ManagerOptions::default(),
)
.unwrap()
}
Expand Down
Loading