Auto-detect hex encoded bytes in sha3 #4108
Conversation
| @@ -67,3 +68,7 @@ export function toHex (str) { | |||
|
|
|||
| return `0x${(str || '').toLowerCase()}`; | |||
| } | |||
|
|
|||
| export function isHex(str) { | |||
There was a problem hiding this comment.
util/types.js has an isHex - it is very confusing having the 2 now, not sure what the differences are and what the various uses would be - especially from an external point. In addition, this file is for formatting functions, util/types would be for testing.
| @@ -28,3 +30,4 @@ export function sha3 (value, options) { | |||
|
|
|||
| return `0x${hash}`; | |||
| } | |||
| sha3.text = (val) => sha3(val, { encoding: 'raw' }); | |||
There was a problem hiding this comment.
Extra line break required before, no squashing.
jacogr
added
A5-grumble 🔥
tomusdrw
added
A0-pleasereview 🤓
| @@ -29,6 +29,10 @@ export function isFunction (test) { | |||
| } | |||
|
|
|||
| export function isHex (_test) { | |||
| if (!isString(_test)) { | |||
There was a problem hiding this comment.
+1 Was just wondering about this. (Apart from the use-case, makes it more robust overall)
| @@ -15,6 +15,7 @@ | |||
| // along with Parity. If not, see <http://www.gnu.org/licenses/>. | |||
|
|
|||
| import { range } from 'lodash'; | |||
| import { isString } from './types'; | |||
| @@ -14,7 +14,7 @@ | |||
| // You should have received a copy of the GNU General Public License | |||
| // along with Parity. If not, see <http://www.gnu.org/licenses/>. | |||
|
|
|||
| import { bytesToHex, hexToBytes, hexToAscii, bytesToAscii, asciiToHex } from './format'; | |||
| import { bytesToHex, hexToBytes, hexToAscii, bytesToAscii, asciiToHex, isHex } from './format'; | |||
This was the original approach of That said, I don't see any huge issue with allowing a second |
|
Changes Unknown when pulling 63225b2 on js-sha3 into ** on master**. |
| @@ -91,7 +91,7 @@ export default class Registry { | |||
|
|
|||
| lookupAddress (_name) { | |||
| const name = _name.toLowerCase(); | |||
| const sha3 = this._api.util.sha3(name); | |||
| const sha3 = this._api.util.sha3.text(name); | |||
There was a problem hiding this comment.
Ugly as sin, but ok, I don't have any other suggestions atm ;)
|
tests failing |
|
fwiw i don't think solidity function names are much of an issue since they can't start with |
jacogr
added
A8-looksgood 🦄
|
Changes Unknown when pulling a491c92 on js-sha3 into ** on master**. |
* Auto-detect hex encoded bytes in sha3 * Using types/isHex * Removing unused imports
* Ignore get_price_info test by default. (#4112) * Auto-detect hex encoded bytes in sha3 (#4108) * Auto-detect hex encoded bytes in sha3 * Using types/isHex * Removing unused imports * Use binary chop to estimate gas accurately (#4100) * Initial sketch. * Building. * Fix a few things. * Fix issue, add tracing. * Address grumbles * Raise upper limit if needed * Fix test. * Fixing decoding API with signatures in names (#4125) * Fix call/estimate_gas (#4121) * Return 0 instead of error with out of gas on estimate_gas * Fix stuff up.
Closes #4081
@gavofyork TBH I find previous behaviour more sensible, we have couple of contexts where we explicitly don't want to interpret strings starting with
0xas bytes (e-mail addresses, solidity function names, etc.).It would make sense if sha3 accepted ONLY 0x-prefixed, hex-encoded strings, then in most context we would use it as:
sha3(asciiToHex("asdf")), right now we kind of mix both approaches.