Domain separator during merge #40
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
0xkanekiken
force-pushed
the
kaneki-domain-separator
branch
from
c69c88f to
03a7f48
Compare
grjte
suggested changes
Jan 23, 2023
There was a problem hiding this comment.
Thanks @0xkanekiken looks to me good overall! Just a few small things inline
Al-Kindi-0
reviewed
Jan 23, 2023
Signed-off-by: 0xKanekiKen <100861945+0xKanekiKen@users.noreply.github.com>
0xkanekiken
force-pushed
the
kaneki-domain-separator
branch
2 times, most recently
from
7bc816f to
f0d0ad9
Compare
|
Thanks @grjte and @Al-Kindi-0 for the review. I've worked on your suggestions. |
0xkanekiken
force-pushed
the
kaneki-domain-separator
branch
from
f0d0ad9 to
b977239
Compare
0xkanekiken
requested review from
Al-Kindi-0 and
grjte
and removed request for
Al-Kindi-0
grjte
approved these changes
Jan 23, 2023
Signed-off-by: 0xKanekiKen <100861945+0xKanekiKen@users.noreply.github.com>
0xkanekiken
force-pushed
the
kaneki-domain-separator
branch
from
b977239 to
5757b89
Compare
|
Thanks! I've updated the doc string. |
bobbinth
reviewed
Jan 24, 2023
| @@ -51,6 +52,40 @@ fn hash_elements_vs_merge() { | |||
| assert_eq!(m_result, h_result); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn hash_elements_vs_merge_in_domain() { | |||
There was a problem hiding this comment.
I think the name of this test is a bit confusing: we are not testing whether hash_elements() produces a different result from merge_in_domain() but rather if merge_in_domain() is valid. So, I would probably rename this test to merge_in_domain().
Also, I wonder if we can replace this test with tests which do the following:
merge()andmerge_in_domain()give the same result when domain isZERO.merge_in_domain()gives two different results if the domains are different.
This was referenced Feb 6, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe your changes
This PR addresses #35. It introduces a new method
merge_in_domainas mentioned by @bobbinth here when impl Rpo256.Checklist before requesting a review
nextaccording to naming convention.