Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change: Update sdb/storage-related ops warm check fields #452

Merged
merged 2 commits into from
Apr 12, 2022

Conversation

CPerezz
Copy link
Member

@CPerezz CPerezz commented Apr 11, 2022

TxAccessListAccountOp, TxAccessListAccountStorageOp and
AccountDestructedOp contain fields called value and value_prev
which basically are bools marking whether the access to these values in the operation are WARM or not.

Makes more sense to name them as is_warm/is_warm_prev
(is_destructed/is_destructed_prev for AccountDestructedOp.

Resolves: #442

@CPerezz CPerezz requested a review from a team as a code owner April 11, 2022 13:22
@CPerezz CPerezz requested review from ChihChengLiang and removed request for a team April 11, 2022 13:22
@github-actions github-actions bot added crate-bus-mapping Issues related to the bus-mapping workspace member T-opcode Type: opcode-related and focused PR/Issue labels Apr 11, 2022
@github-actions github-actions bot added crate-zkevm-circuits Issues related to the zkevm-circuits workspace member and removed T-opcode Type: opcode-related and focused PR/Issue labels Apr 11, 2022
Copy link
Member

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

Overall looks good! Please take a look at my comments to update the string used for debug format :)

bus-mapping/src/operation.rs Show resolved Hide resolved
bus-mapping/src/operation.rs Show resolved Hide resolved
bus-mapping/src/operation.rs Show resolved Hide resolved
`TxAccessListAccountOp`, `TxAccessListAccountStorageOp` and
`AccountDestructedOp` contain fields called `value` and `value_prev`
which basically are `bool`s marking whether the access to these values in the operation are WARM or not.

Makes more sense to name them as `is_warm`/`is_warm_prev`
(is_destructed/is_destructed_prev` for `AccountDestructedOp`.

Resolves: #442
Copy link
Member

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

LGTM! This is a nice readability improvement :)

@CPerezz CPerezz merged commit d525968 into main Apr 12, 2022
@CPerezz CPerezz deleted the storageop_field_rename branch April 12, 2022 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename some Ops value and value_prev fields as the names don't identify with bool
2 participants