check for cashAdrr in isValidRecipient #286
Conversation
valpinkman
force-pushed
the
feat/cashadrr-error
branch
from
c14af76
to
9a09bfb
Compare
valpinkman
force-pushed
the
feat/cashadrr-error
branch
from
9a09bfb
to
6fc51d9
Compare
| @@ -17,6 +17,11 @@ export const isValidRecipient: F = withLibcoreF(core => async arg => { | |||
| return res; | |||
| } | |||
| const { currency, recipient } = arg; | |||
|
|
|||
| if (recipient.startsWith("bitcoincash:")) { | |||
There was a problem hiding this comment.
IMHO it's not enough to only test for the bitcoincash: prefix, especially since it collides with the URI scheme used in QR Codes (see #60 ):
- A cashaddr QR formatted as
bitcoincash:<addr>rather thanbitcoincash:bitcoincash:<addr>would not be caught by this test - Wallets or websites omitting the
bitcoincash:prefix or users only copying the last part of the address would not be caught either - On the other hand, implementations displaying a supported bitcoin addresses with the
bitcoincash:URI scheme would trigger a not supported error
There was a problem hiding this comment.
so technically it would still catch if you manually copy paste in that field. only the QR code scanning can have a problem.
I kinda wonder if we should just make the qrcode scanner preserving bitcoincash:* as the recipient, meaning we will not support bitcoincash:* as a "uri scheme" qrcode bit will assume it's an address part already
the code to modify would be in URIScheme
There was a problem hiding this comment.
Regardless of the QR issue (the official specs require to not have a double prefix in this context btw), using CashAddr without prefix is not uncommon and should be handled: Bitcoin ABC own CashAddr announcement states:
Note the prefix “bitcoincash:”, which is technically always part of the address, although the prefix may be optional or missing entirely, depending on the wallet or the implementation.
EDIT: It's also in the specs: When presented to users, the prefix may be omitted
Why not use the reference bitcoincashjs lib to test the address format?
https://github.com/bitcoincashjs/bchaddrjs#test-for-address-format
There was a problem hiding this comment.
it's more libcore that should provide a way to do that. live-common is supposed to be as agnostic as possible & we will try to drop dep like ripple-lib and ethereumjs-tx
|
for internal discussion |
No description provided.