Skip to content
This repository has been archived by the owner on Jul 15, 2022. It is now read-only.

check for cashAdrr in isValidRecipient #286

Closed
wants to merge 1 commit into from
Closed

check for cashAdrr in isValidRecipient #286

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/libcore/isValidRecipient.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// @flow

import { InvalidAddress } from "@ledgerhq/errors";
import { InvalidAddress, CashAddrNotSupported } from "@ledgerhq/errors";
import type { CryptoCurrency } from "../types";
import { withLibcoreF } from "./access";
import customAddressValidationByFamily from "../generated/customAddressValidation";
Expand All @@ -17,6 +17,11 @@ export const isValidRecipient: F = withLibcoreF(core => async arg => {
return res;
}
const { currency, recipient } = arg;

if (recipient.startsWith("bitcoincash:")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 than bitcoincash: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

Copy link
Contributor

@gre gre Jun 21, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@gre gre Jun 21, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return Promise.reject(new CashAddrNotSupported());
}

const poolInstance = core.getPoolInstance();
const currencyCore = await poolInstance.getCurrency(currency.id);
const value = await core.Address.isValid(recipient, currencyCore);
Expand Down