FM-163: clean code and finish review

fendermint/app/src/cmd/run.rs

@@ -6,7 +6,9 @@ use fendermint_abci::ApplicationService;
 use fendermint_app::{App, AppStore};
 use fendermint_rocksdb::{blockstore::NamespaceBlockstore, namespaces, RocksDb, RocksDbConfig};
 use fendermint_vm_interpreter::{
-    bytes::BytesMessageInterpreter, chain::ChainMessageInterpreter, fvm::FvmMessageInterpreter,
+    bytes::BytesMessageInterpreter,
+    chain::ChainMessageInterpreter,
+    fvm::{FvmMessageInterpreter, DEFAULT_GAS_RATE},
     signed::SignedMessageInterpreter,
 };
 use tracing::info;
@@ -20,7 +22,8 @@ cmd! {
 }
 
 async fn run(settings: Settings) -> anyhow::Result<()> {
-    let interpreter = FvmMessageInterpreter::<NamespaceBlockstore>::new();
+    let interpreter =
+        FvmMessageInterpreter::<NamespaceBlockstore>::new(DEFAULT_GAS_RATE, DEFAULT_GAS_RATE);
     let interpreter = SignedMessageInterpreter::new(interpreter);
     let interpreter = ChainMessageInterpreter::new(interpreter);
     let interpreter = BytesMessageInterpreter::new(interpreter);

fendermint/vm/interpreter/src/fvm/exec.rs

@@ -10,7 +10,7 @@ use fvm_shared::{address::Address, MethodNum, BLOCK_GAS_LIMIT};
 
 use crate::ExecInterpreter;
 
-use super::{state::FvmExecState, FvmMessage, FvmMessageInterpreter};
+use super::{state::FvmExecState, FvmMessage, FvmMessageInterpreter, DEFAULT_GAS_RATE};
 
 /// The return value extended with some things from the message that
 /// might not be available to the caller, because of the message lookups
@@ -26,7 +26,7 @@ pub struct FvmApplyRet {
 
 impl<DB> Default for FvmMessageInterpreter<DB> {
     fn default() -> Self {
-        Self::new()
+        Self::new(DEFAULT_GAS_RATE, DEFAULT_GAS_RATE)
     }
 }
 

fendermint/vm/interpreter/src/fvm/genesis.rs

@@ -195,7 +195,7 @@ mod tests {
             .await
             .expect("failed to create state");
 
-        let interpreter = FvmMessageInterpreter::new();
+        let interpreter = FvmMessageInterpreter::new(1.05, 1.05);
 
         let (state, out) = interpreter
             .init(state, genesis.clone())

fendermint/vm/interpreter/src/fvm/mod.rs

@@ -21,16 +21,28 @@ pub use query::FvmQueryRet;
 
 pub type FvmMessage = fvm_shared::message::Message;
 
+/// Default gas overestimation and search step
+/// default value
+pub const DEFAULT_GAS_RATE: f64 = 1.25;
+
 /// Interpreter working on already verified unsigned messages.
 #[derive(Clone)]
 pub struct FvmMessageInterpreter<DB> {
     _phantom_db: PhantomData<DB>,
+    /// Overestimation rate applied to gas to ensure that the
+    /// message goes through in the gas estimation.
+    gas_overestimation_rate: f64,
+    /// Gas search step increase used to find the optimal gas limit.
+    /// It determines how fine-grained we want the gas estimation to be.
+    gas_search_step: f64,
 }
 
 impl<DB> FvmMessageInterpreter<DB> {
-    pub fn new() -> Self {
+    pub fn new(gas_overestimation_rate: f64, gas_search_step: f64) -> Self {
         Self {
             _phantom_db: PhantomData,
+            gas_overestimation_rate,
+            gas_search_step,
         }
     }
 }

fendermint/vm/interpreter/src/fvm/query.rs

@@ -72,11 +72,15 @@ where
                         // return immediately if there is something is returned,
                         // it means that the message failed to execute so there's
                         // no point on estimating the gas.
+                        println!(">>>>>>>> RETURNING AFTER ESTIMATE GASSED MSG");
                         ret
                     }
                     None => {
                         // perform a gas search for an accurate value
-                        self.gas_search(&state, &msg)?
+                        println!(">>>>>>>> RETURNING BEFORE GAS SEARCH: {:?}", &msg.gas_limit);
+                        let ret = self.gas_search(&state, &msg)?;
+                        println!(">>>>>>>> RETURNING AFTER GAS SEARCH: {:?}", &ret.gas_limit);
+                        ret
                     }
                 };
 
@@ -101,16 +105,6 @@ impl<DB> FvmMessageInterpreter<DB>
 where
     DB: Blockstore + 'static + Send + Sync + Clone,
 {
-    /// Overestimation rate applied to gas to ensure that the
-    /// message goes through in the gas estimation.
-    const GAS_OVERESTIMATION_RATE: f64 = 1.25;
-    /// Default gas premium value. Inferred through a quick search through
-    /// InvokeEVM messages in filfox. The default value is only used if
-    /// the user hasn't specified a gas premium.
-    const DEFAULT_GAS_PREMIUM: u64 = 20000;
-    /// Gas search step increase used to find the optimal gas limit.
-    const GAS_SEARCH_STEP: f64 = 1.2;
-
     fn estimate_gassed_msg(
         &self,
         state: &FvmQueryState<DB>,
@@ -130,20 +124,18 @@ where
                 gas_limit: 0,
             }));
         }
