Merge JoinMarket-Org/joinmarket-clientserver#1510: Improve BIP21 parser, add typehints and improve test coverage

88bd45b02e Parse URI params in guaranteed order, for duplicates, last one wins (Kristaps Kaupe)
cd1f394381 Add test coverage for is_bip21_uri() (Kristaps Kaupe)
6b2a248bb5 Add typehints to BIP21 code (Kristaps Kaupe)

Pull request description:

  First commit adds typehints to BIP21 code, which now I try to do with any code I seriously touch.

  Second commit adds test coverage for `is_bip21_uri()` public function.

  Third commit makes sure we parse URI parameters in guaranteed order, if there are duplicates, last one wins. See https://github.com/bitcoin/bitcoin/pull/27928 for context.

ACKs for top commit:
  AdamISZ:
    tACK 88bd45b02e

Tree-SHA512: fcc0055dc2e12cff7d9246c6bd0207c7481da1070ca0d15b478456a1a5b57db61820fba3764ce5d9947565ded724fec2ef3c9856ab5055b5d2b0244395517afa

jmbitcoin/jmbitcoin/bip21.py

@@ -4,49 +4,51 @@
 # this are expected to do address validation independently anyway.
 
 from jmbitcoin import amount_to_sat
-from urllib.parse import parse_qs, quote, unquote_plus, urlencode, urlparse
+from typing import Dict, List, Tuple, Union
+from urllib.parse import parse_qsl, quote, unquote_plus, urlencode, urlparse
 import re
 
 
-def is_bip21_uri(uri):
+def is_bip21_uri(uri: str) -> bool:
     parsed = urlparse(uri)
     return parsed.scheme.lower() == 'bitcoin' and parsed.path != ''
 
 
-def is_bip21_amount_str(amount):
+def _is_bip21_amount_str(amount: str) -> bool:
     return re.compile(r"^[0-9]{1,8}(\.[0-9]{1,8})?$").match(str(amount)) != None
 
 
-def validate_bip21_amount(amount):
-    if not is_bip21_amount_str(amount):
+def _validate_bip21_amount(amount: str) -> None:
+    if not _is_bip21_amount_str(amount):
         raise ValueError("Invalid BTC amount " + str(amount))
 
 
-def decode_bip21_uri(uri):
+def decode_bip21_uri(uri: str) -> Dict[str, Union[str, int]]:
     if not is_bip21_uri(uri):
         raise ValueError("Not a valid BIP21 URI: " + uri)
     result = {}
     parsed = urlparse(uri)
     result['address'] = parsed.path
-    params = parse_qs(parsed.query)
-    for key in params:
+    params = parse_qsl(parsed.query)
+    for key, value in params:
         if key.startswith('req-'):
             raise ValueError("Unknown required parameter " + key +
                 " in BIP21 URI.")
         if key == 'amount':
-            amount_str = params['amount'][0]
-            validate_bip21_amount(amount_str)
+            _validate_bip21_amount(value)
             # Convert amount to sats, as used internally by JM
-            result['amount'] = amount_to_sat(amount_str + "btc")
+            result['amount'] = amount_to_sat(value + "btc")
         else:
-            result[key] = unquote_plus(params[key][0])
+            result[key] = unquote_plus(value)
     return result
 
 
-def encode_bip21_uri(address, params, safe=""):
+def encode_bip21_uri(address: str,
+                     params: Union[dict, List[Tuple[str, Union[float, int, str]]]],
+                     safe: str = "") -> str:
     uri = 'bitcoin:' + address
     if len(params) > 0:
         if 'amount' in params:
-            validate_bip21_amount(params['amount'])
+            _validate_bip21_amount(params['amount'])
         uri += '?' + urlencode(params, safe=safe, quote_via=quote)
     return uri

jmbitcoin/test/test_bip21.py

@@ -2,6 +2,21 @@ import jmbitcoin as btc
 import pytest
 
 
+def test_is_bip21_uri():
+    # invalid URIs
+    assert(not btc.is_bip21_uri(''))
+    assert(not btc.is_bip21_uri('nfdjksnfjkdsnfjkds'))
+    assert(not btc.is_bip21_uri('175tWpb8K1S7NmH4Zx6rewF9WQrcZv245W'))
+    assert(not btc.is_bip21_uri('175tWpb8K1S7NmH4Zx6rewF9WQrcZv245W?amount=20.3'))
+    assert(not btc.is_bip21_uri('bitcoin:'))
+    assert(not btc.is_bip21_uri('bitcoin:?amount=20.3'))
+    # valid URIs
+    assert(btc.is_bip21_uri('bitcoin:175tWpb8K1S7NmH4Zx6rewF9WQrcZv245W'))
+    assert(btc.is_bip21_uri('BITCOIN:175tWpb8K1S7NmH4Zx6rewF9WQrcZv245W'))
+    assert(btc.is_bip21_uri('BitCoin:175tWpb8K1S7NmH4Zx6rewF9WQrcZv245W'))
+    assert(btc.is_bip21_uri('bitcoin:175tWpb8K1S7NmH4Zx6rewF9WQrcZv245W?label=Luke-Jr'))
+
+
 def test_bip21_decode():
 
     # These should raise exception because of not being valid BIP21 URI's
@@ -60,6 +75,17 @@ def test_bip21_decode():
     assert(parsed['somethingyoudontunderstand'] == '50')
     assert(parsed['somethingelseyoudontget'] == '999')
 
+    # Test multiple amount parameters, last value should win.
+    parsed = btc.decode_bip21_uri(
+        'bitcoin:175tWpb8K1S7NmH4Zx6rewF9WQrcZv245W?amount=20.3&amount=50&label=Luke-Jr')
+    assert(parsed['address'] == '175tWpb8K1S7NmH4Zx6rewF9WQrcZv245W')
+    assert(parsed['amount'] == 5000000000)
+    assert(parsed['label'] == 'Luke-Jr')
+    # Here are two amount parameters, first valid, second not valid, so URI is not valid.
+    with pytest.raises(ValueError):
+        btc.decode_bip21_uri(
+            'bitcoin:175tWpb8K1S7NmH4Zx6rewF9WQrcZv245W?amount=20.3&amount=100,000&label=Luke-Jr')
+
 
 def test_bip21_encode():
     assert(