privacy-scaling-explorations / zkevm-circuits

https://privacy-scaling-explorations.github.io/zkevm-circuits/
Other
818 stars 854 forks source link

Rename Word::zero to Word::zero_expr #1748

Closed ChihChengLiang closed 8 months ago

ChihChengLiang commented 8 months ago

Description

Address https://github.com/privacy-scaling-explorations/zkevm-circuits/pull/1746#issuecomment-1918615874, we rename the constructors for zero word and one word.

Type of change

Contents

KimiWu123 commented 8 months ago

Another thoughts: how about define a trait with one(), zero() and let implement it for for some basic types ? Idea is to keep succinct Word::one() Word:zero() and let type system infer for us

I tried to to have a trait for zero() and one(), but the compiler complained something like one() has been implemented.... Probably, there are some tricks to fix the error but I'm not familiar with it. Maybe you could give it a try!

hero78119 commented 8 months ago

Another thoughts: how about define a trait with one(), zero() and let implement it for for some basic types ? Idea is to keep succinct Word::one() Word:zero() and let type system infer for us

I tried to to have a trait for zero() and one(), but the compiler complained something like one() has been implemented.... Probably, there are some tricks to fix the error but I'm not familiar with it. Maybe you could give it a try!

I tried to implement it in another PR and seems to work https://github.com/privacy-scaling-explorations/zkevm-circuits/pull/1754

If new PR is acceptable I think we can close this one.

ChihChengLiang commented 8 months ago

@hero78119 Nice approach on #1754. I built on top of #1754 to get #1758, which removes the need to type hint the word.

@hero78119 @KimiWu123 Can you share your opinion on #1758?

hero78119 commented 8 months ago

@hero78119 Nice approach on #1754. I built on top of #1754 to get #1758, which removes the need to type hint the word.

@hero78119 @KimiWu123 Can you share your opinion on #1758?

Just different topic

For unifying compress_f() to compress(), due to it got different implementation on different generic type, so it's can't be resolve like the same way. We might require feature like

Maybe we keep compress_f() first and left it to future work

ChihChengLiang commented 8 months ago

Close in favor of #1758