Skip to content
New issue

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

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

Already on GitHub? # to your account

Add RIPEMD160 hashing function #477

Closed
wants to merge 0 commits into from
Closed

Conversation

paluh
Copy link
Collaborator

@paluh paluh commented May 28, 2024

This implementation closely follows the Keccak256.hs and also relies on cryptonite.

The PR is also related to CIP which was already pre-approved on the last CIPs Reviewers Meeting: cardano-foundation/CIPs#826
I've started the work on the Plutus PR and this function call was tested on that side: https://github.com/IntersectMBO/plutus/pull/6147/files#diff-53baa0990bb994c74e7cfd0bd3bfb7395d709593ea08a8e2fc7637dd28945c87R677

Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you. It would be nice to have some tests for hash functions, but that is also applicable to Keccak and others, so we'll postpone this to a later time. Since you are gonna be testing this functionality on the Plutus side anyways, I am not really worried about it.

You'll need #478 in order to pass CI, so if you could review that PR and then rebase on master once it is merged you'll be able to merge this one.

Also, after this one is merged if you can create a backport PR that would be great. Here are some guidelines for the backport process we use here in cardano-base: https://github.com/IntersectMBO/cardano-base/blob/master/RELEASING.md#backporting-changes

@paluh
Copy link
Collaborator Author

paluh commented May 30, 2024

Thanks a lot for quick response and review!

It would be nice to have some tests for hash functions, but that is also applicable to Keccak and others, so we'll postpone this to a later time. Since you are gonna be testing this functionality on the Plutus side anyways, I am not really worried about it.

I happy to provide/move/copy some of the unit tests from Plutus repo in a separate PR.

You'll need #478 in order to pass CI, so if you could review that PR and then rebase on master once it is merged you'll be able to merge this one.

Understood. So I'm waiting for the other merge to rebase.

Also, after this one is merged if you can create a backport PR that would be great. Here are some guidelines for the backport process we use here in cardano-base: https://github.com/IntersectMBO/cardano-base/blob/master/RELEASING.md#backporting-changes

The guide is really clear so I can follow :-) Should I backport both my change as well as #478?

@lehins
Copy link
Collaborator

lehins commented May 30, 2024

So I'm waiting for the other merge to rebase.

That other PR needs to be approved by someone to be merged, so if you could review and approve it then it will get merged right away 😉

I happy to provide/move/copy some of the unit tests from Plutus repo in a separate PR.

That's fine, don't worry about it.

Should I backport both my change as well as #478?

Yeah, I think you'll need to in order to pass the CI.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants