Fix signTransaction transaction parameter type
#400
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `signTransaction` method of the KeyringController had specified the wrong type for the `transaction` parameter. It mistakenly declared it as being of type `Transaction`, the same type we use in our Transaction controller. Instead the type it expects is an instance of `ethereumjs-tx`. This package doesn't have type definitions though, so for now I've changed it to `unknown`, and updated the JSDoc comment to explain what it should be.
brad-decker
approved these changes
Mar 16, 2021
| @@ -369,11 +368,11 @@ export class KeyringController extends BaseController<KeyringConfig, KeyringStat | |||
| /** | |||
| * Signs a transaction by calling down into a specific keyring | |||
| * | |||
| * @param transaction - Transaction object to sign | |||
| * @param transaction - Transaction object to sign. Must be a `ethereumjs-tx` transaction instance. | |||
There was a problem hiding this comment.
We could create a type for this representing the minimum set of fields required
I'm ambivalent on whether we do this here or in a future change.
There was a problem hiding this comment.
Good idea. I'm somewhat inclined to leave it for now though, as fixing up the keyring controller is on the path to snaps anyway. I'm anticipating that we'll be changing this method signature to be more type-friendly in the near future, and removing the dependence upon ethereumjs-tx. At least I want to, now that I've discovered this!
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
The `signTransaction` method of the KeyringController had specified the wrong type for the `transaction` parameter. It mistakenly declared it as being of type `Transaction`, the same type we use in our Transaction controller. Instead the type it expects is an instance of `ethereumjs-tx`. This package doesn't have type definitions though, so for now I've changed it to `unknown`, and updated the JSDoc comment to explain what it should be.
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
The `signTransaction` method of the KeyringController had specified the wrong type for the `transaction` parameter. It mistakenly declared it as being of type `Transaction`, the same type we use in our Transaction controller. Instead the type it expects is an instance of `ethereumjs-tx`. This package doesn't have type definitions though, so for now I've changed it to `unknown`, and updated the JSDoc comment to explain what it should be.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
signTransactionmethod of the KeyringController had specified the wrong type for thetransactionparameter. It mistakenly declared it as being of typeTransaction, the same type we use in our Transaction controller.Instead the type it expects is an instance of
ethereumjs-tx. This package doesn't have type definitions though, so for now I've changed it tounknown, and updated the JSDoc comment to explain what it should be.