IsBtcAddress not working with testnet addresses #639

[vlapo]

Describe the bug
Could you please check issue typestack/class-validator#639 ? Cc @michaelcerne

Additional context
Issue in class-validator lib

[kathawala]

In addition to this, there are several issues with the current IsBtcAddress validator.

Currently the following addresses are all claimed to be valid by the IsBtcAddress validator but they are in fact invalid:

"17VZNX1SN5NlKa8UQFxwQbFeFc3iqRYhem" (any string with a lowercase 'L" will also be invalid but pass as valid)
"17VZNX1SN5NtKa8UQFxwQbFeFc3iqRYhemt"

There are probably others as well. Bitcoin addresses cannot be validated solely through regex (as is currently done in this library). Looking through this other bitcoin validator you can see that there is some checksumming and encoding, decoding that needs to take place when validating a bitcoin address. Is this library open to PRs which pull in dependencies for cryptography or base58 encoding / decoding?

[profnandaa]

Thanks for raising. PR welcome!

[kathawala]

@profnandaa I'm working on a PR but it requires an external dependency, and I am having trouble installing any external dependency and getting rollup and babel to bundle it nicely.

I am having exactly the same issues which were described in #926. and I have tried all the suggested rollup plugins to no avail. If you could help at all, that would be appreciated. Is there a tried-and-true way to bundle in any dependencies from npm into this library?

[profnandaa]

Is the external dep a dev dep or a regular dep? If it’s the later, we will have great hesitation since we have never had one... ./na

On Sun, Aug 2, 2020 at 10:36 AM Farhan Kathawala ***@***.***> wrote: @profnandaa <https://github.com/profnandaa> I'm working on a PR but it requires an external dependency, and I am having trouble installing any external dependency and getting rollup and babel to bundle it nicely. I am having exactly the same issues which were described in #926 <#926>. and I have tried all the suggested rollup plugins to no avail. If you could help at all, that would be appreciated. Is there a tried-and-true way to bundle in any dependencies from npm into this library? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1386 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB7ZEP3GRLVYNXHQPC65GTR6UJP7ANCNFSM4PDWQ24Q> .

-- Sent from a tiny device while on the move.

[kathawala]

@profnandaa It's a regular dep :(

The reason I asked if the project was open to PRs which pull in deps for cryptography and all is that I don't want to hand implement crypto and encoding/decoding functions if I don't have to. But if the project is not wanting external dependencies I can reproduce those functions in code.

[genus1]

The reason this does not work is because it is only decoding the address to base58.
Once decoded you need to 256 hash the first 21 bytes and compare the 256 hash to the next four bites of the decoded address.
Rename this function to validate base58.

[ezkemboi]

I am working on this one and I will raise a PR.

[thegrandpoobah]

I just want to point out that I don't think that @ezkemboi 's PR fixes the described issue with testnet addresses. As far as I am aware, Testnet addresses start with m and that is not captured in the regex currently in use. Arguably you would want another method named isBTCTestnetAddress or something to distinguish between the two since you (maybe?) don't want to use testnet and mainnet addresses interchangeably, but the original described issue is still open.

[profnandaa]

Other than have a different validator, we could go with a `isTestnet` in the options object. ./na

…

On Thu, Dec 31, 2020 at 10:26 PM Sahab Yazdani ***@***.***> wrote: I just want to point out that I don't think that @ezkemboi <https://github.com/ezkemboi> 's PR fixes the described issue with testnet addresses. As far as I am aware, Testnet addresses start with m and that is not captured in the regex currently in use. Arguably you would want another method named isBTCTestnetAddress or something to distinguish between the two since you (maybe?) don't want to use testnet and mainnet addresses interchangeably, but the original described issue is still open. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#1386 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB7ZEM4F5Y6IGVTIFEPFGDSXTF47ANCNFSM4PDWQ24Q> .

-- *___Nandaa *Anthony *@profnandaa <https://github.com/profnandaa>*