MetacoSA / NBitcoin

Comprehensive Bitcoin library for the .NET framework.
MIT License
1.88k stars 847 forks source link

BitcoinAddress.Create(string) is obsolete #391

Closed bazooka70 closed 6 years ago

bazooka70 commented 6 years ago

It was kinda cool feature. I'm guessing this is b/c of the alt coins support? Is it true for BTC also? From my initial test there is no problem with it with BTC addresses.

nopara73 commented 6 years ago

I think there's another way to do this. Maybe new BitcoinAddress() or BitcoinAddress.Parse() or BitcoinAddress.FromWif() or something like this, being familiar with Nicolas's patterns. If you cannot find it, I'll dig it up for you.

bazooka70 commented 6 years ago

@nopara73 , I'm almost sure there will be no such capability b/c detecting all altcoins addresses now is a pain in the ass. But hopefully you might know better! if I'm right I only hope that BitcoinAddress.Create(string, null) will work as before for BTC, and wont be completely deprecated later. I can (try to) detect other alt coins with some effort.

nopara73 commented 6 years ago

Is it deprecated with expected Network provided, too?

bazooka70 commented 6 years ago

@nopara73, No. as far as I can tell BitcoinAddress.Create(string, expectedNetwork) parses any registered network as expected. but @NicolasDorier can confirm.

nopara73 commented 6 years ago

The difference is: if the expected network doesn't match, it throws an exception.

BitcoinAddress.Create(string) is deprecated and you must use BitcoinAddress.Create(string, Network). If you want the same that you have with BitcoinAddress.Create(string), but you don't want the compiler to complain about deprecation, then simply do something like this:

public static class BitcoinAddressParser
{
    public static BitcoinAddress Parse(string address)
    {
        try
        {
            return BitcoinAddress.Create(address, Network.Main);
        }
        catch
        {
            try
            {
                return BitcoinAddress.Create(address, Network.TestNet);
            }
            catch
            {
                return BitcoinAddress.Create(address, Network.RegTest);
            }
        }
    }
}

Then call:

BitcoinAddressParser.Parse("12345678asdfghj");
bazooka70 commented 6 years ago

@nopara73 , an easier way is to simply call BitcoinAddress.Create(string, null) The compiler won't complain and will do the same job, but only for BTC (i.e. Network.Main, Network.TestNet, Network.RegTest) , not other alt coins Networks (exception will be thrown).

To do this for other alt coins your method could be used e.g.

public static class BitcoinAddressParser
{
    public static BitcoinAddress Parse(string address)
    {
        try
        {
            // try BTC
            return BitcoinAddress.Create(address, null);
        }
        catch
        {
            // try BCH
            try
            {
                return BitcoinAddress.Create(address, BCash.Mainnet);
            }
            catch
            {
                // etc for BCash.Testnet, BCash.Regtest
                // ...
                // etc (LTC, DOGE...)
            }
        }
    }
}

Very ugly :-P

nopara73 commented 6 years ago

Yes, but that's a bug, which you should expect later to be fixed and break your code.

Btw, there's also Network.Parse<BitcoinAddress>("xyz");.

bazooka70 commented 6 years ago

Why do you think it's a bug? (but i agree later it could break the code if Nicolas deprecate it completly not allowing null as the second parameter...)

Internally BitcoinAddress.Create() calls Network.Parse<BitcoinAddress>("xyz", expectedNetwork) unless I missed something... (Line 96, and 110)

https://github.com/MetacoSA/NBitcoin/blob/master/NBitcoin/BitcoinAddress.cs

nopara73 commented 6 years ago

Why do you think it's a bug?

Because that's what had been deprecated. Btw I'm not exactly sure if Network.Parse<BitcoinAddress>("xyz"); is correct way to do things. They may very well just forgot to deprecate this function without provided network, too.

NicolasDorier commented 6 years ago

It is obsolete for 4 reasons:

  1. It is very slow
  2. On the backend you should NEVER store bitcoin addresses, only scriptPUbKey. There is no reason to store addresses this is only a UX concern. Storing addresses only make your code slower and less flexible to change. (As you can always recover the BitcoinAddress from scriptPubKey for UI purpose)
  3. On the frontend, you must send an error message to the user if he is sending an address with incorrect format. (This is what BitcoinAddress.Create(String, Network) does)
  4. With the multiplication of shitcoin, lot's of them share the same base58 prefixes, if you use BitcoinAddress.Create(String) it will break when you add another shitcoin down the road.

So at the end of the day, there is no good reason to use BitcoinAddress.Create and if you designed your code correctly, everytimes you need to parse a string, you probably already have the Network somewhere in the scope.

bazooka70 commented 6 years ago

@NicolasDorier , You got very strong points there against using BitcoinAddress.Create. but still it was a nice feature. I will really try to get rid of all BitcoinAddress.Create(address) in the backend (I used it mainly with QBitNinja). Am I right about my assumption for BitcoinAddress.Create(address, null)? will it work fine for BTC?

NicolasDorier commented 6 years ago

@bazooka70 QBitNinjaClient know his Network, so you can easily use this member to parse your address.

Yes it will work for bitcoin, .Create always try first with bitcoin network, but I might remove this one day.

bazooka70 commented 6 years ago

The same goes for ExtPubKey.Parse(string, expectedNetwork) now?

NicolasDorier commented 6 years ago

yes

bazooka70 commented 6 years ago

The main usage I think that I will be missing is when I send funds to a raw address, I could validate that the destination address is valid for my wallet network. think about it...

NicolasDorier commented 6 years ago

well you still can valid, this is the whole point.

bazooka70 commented 6 years ago

Ahhh yes, I can see your point now! thanks. OK, I am going to close this now. I see no point in leaving this open unless you plan to un-deprecate it some day... ;)