feat: Add zk gas per pubdata flag #845
Conversation
…eat-zk-gas-per-pubdata-flag
| @@ -133,6 +133,10 @@ pub struct ZkSyncArgs { | |||
| )] | |||
| #[serde(skip_serializing_if = "Option::is_none")] | |||
| pub suppressed_errors: Option<Vec<ZkSolcError>>, | |||
|
|
|||
| /// Gas per pubdata | |||
There was a problem hiding this comment.
| /// Gas per pubdata | |
| /// Gas per pubdata to be set on transactions when broadcasting on scripts |
|
|
||
| /// Gas per pubdata | ||
| #[clap(long = "zk-gas-per-pubdata", value_name = "GAS_PER_PUBDATA")] | ||
| pub gas_per_pubdata: Option<u64>, |
There was a problem hiding this comment.
It is not conceptually right to add this here as this is not a build option (same for paymaster_input as well). As those options are not for building they are for transactions. There's an analogous EVM set of opts that are in cli/src/opts/transactions.rs
There was a problem hiding this comment.
Also if we are just using for broadcast maybe it can just live in script args for now
| @@ -143,7 +149,10 @@ pub async fn estimate_fee<P: ZksyncProvider<T>, T: Transport + Clone>( | |||
| let max_priority_fee = tx.max_priority_fee_per_gas().unwrap_or(fee.max_priority_fee_per_gas); | |||
| tx.set_max_fee_per_gas(max_fee); | |||
| tx.set_max_priority_fee_per_gas(max_priority_fee); | |||
| tx.set_gas_per_pubdata(fee.gas_per_pubdata_limit); | |||
| match tx.gas_per_pubdata() { | |||
There was a problem hiding this comment.
Maybe pass the option here directly as a function argument instead of setting before? If option is None then use the rpc result, else use the option value.
| @@ -143,7 +149,10 @@ pub async fn estimate_fee<P: ZksyncProvider<T>, T: Transport + Clone>( | |||
| let max_priority_fee = tx.max_priority_fee_per_gas().unwrap_or(fee.max_priority_fee_per_gas); | |||
| tx.set_max_fee_per_gas(max_fee); | |||
| tx.set_max_priority_fee_per_gas(max_priority_fee); | |||
| tx.set_gas_per_pubdata(fee.gas_per_pubdata_limit); | |||
| match tx.gas_per_pubdata() { | |||
| Some(v) if !v.is_zero() => (), | |||
| @@ -23,13 +23,20 @@ pub struct ZkEnv { | |||
| pub fair_l2_gas_price: u64, | |||
| /// fair pubdata price | |||
| pub fair_pubdata_price: u64, | |||
| /// gas per pubdata | |||
| pub gas_per_pubdata: Option<u64>, | |||
There was a problem hiding this comment.
This should not belong to ZkEnv zk env simulates network params. This is a field set by the user specifying what price they should pay at max for gas_per_pubdata (If the network has a value that's higher, the tx gets reverted). If we need to keep in context, we should put this wherever paymaster_params are set as this is a value of the same nature (a param to add to transactions)
| @@ -141,6 +141,11 @@ pub async fn send_transaction( | |||
| }, | |||
| ); | |||
| } | |||
|
|
|||
| if let Some(gas_per_pubdata) = zk_tx_meta.gas_per_pubdata { | |||
There was a problem hiding this comment.
Since the only use we are giving this is overriding the value when broadcasting, we don't need to do this dance of adding the config value to the CheatcodeInspector then setting on ZkTransactionMetadata. We can just access this from right here by using self.script_config.config.zksync.gas_per_pubdata. The value of adding to ZkTransactionMetadata would be if we also used this when creating the broadcastable transactions/running the simulation, but until then probably not worth it.
What 💻
Why ✋
Evidence 📷
Include screenshots, screen recordings, or
consoleoutput here demonstrating that your changes work as intendedDocumentation 📚
Please ensure the following before submitting your PR: