Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

Simplify Zero and One Words construction and Address compression #1746

Merged
merged 7 commits into from
Jan 31, 2024

Conversation

ChihChengLiang
Copy link
Collaborator

@ChihChengLiang ChihChengLiang commented Jan 30, 2024

Description

While reviewing #1699, I noticed some room to improve the ergonomics of the words.

Issue Link

Type of change

New feature (non-breaking change which adds functionality)

Contents

  • Replaced the address_word_to_expr function with the compress method so that we can compress a word directly without finding that function to use.
  • introduced zero_f and one_f to create zero and one for Word<F>.

Rationale

I noticed that it is hard to create a one() and zero() method for both Word<Expression<F>> and Word<F>. The compiler complains about implementing duplicated methods. Since we already use one() and zero() in many places for Word<Expression<F>>, I named zero_f and one_f for the methods for Word<F>.

@github-actions github-actions bot added the crate-zkevm-circuits Issues related to the zkevm-circuits workspace member label Jan 30, 2024
@KimiWu123 KimiWu123 self-requested a review January 31, 2024 05:58
Copy link
Contributor

@KimiWu123 KimiWu123 left a comment

Choose a reason for hiding this comment

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

LGTM! The code looks nicer and cleaner now!

However, one_f and zero_f are not straightforward to me and it's easy to get confused. Give couples of hours to see if I can find a better solution. If no, then I'll approve it. 🙏

@hero78119 hero78119 self-requested a review January 31, 2024 08:15
@KimiWu123
Copy link
Contributor

LGTM! The code looks nicer and cleaner now!

However, one_f and zero_f are not straightforward to me and it's easy to get confused. Give couples of hours to see if I can find a better solution. If no, then I'll approve it. 🙏

@ChihChengLiang, I didn't find any good solutions. But I was thinking to use zero_expr() for Word<Expression<F>> and zero() for Word<F>. The methond name is more aligned with the generic type (_expr maps <Expression<F>>).

Copy link
Member

@hero78119 hero78119 left a comment

Choose a reason for hiding this comment

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

LGTM!

@ChihChengLiang
Copy link
Collaborator Author

@KimiWu123 I think you're reasoning makes sense to me. I'll merge this then submit a new one for the remaining.
Thank you @KimiWu123 and @hero78119 for the review.

@ChihChengLiang ChihChengLiang added this pull request to the merge queue Jan 31, 2024
Merged via the queue into main with commit 7f35654 Jan 31, 2024
13 checks passed
@ChihChengLiang ChihChengLiang deleted the improve-wordexpr branch January 31, 2024 11:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
crate-zkevm-circuits Issues related to the zkevm-circuits workspace member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants