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

Replace word::Word with WordLoHi #1747

Merged
merged 3 commits into from
Feb 7, 2024
Merged

Replace word::Word with WordLoHi #1747

merged 3 commits into from
Feb 7, 2024

Conversation

zemse
Copy link
Member

@zemse zemse commented Jan 31, 2024 •
edited
Loading

Description

Replacing Word with WordHiLo.

Issue Link

#1736

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Refactor

Contents

  • Replace word::Word with WordLoHi
  • Replace word::WordCell with WordLoHiCell

Rationale

  • WordLimbs and WordCell is used somewhere so couldn't go with that.
  • ZWord and CWord could be used, however, I saw that Word is actually an alias for Word2 (source). Which is an alias of WordLimbs<2>. So Word2 could be better than ZWord or CWord, but I think WordHiLo is even better and makes the code more obvious, since we have hi-lo terminology in many places.

How Has This Been Tested?

Built all packages locally.

@github-actions github-actions bot added the crate-zkevm-circuits Issues related to the zkevm-circuits workspace member label Jan 31, 2024
@zemse zemse linked an issue Jan 31, 2024 that may be closed by this pull request
Two Word types in zkEVM codebase #1736
Closed
@github-actions github-actions bot added the CI Issues related to the Continuous Integration mechanisms of the repository. label Jan 31, 2024
ChihChengLiang
ChihChengLiang reviewed Feb 1, 2024
View reviewed changes
Copy link
Collaborator

@ChihChengLiang ChihChengLiang left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice renaming! After this PR, we no longer need to write word::Word everywhere.
I left some comments.

.github/pull_request_template.md Show resolved Hide resolved
zkevm-circuits/src/circuit_tools/cell_manager.rs Outdated Show resolved Hide resolved
@zemse zemse changed the title Replace word::Word with WordHiLo Replace word::Word with WordLoHi Feb 6, 2024
ChihChengLiang
ChihChengLiang approved these changes Feb 6, 2024
View reviewed changes
Copy link
Collaborator

@ChihChengLiang ChihChengLiang left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the fix ❤️

@KimiWu123 KimiWu123 self-requested a review February 7, 2024 05:59
KimiWu123
KimiWu123 approved these changes Feb 7, 2024
View reviewed changes
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! Feel free to merge it after light testing is fixed.

@zemse zemse added this pull request to the merge queue Feb 7, 2024
Merged via the queue into privacy-scaling-explorations:main with commit 8ba838b Feb 7, 2024
15 checks passed
@zemse zemse deleted the 1736-word-rename branch February 7, 2024 10:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@KimiWu123 KimiWu123 KimiWu123 approved these changes

@ChihChengLiang ChihChengLiang ChihChengLiang approved these changes

Assignees
No one assigned
Labels
CI Issues related to the Continuous Integration mechanisms of the repository. crate-zkevm-circuits Issues related to the zkevm-circuits workspace member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Two Word types in zkEVM codebase
3 participants
@zemse @ChihChengLiang @KimiWu123