search

ethereumjs / ethereumjs-tx

Project is in active development and has been moved to the EthereumJS VM monorepo.
https://github.com/ethereumjs/ethereumjs-vm/tree/master/packages/tx
Mozilla Public License 2.0
781 stars 237 forks source link

Include signature by default in FakeTransaction.hash #97

Closed albrow closed 6 years ago

albrow commented 6 years ago

The documentation for FakeTransaction.hash shows that includeSignature is supposed to be true by default. However, this is not the case and the implementation treats includeSignature as false by default.

This was causing us issues with Ganache, where we were seeing the same transaction hash for different transactions (because they were all using a FakeTransaction without overriding the signature). It looks like #81 was designed to fix this issue, but since Ganache does not explicitly provide the argument true to FakeTransaction.hash, the issue was still occurring.

I believe the implementation should match the documentation and includeSignature should be true by default. Doing so also fixes the duplicate transaction hashes we were seeing in Ganache.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-1.2%) to 97.619% when pulling b3fe579ac1874e157c1f6f966966964bbf0dbd4a on 0xProject:fake-tx-include-signature-by-default into 6ea5b5030fb6a7f9786ef7fb158c0593a517fef7 on ethereumjs:master.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-1.2%) to 97.619% when pulling b3fe579ac1874e157c1f6f966966964bbf0dbd4a on 0xProject:fake-tx-include-signature-by-default into 6ea5b5030fb6a7f9786ef7fb158c0593a517fef7 on ethereumjs:master.

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 98.78% when pulling b2eedb069160e948fdf320564e3cbbbd8534a743 on 0xProject:fake-tx-include-signature-by-default into 6ea5b5030fb6a7f9786ef7fb158c0593a517fef7 on ethereumjs:master.

fanatid commented 6 years ago

Please remove yarn.lock

LogvinovLeon commented 6 years ago

@fanatid Done

LogvinovLeon commented 6 years ago

@fanatid Could we merge it and publish a new version? Are we waiting on other reviewers?

fanatid commented 6 years ago

@LogvinovLeon I'm not in npm package owners. Ping @holgerd77

LogvinovLeon commented 6 years ago

@holgerd77 is on vacation starting from Saturday for 2 weeks. I pinged him already. No worries. It's not urgent. We're just using our fork for now.

fabioberger commented 6 years ago

@holgerd77 are you back from vacation? ;) Would be great if we could get this merged and published.

holgerd77 commented 6 years ago

@fabioberger Yes (was great! :-)), I'll probably find some time to have a look into this tomorrow and hopefully directly release!

holgerd77 commented 6 years ago

Ok, just published the new release v1.3.5.

fabioberger commented 6 years ago

Thanks @holgerd77!