-        msg.gas_limit = (ret.msg_receipt.gas_used as f64 * Self::GAS_OVERESTIMATION_RATE) as u64;
+
+        msg.gas_limit = (ret.msg_receipt.gas_used as f64 * self.gas_overestimation_rate) as u64;
+
         if msg.gas_premium.is_zero() {
-            // TODO: Instead of assigning a default value here, we should analyze historical
-            // blocks from the current height to estimate an accurate value for this premium.
-            // To achieve this we would need to perform a set of ABCI queries.
-            // In the meantime, this value should be good enough to make sure that the
-            // message is included in a block.
-            // this is triggered only if the user hasn't specified a gas premium, `ethers` and
-            // tooling would generally set this themselves.
-            msg.gas_premium = TokenAmount::from_nano(BigInt::from(Self::DEFAULT_GAS_PREMIUM));
+            // We need to set the gas_premium to some value other than zero for the
+            // gas estimation to work accurately (I really don't know why this is
+            // the case but after a lot of testing, setting this value to zero rejects the transaction)
+            msg.gas_premium = TokenAmount::from_nano(BigInt::from(1));
         }
         if msg.gas_fee_cap.is_zero() {
             // Compute the fee cap from gas premium and applying an additional overestimation.
-            let overestimated_limit = (msg.gas_limit as f64 * Self::GAS_OVERESTIMATION_RATE) as u64;
+            let overestimated_limit = (msg.gas_limit as f64 * self.gas_overestimation_rate) as u64;
             msg.gas_fee_cap = std::cmp::min(
                 TokenAmount::from_atto(BigInt::from(overestimated_limit)) + &msg.gas_premium,
                 TokenAmount::from_atto(BLOCK_GAS_LIMIT),
@@ -152,6 +144,8 @@ where
             // TODO: In Lotus historical values of the base fee and a more accurate overestimation is performed
             // for the fee cap. If we issues with messages going through let's consider the historical analysis.
         }
+        // TODO: Set an optimal `gas_premium` and `gas_fee_cap` if they are not set
+        // according to the current base_fee?
 
         Ok(None)
     }
@@ -164,10 +158,12 @@ where
 
         loop {
             if let Some(ret) = self.estimation_call_with_limit(state, msg.clone(), curr_limit)? {
+                println!(">>> RETURNED LIMIT IN GAS_SEARCH: {:?}", ret.gas_limit);
                 return Ok(ret);
             }
 
-            curr_limit = (curr_limit as f64 * Self::GAS_SEARCH_STEP) as u64;
+            curr_limit = (curr_limit as f64 * self.gas_search_step) as u64;
+            println!(">>> CURRENT LIMIT IN GAS_SEARCH: {:?}", curr_limit);
             if curr_limit > BLOCK_GAS_LIMIT {
                 return Ok(GasEstimate {
                     exit_code: ExitCode::OK,