This repository has been archived by the owner on Feb 21, 2024. It is now read-only.
Adjust transaction parameters and prefixed string normalizers #257
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
JamesLefrere
commented
Jul 17, 2019
| @@ -248,7 +248,7 @@ export const transactionObjectValidator = ({ | |||
| * Only check if the address (`to` prop) is in the correct | |||
| * format, if one was provided in the initial transaction object | |||
| */ | |||
| if (typeof to !== 'undefined') { | |||
| if (to) { | |||
There was a problem hiding this comment.
A truthy check should suffice for to; it can be null or undefined (or a string).
JamesLefrere
commented
Jul 17, 2019
JamesLefrere
commented
Jul 17, 2019
| /* | ||
| * Encrypted JSON Keystore | ||
| */ | ||
| keystore: string; |
There was a problem hiding this comment.
Flow showed me that these had to be removed, because they conflicted with the getters (which are typed appropriately).
JamesLefrere
commented
Jul 17, 2019
| @@ -297,7 +297,7 @@ export const signMessage = async ({ | |||
| const toSign = messageOrDataValidator({ message, messageData }); | |||
| warning(messages.messageSignatureOnlyTrezor); | |||
| try { | |||
| const { signature: signedMessage } = await payloadListener({ | |||
| const { signature: signedMessage = '' } = await payloadListener({ | |||
There was a problem hiding this comment.
Flow showed that this was necessary; signature is optional in the return value, but I'm not sure whether this is a problem.
JamesLefrere
commented
Jul 17, 2019
| @@ -189,6 +193,9 @@ describe('`Trezor` Hardware Wallet Module Static Methods', () => { | |||
| ); | |||
| }); | |||
| test('Signs a transaction without a destination address', async () => { | |||
| payloadListener.mockImplementation(() => ({ | |||
There was a problem hiding this comment.
This test was silently failing :/ (reported as RUNS not PASS)
* Fix Flow errors for string normalizers, require a string value * Use a bound function for address/hex sequence normalizers
* Adjust parameters for `ethereumjs-tx` transactions (recent breaking changes) and ensure that the chain ID is supplied; this fixes a bug where the recovery param wasn't validated correctly * Fix logic for supplying a `to` property for transactions (it can also be `null`, not just `undefined`) * Fix some Flow types * Fix some comments * Update/fix tests
JamesLefrere
force-pushed
the
fix/ethereumjs-tx-params
branch
from
f9acd9a
to
eea5398
Compare
rdig
approved these changes
Jul 18, 2019
| /** | ||
| * Nomalize an Ethereum address | ||
| * Normalize an Ethereum address |
There was a problem hiding this comment.
Hey, you even spell checked my comments. Top marks 💯
| @@ -189,6 +193,9 @@ describe('`Trezor` Hardware Wallet Module Static Methods', () => { | |||
| ); | |||
| }); | |||
| test('Signs a transaction without a destination address', async () => { | |||
| payloadListener.mockImplementation(() => ({ | |||
| * provided in the initial transaction object. | ||
| */ | ||
| typeof to !== 'undefined' ? { to: addressNormalizer(to) } : {}, | ||
| ), | ||
| ...(to ? { to: addressNormalizer(to) } : {}), |
There was a problem hiding this comment.
I'll need to manually check the Trezor service, since I remember it going crazy if we passed in values it didn't expect.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This PR makes some changes to transactions created with
ethereumjs-txsuch that thechainandtoproperties are supplied correctly; this fixes a bug where the recovery param wasn't validated correctly.Changes
ethereumjs-txtransactions (recent breaking changes) and ensure that the chain ID is supplied; this fixes a bug where the recovery param wasn't validated correctlytoproperty for transactions (it can also benull, not justundefined)