Remove assert, use typeforce typing #434

dcousens · 2015-08-12T03:03:06Z

Will wait on ACK before merge.

Curious on your opinions about this...

dcousens · 2015-08-12T03:07:50Z

src/types.js

+// external dependent types
+function BigInt (value) { return !typeforce.Null(value) && value.constructor === bigi }
+function ECCurve (value) { return !typeforce.Null(value) && value.constructor === ecurve.Curve }
+function ECPoint (value) { return !typeforce.Null(value) && value.constructor === ecurve.Point }


@weilu this does revert our previous duck typing change.
I've never had the issue of de-duping where it wasn't symptomatic of something potentially disastrous anyway, so I'm not sure why we allowed it in the first place.

ECPair and ecdsa are the only interfaces that accept bigi / ECPoint's, and we intend to remove their individual use entirely for 2.0.0, so this shouldn't even be an issue.

If you feel otherwise, I'll change these back to typeforce.quacksLike which is the identical behaviour to what was there before.

I remember an important reason why we went with duck typing is to deal with having multiple versions of the same library in the same app. I still think that's relevant so my preference is still duck typing.

I remember an important reason why we went with duck typing is to deal with having multiple versions of the same library in the same app.

Right.

I've never had the issue of de-duping where it wasn't symptomatic of something potentially disastrous anyway, so I'm not sure why we allowed it in the first place.

I feel like such a situation is dangerous enough that we shouldn't let it occur though?
Especially since this duck typing is only really enforced on types that, if calculated wrong, directly impact where a user might send or receive funds.

This is the other breaking change, but, that has already been pointed out.

This specific change was reverted in #450 as it was still an issue. quacksLike behaviour was restored.

dcousens · 2015-08-12T03:25:23Z

I'm happy to break out the typeforce typing changes if theres any issues, as the move away from assert is a much simpler decision and it does have highly tangible benefits.

jprichardson · 2015-08-12T13:36:03Z

as the move away from assert is a much simpler decision and it does have highly tangible benefits.

I think for posterity, these benefits should be explicitly stated.

weilu · 2015-08-12T17:33:32Z

I don't get why you are getting rid of asserts. IMHO in most of the places, assert is still more direct and semantically straightforward. It's not like we are throwing any custom Error types for better error handling.

dcousens · 2015-08-12T22:47:57Z

I think for posterity, these benefits should be explicitly stated.

In a production application, with over 30 high level dependencies, bitcoinjs-lib, bs58, bip39, bs58check, are the only dependencies using assert for error handling. All other conclusions aside, this is a specific pattern we have adopted only in our ecosystem.
assert explicitly makes code coverage difficult, because the branch is deeper down in the application. typeforce changes do not help this, however, the majority of typeforce changes were type checking. The majority of logic based changes that now exist as throw conditions are now visible in code coverage.
assert only accepts value arguments (like any function), therefore error messages are evaluated/built on every call. This has caused major performance problems for us in the past where JSON.stringify or toString has been more complex then expected and excessively slowed down applications.
assert.equal is more often used over assert.strictEqual, most likely because it is shorter to write. We actually have our linter fail our tests if we wrote this code normally, and yet, we have no qualms with this. While that could just be fixed by enforcing strictEqual, it is more verbose and often longer code than just writing out the explicit branch anyway.

It's not like we are throwing any custom Error types for better error handling.

The errors thrown are mostly the same, as out lined by the tests. The only places it did change was in the use of typeforce which has a consistent error format of Expected TYPE, got VALUETYPE VALUE.

dcousens · 2015-08-14T01:38:49Z

Rebased on master. Added 94e16fb, 6002d9a.

dcousens · 2015-08-15T11:39:45Z

@jprichardson @weilu, shall I merge or?

To clarify, in the end, I'd like to use typeforce less, but, for now, this is how it is.

weilu · 2015-08-17T08:46:51Z

@dcousens Given that the types of the errors and in some cases the messages are different, I wonder if we need to bump major for this?

dcousens · 2015-08-17T08:48:39Z

@weilu I wasn't going to bother back porting this to 1.5.x, the change is worth it, but it doesn't fix/add anything such that it would be worth back porting.

dcousens · 2015-08-17T08:50:42Z

test/fixtures/transaction_builder.json

