feat: add sload op (non-bus-mapping version)#334
Conversation
github-actions
Bot
added
the
crate-bus-mapping
0xmountaintop
force-pushed
the
re-init
branch
from
167b1c3 to
8c3987c
Compare
github-actions
Bot
removed
the
crate-bus-mapping
0xmountaintop
changed the title
sload opsload op (non-bus-mapping version)
CPerezz
left a comment
CPerezz
left a comment
There was a problem hiding this comment.
A couple of nits and questions.
But so far LGTM. I don't see any inconsistencies with the specs.
| /// Transaction ID: Transaction index in the block starting at 1. | ||
| pub tx_id: usize, | ||
| /// Storage Value before the transaction | ||
| pub committed_value: Word, |
There was a problem hiding this comment.
I'm not sure committed_value is the best name to describe the Value before the transaction.
Since we have value_prev we can also have value_after maybe.
| } | ||
| } | ||
|
|
||
| pub fn aux_pair(&self) -> (usize, Word) { |
There was a problem hiding this comment.
If this is going to be exported as pub, add some docs for it please.
| .input | ||
| .0 | ||
| .iter() | ||
| .fold(0, |acc, byte| acc + if *byte == 0 { 4 } else { 16 }); |
There was a problem hiding this comment.
We might want to add Byte storage costs or something similar in like an enum form. So that these values are contained in a const or somewhere similar and if they change or whatever, we just need to change the constant to update them all.
No problem. I will fix it |
ed255
left a comment
ed255
left a comment
There was a problem hiding this comment.
LGTM! Thanks for working on this opcode :)
I've left a suggestion in my review, feel free to take a look and update if you consider it appropriate. Otherwise we can proceed with merging this PR!
…reum#334) * modify pi circuit to be compatible with bridge contract * fix * fmt
4 participants
privacy-ethereum/zkevm-specs#106
Notes:
sstoreare more complicated.sloadop (bus-mapping version) #347