-
Notifications
You must be signed in to change notification settings - Fork 856
Feat/CREATE Part C - bus-mapping and gadget implementation #1430
Conversation
d23050d
to
d37a19a
Compare
d37a19a
to
6b274a1
Compare
64a4d9f
to
a6ad558
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add small comments, others LGTM.
Hi @KimiWu123, please help resolve the fixed conversations. Seems there is no Resolve conversation
button on my side. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Added just a couple of questions.
CircuitTestBuilder::new_from_test_ctx(ctx).run(); | ||
} | ||
|
||
// RETURN or REVERT with data of [0x60; 5] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a short description why this particular initialization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The init code content doesn't matter, could put any random data. We don't really run the piece of the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I thought so - perhaps just add a comment that any code would do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in ec9c6de
code | ||
} | ||
|
||
fn creater_bytecode( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: perhaps change creater
to creator
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 59cf61f
code.append(&bytecode! { | ||
PUSH1(initialization_bytes.len()) // size | ||
PUSH1(32 - initialization_bytes.len()) // length | ||
PUSH2(23414) // value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PUSH2(23414) // value | |
PUSH2(23414) // value (endowment in CREATE2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 59cf61f
let gas_cost = GasCost::CREATE.expr() + memory_expansion.gas_cost() + keccak_gas_cost; | ||
let gas_remaining = cb.curr.state.gas_left.expr() - gas_cost.clone(); | ||
let gas_left = ConstantDivisionGadget::construct(cb, gas_remaining.clone(), 64); | ||
let callee_gas_left = gas_remaining - gas_left.quotient(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does there need to be also the reduction of gas (for CreateDataGas
) for storing the code when there is no error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right! fixed in cccd6a8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approve for @silathdiir to make the auto-merge work.
Description
NOTE: This is an updated version of #1358
This PR is actually based on top of #1425
Issue Link
#1130
Type of change
Contents
Bus mapping implementation for CREATE/CREATE2
EVM circuit's gadget for CREATE/CREATE2
How Has This Been Tested?
Tests can be found here: https://github.com/scroll-tech/zkevm-circuits/blob/2d2bfc6ccf179ade1a8d063f9586b93e5283a557/zkevm-circuits/src/evm_circuit/execution/create.rs#L678