Repository navigation
FIX: validate prefix, case, version and keys of silent payment code - #42
Conversation
|
No actionable comments were generated in the recent review.
|
Overtorment
left a comment
There was a problem hiding this comment.
Checked the decoder against BIP-352 (bech32m with a 1023 limit, hrp sp, v0 exactly 66 bytes, v1-30 first 66 bytes, v31 rejected, both keys checked as points) and it looks right. Tests, typecheck and prettier pass locally. One heads-up about the BlueWallet side before we bump the dependency there.
| SilentPayment.decodePaymentCode(pc); | ||
| return true; |
There was a problem hiding this comment.
Heads-up for when we bump this in BlueWallet: isPaymentCodeValid("SP1…") now returns true, but BlueWallet still does case-sensitive startsWith('sp1') in SendDetails.tsx (the allowSilentPaymentSend gate) and in AbstractHDElectrumWallet.createTransaction (hasSilentPaymentOutput). So an uppercase code gets past validation, skips the wallet-capability check, never goes through sp.createTransaction, and then fails in psbt with SP1… has no matching Script. It doesn't lose funds, but it's a confusing failure, and wallets that can't send SP don't get the proper error.
This lib is fine as it is. The bump PR needs to lowercase the address (or use .toLowerCase().startsWith('sp1')) in those two places. Alternatively, export a small isSilentPaymentCode(address) helper from here so the prefix check lives in one place.
Overtorment
left a comment
There was a problem hiding this comment.
LGTM. The uppercase SP1 note is non-blocking and can be handled on the BlueWallet side when bumping the dependency.
This library now depends on tiny-secp256k1 v2 (WASM) I think we should probably refactor this library to make ecc implementation pluggable, like bitcoinjs does. WDYT ? |
Why
isPaymentCodeValidtoo loose,createTransactiondecodes on its own:tsp1…,foo1…with valid checksum →trueSP1…→ valid, butcreateTransactionchecksstartsWith("sp1")case-sensitive and passes it through as regular address. Uppercase is common in BIP21 QR codesB_scan/B_mnot checked to be points, error only later increateTransactionHow
New
SilentPayment.decodePaymentCode(pc)→{ Bscan, Bm }or throws:sp(lib is mainnet only:pubkeyToAddressusesnetworks.bitcoin)ecc.isPointfor both keysisPaymentCodeValidandcreateTransactionboth use it. Tests for each case + uppercasecreateTransactiongives same output as lowercase. New tests fail on master.npm test(105 passed),typecheck,prettiergreen.Summary by CodeRabbit
spprefix are now recognized without regard to letter case, and supported code versions can be validated.