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

Signing fixes back-ported from 2.x #3125

Merged
merged 4 commits into from
Oct 13, 2019
Merged

Signing fixes back-ported from 2.x #3125

merged 4 commits into from
Oct 13, 2019

Conversation

nivida
Copy link
Contributor

@nivida nivida commented Oct 13, 2019

Fixes #1998, #2033, and #1074

@nivida nivida added Bug Addressing a bug In Progress Currently being worked on 1.x 1.0 related issues labels Oct 13, 2019
@nivida nivida requested a review from cgewecke October 13, 2019 05:28
@coveralls
Copy link

coveralls commented Oct 13, 2019

Coverage Status

Coverage decreased (-0.1%) to 82.936% when pulling 88f42a1 on back-port/1074 into d278254 on 1.x.

Copy link
Collaborator

@cgewecke cgewecke left a comment

Choose a reason for hiding this comment

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

Looks good! Just left a couple small notes

…' prefix added to messageHash for consistency reasons
@nivida nivida requested a review from cgewecke October 13, 2019 06:52
Copy link
Collaborator

@cgewecke cgewecke left a comment

Choose a reason for hiding this comment

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

LGTM.

@nivida nivida removed the In Progress Currently being worked on label Oct 13, 2019
@nivida nivida merged commit 02c0346 into 1.x Oct 13, 2019
@cgewecke
Copy link
Collaborator

@nivida Apologies, I missed something important in the review here. Additionally, there was a bug in the E2E CI (fixed with #3138) which meant it wasn't obvious an eth.accounts.sign test was failing for geth dev.

The problem is that ethereumjs-tx requires an additional object passed to the Transaction constructor when a tx is being sent to a non-mainnet chain. You can see an example in the ejs-tx signing test removed from #3122.

nachomazzara pushed a commit to nachomazzara/web3.js that referenced this pull request Jun 4, 2020
* signing fixes back-ported from 2.x

* CHANGELOG.md updated

* transactionHash property added, docs updated, tests extended, and '0x' prefix added to messageHash for consistency reasons
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues Bug Addressing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants