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.

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 more properties to ChannelConfig from LDK original struct #165

Merged
merged 1 commit into from
Oct 23, 2023

Conversation

jbesraa
Copy link
Contributor

@jbesraa jbesraa commented Sep 27, 2023

resolves #163
Adds: counterparty_unspendable_punishment_reserve,
counterparty_outbound_htlc_minimum_msat
counterparty_outbound_htlc_maximum_msat
counterparty_forwarding_info_fee_base_msat
counterparty_forwarding_info_fee_proportional_millionths
counterparty_forwarding_info_cltv_expiry_delta
next_outbound_htlc_limit_msat
next_outbound_htlc_minimum_msat
force_close_spend_delay
inbound_htlc_minimum_msat
inbound_htlc_maximum_msat
config

src/types.rs Show resolved Hide resolved
@jbesraa jbesraa force-pushed the add-props-to-ChannelConfig branch 2 times, most recently from f19c9d1 to 8ba527b Compare September 27, 2023 10:14
@jbesraa
Copy link
Contributor Author

jbesraa commented Sep 27, 2023

@tnull how do I fix the Arc<ChannelConfig> thingy in the uniffi? should I edit the LDKNode.swift file?

Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Thanks!

src/types.rs Outdated
/// The smallest value HTLC (in msat) the remote peer will accept, for this channel. This field
/// is only `None` before we have received either the `OpenChannel` or `AcceptChannel` message
/// from the remote peer, or for `ChannelCounterparty` objects serialized prior to LDK 0.0.107.
pub counterparty_outbound_htlc_minimum_msat: Option<u64>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

As LDK Node 0.1 depends on 0.0.115, we can unwrap all these Options here and below that are only None for versions prior to that, i.e., make this u64 and remove the corresponding part of the comment.

src/types.rs Outdated
pub counterparty_forwarding_info_fee_base_msat: Option<u32>,
/// Amount in millionths of a satoshi the channel will charge per transferred satoshi.
pub counterparty_forwarding_info_fee_proportional_millionths: Option<u32>,
/// The minimum difference in cltv_expiry between an ingoing HTLC and its outgoing counterpart,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// The minimum difference in cltv_expiry between an ingoing HTLC and its outgoing counterpart,
/// The minimum difference in CLTV expiry between an ingoing HTLC and its outgoing counterpart,

src/types.rs Outdated
/// Amount in millionths of a satoshi the channel will charge per transferred satoshi.
pub counterparty_forwarding_info_fee_proportional_millionths: Option<u32>,
/// The minimum difference in cltv_expiry between an ingoing HTLC and its outgoing counterpart,
/// such that the outgoing HTLC is forwardable to this counterparty. See `msgs::ChannelUpdate`'s
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's drop the "See ..." sentence as it doesn't make sense in the LDK Node context.

src/types.rs Outdated
///
/// This value will be `None` for outbound channels until the counterparty accepts the channel.
pub force_close_spend_delay: Option<u16>,
/// The stage of the channel's shutdown.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This and the next line belong to another field and should be removed.

src/types.rs Outdated
/// `None` for `ChannelDetails` serialized on LDK versions prior to 0.0.116.
/// The smallest value HTLC (in msat) we will accept, for this channel. This field
/// is only `None` for `ChannelDetails` objects serialized prior to LDK 0.0.107
pub inbound_htlc_minimum_msat: Option<u64>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

As noted above, this is another case which we can make u64.

src/types.rs Outdated
/// Set of configurable parameters that affect channel operation.
///
/// This field is only `None` for `ChannelDetails` objects serialized prior to LDK 0.0.109.
pub config: Option<ChannelConfig>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately we need to make this an Arc<ChannelConfig> to accommodate bindings.

src/types.rs Outdated
.counterparty
.forwarding_info
.as_ref()
.and_then(|f| Some(f.fee_base_msat)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: We can just use map instead of and_then(.. Some(..)) here and elsewhere.

@tnull
Copy link
Collaborator

tnull commented Sep 27, 2023

@tnull how do I fix the Arc<ChannelConfig> thingy in the uniffi? should I edit the LDKNode.swift file?

No, unfortunately we just need to define the field as Arc<ChannelConfig>, see #165 (comment).

src/types.rs Show resolved Hide resolved
src/types.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

One doc nit, otherwise LGTM!

src/types.rs Show resolved Hide resolved
src/types.rs Outdated
pub counterparty_outbound_htlc_maximum_msat: Option<u64>,
/// Base routing fee in millisatoshis.
pub counterparty_forwarding_info_fee_base_msat: Option<u32>,
/// Amount in millionths of a satoshi the channel will charge per transferred satoshi.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit:

Suggested change
/// Amount in millionths of a satoshi the channel will charge per transferred satoshi.
/// Proportional fee, in millionths of a satoshi, the channel will charge per transferred satoshi.

@jbesraa jbesraa force-pushed the add-props-to-ChannelConfig branch 6 times, most recently from 88c90c8 to 3d3fb22 Compare October 21, 2023 08:38
 Adds: `counterparty_unspendable_punishment_reserve`,
	`counterparty_outbound_htlc_minimum_msat`
	`counterparty_outbound_htlc_maximum_msat`
	`counterparty_forwarding_info_fee_base_msat`
	`counterparty_forwarding_info_fee_proportional_millionths`
	`counterparty_forwarding_info_cltv_expiry_delta`
	`next_outbound_htlc_limit_msat`
	`next_outbound_htlc_minimum_msat`
	`force_close_spend_delay`
        `inbound_htlc_minimum_msat`
	`inbound_htlc_maximum_msat`
	`config`
@tnull tnull merged commit d9a8c8a into lightningdevkit:main Oct 23, 2023
9 checks passed
@jbesraa jbesraa deleted the add-props-to-ChannelConfig branch October 24, 2023 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ChannelDetails is missing properties from original
2 participants