2.0.0

[dcousens]

Resolves all remaining points in https://github.com/bitcoinjs/bip38/issues/5. Specifically:

• consider removing coinstring [DONE]
• consider removing class instantiation, pass script params as optional param [DONE]

Script params were not put as optional, as there has been at least some consensus over time that the parameters given are suitable. Happy to revise that.

I did not only keep the raw methods, as I felt the base58check encoded BIP38 string was important and specific to the encoding of this module.

Instead, I remove encoding/decoding of WIF format private keys as part of the encrypt interface. You now specifically pass in a private key 32-byte buffer with a compression flag.

[jprichardson]

Why remove the instantiation? I don't think I'm in favor of that unless a good case could be made.

[dcousens]

Because if I want a parameter containing closure, I can really easily make it.

In any case, with this PR, what state would it contain? All stateful parameters were removed, they don't even exist in the proposed interface.

• scryptParams is now constant.
• versions were removed.

Even if they did exist, a set of free functions without context are always easier to reason about than having context. Especially since that context is doing nothing but wrapping some default parameters. To conclude:

It is an implicit contract of state for something that isn't stateful.

[jprichardson]

It is an implicit contract of state for something that isn't stateful.

But there is state, namely the scrypt parameters.

[dcousens]

@jprichardson they are not state, they are parameters. State is variable. scryptParams is immutable (unless changed by the user, no different than changing parameters)

If this were a React component, would you be putting scryptParams in this.state or this.props?

[dcousens]

Any consensus on this? ping @jprichardson ?

[jprichardson]

I'll follow-up tomorrow or Wed...

On Mon, Aug 31, 2015 at 8:02 AM, Daniel Cousens notifications@github.com wrote:

Any consensus on this? ping @jprichardson https://github.com/jprichardson ?

— Reply to this email directly or view it on GitHub https://github.com/bitcoinjs/bip38/pull/13#issuecomment-136364655.

Simple & Secure Bitcoin Wallet: https://www.coinbolt.com Bitcoin / JavaScript: http://cryptocoinjs.com Follow JP Richardson on Twitter: https://twitter.com/jprichardson

[jprichardson]

Here's what I'd accept...

A factory method called create that takes as input the scrypt params. It would then return an instance of the Bip38 object. I'd also accept static methods that use the last param as scrypt params and default to the bip38 params.

ES2015 partial code: (not suggesting that it has to be ES2015, it's just more concise to communicate)

class Bip38 {
  constructor (scryptParams) { ... }

  encrypt (...) { ... }

  decrypt (...) { ... }

  static encrypt (..., scryptParams) { ... }

  static decrypt (..., scryptParams) { ... }

  static create (scryptParams) { ... }
}

If you have a better suggestion, please use some code to communicate for the sake of clarity. Thanks.

[dcousens]

I'd also accept static methods that use the last param as scrypt params and default to the bip38 params.

That is what we have now? No? Granted, we just have to add that param to the end of the methods.

[dcousens]

@jprichardson what is the difference between BIP38 and a synchronous pbkdf2? Ignoring that pbkdf2 is a one way function.

Conceptually, its the same. Yet in 1 case you're presumably happy with a free function, the other, an object.

[jprichardson]

Conceptually, its the same. Yet in 1 case you're presumably happy with a free function, the other, an object.

I'm really warming up to the idea of just providing the functions. Especially since I can make it look really clean with ES6 now...

[dcousens]

Any further thoughts on this @jprichardson ?

[jprichardson]

I'm alright with this now, but I'm gonna do it in ES6 (Babel).

[dcousens]

Maybe merge this then do your changes @jprichardson? I'll rebase if necessary

[jprichardson]

I'll rebase if necessary

Would be awesome.

[dcousens]

Rebased and tested.

Note, there are no tests for the progressCallbacks, so, they should be added/tested before release (if you get that far quickly).

[jprichardson]

Rebased and tested.

Awesome, I'm not satisfied with some of the dependencies just yet, addressing them on one by one basis. To start: https://github.com/bitcoinjs/wif/issues/5

[dcousens]

@jprichardson wif has been updated, any other changes you want before I go ahead and rebase/commit the changes?

[jprichardson]

Will review tomorrow.

[dcousens]

@jprichardson yay

[jprichardson]

Alright, almost all of it looks good. Some thoughts:

1. Documentation is not updated to reflect changes.
2. Returning d provides zero context to the end user unless you're familiar with ECC/ECDSA. Open to something else though.
3. Also function parameters named string or buffer only provide type information, but provide no semantic context. I think it's friendlier to the end user to have descriptive names. Could use assert to convey types. Or if you really wanted, append the type at the end of the name (yuck).

We get those patched up, I'll be willing and ready to merge :)

[dcousens]

Also function parameters named string or buffer only provide type information, but provide no semantic context. I think it's friendlier to the end user to have descriptive names.

You're decrypting a string to a private key. IMHO that is what is relevant? The type is all I'm passing IMHO. I guess I don't know what other semantic context I'd give it... "candidate"?

wifString?

[dcousens]

Oh god lets finish this haha

[jprichardson]

haha, alright, I'll tackle this bad boy on Monday.

[jprichardson]

Why are the tests failing?

[dcousens]

@jprichardson might be related to pbkdf2 tests failing.

Or not

[dcousens]

I know its low priority. But having coinstring, and old versions of bs58 and co still get pulled into my projects is odd just because of this module :smiley:

[dcousens]

@jprichardson squashed and rebased. This is good to go and very quick to review. :+1:

[jprichardson]

Why the failing tests?

[dcousens]

@jprichardson pushed up the missing compression flags in 63d1d4e. They were why the verify wasn't working ... yay :+1:

[dcousens]

@jprichardson 0.10 is failing as it was dropped by the underlying crypto packages, so I've removed it from travis. I also removed 4.2? Not sure why it is there.

[dcousens]

https://travis-ci.org/bitcoinjs/bip38/jobs/180905933 is timing out, restarted it and hopefully it passes.

Gets like 90% of the way before dying lol.

[dcousens]

Increased the timeout, and its all good now! :dancer:

[jprichardson]

Awesome, thanks for your work on this.

1. You murdered the docs. They didn't even make it to the potty training stage. Poor docs.
2. https://github.com/bitcoin/bips/blob/master/bip-0038.mediawiki#discussion-item-scrypt-parameters - we gotta have configurable Scrypt params. Can be an optional param.

Thoughts?

[dcousens]

@jprichardson if the parameters are changed, won't that stop people being able to decrypt unless they know those parameters?

[dcousens]

ping @jprichardson for 5d5a3d0

[coveralls]

Changes Unknown when pulling 4523d6ea937f950ebcb19e81763645700d34add6 on 200 into on master.

[dcousens]

ping @jprichardson, this should be good to go

edit: although the scrypt params option isn't documented... (I promise if you merge, i'll add it straight after)

[jprichardson]

edit: although the scrypt params option isn't documented... (I promise if you merge, i'll add it straight after)

👍