Bad return in web3.eth.accounts.signTransaction

[michaelsbradleyjr]

See https://github.com/ethereum/web3.js/blob/1.x/packages/web3-eth-accounts/src/index.js#L238.

The inner signed function returns from that try block, but instead it seems like it should assign the object to result. In the case that return is reached then L252 is not executed. That's what I was running into in #3281 — I finally realized when stepping through signTransaction in vscode's debugger.

---

There is another concern I have about web3.eth.accounts.signTransaction, and I'm not sure how many other function throughout the web3 codebase may be in the same situation. While I'll explain it here, my guess is there should be another issue to discuss the matter more specifically.

So, signTransaction can take a callback, and if none is provided it sets up a dummy callback (cf. L134). However, it always both calls the callback and returns a promise. That's really not good 💣 💥 , but there may be something I've misunderstood so I'm hoping the web3 team can provide feedback on this point.

The implication is that callers that want to use a callback also need to guard against unhandled promise rejections at their call sites, and they should not need to do that. In other words, if I'm passing (err, val) => {...} as the callback then handling of the error should be total with respect to the callback and it should not be necessary to do:

web3.eth.accounts.signTransaction(..., callback).catch(...)

When implementing functions that optionally take a Node.js style callback as the last argument and otherwise return a promise, it might be good to implement with Node's callbackify (src), or the callbackify package may be even better since it handles "callback-mode vs. promise-mode" internally while with Node's built-in callbackify you have to do that yourself.

The main idea is to author all functions/methods that do something asynchronous with a "promise first" mentality. And then consistently provide for callback usage by passing them through callbackify. When used in callback-mode the function/method always returns undefined and err/val handling is total with respect to the callback (which is never called immediately, also very important); while in promise-mode a promise is always returned and the caller needs to use it with await or .then/.catch.

Versions

web3 1.2.4

[cgewecke]

Have opened a PR fixing the marooned callback in #3285.

the callback then handling of the error should be total with respect to the callback and it should not be necessary to do:

Yes. This definitely needs further investigation - thanks so much for raising.