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.

Sign up for GitHub

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

Jump to bottom

fix: make signTransaction return a promise, fixes #1213 #1214

Merged
merged 4 commits into from
Jan 22, 2018
Merged

fix: make signTransaction return a promise, fixes #1213 #1214

merged 4 commits into from
Jan 22, 2018

Conversation

phra
Copy link
Contributor

@phra phra commented Dec 6, 2017

No description provided.

@frozeman
Copy link
Contributor

This is breaking 7 tests, can you please run npm test yourself?

This was a decision i made to allow accounts.signTransaction() to return synchronous when chain, id gasPrice etc is given. I am also not happy with the inconsistency it creates, but deemed this more convienient. As described here: http://web3js.readthedocs.io/en/1.0/web3-eth-accounts.html#id5

I am still torn, but it might be better to return a promise at all times. Can you fix the tests as well?

@phra
Copy link
Contributor Author

phra commented Dec 11, 2017 •
edited

@frozeman i will try later to fix the tests!

imho returning always a promise it's the best option based on my experience, e.g. having a complex signature implies also a more complex dev ux for TypeScript..

@frozeman
Copy link
Contributor

Please fix the test :)

@phra
Copy link
Contributor Author

phra commented Dec 21, 2017

sorry, i forgot. i will do it now.

phra added a commit to phra/web3.js that referenced this pull request Dec 21, 2017
@phra
test: fix failing tests for web3#1214
f2c8228
@phra phra force-pushed the fix/sign-transaction-promise branch from f2c8228 to 440285e Compare December 21, 2017 21:38
phra added a commit to phra/web3.js that referenced this pull request Dec 21, 2017
@phra
test: fix failing tests for web3#1214
440285e
@phra
Copy link
Contributor Author

phra commented Dec 21, 2017

@frozeman tests and conflicts are fixed. have a look at the void async function(done) trick, it can be used to gradually migrate the codebase to promises and async functions. 🍻

@coveralls
Copy link

coveralls commented Dec 21, 2017 •
edited

Coverage Status

Coverage remained the same at 85.709% when pulling 440285e on phra:fix/sign-transaction-promise into 6816ce5 on ethereum:1.0.

@phra
Copy link
Contributor Author

phra commented Dec 21, 2017 •
edited

tests are failing on node < 7.
native async function is missing in those runtime versions.
node LTS 6 will reach end of life in april 2018.

@frozeman
Copy link
Contributor

Sorry for the delay.
There is no reason why we should use void async here, please just call done() so these tests works also on legacy systems

@frozeman
Copy link
Contributor

Currently there is a 1.0ES6 branch, but the current version is ES5 and we cant introduce now ES6 in one place here.

@phra phra force-pushed the fix/sign-transaction-promise branch from 440285e to cdf7bda Compare January 12, 2018 10:36
@phra phra force-pushed the fix/sign-transaction-promise branch from 3baffe7 to 57d62ee Compare January 12, 2018 10:50
@phra
Copy link
Contributor Author

phra commented Jan 12, 2018 •
edited

@frozeman done, but i strongly suggest to implement a transpiling step in the build to enable es6 usage on legacy versions. or even better port the code to typescript! 🚀

EDIT: travis seems stuck.

@coveralls
Copy link

coveralls commented Jan 12, 2018 •
edited

Coverage Status

Coverage remained the same at 85.698% when pulling 57d62ee on phra:fix/sign-transaction-promise into d051bd7 on ethereum:1.0.

@coveralls
Copy link

coveralls commented Jan 12, 2018 •
edited

Coverage Status

Coverage remained the same at 85.698% when pulling 57d62ee on phra:fix/sign-transaction-promise into d051bd7 on ethereum:1.0.

@coveralls
Copy link

coveralls commented Jan 12, 2018 •
edited

Coverage Status

Coverage remained the same at 85.698% when pulling 57d62ee on phra:fix/sign-transaction-promise into d051bd7 on ethereum:1.0.

@phra
Copy link
Contributor Author

phra commented Jan 12, 2018

CI is green. ✅

@phra
Copy link
Contributor Author

phra commented Jan 16, 2018

the PR is stale again..

@frozeman frozeman merged commit dea396d into web3:1.0 Jan 22, 2018
@phra phra deleted the fix/sign-transaction-promise branch January 22, 2018 14:42
nachomazzara pushed a commit to nachomazzara/web3.js that referenced this pull request Jun 4, 2020
@phra @nachomazzara
fix: make signTransaction return a promise, fixes web3#1213 (web3#1214)
ca001d5 
* fix: make signTransaction return a promise, fixes web3#1213

* test: fix failing tests for web3#1214

* refactor: backport sign transaction changes to es5
@snyk-bot snyk-bot mentioned this pull request Sep 15, 2021
Open
@snyk-bot snyk-bot mentioned this pull request Dec 31, 2021
Open
This was referenced Jun 21, 2023
Open
Open
@Woodpile37 Woodpile37 mentioned this pull request Aug 19, 2023
Merged
@adamlaska adamlaska mentioned this pull request Nov 30, 2023
Open
@baby636 baby636 mentioned this pull request Dec 1, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frozeman frozeman frozeman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@phra @frozeman @coveralls