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

Two Word types in zkEVM codebase #1736

Closed
zemse opened this issue Jan 17, 2024 · 3 comments · Fixed by #1747
Closed

Two Word types in zkEVM codebase #1736

zemse opened this issue Jan 17, 2024 · 3 comments · Fixed by #1747
Labels
T-feature Type: new features

Comments

@zemse
Copy link
Member

zemse commented Jan 17, 2024

Describe the feature you would like

Currently, there are two Word types in the codebase

It would be great if one of these is renamed.

Additional context

No response
@zemse zemse added the T-feature Type: new features label Jan 17, 2024
@rrtoledo
Copy link
Contributor

There were some proposals for Circuit's words at an online meeting including:

  • ZWord
  • CWord
  • WordLimbs
  • WordCell

@rrtoledo
Copy link
Contributor

There are also some types with the same names in bus mapping and zkevm-circuits such as Block (defined in bus-mapping/src/circuit_input_builder/block.rs and zkevm-circuits/src/witness/block.rs).
Do you think we should open an issue per homonyms or turn this one here for all?

@ChihChengLiang
Copy link
Collaborator

@rrtoledo Let's address the renaming of the Blocks in a new issue.

Feel free to choose any name you see fit for Word.

@zemse zemse mentioned this issue Jan 31, 2024
Merged
5 tasks
@zemse zemse linked a pull request Jan 31, 2024 that will close this issue
Replace word::Word with WordLoHi #1747
Merged
5 tasks
github-merge-queue bot pushed a commit that referenced this issue Feb 7, 2024
@zemse
Replace word::Word with WordLoHi (#1747)
8ba838b 
### 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
- [x] 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](https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/7f356548dd6dd614289b9700a40fbc80c4949b47/zkevm-circuits/src/util/word.rs#L203)).
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.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
T-feature Type: new features
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Replace word::Word with WordLoHi zemse/zkevm-circuits
3 participants
@ChihChengLiang @rrtoledo @zemse