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

feat : Implement Hasher trait for Tip5 #248

Merged

Conversation

elielnfinic
Copy link
Contributor

@elielnfinic elielnfinic commented Jan 15, 2025

This PR fixes #207

@elielnfinic elielnfinic marked this pull request as ready for review January 15, 2025 12:57
Copy link
Member

@jan-ferdinand jan-ferdinand left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on! Right now, there are some problems with the implementation. They should be relatively easy to resolve, though. 🙂

twenty-first/src/math/tip5.rs Outdated Show resolved Hide resolved
twenty-first/src/math/tip5.rs Outdated Show resolved Hide resolved
twenty-first/src/math/tip5.rs Outdated Show resolved Hide resolved
twenty-first/src/math/tip5.rs Outdated Show resolved Hide resolved
twenty-first/src/math/tip5.rs Show resolved Hide resolved
twenty-first/src/math/tip5.rs Outdated Show resolved Hide resolved
twenty-first/src/math/tip5.rs Outdated Show resolved Hide resolved
twenty-first/src/math/tip5.rs Outdated Show resolved Hide resolved
twenty-first/src/math/tip5.rs Outdated Show resolved Hide resolved
twenty-first/src/math/tip5.rs Outdated Show resolved Hide resolved
@jan-ferdinand
Copy link
Member

Note also that we're using conventional commits for our git commit messages. Before merging this PR, I'd like you to squash the commits and use a message like “feat(Tip5): Implement the Hasher trait”. Feel free to change the message if you like. 👍

@elielnfinic
Copy link
Contributor Author

Thanks @jan-ferdinand for your review and suggestions. I hope I was able to resolve your review messages. Please check out this new commit.

Copy link
Member

@jan-ferdinand jan-ferdinand left a comment

Choose a reason for hiding this comment

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

Cool, this is coming together nicely. I've left a few nits inline. Since I have crammed a bunch of comments into a rather tight space, here's what I re-wrote the second loop to on my end. Feel free to pick whetever you like, there's no obligation to take any of it.

for chunk in bfield_elements.chunks(Tip5::RATE).into_iter() {
    let mut buffer = [BFieldElement::ZERO; Tip5::RATE];
    for (buffer_elem, chunk_elem) in buffer.iter_mut().zip(chunk) {
        *buffer_elem = chunk_elem;
    }
    self.absorb(buffer)
}

Lastly, the commit message looks good now. 👌 Can I ask you to also change the commit's description? “Adding test” and “Renaming test” don't really describe what's going on. 😉 You can also leave it empty if you want; I think the description by itself can be sufficient. Your choice. 🙂

twenty-first/src/math/tip5.rs Outdated Show resolved Hide resolved
twenty-first/src/math/tip5.rs Outdated Show resolved Hide resolved
twenty-first/src/math/tip5.rs Outdated Show resolved Hide resolved
twenty-first/src/math/tip5.rs Outdated Show resolved Hide resolved
twenty-first/src/math/.tip5.rs.pending-snap Outdated Show resolved Hide resolved
@elielnfinic
Copy link
Contributor Author

I like this suggestion. Thank you.

@coveralls
Copy link

Coverage Status

coverage: 97.811% (-0.01%) from 97.822%
when pulling 94bdc3a on elielnfinic:feat/hasher-for-tip5
into 2dba318 on Neptune-Crypto:master.

@jan-ferdinand
Copy link
Member

This is looking great now! ⭐

The only remaining nit I have is the commit's description (the message is great). You can change the description in a number of ways, for example

git commit --amend -m "feat(Tip5) : Implement the Hasher trait" -m "new description"

or using an interactive rebase with the “reword” option.

@jan-ferdinand jan-ferdinand self-requested a review January 20, 2025 12:16
@elielnfinic
Copy link
Contributor Author

My bad, I believe I had to clean that description the rebase msg file.
Just amended the commit.

Thank you.

@jan-ferdinand jan-ferdinand merged commit 9a6a74e into Neptune-Crypto:master Jan 21, 2025
@jan-ferdinand
Copy link
Member

Thanks for your contribution! 🎉

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.

implement Hasher for Tip5
3 participants