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

Enforce hex prefixing for address strings #241

Merged
merged 2 commits into from
Apr 24, 2020
Merged

Conversation

cgewecke
Copy link
Contributor

@cgewecke cgewecke commented Apr 23, 2020

Part of #172 Breaking API Changes

The proposal to make hex param checking more strict in #172 comes out of review discussions in #170. There @holger suggested:

Then we should at least pass input params to a dedicated check method and throw if no hex-prefix provided, and always return prefixed hex strings.

PR adds logic to enforce hex-prefixing for all address string inputs. Affected methods are:

  • isValidAddress
  • isZeroAddress
  • toChecksumAddress
  • isValidChecksumAddress (via isValidAddress)

isPrecompile is excluded because it's targeted for removal in #242.

@github-actions
Copy link

Coverage Status

Coverage increased (+0.01%) to 99.656% when pulling 5670bdd on enforce-hex-value-format into 3bbc948 on master.

@lgtm-com
Copy link

lgtm-com bot commented Apr 23, 2020

This pull request introduces 1 alert when merging 5670bdd into 3bbc948 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

src/account.ts Outdated Show resolved Hide resolved
src/account.ts Outdated
if (!ethjsUtil.isHexString(input)) {
throw new Error(msg)
}
}
Copy link
Contributor Author

@cgewecke cgewecke Apr 23, 2020

Choose a reason for hiding this comment

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

Using isHexString instead of isHexPrefixed here because the latter throws its own error if given a non-string input.

@cgewecke cgewecke marked this pull request as ready for review April 23, 2020 21:15
@cgewecke cgewecke requested review from holgerd77 and removed request for holgerd77 April 23, 2020 21:16
@holgerd77
Copy link
Member

@cgewecke Great, thanks for picking this up! 😄 Did I mention that you should really question what's written up in the API Change Table and give this also your own thoughts respectively reflect if you see these as useful/adequate API changes.

I've written this up quite some time ago and there actually was not yet much discussion around this yet, so I would encourage all others to give this some thought as well.

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.

This looks all good, will merge here as well.

isValidAddress('x2f015c60e0be116b1f0cd534704db9c92118fb6a')
})
assert.throws(function() {
isValidAddress('0X52908400098527886E0F7030069857D2E4169EE7')
Copy link
Member

Choose a reason for hiding this comment

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

Nice. 👍

@holgerd77 holgerd77 merged commit ebe0723 into master Apr 24, 2020
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.

None yet

2 participants