Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Key: add helper method GetSeedBytes #969

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

knocte
Copy link
Contributor

@knocte knocte commented Feb 5, 2021

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

@@ -407,6 +408,18 @@ public BitcoinSecret GetWif(Network network)
return new BitcoinSecret(this, network);
}

public byte[] GetSeedBytes()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Call it ToBytes and follow how I implemented it in PubKey.
Your version is super inefficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sure

public byte[] GetSeedBytes()
{
AssertNotDiposed();
byte[] bytes = Enumerable.Repeat((byte)0x00, KEY_SIZE).ToArray();
Copy link
Collaborator

Choose a reason for hiding this comment

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

new byte[32] is clearly overrated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops! didn't realize that the bytes would be overriden later anyway, was just monkey-translating F# code to C# code from somewhere else

@knocte
Copy link
Contributor Author

knocte commented Feb 16, 2021

@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
Copy link
Collaborator

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 added a commit to nblockchain/geewallet that referenced this pull request Jun 1, 2021
knocte added a commit to nblockchain/geewallet that referenced this pull request Jun 1, 2021
knocte added a commit to nblockchain/geewallet that referenced this pull request Jun 1, 2021
knocte added a commit to nblockchain/geewallet that referenced this pull request Jun 1, 2021
@knocte
Copy link
Contributor Author

knocte commented Jun 1, 2021

Yes.

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

@NicolasDorier
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants