feat: don't charge for pubdata in Validium mode #32
Conversation
…nc-era into test-erc20-pubdata-0
Oppen
force-pushed
the
validium_mode_pubdata_0_env
branch
from
d4cc570 to
573975a
Compare
Oppen
force-pushed
the
validium_mode_pubdata_0_env
branch
from
0ea1067 to
c71ff9c
Compare
* Add validium example readme * update validium.md and change test name * fmt --------- Co-authored-by: toni-calvin <[email protected]>
Oppen
changed the title
ETH_SENDER_SENDER_VALIDIUM_MODE to check whether we need to cha…
* Update output readme * Add a note for transactions * Fix note
* remove logs from pubdata * update comment
* Refactor readme example * Fix some comments * Remove validium.md
There was a problem hiding this comment.
I understand that this is a PoC phase, so please don't treat my comments as criticism of the existing solution; I understand that there will be some refactoring/rework before validium mode is added to the main codebase.
I just thought it may be helpful to elaborate on our expectations code-wise.
| // This value will typically be a lot less than u64 | ||
| // unless the gas price on L1 goes beyond tens of millions of gwei | ||
| // TODO: make this check only once | ||
| let validium_mode = std::env::var("ETH_SENDER_SENDER_VALIDIUM_MODE") == Ok("true".to_string()); |
There was a problem hiding this comment.
VM should have no assumptions regarding available environment variables.
In general, the only source of environment variables right now is the zksync_env_config crate.
In the future, we'll have different configuration schemas not based on the env variables (think JSON configs), so you should present configuration in an explicit and serialization-agnostic form.
| @@ -456,9 +456,17 @@ fn get_pubdata_price_bytes(initial_value: U256, final_value: U256, is_initial: b | |||
| compress_with_best_strategy(initial_value, final_value).len() as u32; | |||
|
|
|||
| if is_initial { | |||
| (BYTES_PER_DERIVED_KEY as u32) + compressed_value_size | |||
| println!("is initial"); | |||
There was a problem hiding this comment.
Debugging statements obviously should not be present in the code that is to be merged.
There was a problem hiding this comment.
Of course, there will be a clean PR to your repo when finished
There was a problem hiding this comment.
Btw, we'll remove this rn cause is not useful anymore
| @@ -0,0 +1,23 @@ | |||
| // SPDX-License-Identifier: MIT | |||
There was a problem hiding this comment.
Once we get closer to the mergeable solution, we'd expect the proper logical structure of the project (this applies to test code as well -- I assume this package will evolve to some form of integration test).
Thanks for the review! It is very useful for us to have fast feedback on our changes in the codebase to converge on a solution that fits your expectations. I'll try to answer the comments asap. As a suggestion for the future, our main branch is |
feat: remove get_l1_processing_details method
What ❔
Check for the
ETH_SENDER_SENDER_VALIDIUM_MODEvariable while computing the fees to avoid charging for pubdata when that is set totrue.Adds an integration test that prints the fees of minting and transferring a token.
Why ❔
Users and operators expect reduced costs compared to rollup due to not storing the pubdata in the L1.
Run
zktool:Inside
zksync-erarunzktool to initiate validium:validium.mdguide.Debugging