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

feat: add sload op (bus-mapping version)#347

Closed
0xmountaintop wants to merge 49 commits into
privacy-ethereum:mainfrom
scroll-tech:busmapping/sload_new
Closed

feat: add sload op (bus-mapping version)#347
0xmountaintop wants to merge 49 commits into
privacy-ethereum:mainfrom
scroll-tech:busmapping/sload_new

Conversation

@0xmountaintop
Copy link
Copy Markdown
Collaborator

@0xmountaintop 0xmountaintop commented Feb 24, 2022

privacy-ethereum/zkevm-specs#106

Notes:

  1. would like to separate sload and sstore PR. because the gas calculation and the TxRefund calculation for sstore are more complicated.
  2. non-bus-mapping version: feat: add sload op (non-bus-mapping version) #334

Since the bus-mapping version tests cannot really cover 1) is_warm 2) reverted cases, I just keep both "bus-mapping" and "handwrite rw_table" versions.

@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 crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels Feb 24, 2022
@ChihChengLiang
Copy link
Copy Markdown
Collaborator

Can we also rebase and squash some commits for reviewability?

@CPerezz
Copy link
Copy Markdown
Contributor

CPerezz commented Mar 4, 2022

If it's not ready for review also, can we set it as WIP? And put the PR as draft?

@0xmountaintop
Copy link
Copy Markdown
Collaborator Author

0xmountaintop commented Mar 4, 2022

It can be reviewed. it's just a different version of #334 and I am not sure which version is preferred.

But if "rebase/squash" before review is preferable as @ChihChengLiang suggests, I can turn it to WIP or draft.

@0xmountaintop 0xmountaintop marked this pull request as draft March 4, 2022 13:59
@CPerezz
Copy link
Copy Markdown
Contributor

CPerezz commented Mar 4, 2022

It can be reviewed. it's just a different version of #334 and I am not sure which version is preferred.

But if "rebase/squash" before review is preferable as @ChihChengLiang suggests, I can turn it to WIP or draft.

So we should review both and decide? What do they differ on?
For me, we can add the non-bus-mapping version and file an issue for the test addition once #349 is merged.

That PR will allow to build the tests you need.

@0xmountaintop
Copy link
Copy Markdown
Collaborator Author

For me, we can add the non-bus-mapping version and file an issue for the test addition once #349 is merged.

That PR will allow to build the tests you need.

Sounds good, I will close this for now.

@0xmountaintop 0xmountaintop deleted the busmapping/sload_new branch May 11, 2023 02:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member T-opcode Type: opcode-related and focused PR/Issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants