Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using byte lengths instead of hexstring lengths #101

Merged
merged 2 commits into from
Dec 11, 2017

Conversation

notatestuser
Copy link
Contributor

As discussed in #80

src/utils.js Outdated
if (typeof num !== 'number') throw new Error('num must be numeric')
if (num < 0) throw new RangeError('num is unsigned (>= 0)')
size = size * 2
if (size % 2 !== 0) throw new RangeError('size must be a multiple of 2')
Copy link
Member

Choose a reason for hiding this comment

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

i think the error msg should be about it being an integer. If it is an integer, it will always be multiple of 2 as you multipled it by 2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, it's essentially acting as an integer check there. I'll change that check/message

Copy link
Member

Choose a reason for hiding this comment

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

mm the rest looks good, will merge once i see this change

@notatestuser notatestuser force-pushed the feature/byte-lengths branch 2 times, most recently from e1401db to 94c8296 Compare December 11, 2017 05:51
@snowypowers snowypowers merged commit be83a8a into CityOfZion:dev Dec 11, 2017
@notatestuser
Copy link
Contributor Author

Should be careful with this change if we think dependent apps might be consuming the utils directly.

@snowypowers
Copy link
Member

good point. will ask around. v3? ugh dont feel like i should be jumping the majors so fast hahaaha

@notatestuser
Copy link
Contributor Author

If we're following semantic versioning then yeah it should be v3 since those functions were exported.

However now that everyone uses a lockfile with npm/yarn nowadays and because this is technically still just the library for neon-wallet a minor jump could be okay imo.

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.

None yet

2 participants