lightningd: deprecate msatoshi in sendpay route.

We should be using amount_msat always. Many tests were not. Plus, deprecating it simplifies the code. Signed-off-by: Rusty Russell <[email protected]> Changelog-Deprecated: JSONRPC: `sendpay` `route` elements `msatoshi` (use `amount_msat`)
richur · Jun 20, 2022 · 08d5776 · 08d5776
1 parent c3efba1
commit 08d5776
Show file tree

Hide file tree

Showing 10 changed files with 50 additions and 73 deletions.
diff --git a/cln-grpc/proto/node.proto b/cln-grpc/proto/node.proto
@@ -322,7 +322,7 @@ message SendpayResponse {
 }
 
 message SendpayRoute {
-	optional Amount amount_msat = 5;
+	Amount amount_msat = 5;
 	bytes id = 2;
 	uint32 delay = 3;
 	string channel = 4;

diff --git a/cln-grpc/src/convert.rs b/cln-grpc/src/convert.rs
@@ -980,7 +980,7 @@ impl From<&pb::ListfundsRequest> for requests::ListfundsRequest {
 impl From<&pb::SendpayRoute> for requests::SendpayRoute {
     fn from(c: &pb::SendpayRoute) -> Self {
         Self {
-            amount_msat: c.amount_msat.as_ref().map(|a| a.into()), // Rule #1 for type msat?
+            amount_msat: c.amount_msat.as_ref().unwrap().into(), // Rule #1 for type msat
             id: cln_rpc::primitives::Pubkey::from_slice(&c.id).unwrap(), // Rule #1 for type pubkey
             delay: c.delay as u16, // Rule #1 for type u16
             channel: cln_rpc::primitives::ShortChannelId::from_str(&c.channel).unwrap(), // Rule #1 for type short_channel_id

diff --git a/cln-rpc/src/model.rs b/cln-rpc/src/model.rs
@@ -138,8 +138,8 @@ pub mod requests {
 
 	#[derive(Clone, Debug, Deserialize, Serialize)]
 	pub struct SendpayRoute {
-	    #[serde(alias = "amount_msat", skip_serializing_if = "Option::is_none")]
-	    pub amount_msat: Option<Amount>,
+	    #[serde(alias = "amount_msat")]
+	    pub amount_msat: Amount,
 	    #[serde(alias = "id")]
 	    pub id: Pubkey,
 	    #[serde(alias = "delay")]

diff --git a/contrib/pyln-testing/pyln/testing/utils.py b/contrib/pyln-testing/pyln/testing/utils.py
@@ -1026,7 +1026,7 @@ def pay(self, dst, amt, label=None):
         assert len(invoices) == 1 and invoices[0]['status'] == 'unpaid'
 
         routestep = {
-            'msatoshi': amt,
+            'amount_msat': amt,
             'id': dst_id,
             'delay': 5,
             'channel': '1x1x1'  # note: can be bogus for 1-hop direct payments

diff --git a/doc/schemas/sendpay.request.json b/doc/schemas/sendpay.request.json
@@ -12,6 +12,7 @@
       "items": {
         "type": "object",
         "required": [
+          "amount_msat",
           "id",
           "delay",
           "channel"

diff --git a/lightningd/pay.c b/lightningd/pay.c
@@ -1366,7 +1366,7 @@ static struct command_result *param_route_hops(struct command *cmd,
 
 	*hops = tal_arr(cmd, struct route_hop, tok->size);
 	json_for_each_arr(i, t, tok) {
-		struct amount_msat *msat, *amount_msat;
+		struct amount_msat *amount_msat;
 		struct node_id *id;
 		struct short_channel_id *channel;
 		unsigned *delay, *direction;
@@ -1375,13 +1375,10 @@ static struct command_result *param_route_hops(struct command *cmd,
 		int *ignored;
 
 		if (!param(cmd, buffer, t,
-			   /* Only *one* of these is required */
-			   p_opt("msatoshi", param_msat, &msat),
-			   p_opt("amount_msat", param_msat, &amount_msat),
-			   /* These three actually required */
-			   p_opt("id", param_node_id, &id),
-			   p_opt("delay", param_number, &delay),
-			   p_opt("channel", param_short_channel_id, &channel),
+			   p_req("amount_msat|msatoshi", param_msat, &amount_msat),
+			   p_req("id", param_node_id, &id),
+			   p_req("delay", param_number, &delay),
+			   p_req("channel", param_short_channel_id, &channel),
 			   /* Allowed (getroute supplies it) but ignored */
 			   p_opt("direction", param_number, &direction),
 			   p_opt("style", param_route_hop_style, &ignored),
@@ -1390,28 +1387,7 @@ static struct command_result *param_route_hops(struct command *cmd,
 			   NULL))
 			return command_param_failed();
 
-		if (!msat && !amount_msat)
-			return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
-					    "%s[%zi]: must have msatoshi"
-					    " or amount_msat", name, i);
-		if (!id || !channel || !delay)
-			return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
-					    "%s[%zi]: must have id, channel"
-					    " and delay", name, i);
-		if (msat && amount_msat && !amount_msat_eq(*msat, *amount_msat))
-			return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
-					    "%s[%zi]: msatoshi %s != amount_msat %s",
-					    name, i,
-					    type_to_string(tmpctx,
-							   struct amount_msat,
-							   msat),
-					    type_to_string(tmpctx,
-							   struct amount_msat,
-							   amount_msat));
-		if (!msat)
-			msat = amount_msat;
-
-		(*hops)[i].amount = *msat;
+		(*hops)[i].amount = *amount_msat;
 		(*hops)[i].node_id = *id;
 		(*hops)[i].delay = *delay;
 		(*hops)[i].scid = *channel;

diff --git a/tests/test_closing.py b/tests/test_closing.py
@@ -1856,7 +1856,7 @@ def test_onchaind_replay(node_factory, bitcoind):
     inv = l2.rpc.invoice(10**8, 'onchaind_replay', 'desc')
     rhash = inv['payment_hash']
     routestep = {
-        'msatoshi': 10**8 - 1,
+        'amount_msat': 10**8 - 1,
         'id': l2.info['id'],
         'delay': 101,
         'channel': '1x1x1'
@@ -1916,7 +1916,7 @@ def test_onchain_dust_out(node_factory, bitcoind, executor):
     inv = l2.rpc.invoice(1, 'onchain_dust_out', 'desc')
     rhash = inv['payment_hash']
     routestep = {
-        'msatoshi': 1,
+        'amount_msat': 1,
         'id': l2.info['id'],
         'delay': 5,
         'channel': '1x1x1'
@@ -1988,7 +1988,7 @@ def test_onchain_timeout(node_factory, bitcoind, executor):
     rhash = inv['payment_hash']
     # We underpay, so it fails.
     routestep = {
-        'msatoshi': 10**8 - 1,
+        'amount_msat': 10**8 - 1,
         'id': l2.info['id'],
         'delay': 5,
         'channel': '1x1x1'
@@ -2460,7 +2460,7 @@ def test_onchain_feechange(node_factory, bitcoind, executor):
     rhash = inv['payment_hash']
     # We underpay, so it fails.
     routestep = {
-        'msatoshi': 10**8 - 1,
+        'amount_msat': 10**8 - 1,
         'id': l2.info['id'],
         'delay': 5,
         'channel': '1x1x1'
@@ -2545,7 +2545,7 @@ def test_onchain_all_dust(node_factory, bitcoind, executor):
     rhash = inv['payment_hash']
     # We underpay, so it fails.
     routestep = {
-        'msatoshi': 10**7 - 1,
+        'amount_msat': 10**7 - 1,
         'id': l2.info['id'],
         'delay': 5,
         'channel': '1x1x1'

diff --git a/tests/test_connection.py b/tests/test_connection.py
@@ -743,7 +743,7 @@ def test_reconnect_sender_add1(node_factory):
     rhash = inv['payment_hash']
     assert only_one(l2.rpc.listinvoices('test_reconnect_sender_add1')['invoices'])['status'] == 'unpaid'
 
-    route = [{'msatoshi': amt, 'id': l2.info['id'], 'delay': 5, 'channel': '1x1x1'}]
+    route = [{'amount_msat': amt, 'id': l2.info['id'], 'delay': 5, 'channel': '1x1x1'}]
 
     for i in range(0, len(disconnects)):
         with pytest.raises(RpcError):
@@ -782,7 +782,7 @@ def test_reconnect_sender_add(node_factory):
     rhash = inv['payment_hash']
     assert only_one(l2.rpc.listinvoices('testpayment')['invoices'])['status'] == 'unpaid'
 
-    route = [{'msatoshi': amt, 'id': l2.info['id'], 'delay': 5, 'channel': '1x1x1'}]
+    route = [{'amount_msat': amt, 'id': l2.info['id'], 'delay': 5, 'channel': '1x1x1'}]
 
     # This will send commit, so will reconnect as required.
     l1.rpc.sendpay(route, rhash, payment_secret=inv['payment_secret'])
@@ -816,7 +816,7 @@ def test_reconnect_receiver_add(node_factory):
     rhash = inv['payment_hash']
     assert only_one(l2.rpc.listinvoices('testpayment2')['invoices'])['status'] == 'unpaid'
 
-    route = [{'msatoshi': amt, 'id': l2.info['id'], 'delay': 5, 'channel': '1x1x1'}]
+    route = [{'amount_msat': amt, 'id': l2.info['id'], 'delay': 5, 'channel': '1x1x1'}]
     l1.rpc.sendpay(route, rhash, payment_secret=inv['payment_secret'])
     for i in range(len(disconnects)):
         l1.daemon.wait_for_log('Already have funding locked in')
@@ -846,7 +846,7 @@ def test_reconnect_receiver_fulfill(node_factory):
     rhash = inv['payment_hash']
     assert only_one(l2.rpc.listinvoices('testpayment2')['invoices'])['status'] == 'unpaid'
 
-    route = [{'msatoshi': amt, 'id': l2.info['id'], 'delay': 5, 'channel': '1x1x1'}]
+    route = [{'amount_msat': amt, 'id': l2.info['id'], 'delay': 5, 'channel': '1x1x1'}]
     l1.rpc.sendpay(route, rhash, payment_secret=inv['payment_secret'])
     for i in range(len(disconnects)):
         l1.daemon.wait_for_log('Already have funding locked in')
@@ -3189,9 +3189,9 @@ def test_feerate_stress(node_factory, executor):
     scid12 = l1.get_channel_scid(l2)
     scid23 = l2.get_channel_scid(l3)
 
-    routel1l3 = [{'msatoshi': '10002msat', 'id': l2.info['id'], 'delay': 11, 'channel': scid12},
-                 {'msatoshi': '10000msat', 'id': l3.info['id'], 'delay': 5, 'channel': scid23}]
-    routel2l1 = [{'msatoshi': '10000msat', 'id': l1.info['id'], 'delay': 5, 'channel': scid12}]
+    routel1l3 = [{'amount_msat': '10002msat', 'id': l2.info['id'], 'delay': 11, 'channel': scid12},
+                 {'amount_msat': '10000msat', 'id': l3.info['id'], 'delay': 5, 'channel': scid23}]
+    routel2l1 = [{'amount_msat': '10000msat', 'id': l1.info['id'], 'delay': 5, 'channel': scid12}]
 
     rate = 1875
     NUM_ATTEMPTS = 25
@@ -3245,7 +3245,7 @@ def test_pay_disconnect_stress(node_factory, executor):
                                                                   '-WIRE_COMMITMENT_SIGNED']}])
 
         scid12 = l1.get_channel_scid(l2)
-        routel2l1 = [{'msatoshi': '10000msat', 'id': l1.info['id'], 'delay': 5, 'channel': scid12}]
+        routel2l1 = [{'amount_msat': '10000msat', 'id': l1.info['id'], 'delay': 5, 'channel': scid12}]
 
         # Get invoice from l1 to pay.
         inv = l1.rpc.invoice(10000, "invoice", "invoice")
@@ -3414,7 +3414,7 @@ def test_htlc_retransmit_order(node_factory, executor):
     invoices = [l2.rpc.invoice(1000, str(x), str(x)) for x in range(NUM_HTLCS)]
 
     routestep = {
-        'msatoshi': 1000,
+        'amount_msat': 1000,
         'id': l2.info['id'],
         'delay': 5,
         'channel': '1x1x1'  # note: can be bogus for 1-hop direct payments
@@ -3534,7 +3534,7 @@ def test_upgrade_statickey_onchaind(node_factory, executor, bitcoind):
 
     # Make sure another commitment happens, sending failed payment.
     routestep = {
-        'msatoshi': 1,
+        'amount_msat': 1,
         'id': l2.info['id'],
         'delay': 5,
         'channel': '1x1x1'  # note: can be bogus for 1-hop direct payments
@@ -3658,7 +3658,7 @@ def test_upgrade_statickey_fail(node_factory, executor, bitcoind):
                                                'hold-result': 'fail'}])
 
     # This HTLC will fail
-    l1.rpc.sendpay([{'msatoshi': 1000, 'id': l2.info['id'], 'delay': 5, 'channel': '1x1x1'}], '00' * 32, payment_secret='00' * 32)
+    l1.rpc.sendpay([{'amount_msat': 1000, 'id': l2.info['id'], 'delay': 5, 'channel': '1x1x1'}], '00' * 32, payment_secret='00' * 32)
 
     # Each one should cause one disconnection, no upgrade.
     for d in l1_disconnects + l2_disconnects:
@@ -3731,7 +3731,7 @@ def test_htlc_failed_noclose(node_factory):
 
     inv = l2.rpc.invoice(1000, "test", "test")
     routestep = {
-        'msatoshi': FUNDAMOUNT * 1000,
+        'amount_msat': FUNDAMOUNT * 1000,
         'id': l2.info['id'],
         'delay': 5,
         'channel': '1x1x1'  # note: can be bogus for 1-hop direct payments
@@ -3886,12 +3886,12 @@ def test_multichan(node_factory, executor, bitcoind):
         scid23b = scids[0]
 
     # Test paying by each,
-    route = [{'msatoshi': 100001001,
+    route = [{'amount_msat': 100001001,
               'id': l2.info['id'],
               'delay': 11,
               # Unneeded
               'channel': scid12},
-             {'msatoshi': 100000000,
+             {'amount_msat': 100000000,
               'id': l3.info['id'],
               'delay': 5,
               'channel': scid23a}]

diff --git a/tests/test_pay.py b/tests/test_pay.py
@@ -226,7 +226,7 @@ def test_pay0(node_factory):
     rhash = inv['payment_hash']
 
     routestep = {
-        'msatoshi': 0,
+        'amount_msat': 0,
         'id': l2.info['id'],
         'delay': 10,
         'channel': chanid
@@ -560,7 +560,7 @@ def invoice_unpaid(dst, label):
         return len(invoices) == 1 and invoices[0]['status'] == 'unpaid'
 
     routestep = {
-        'msatoshi': amt,
+        'amount_msat': amt,
         'id': l2.info['id'],
         'delay': 5,
         'channel': '1x1x1'
@@ -569,15 +569,15 @@ def invoice_unpaid(dst, label):
     # Insufficient funds.
     with pytest.raises(RpcError):
         rs = copy.deepcopy(routestep)
-        rs['msatoshi'] = rs['msatoshi'] - 1
+        rs['amount_msat'] = rs['amount_msat'] - 1
         l1.rpc.sendpay([rs], rhash, payment_secret=inv['payment_secret'])
         l1.rpc.waitsendpay(rhash)
     assert invoice_unpaid(l2, 'testpayment2')
 
     # Gross overpayment (more than factor of 2)
     with pytest.raises(RpcError):
         rs = copy.deepcopy(routestep)
-        rs['msatoshi'] = rs['msatoshi'] * 2 + 1
+        rs['amount_msat'] = rs['amount_msat'] * 2 + 1
         l1.rpc.sendpay([rs], rhash, payment_secret=inv['payment_secret'])
         l1.rpc.waitsendpay(rhash)
     assert invoice_unpaid(l2, 'testpayment2')
@@ -633,7 +633,7 @@ def invoice_unpaid(dst, label):
     # Check receiver
     assert only_one(l2.rpc.listinvoices('testpayment2')['invoices'])['status'] == 'paid'
     assert only_one(l2.rpc.listinvoices('testpayment2')['invoices'])['pay_index'] == 1
-    assert only_one(l2.rpc.listinvoices('testpayment2')['invoices'])['amount_received_msat'] == rs['msatoshi']
+    assert only_one(l2.rpc.listinvoices('testpayment2')['invoices'])['amount_received_msat'] == rs['amount_msat']
     assert only_one(l2.rpc.listinvoices('testpayment2')['invoices'])['payment_preimage'] == preimage
 
     # Balances should reflect it.
@@ -656,13 +656,13 @@ def check_balances():
     assert preimage == preimage2
     l1.daemon.wait_for_log('Payment ./.: .* COMPLETE')
     assert only_one(l2.rpc.listinvoices('testpayment2')['invoices'])['status'] == 'paid'
-    assert only_one(l2.rpc.listinvoices('testpayment2')['invoices'])['amount_received_msat'] == rs['msatoshi']
+    assert only_one(l2.rpc.listinvoices('testpayment2')['invoices'])['amount_received_msat'] == rs['amount_msat']
 
     # Overpaying by "only" a factor of 2 succeeds.
     inv = l2.rpc.invoice(amt, 'testpayment3', 'desc')
     rhash = inv['payment_hash']
     assert only_one(l2.rpc.listinvoices('testpayment3')['invoices'])['status'] == 'unpaid'
-    routestep = {'msatoshi': amt * 2, 'id': l2.info['id'], 'delay': 5, 'channel': '1x1x1'}
+    routestep = {'amount_msat': amt * 2, 'id': l2.info['id'], 'delay': 5, 'channel': '1x1x1'}
     l1.rpc.sendpay([routestep], rhash, payment_secret=inv['payment_secret'])
     preimage3 = l1.rpc.waitsendpay(rhash)['payment_preimage']
     assert only_one(l2.rpc.listinvoices('testpayment3')['invoices'])['status'] == 'paid'
@@ -1082,11 +1082,11 @@ def test_forward(node_factory, bitcoind):
     amt = 100000000
     fee = amt * 10 // 1000000 + 1
 
-    baseroute = [{'msatoshi': amt + fee,
+    baseroute = [{'amount_msat': amt + fee,
                   'id': l2.info['id'],
                   'delay': 12,
                   'channel': chanid1},
-                 {'msatoshi': amt,
+                 {'amount_msat': amt,
                   'id': l3.info['id'],
                   'delay': 6,
                   'channel': chanid2}]
@@ -1487,11 +1487,11 @@ def test_forward_local_failed_stats(node_factory, bitcoind, executor):
     payment_hash = inv['payment_hash']
     fee = amount * 10 // 1000000 + 1
 
-    route = [{'msatoshi': amount,
+    route = [{'amount_msat': amount,
               'id': l2.info['id'],
               'delay': 12,
               'channel': c12},
-             {'msatoshi': amount,
+             {'amount_msat': amount,
               'id': l4.info['id'],
               'delay': 6,
               'channel': c24}]
@@ -1513,11 +1513,11 @@ def test_forward_local_failed_stats(node_factory, bitcoind, executor):
     payment_hash = inv['payment_hash']
     fee = amount * 10 // 1000000 + 1
 
-    route = [{'msatoshi': amount + fee,
+    route = [{'amount_msat': amount + fee,
               'id': l2.info['id'],
               'delay': 12,
               'channel': c12},
-             {'msatoshi': amount,
+             {'amount_msat': amount,
               'id': l5.info['id'],
               'delay': 6,
               'channel': c25}]
@@ -1563,11 +1563,11 @@ def test_forward_local_failed_stats(node_factory, bitcoind, executor):
     fee = amount * 10 // 1000000 + 1
 
     # We underpay, so it fails.
-    route = [{'msatoshi': amount + fee - 1,
+    route = [{'amount_msat': amount + fee - 1,
               'id': l2.info['id'],
               'delay': 12,
               'channel': c12},
-             {'msatoshi': amount - 1,
+             {'amount_msat': amount - 1,
               'id': l4.info['id'],
               'delay': 5,
               'channel': c24}]
@@ -1702,7 +1702,7 @@ def exhaust_channel(opener, peer, scid, already_spent=0):
         lbl = ''.join(random.choice(string.ascii_letters) for _ in range(20))
         inv = peer.rpc.invoice(maxpay, lbl, "exhaust_channel")
         routestep = {
-            'msatoshi': maxpay,
+            'amount_msat': maxpay,
             'id': peer.info['id'],
             'delay': 10,
             'channel': scid
@@ -2686,7 +2686,7 @@ def test_error_returns_blockheight(node_factory, bitcoind):
     """Test that incorrect_or_unknown_payment_details returns block height"""
     l1, l2 = node_factory.line_graph(2)
 
-    l1.rpc.sendpay([{'msatoshi': 100,
+    l1.rpc.sendpay([{'amount_msat': 100,
                      'id': l2.info['id'],
                      'delay': 10,
                      'channel': l1.get_channel_scid(l2)}],