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

fix money validate range #815

Closed
wants to merge 18 commits into from
Closed

fix money validate range #815

wants to merge 18 commits into from

Conversation

heroboy
Copy link
Contributor

@heroboy heroboy commented Oct 29, 2018

fix #812

@Hadis-Knj
Copy link

Would be ✨ if you write a test for it too

@Hadis-Knj
Copy link

@heroboy thanks for preparing this PR, it looks good to me, would you also please fix the linter issues, you can run npm run lint to get the log of the issues

@heroboy
Copy link
Contributor Author

heroboy commented Nov 23, 2018

@Hadis-Fard any update on this? What I need to do more?

@arthurschreiber
Copy link
Collaborator

@heroboy Thanks for this fix! I'll try and get it merged asap! 🙇

this.writeInt32LE(Math.floor(value * SHIFT_RIGHT_32));
//floor(-1.9) === -2
//-1.9|0 === -1
this.writeInt32LE((value * SHIFT_RIGHT_32) | 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would be simpler. I think this.writeInt32LE is already doing the | 0 operation internally. 👍

Suggested change
this.writeInt32LE((value * SHIFT_RIGHT_32) | 0);
this.writeInt32LE(value * SHIFT_RIGHT_32);

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 don't think so. For example: buf.writeInt32LE((0x7fffffff)+0.5) doesn't work. The |0 is to ensure the parameter is an int. And without this, it doesn't pass my money-type-test.

@heroboy
Copy link
Contributor Author

heroboy commented Dec 3, 2018

hi @arthurschreiber @Hadis-Fard
I rewrite the writeMoney function, because my original version is not correct to write negative value. For example, if value is -100:

this.writeInt32LE(Math.floor(value * SHIFT_RIGHT_32)); // write -1, correct
this.writeInt32LE((value * SHIFT_RIGHT_32) | 0);//write 0, not correct

I add some integration test to test these. Hope this works.

@heroboy
Copy link
Contributor Author

heroboy commented Apr 15, 2019

any update on this? If my pr is not good, I don't mind you making yours.
I just want to fix this.

@IanChokS IanChokS added the Follow up Recent question asked by tedious members for old issues or discussion that requires member input label Dec 13, 2019
@IanChokS
Copy link
Member

IanChokS commented Dec 13, 2019

@heroboy Looks like your branch is a bit out of date. Do you mind updating it, resolving the conflict and we can try to move forward with this? Thanks!

@codecov
Copy link

codecov bot commented Dec 16, 2019

Codecov Report

Merging #815 into master will decrease coverage by 1.25%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #815      +/-   ##
==========================================
- Coverage   80.77%   79.51%   -1.26%     
==========================================
  Files          86       86              
  Lines        4384     4403      +19     
  Branches      793      794       +1     
==========================================
- Hits         3541     3501      -40     
- Misses        584      644      +60     
+ Partials      259      258       -1     
Impacted Files Coverage Δ
src/data-types/money.ts 92.30% <0.00%> (-7.70%) ⬇️
src/tracking-buffer/writable-tracking-buffer.ts 64.45% <0.00%> (-7.36%) ⬇️
src/incoming-message-stream.ts 92.30% <0.00%> (-5.13%) ⬇️
src/connection.ts 60.68% <0.00%> (-3.63%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fb901a...3d8cccd. Read the comment docs.

@heroboy
Copy link
Contributor Author

heroboy commented Dec 17, 2019

Fix all. but I don't know how to fix the commit lint.

@IanChokS
Copy link
Member

@heroboy Thanks for updating this PR! I'm not sure how to fix the lint issues as well, but I'll ping @arthurschreiber for him to take a look at this. 🙇

@IanChokS IanChokS added Action Required An action is needed by Tedious member Response needed Response by tedious member is needed labels Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Action Required An action is needed by Tedious member Follow up Recent question asked by tedious members for old issues or discussion that requires member input Response needed Response by tedious member is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Too large money value will make nodejs crash.
5 participants