This PR is an attempt to make it clear how the current BIP21 implementation behaves in Bitcoin Core. Especially, I'm interested whether one can specify multiple amount (message, etc.) parameters.
My primary end goal is to answer this question of mine but I figured that maybe it's worth a PR. If not, I'll close the PR.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | MarcoFalke, kevkevinpal |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
lgtm ACK f1d807e383942ae20e080174d33ac17afd649351
ACK f1d807e
you mentioned you wanted to test multiple message params but I'm only seeing you do multiple amount params
@kevkevinpal My original understanding was that having multiple amount parameters will end up with an error. However, it's clear from the implementation that it's not what is actually happening in Bitcoin Core. I'm happy that this PR can make it clear.
I can add more tests to show that this actually happens for label and message as well if you like.
One reason why I'm a bit concerned about allowing amount multiple times in BIP21 (in actual implementations) is that one can try to use it to attempt to trick people. Say a Bitcoin wallet would trim a BIP21 URI like this:
bitcoin:<address>?amount=0.01&message=someLongText&amount=1.0
to something like
bitcoin:<address>?amount=0.01&message=someLongText...
User looks at that, it looks OK, clicks a confirm button fast ... Obviously, the mentioned example is pretty lame, and I believe/hope it's an artificial example that cannot happen in practice. Though, it feels like this is asking for these kinds of clever/social attempts to trick people. I'm not exactly sure if there is a pro for supporting multiple amount parameters (or message, and label).
If there is something I'm missing, please let me know.
86 | + // Escape sequences are not supported. 87 | + uri.setUrl(QString("bitcoin:175tWpb8K1S7NmH4Zx6rewF9WQrcZv245W?amount=100&label=%3F")); 88 | + QVERIFY(GUIUtil::parseBitcoinURI(uri, &rv)); 89 | + QVERIFY(rv.address == QString("175tWpb8K1S7NmH4Zx6rewF9WQrcZv245W")); 90 | + QVERIFY(rv.amount == 10000000000LL); 91 | + QVERIFY(rv.label == QString("%3F"));
It this correct? I mean, according to BIP21, not existing code. BIP21 specifies it as "valid characters of an RFC 3986 URI query component, excluding the "=" and "&" characters" and %xx should be decoded according to RFC 3986. That should be only way how to present "=" and "&" in message and label.
72 | + QVERIFY(rv.amount == 20000000000LL); 73 | + QVERIFY(rv.label == QString("Wikipedia Example")); 74 | + 75 | + // The first amount value is correct. However, the second amount value is not valid. Hence, the URI is not valid. 76 | + uri.setUrl(QString("bitcoin:175tWpb8K1S7NmH4Zx6rewF9WQrcZv245W?amount=100&amount=1,000&label=Wikipedia Example")); 77 | + QVERIFY(!GUIUtil::parseBitcoinURI(uri, &rv));
Guess should also check the case when the first amount value is invalid, second valid. Should it be considered valid or invalid URI then?