search

MetacoSA / NBitcoin

Comprehensive Bitcoin library for the .NET framework.
MIT License
1.87k stars 846 forks source link

Key: add helper method GetSeedBytes #969

Open knocte opened 3 years ago

knocte commented 3 years ago

My team uses this in more than one place and would be good to have it in NBitcoin out of the box.

knocte commented 3 years ago

@NicolasDorier hey, I just realized that there already is a .ToBytes() method for Key, however:

  1. It's not really in Key class, but it's an extension method of IBitcoinSerializable. Is it enough to have it there, or is it better that it lives in Key class for better visibility? (so that you don't need to import IBitcoinSerializable's namespace) We could even call the extension method from Key.ToBytes() but this leads me to the next point:
  2. Is the extension method as bad (performance wise) as my initial approach? If yes, I'll just go and do what you suggested (use same approach there is in PubKey).

Let me know what's best please.

NicolasDorier commented 3 years ago

It's not really in Key class, but it's an extension method of IBitcoinSerializable. Is it enough to have it there, or is it better that it lives in Key class for better visibility?

Yes.

Is the extension method as bad (performance wise) as my initial approach? If yes, I'll just go and do what you suggested (use same approach there is in PubKey).

Yes it is bad, that said in practice this should hardly matter but I think just using the PubKey approach is also simpler.

knocte commented 3 years ago

Yes.

Sorry, if I ask (A or B)? you cannot answer "Yes" unless you're trying to confuse me haha.

NicolasDorier commented 3 years ago

sorry lol, I think we should not add GetSeedBytes when the extension method already have a ToBytes that works. This is very confusing for users, as they would not know which one to call.