add NWC commands #1152

elnosh · 2024-04-21T23:16:49Z

for #1120 added multi_pay_invoice, pay_keysend, multi_pay_keysend and list_transactions NWC commands. I added those requests to the mutinynet faucet to help me test these from the UI.

Reasoning for certain changes I did:

I changed the return type and place where we were broadcasting the response events from the NWC requests. Discussed with @benthecarman and did this because some of the new commands (multi_pay_invoice and multi_pay_keysend) can return multiple events. So instead of waiting to return them until start_nostr method, now broadcasting in the respective handle methods where the response events are generated.
changed the type for add_payment and remove_payment because they were previously taking a Bolt11Invoice so I changed it to TrackedPayment to now be able to accommodate adding keysend payments as well instead of only invoices.
added an optional preimage param to the keysend payment method. This is to generate the preimage deterministically from the request event for the nwc keysend payment to avoid paying the same keysend twice if we try to process the same event.
added some methods to the InvoiceHandler trait that I needed for the new commands.

benthecarman

overall looking good

benthecarman · 2024-04-22T17:53:06Z

mutiny-core/src/nostr/nwc.rs

+        nostr_manager: &NostrManager<S, P, C>,
+    ) -> anyhow::Result<Option<NwcResponse>> {


for all of these you're passing in the NostrManager which forces you to put all the type params and stuff but you only end up calling client.send_event, would be better to just pass in the nostr client

ahh yeah, thanks

benthecarman · 2024-04-22T17:56:11Z

mutiny-core/src/nostr/nwc.rs

+            let incoming = match params.transaction_type.unwrap() {
+                TransactionType::Incoming => true,
+                TransactionType::Outgoing => false,
+            };


this can be simplified to let incoming = params.transaction_type.unwrap() == TransactionType::Incoming

benthecarman · 2024-04-22T18:34:28Z

mutiny-core/src/nostr/nwc.rs

                };
+                let preimage = PaymentPreimage(Sha256::hash(input.as_bytes()).into_32());


We need to incorporate some sort of secret here so its not derivable from public data, should also hash in the server private key for this nwc.

mutiny-core/src/nostr/nwc.rs

benthecarman · 2024-04-22T19:45:44Z

mutiny-core/src/nostr/nwc.rs

+                        code: ErrorCode::PaymentFailed,
+                        message: String::from("not enough balance to make payment"),


I think it'd be better to do an UNAUTHORIZED here with a better error string about the budget

what error message do you think would be good here?

"Keysend only supported for profiles with a budget"

benthecarman · 2024-04-22T19:46:36Z

mutiny-core/src/nostr/nwc.rs

+                            .unwrap_or(self.profile.name.clone());
+                        // attempt keysend payment now
+                        match node
+                            .keysend(to_node_pubkey, sats, None, vec![label], Some(preimage.0))


you're never doing anything with the TLV records will need to add that to the keysend function too

@TonyGiorgio passing that optional preimage here

@benthecarman did some changes to include the TLV records. I changed the previous message param and passing new Vec<CustomTLV>. This would require changes in the frontend tho. I saw the frontend was not passing a message anyways so it would be changing from undefined to an empty array. Let me know what you think

TonyGiorgio · 2024-04-22T16:11:30Z

mutiny-core/src/node.rs

