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

Ripple hashes integration #1039

Merged
merged 12 commits into from
Oct 2, 2019
Merged

Conversation

Stormtv
Copy link
Contributor

@Stormtv Stormtv commented Sep 25, 2019

According to #960 there was a need to remove the ripple hashes dependency and locally include it in this project.

I went through and copied the functionality of ripple hashes and rewrote the library in Typescript and implemented it and the unit tests associated with it.

Let me know if you have any questions or feedback.

Thanks!

@intelliot intelliot self-requested a review September 26, 2019 10:37
@Stormtv
Copy link
Contributor Author

Stormtv commented Sep 26, 2019

One thing I removed from the ripple hashes lib was a dependency of https://github.com/crypto-browserify/createHash since this package only loads the createHash function from the node crypto package. I assume the existing webpack should browserify this without this package.

src/common/hashes/hashprefix.ts Outdated Show resolved Hide resolved
src/common/hashes/hashprefix.ts Outdated Show resolved Hide resolved
src/common/hashes/sha512hash.ts Outdated Show resolved Hide resolved
src/common/hashes/shamap.ts Outdated Show resolved Hide resolved
src/common/hashes/shamap.ts Outdated Show resolved Hide resolved
test/hashes-test.js Outdated Show resolved Hide resolved
test/hashes-test.js Outdated Show resolved Hide resolved
test/shamap-test.js Outdated Show resolved Hide resolved
test/shamap-test.js Outdated Show resolved Hide resolved
test/shamap-test.js Outdated Show resolved Hide resolved
Copy link
Contributor

@keefertaylor keefertaylor left a comment

Choose a reason for hiding this comment

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

With regards to the number / string change, I think your change looks reasonable. You might consider adding some tests to make sure that you have reasonable behavior when someone passes you 42.

I have mixed feelings on whether this made the code more readable. I do think it's slightly easier to assert that a number is 0-15 than a string is "0"-"F", so I think that's a win.

src/common/hashes/hash-prefix.ts Outdated Show resolved Hide resolved
src/common/hashes/sha512hash.ts Outdated Show resolved Hide resolved
src/common/hashes/shamap.ts Outdated Show resolved Hide resolved
src/common/hashes/shamap.ts Outdated Show resolved Hide resolved
@keefertaylor
Copy link
Contributor

Ship it!

@Stormtv
Copy link
Contributor Author

Stormtv commented Oct 1, 2019

Alright I think I've finished all the review changes lmk if there is anything else. If not this is good to go.

@sublimator
Copy link
Contributor

sublimator commented Oct 1, 2019 via email

@keefertaylor
Copy link
Contributor

@sublimator Can you clarify what you think is still missing here?

@sublimator
Copy link
Contributor

sublimator commented Oct 1, 2019 via email

@sublimator
Copy link
Contributor

sublimator commented Oct 1, 2019 via email

@Stormtv
Copy link
Contributor Author

Stormtv commented Oct 1, 2019

@sublimator No rush on this. If you need more time to review it we can wait to merge this until your approval.

@sublimator
Copy link
Contributor

sublimator commented Oct 1, 2019 via email

@sublimator
Copy link
Contributor

sublimator commented Oct 1, 2019 via email

@intelliot intelliot merged commit 14e6bf5 into XRPLF:develop Oct 2, 2019
@intelliot
Copy link
Collaborator

Looks good to me, big thanks to all of you for the work and reviews! I went ahead and squashed this PR with the merge, hope that's OK. Let me know if anyone experiences any issues.

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.

4 participants