Util/validation update #4051
Util/validation update #4051
Conversation
jacogr
commented
Jan 5, 2017
jacogr
commented
- add pre-emptive null code check in validateCode
- don't pass external api to any functions
- introduce tests for full suite
| @@ -42,9 +44,14 @@ export function validateAbi (abi) { | |||
| try { | |||
| abiParsed = JSON.parse(abi); | |||
|
|
|||
| if (!util.isArray(abiParsed)) { | |||
| if (!apiutil.isArray(abiParsed)) { | |||
There was a problem hiding this comment.
While we're at it: I'd prefer Array.isArray here. It's less brittle and it's widely known so you don't have to jump to another place to find out what it does.
There was a problem hiding this comment.
Honestly see no issue with the age-old Object.prototype.toString.call(obj) === '[object Array] underlying implementation there. Could also use lodash isArray here, etc. Don't see benefit of the one over the other, if the underlying implementation is an issue, would then see that adjusted since it is used is multiple places.
There was a problem hiding this comment.
the underlying implementation is an issue. i can easily implement something that is not an array (not even iterable) but passes apiutil.isArray.
I'm suggesting you a API-compatible one-line change to the more robust, core JS Array.isArray. I'm also fine with lodash isArray`. I just see no reason to use homegrown check, especially if they're more brittle then the builtins.
I don't want to start an ideological war, this is only about reducing possible sources of errors.
There was a problem hiding this comment.
Not an ideological war - there is a reason the api utilities has the isFunction, isArray, isObject, etc. This is to allow consistency instead of things being done differently all-over. So if the underlying implementations can be made better and filter through to everything, we should do so by all means.
There was a problem hiding this comment.
Ok, then let's make apiutil.isArray reference Array.isArray.
| }); | ||
|
|
||
| it('fails on a non-full length 00..00 value', () => { | ||
| expect(isNullAddress(NULL_ADDRESS.slice(-1 * (NULL_ADDRESS.length - 2)))).to.be.false; |
| expect(isNullAddress(NULL_ADDRESS.slice(-1 * (NULL_ADDRESS.length - 2)))).to.be.false; | ||
| }); | ||
|
|
||
| it('fails on a valid addess, non 00..00 value', () => { |
There was a problem hiding this comment.
[tiny grumble] "fails" indicates that it throws an error, but it rather returns false.
|
Would like |
derhuerst
added
A8-looksgood 🦄