@@ -1617,6 +1617,7 @@ impl<S: MutinyStorage> Node<S> {
        message: Option<String>,
        labels: Vec<String>,
        payment_id: PaymentId,
+        preimage: Option<[u8; 32]>,


This is added but it's never used. I don't know why it would be useful.

this is used from the NWC stuff, allows for specifying a preimage for the keysend.

We need it for ourself as well so we don't generate a completely random preimage for the event, need to do it deterministically so if we process the event multiple times we won't send the payment multiple times

I'm generating the preimage deterministically from the nwc request event in the handle_nwc_keysend_payment method and passing it as an option. Got from @benthecarman that this would be needed to avoid making another keysend payment for the same event in case we try to process that same request event more than once.

whoops, saw your comment after posted mine lol

I looked and didn't see it. Where is it?

setting that optional preimage here https://github.com/elnosh/mutiny-node/blob/nwc-commands/mutiny-core/src/nostr/nwc.rs#L1258-L1269

and passing it here https://github.com/elnosh/mutiny-node/blob/nwc-commands/mutiny-core/src/nostr/nwc.rs#L1332-L1334

elnosh · 2024-04-23T18:13:04Z

thanks for the feedback. Added some changes based on it

benthecarman

few nits, and hoping you can improve the tests a bit

benthecarman · 2024-04-23T18:11:50Z

mutiny-core/src/node.rs

+                .into_iter()
+                .map(|tlv| (tlv.tlv_type, tlv.value.encode()))
+                .collect();
+
            RecipientOnionFields::secret_only(payment_secret)


I think we need to change this to spontaneous_empty as well, probably will get the same error as before

benthecarman · 2024-04-23T18:12:26Z

mutiny-core/src/node.rs

            RecipientOnionFields::secret_only(payment_secret)
-                .with_custom_tlvs(vec![(34349334, msg.encode())])
+                .with_custom_tlvs(custom_tlvs)
                .map_err(|_| {
                    log_error!(self.logger, "could not encode keysend message");
                    MutinyError::InvoiceCreationFailed


can you make this InvalidArgumentsError, that'd be more fitting here

benthecarman · 2024-04-23T20:11:40Z

mutiny-core/src/nostr/nwc.rs

+            SpendingConditions::Budget(mut budget) => {
+                // generate deterministic preimage for keysend payment from event info
+                let preimage = match params.preimage {
+                    Some(preimage) => PaymentPreimage(Sha256::hash(preimage.as_bytes()).into_32()),


shouldn't hash the preimage here, we should be using what they gave us

benthecarman · 2024-04-23T20:23:14Z

mutiny-core/src/nostr/nwc.rs

+        if params.limit.is_some() {
+            let limit = params.limit.unwrap() as usize;
+            if limit - 1 < transactions.len() {
+                transactions = transactions[..limit].to_vec();
+            }
+        }
+        if params.offset.is_some() {
+            let offset = params.offset.unwrap() as usize;
+            if offset - 1 < transactions.len() {
+                transactions = transactions[offset..].to_vec();
+            }
+        }


we have similar logic here: https://github.com/MutinyWallet/mutiny-node/blob/master/mutiny-core/src/lib.rs#L1805

i would just copy this logic, should probably have tests around this as well

benthecarman · 2024-04-23T21:05:42Z

mutiny-core/src/nostr/nwc.rs

+        node.expect_keysend()
+            .once()
+            .returning(move |_, _, _, _, preimage| {
+                let payment_preimage = PaymentPreimage(preimage.unwrap());


should assert the preimage, pubkey, and amount are what we expect

TonyGiorgio · 2024-04-24T16:45:06Z

I don't think we should support any of these commands. Sorry for the wasted effort but a strong NACK from me.

elnosh · 2024-04-24T16:51:52Z

I don't think we should support any of these commands. Sorry for the wasted effort but a strong NACK from me.

no worries! got to learn about NWC and how that part of Mutiny works, so not a waste at all

elnosh force-pushed the nwc-commands branch from 39287c4 to 4760ad1 Compare April 22, 2024 16:00

benthecarman linked an issue Apr 22, 2024 that may be closed by this pull request

Add missing NWC commands #1120

Closed

4 tasks

benthecarman requested changes Apr 22, 2024

View reviewed changes

benthecarman added the nostr label Apr 22, 2024

TonyGiorgio requested changes Apr 22, 2024

View reviewed changes

elnosh force-pushed the nwc-commands branch from 4760ad1 to eab3ddc Compare April 23, 2024 17:56

benthecarman requested changes Apr 23, 2024

View reviewed changes

elnosh force-pushed the nwc-commands branch from eab3ddc to d9664b4 Compare April 24, 2024 03:52

add multi_pay_invoice, pay_keysend and multi_pay_keysend nwc commands

00078be

elnosh force-pushed the nwc-commands branch 2 times, most recently from a382b4b to f855a0a Compare April 24, 2024 04:40

add list_transactions nwc command

f507b8a

elnosh force-pushed the nwc-commands branch from f855a0a to f507b8a Compare April 24, 2024 15:14

elnosh closed this Apr 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add NWC commands #1152

add NWC commands #1152

elnosh commented Apr 21, 2024

benthecarman left a comment

benthecarman Apr 22, 2024

elnosh Apr 22, 2024

benthecarman Apr 22, 2024

benthecarman Apr 22, 2024

benthecarman Apr 22, 2024

elnosh Apr 22, 2024

benthecarman Apr 22, 2024

benthecarman Apr 22, 2024

elnosh Apr 22, 2024

elnosh Apr 23, 2024

TonyGiorgio Apr 22, 2024

benthecarman Apr 22, 2024

elnosh Apr 22, 2024

elnosh Apr 22, 2024

TonyGiorgio Apr 22, 2024

elnosh Apr 23, 2024

elnosh commented Apr 23, 2024

benthecarman left a comment

benthecarman Apr 23, 2024

benthecarman Apr 23, 2024

benthecarman Apr 23, 2024

benthecarman Apr 23, 2024

benthecarman Apr 23, 2024

TonyGiorgio commented Apr 24, 2024

elnosh commented Apr 24, 2024

		nostr_manager: &NostrManager<S, P, C>,
		) -> anyhow::Result<Option<NwcResponse>> {

		};
		let preimage = PaymentPreimage(Sha256::hash(input.as_bytes()).into_32());

		code: ErrorCode::PaymentFailed,
		message: String::from("not enough balance to make payment"),

add NWC commands #1152

add NWC commands #1152

Conversation

elnosh commented Apr 21, 2024

benthecarman 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elnosh commented Apr 23, 2024

benthecarman 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

TonyGiorgio commented Apr 24, 2024

elnosh commented Apr 24, 2024