search

IntersectMBO / cardano-base

Code used throughout the Cardano eco-system
Apache License 2.0
95 stars 41 forks source link

Add RIPEMD160 hashing function #477

Closed paluh closed 5 months ago

paluh commented 5 months ago

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: https://github.com/cardano-foundation/CIPs/pull/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

paluh commented 5 months ago

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 commented 5 months ago

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 :wink:

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 https://github.com/IntersectMBO/cardano-base/pull/478?

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