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 obfuscate coordinates implementation #930

Merged
merged 2 commits into from
Sep 14, 2018

Conversation

Gnito
Copy link
Contributor

@Gnito Gnito commented Sep 14, 2018

Some coordinates were outside of given offset radius.

@Gnito Gnito requested a review from kpuputti September 14, 2018 11:46
@Gnito
Copy link
Contributor Author

Gnito commented Sep 14, 2018

Post review more than welcome

@Gnito Gnito merged commit 767c4c7 into master Sep 14, 2018
@Gnito Gnito deleted the fix-obfuscate-coordinates-impl branch September 14, 2018 11:47
@@ -12,6 +12,9 @@ way to update this template, but currently, we follow a pattern:

---
## Upcoming version 2018-09-XX
* [fix] obfuscatedCoordinatesImpl didn't always return coordinates within given
offset radius.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a generic note on the abstraction level of this description: I think it is too detailed. Something like "Improved fuzzy location calculation" would probably suffice for people who read the changelog.

/**
* This obfuscatedCoordinatesImpl function is a temporary solution for the coordinate obfuscation.
* In the future, improved version needs to have protectedData working and
* available in accepted transaction.
*
* Based on:
* https://gis.stackexchange.com/questions/25877/generating-random-locations-nearby#answer-213898
Copy link
Contributor

Choose a reason for hiding this comment

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

I truly wish we had better sources for reference than a random answer with three upvotes :/ But I hope you tested this well, no need to change if it works.

Copy link
Contributor

@kpuputti kpuputti left a comment

Choose a reason for hiding this comment

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

Just a couple of notes, otherwise nothing special to add to this PR.

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.

2 participants