Skip to content
This repository has been archived by the owner on Jun 17, 2021. It is now read-only.

Enforce buffer inputs for account methods #243

Merged
merged 2 commits into from
Apr 24, 2020

Conversation

cgewecke
Copy link
Contributor

@cgewecke cgewecke commented Apr 24, 2020

(PR builds on & targets #241)

Part of #172 Breaking API Changes

  • Removes implicit Buffer conversion in accounts.ts
  • Removes support for string inputs to generateAddress2
  • Enforces Buffer inputs for
    • generateAddress
    • generateAddress2
    • pubToAddress
    • privateToPublic
    • importPublic

isPrecompiled is ignored here because it's targeted for removal in #242

@cgewecke cgewecke marked this pull request as ready for review April 24, 2020 02:44
@cgewecke cgewecke force-pushed the enforce-hex-value-format branch from 11fc1c1 to 78626a4 Compare April 24, 2020 06:31
@cgewecke cgewecke force-pushed the enforce-buffer-accounts branch from 5f26449 to f3192cf Compare April 24, 2020 06:35
@cgewecke cgewecke force-pushed the enforce-buffer-accounts branch from f3192cf to 54bebef Compare April 24, 2020 06:38
@cgewecke cgewecke requested a review from holgerd77 April 24, 2020 07:43
Copy link
Member

@holgerd77 holgerd77 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!

@@ -183,7 +180,7 @@ export const privateToPublic = function(privateKey: Buffer): Buffer {
* Converts a public key to the Ethereum format.
*/
export const importPublic = function(publicKey: Buffer): Buffer {
publicKey = toBuffer(publicKey)
assertIsBuffer(publicKey)
if (publicKey.length !== 64) {
publicKey = secp256k1.publicKeyConvert(publicKey, false).slice(1)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, account.ts changes look good.

@holgerd77 holgerd77 merged commit d13c655 into enforce-hex-value-format Apr 24, 2020
@cgewecke
Copy link
Contributor Author

Ah sorry @holgerd77 I had this pointed at #241 so that the diff was clearer. Will re-open

@holgerd77
Copy link
Member

Oh, sorry, didn't notice

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants