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

Make user supplied callback fire in accounts.signTransaction #3285

Merged
merged 2 commits into from
Jan 6, 2020
Merged

Conversation

cgewecke
Copy link
Collaborator

@cgewecke cgewecke commented Dec 20, 2019

Description

Fixes a bug which disabled the ability to call eth.accounts.signTransaction using the callback form.

@michaelsbradleyjr This only addresses the narrowest part of your issue. Have not used Object.assign as you suggest there, mostly to keep with the pattern of Web3's existing code. Please advise if you believe that's critical though.

Fixes #3283

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.
  • I have updated the CHANGELOG.md file in the root folder.
  • I have tested my code on the live network.

@coveralls
Copy link

coveralls commented Dec 20, 2019

Coverage Status

Coverage increased (+0.07%) to 84.875% when pulling e9c715c on issue/3283 into 58f77a6 on 1.x.

@cgewecke cgewecke added 1.x 1.0 related issues Bug Addressing a bug Review Needed Maintainer(s) need to review labels Dec 20, 2019
Copy link
Contributor

@michaelsbradleyjr michaelsbradleyjr 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, and sorry if I didn't communicate clearly when I mentioned "assign":

...it seems like it should assign the object to result

By that I meant exactly what you did (though see my comment below re: declaration) and not Object.assign.

packages/web3-eth-accounts/src/index.js Show resolved Hide resolved
Copy link
Contributor

@nivida nivida 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 Review Needed Maintainer(s) need to review label Jan 6, 2020
@nivida nivida merged commit 0f7e3dc into 1.x Jan 6, 2020
@nivida nivida deleted the issue/3283 branch January 6, 2020 15:08
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.

Bad return in web3.eth.accounts.signTransaction
4 participants