Skip to content

Conversation

@dcousens
Copy link
Contributor

This PR fixes a small mistake in isScriptHashInput which meant a script would throw during classification of a Script rather than return false.

It fixes #344 properly, despite the original test case being fixed by #354.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if we should be checking specifically for a RangeError vs this catch all...

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering the same. How about catching all for now and console.debug the exception just in case someone may wonder at which stage the classification returned. Hopefully they report back if it's an unexpected case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an interesting concept, how would this be any different than returning a console.debug for every other instance where it returned false?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) to 97.97% when pulling c982622 on scriptfix into d68eb49 on master.

@dcousens
Copy link
Contributor Author

Will release 1.4.4 after this is merged.

@dcousens
Copy link
Contributor Author

Going to merge, @weilu if you could still review post-hoc that'd be awesome. :)

dcousens added a commit that referenced this pull request Feb 19, 2015
isScriptHashInput logic fix
@dcousens dcousens merged commit e0b1aa8 into master Feb 19, 2015
@dcousens dcousens deleted the scriptfix branch February 19, 2015 08:02
@weilu
Copy link
Contributor

weilu commented Feb 20, 2015

I did review and suggested that we console.debug the exception caught. The rest looks good

@dcousens dcousens added this to the 1.4.5 milestone Feb 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RangeError: Trying to access beyond buffer length

4 participants