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 invoice constructor with custom payment hash #1894

Merged
merged 1 commit into from
Dec 13, 2022
Merged

Add invoice constructor with custom payment hash #1894

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
103 changes: 82 additions & 21 deletions lightning-invoice/src/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,12 +294,12 @@ pub fn create_invoice_from_channelmanager_with_description_hash_and_duration_sin
network: Currency, amt_msat: Option<u64>, description_hash: Sha256,
duration_since_epoch: Duration, invoice_expiry_delta_secs: u32
) -> Result<Invoice, SignOrCreationError<()>>
where
M::Target: chain::Watch<<K::Target as KeysInterface>::Signer>,
T::Target: BroadcasterInterface,
K::Target: KeysInterface,
F::Target: FeeEstimator,
L::Target: Logger,
where
M::Target: chain::Watch<<K::Target as KeysInterface>::Signer>,
T::Target: BroadcasterInterface,
K::Target: KeysInterface,
F::Target: FeeEstimator,
L::Target: Logger,
{
_create_invoice_from_channelmanager_and_duration_since_epoch(
channelmanager, keys_manager, logger, network, amt_msat,
Expand All @@ -316,12 +316,12 @@ pub fn create_invoice_from_channelmanager_and_duration_since_epoch<M: Deref, T:
network: Currency, amt_msat: Option<u64>, description: String, duration_since_epoch: Duration,
invoice_expiry_delta_secs: u32
) -> Result<Invoice, SignOrCreationError<()>>
where
M::Target: chain::Watch<<K::Target as KeysInterface>::Signer>,
T::Target: BroadcasterInterface,
K::Target: KeysInterface,
F::Target: FeeEstimator,
L::Target: Logger,
where
M::Target: chain::Watch<<K::Target as KeysInterface>::Signer>,
T::Target: BroadcasterInterface,
K::Target: KeysInterface,
F::Target: FeeEstimator,
L::Target: Logger,
{
_create_invoice_from_channelmanager_and_duration_since_epoch(
channelmanager, keys_manager, logger, network, amt_msat,
Expand All @@ -337,18 +337,62 @@ fn _create_invoice_from_channelmanager_and_duration_since_epoch<M: Deref, T: Der
network: Currency, amt_msat: Option<u64>, description: InvoiceDescription,
duration_since_epoch: Duration, invoice_expiry_delta_secs: u32
) -> Result<Invoice, SignOrCreationError<()>>
where
M::Target: chain::Watch<<K::Target as KeysInterface>::Signer>,
T::Target: BroadcasterInterface,
K::Target: KeysInterface,
F::Target: FeeEstimator,
L::Target: Logger,
where
M::Target: chain::Watch<<K::Target as KeysInterface>::Signer>,
T::Target: BroadcasterInterface,
K::Target: KeysInterface,
F::Target: FeeEstimator,
L::Target: Logger,
{
// `create_inbound_payment` only returns an error if the amount is greater than the total bitcoin
// supply.
let (payment_hash, payment_secret) = channelmanager
.create_inbound_payment(amt_msat, invoice_expiry_delta_secs)
.map_err(|()| SignOrCreationError::CreationError(CreationError::InvalidAmount))?;
_create_invoice_from_channelmanager_and_duration_since_epoch_with_payment_hash(
channelmanager, keys_manager, logger, network, amt_msat, description, duration_since_epoch, invoice_expiry_delta_secs, payment_hash, payment_secret)
}

/// See [`create_invoice_from_channelmanager_and_duration_since_epoch`]
/// This version allows for providing a custom [`PaymentHash`] for the invoice.
/// This may be useful if you're building an on-chain swap or involving another protocol where
/// the payment hash is also involved outside the scope of lightning.
pub fn create_invoice_from_channelmanager_and_duration_since_epoch_with_payment_hash<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

All public methods are required to have documentation - this is causing CI to fail.

channelmanager: &ChannelManager<M, T, K, F, L>, keys_manager: K, logger: L,
network: Currency, amt_msat: Option<u64>, description: String, duration_since_epoch: Duration,
invoice_expiry_delta_secs: u32, payment_hash: PaymentHash
) -> Result<Invoice, SignOrCreationError<()>>
where
M::Target: chain::Watch<<K::Target as KeysInterface>::Signer>,
T::Target: BroadcasterInterface,
K::Target: KeysInterface,
F::Target: FeeEstimator,
L::Target: Logger,
{
let payment_secret = channelmanager
.create_inbound_payment_for_hash(payment_hash,amt_msat, invoice_expiry_delta_secs)
.map_err(|()| SignOrCreationError::CreationError(CreationError::InvalidAmount))?;
_create_invoice_from_channelmanager_and_duration_since_epoch_with_payment_hash(
channelmanager, keys_manager, logger, network, amt_msat,
InvoiceDescription::Direct(
&Description::new(description).map_err(SignOrCreationError::CreationError)?,
),
duration_since_epoch, invoice_expiry_delta_secs, payment_hash, payment_secret
)
Comment on lines +375 to +381
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking neat!

}

fn _create_invoice_from_channelmanager_and_duration_since_epoch_with_payment_hash<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should be able to DRY this up a lot by having _create_invoice_from_channelmanager_and_duration_since_epoch generate the payment hash/secret and then simply call this method. For git-diff-cleanliness you should probably then move _create_invoice_from_channelmanager_and_duration_since_epoch above this function.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for DRYing this is up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TheBlueMatt , do you mean change _create_invoice_from_channelmanager_and_duration_since_epoch so that it also takes a payment_hash and have it also construct the payment_secret inside (as suggested in your other comment). That way, my main function calls this method instead of my new one?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can split up the functions any way you want, but we'll need one util method that takes the payment hash + secret and builds the invoice (doing most of the work) and one like the existing util method that builds its own payment hash + secret. The method you're adding will need to take only the payment hash, building the payment secret itself and then delegating to the common big util function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

im sorry, I am having trouble understanding this request. I am relatively new to this. Can someone help me implement this? I believe I have solved the other problems here, will push when I do this last part.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So compared to your current code, you can replace _create_invoice_from_channelmanager_and_duration_since_epoch with, simply,

    // `create_inbound_payment` only returns an error if the amount is greater than the total bitcoin
    // supply.
    let (payment_hash, payment_secret) = channelmanager
        .create_inbound_payment(amt_msat, invoice_expiry_delta_secs)
        .map_err(|()| SignOrCreationError::CreationError(CreationError::InvalidAmount))?;
+       _create_invoice_from_channelmanager_and_duration_since_epoch_with_payment_hash(channelmanager,
+               keys_manager, logger, network, amt_msat, description, duration_since_epoch,
+               invoice_expiry_delta_secs, payment_hash, payment_secret)

If you do that, and swap the order of that function and the _with_payment_hash variant that you added the total diff in the patch will be substantially smaller.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you! I hope this new commit fixes this as envisioned and addresses the other comments here.

channelmanager: &ChannelManager<M, T, K, F, L>, keys_manager: K, logger: L,
network: Currency, amt_msat: Option<u64>, description: InvoiceDescription, duration_since_epoch: Duration,
invoice_expiry_delta_secs: u32, payment_hash: PaymentHash, payment_secret: PaymentSecret
) -> Result<Invoice, SignOrCreationError<()>>
where
M::Target: chain::Watch<<K::Target as KeysInterface>::Signer>,
T::Target: BroadcasterInterface,
K::Target: KeysInterface,
F::Target: FeeEstimator,
L::Target: Logger,
{
let our_node_pubkey = channelmanager.get_our_node_id();
let channels = channelmanager.list_channels();

Expand Down Expand Up @@ -567,7 +611,7 @@ where
mod test {
use core::time::Duration;
use crate::{Currency, Description, InvoiceDescription};
use bitcoin_hashes::Hash;
use bitcoin_hashes::{Hash, sha256};
use bitcoin_hashes::sha256::Hash as Sha256;
use lightning::chain::keysinterface::PhantomKeysManager;
use lightning::ln::{PaymentPreimage, PaymentHash};
Expand Down Expand Up @@ -665,6 +709,25 @@ mod test {
assert_eq!(invoice.description(), InvoiceDescription::Hash(&crate::Sha256(Sha256::hash("Testing description_hash".as_bytes()))));
}

#[test]
fn test_create_invoice_from_channelmanager_and_duration_since_epoch_with_payment_hash() {
let chanmon_cfgs = create_chanmon_cfgs(2);
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
let payment_hash = PaymentHash([0; 32]);
let payment_secret = &nodes[1].node.create_inbound_payment_for_hash(payment_hash, Some(10_000), 3600);
let invoice = crate::utils::create_invoice_from_channelmanager_and_duration_since_epoch_with_payment_hash(
&nodes[1].node, nodes[1].keys_manager, nodes[1].logger, Currency::BitcoinTestnet,
Some(10_000), "test".to_string(), Duration::from_secs(1234567), 3600,
payment_hash
).unwrap();
assert_eq!(invoice.amount_pico_btc(), Some(100_000));
assert_eq!(invoice.min_final_cltv_expiry(), MIN_FINAL_CLTV_EXPIRY as u64);
assert_eq!(invoice.description(), InvoiceDescription::Direct(&Description("test".to_string())));
assert_eq!(invoice.payment_hash(), &sha256::Hash::from_slice(&payment_hash.0[..]).unwrap());
}

#[test]
fn test_hints_includes_single_channels_to_nodes() {
let chanmon_cfgs = create_chanmon_cfgs(3);
Expand All @@ -691,10 +754,8 @@ mod test {
let _chan_1_0_low_inbound_capacity = create_unannounced_chan_between_nodes_with_value(&nodes, 1, 0, 100_000, 0, channelmanager::provided_init_features(), channelmanager::provided_init_features());
let chan_1_0_high_inbound_capacity = create_unannounced_chan_between_nodes_with_value(&nodes, 1, 0, 10_000_000, 0, channelmanager::provided_init_features(), channelmanager::provided_init_features());
let _chan_1_0_medium_inbound_capacity = create_unannounced_chan_between_nodes_with_value(&nodes, 1, 0, 1_000_000, 0, channelmanager::provided_init_features(), channelmanager::provided_init_features());

let mut scid_aliases = HashSet::new();
scid_aliases.insert(chan_1_0_high_inbound_capacity.0.short_channel_id_alias.unwrap());

match_invoice_routes(Some(5000), &nodes[0], scid_aliases);
Comment on lines -694 to 759
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Can also undo these unrelated changes.

}

Expand Down