@@ -780,7 +780,7 @@
      },
      {
        "description": "Wrong key pair for multisig redeemScript",
-        "exception": "key pair cannot sign for this input",
+        "exception": "Key pair cannot sign for this input",


Breaking change - but it already is different to 1.5.x: 'privateKey cannot sign for this input'

dcousens · 2015-08-17T08:53:51Z

I'd say there is only a few breaking changes, outlined above.

Even then:

ecdsa is no longer exposed.
TransactionBuilder.sign has changed input types, and error message already. Therefore, not an issue.

The only error message that has therefore changed in terms of the 1.5.x API, in this PR, is only Transaction.addInput, which has already had its API changed anyway.

The other errors should all be consistent as types.tuple should behave the same as the previous code.

dcousens · 2015-08-17T08:58:41Z

src/address.js

-
-  if (hash.length !== 20) throw new TypeError('Invalid hash length')
-  if (version & ~0xff) throw new TypeError('Invalid version byte')
+  typeforce(types.tuple(types.Hash160bit, types.UInt8), arguments)


This API is new, so its not a breaking change.

weilu · 2015-08-17T09:27:11Z

I'm fine with not back-porting this to 1.5.x but let's add these breaking changes into our 2.0 release note.

weilu · 2015-08-17T09:27:16Z

Good to merge

Remove assert, use typeforce typing

dcousens · 2015-08-17T12:03:48Z

Thanks @weilu, I added the breaking change tag so we can easily build that release note by reviewing all issues under it later.

dcousens · 2015-08-18T04:24:38Z

Follow up 5d2abb5 for HDNode.

dcousens added the needs review label Aug 12, 2015

dcousens assigned weilu Aug 12, 2015

dcousens reviewed Aug 12, 2015
View reviewed changes

dcousens added this to the 2.0.0 milestone Aug 12, 2015

dcousens added 14 commits August 14, 2015 10:56

package: use typeforce 1.3.0

c7fe710

add types

254b670

ECPair: remove use of assert

8f821f4

block: remove use of assert

395c43a

Script: remove use of assert

1d4a718

scripts: remove use of assert

e4b697a

Transaction: remove use of assert

f2f0747

ECSignature: remove use of assert

c1a965d

TransactionBuilder: remove use of assert

d85df4a

bufferutils: remove use of assert

70c874e

dustThreshold is a UInt53

33a9dc4

ECSignature: use types.tuple for arguments

92d1614

rename __disableExcess to __noStrict

94e16fb

ecdsa: fix up tuple indentation

6002d9a

dcousens mentioned this pull request Aug 14, 2015

No Script #438

Merged

dcousens force-pushed the noassert branch from a12bbf0 to 6002d9a Compare August 14, 2015 01:38

dcousens reviewed Aug 17, 2015
View reviewed changes

dcousens added a commit that referenced this pull request Aug 17, 2015

Merge pull request #434 from bitcoinjs/noassert

21980b6

Remove assert, use typeforce typing

dcousens merged commit 21980b6 into master Aug 17, 2015

dcousens deleted the noassert branch August 17, 2015 12:03

dcousens added the breaking change label Aug 17, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove assert, use typeforce typing #434

Remove assert, use typeforce typing #434

dcousens commented Aug 12, 2015

dcousens Aug 12, 2015

weilu Aug 12, 2015

dcousens Aug 13, 2015

dcousens Aug 17, 2015

dcousens Sep 14, 2015

dcousens commented Aug 12, 2015

jprichardson commented Aug 12, 2015

weilu commented Aug 12, 2015

dcousens commented Aug 12, 2015

dcousens commented Aug 14, 2015

dcousens commented Aug 15, 2015

weilu commented Aug 17, 2015

dcousens commented Aug 17, 2015

dcousens Aug 17, 2015

dcousens commented Aug 17, 2015

dcousens Aug 17, 2015

weilu commented Aug 17, 2015

weilu commented Aug 17, 2015

dcousens commented Aug 17, 2015

dcousens commented Aug 18, 2015

Remove assert, use typeforce typing #434

Remove assert, use typeforce typing #434

Conversation

dcousens commented Aug 12, 2015

dcousens Aug 12, 2015

Choose a reason for hiding this comment

weilu Aug 12, 2015

Choose a reason for hiding this comment

dcousens Aug 13, 2015

Choose a reason for hiding this comment

dcousens Aug 17, 2015

Choose a reason for hiding this comment

dcousens Sep 14, 2015

Choose a reason for hiding this comment

dcousens commented Aug 12, 2015

jprichardson commented Aug 12, 2015

weilu commented Aug 12, 2015

dcousens commented Aug 12, 2015

dcousens commented Aug 14, 2015

dcousens commented Aug 15, 2015

weilu commented Aug 17, 2015

dcousens commented Aug 17, 2015

dcousens Aug 17, 2015

Choose a reason for hiding this comment

dcousens commented Aug 17, 2015

dcousens Aug 17, 2015

Choose a reason for hiding this comment

weilu commented Aug 17, 2015

weilu commented Aug 17, 2015

dcousens commented Aug 17, 2015

dcousens commented Aug 18, 2015