Add storage_rebate to Object and use gas price

sui_core/src/authority.rs

@@ -127,7 +127,8 @@ impl AuthorityState {
             object_id: gas_payment_id,
         })?;
         gas::check_gas_balance(&gas_object, gas_budget)?;
-        let gas_status = gas::start_gas_metering(gas_budget)?;
+        // TODO: Pass in real computation gas unit price and storage gas unit price.
+        let gas_status = gas::start_gas_metering(gas_budget, 1, 1)?;
         Ok((gas_object, gas_status))
     }
 

sui_core/src/authority/temporary_store.rs

@@ -118,32 +118,60 @@ impl<S> AuthorityTemporaryStore<S> {
     /// the gas object hasn't been mutated yet. Passing in `gas_object_size` so that we can also charge
     /// for the gas object mutation in advance.
     pub fn charge_gas_for_storage_changes(
-        &self,
+        &mut self,
         gas_status: &mut SuiGasStatus,
-        gas_object_size: usize,
+        gas_object: &mut Object,
     ) -> SuiResult {
-        for (object_id, (_object_ref, object)) in &self.written {
-            // Objects in written can be either mutation or creation.
-            // We figure it out by looking them up in `self.objects`.
-            let object_size = object.object_data_size();
-            let old_object_size = if let Some(old_obj) = self.objects.get(object_id) {
-                old_obj.object_data_size()
-            } else {
-                0
-            };
-            gas_status.charge_storage_mutation(old_object_size, object_size)?;
+        let mut objects_to_update = vec![];
+        // Also charge gas for mutating the gas object in advance.
+        let gas_object_size = gas_object.object_data_size();
+        gas_object.storage_rebate = gas_status.charge_storage_mutation(
+            gas_object_size,
+            gas_object_size,
+            gas_object.storage_rebate,
+        )?;
+        objects_to_update.push(gas_object.clone());
+
+        for (object_id, (_object_ref, object)) in &mut self.written {
+            let (old_object_size, storage_rebate) =
+                if let Some(old_object) = self.objects.get(object_id) {
+                    (old_object.object_data_size(), old_object.storage_rebate)
+                } else {
+                    (0, 0)
+                };
+            let new_storage_rebate = gas_status.charge_storage_mutation(
+                old_object_size,
+                object.object_data_size(),
+                storage_rebate,
+            )?;
+            if !object.is_read_only() {
+                // We don't need to set storage rebate for immutable objects, as they will
+                // never be deleted.
+                object.storage_rebate = new_storage_rebate;
+                objects_to_update.push(object.clone());
+            }
         }
+
         for object_id in self.deleted.keys() {
             // If an object is in `self.deleted`, and also in `self.objects`, we give storage rebate.
             // Otherwise if an object is in `self.deleted` but not in `self.objects`, it means this
             // object was unwrapped and then deleted. The rebate would have been provided already when
             // mutating the object that wrapped this object.
-            if let Some(old_obj) = self.objects.get(object_id) {
-                gas_status.charge_storage_mutation(old_obj.object_data_size(), 0)?;
+            if let Some(old_object) = self.objects.get(object_id) {
+                gas_status.charge_storage_mutation(
+                    old_object.object_data_size(),
+                    0,
+                    old_object.storage_rebate,
+                )?;
             }
         }
-        // Also charge gas for mutating the gas object in advance.
-        gas_status.charge_storage_mutation(gas_object_size, gas_object_size)?;
+
+        // Write all objects at the end only if all previous gas charges succeeded.
+        // This avoids polluting the temporary store state if this function failed.
+        for object in objects_to_update {
+            self.write_object(object);
+        }
+
         Ok(())
     }
 

sui_core/src/execution_engine.rs

@@ -132,15 +132,18 @@ fn execute_transaction<S: BackingPackageStore>(
         temporary_store.reset();
     }
     temporary_store.ensure_active_inputs_mutated();
-    if let Err(err) = temporary_store
-        .charge_gas_for_storage_changes(&mut gas_status, gas_object.object_data_size())
+    if let Err(err) =
+        temporary_store.charge_gas_for_storage_changes(&mut gas_status, &mut gas_object)
     {
         result = Err(err);
+        // No need to roll back the temporary store here since `charge_gas_for_storage_changes`
+        // will not modify `temporary_store` if it failed.
     }
 
     let cost_summary = gas_status.summary(result.is_ok());
     let gas_used = cost_summary.gas_used();
-    gas::deduct_gas(&mut gas_object, gas_used);
+    let gas_rebate = cost_summary.storage_rebate;
+    gas::deduct_gas(&mut gas_object, gas_used, gas_rebate);
     temporary_store.write_object(gas_object);
 
     // TODO: Return cost_summary so that the detailed summary exists in TransactionEffects for

sui_core/src/gateway_state.rs

@@ -209,7 +209,8 @@ where
     ) -> SuiResult<(Object, SuiGasStatus<'_>)> {
         let gas_object = self.get_object(&gas_payment_id).await?;
         gas::check_gas_balance(&gas_object, gas_budget)?;
-        let gas_status = gas::start_gas_metering(gas_budget)?;
+        // TODO: Pass in real computation gas unit price and storage gas unit price.
+        let gas_status = gas::start_gas_metering(gas_budget, 1, 1)?;
         Ok((gas_object, gas_status))
     }
 

sui_core/src/unit_tests/authority_tests.rs

@@ -1416,15 +1416,8 @@ async fn shared_object() {
         use sui_types::object::MoveObject;
 
         let content = GasCoin::new(shared_object_id, SequenceNumber::new(), 10);
-        let data = Data::Move(MoveObject::new(
-            /* type */ GasCoin::type_(),
-            content.to_bcs_bytes(),
-        ));
-        Object {
-            data,
-            owner: Owner::SharedMutable,
-            previous_transaction: TransactionDigest::genesis(),
-        }
+        let obj = MoveObject::new(/* type */ GasCoin::type_(), content.to_bcs_bytes());
+        Object::new_move(obj, Owner::SharedMutable, TransactionDigest::genesis())
     };
 
     let authority = init_state_with_objects(vec![gas_object, shared_object]).await;

sui_core/src/unit_tests/consensus_tests.rs

@@ -17,7 +17,7 @@ use sui_types::gas_coin::GasCoin;
 use sui_types::messages::{
     CertifiedTransaction, SignatureAggregator, Transaction, TransactionData,
 };
-use sui_types::object::{Data, MoveObject, Object, Owner};
+use sui_types::object::{MoveObject, Object, Owner};
 use sui_types::serialize::serialize_cert;
 use test_utils::sequencer::Sequencer;
 
@@ -47,15 +47,8 @@ fn test_shared_object() -> Object {
     let seed = "0x6666666666666660";
     let shared_object_id = ObjectID::from_hex_literal(seed).unwrap();
     let content = GasCoin::new(shared_object_id, SequenceNumber::new(), 10);
-    let data = Data::Move(MoveObject::new(
-        /* type */ GasCoin::type_(),
-        content.to_bcs_bytes(),
-    ));
-    Object {
-        data,
-        owner: Owner::SharedMutable,
-        previous_transaction: TransactionDigest::genesis(),
-    }
+    let obj = MoveObject::new(/* type */ GasCoin::type_(), content.to_bcs_bytes());
+    Object::new_move(obj, Owner::SharedMutable, TransactionDigest::genesis())
 }
 
 /// Fixture: a few test certificates containing a shared object.

sui_core/src/unit_tests/gas_tests.rs

@@ -82,7 +82,8 @@ async fn test_native_transfer_sufficient_gas() -> SuiResult {
     let effects = result.response.unwrap().signed_effects.unwrap().effects;
     let gas_cost = effects.status.gas_cost_summary();
     assert!(gas_cost.computation_cost > *MIN_GAS_BUDGET);
-    assert_eq!(gas_cost.storage_cost, 0);
+    assert!(gas_cost.storage_cost > 0);
+    // Removing genesis object does not have rebate.
     assert_eq!(gas_cost.storage_rebate, 0);
 
     let object = result
@@ -103,13 +104,14 @@ async fn test_native_transfer_sufficient_gas() -> SuiResult {
 
     // Mimic the process of gas charging, to check that we are charging
     // exactly what we should be charging.
-    let mut gas_status = SuiGasStatus::new_with_budget(*MAX_GAS_BUDGET);
+    let mut gas_status = SuiGasStatus::new_with_budget(*MAX_GAS_BUDGET, 1, 1);
     gas_status.charge_min_tx_gas()?;
+    let obj_size = object.object_data_size();
+    let gas_size = gas_object.object_data_size();
 
-    gas_status.charge_storage_read(object.object_data_size() + gas_object.object_data_size())?;
-    gas_status.charge_storage_mutation(object.object_data_size(), object.object_data_size())?;
-    gas_status
-        .charge_storage_mutation(gas_object.object_data_size(), gas_object.object_data_size())?;
+    gas_status.charge_storage_read(obj_size + gas_size)?;
+    gas_status.charge_storage_mutation(obj_size, obj_size, 0)?;
+    gas_status.charge_storage_mutation(gas_size, gas_size, 0)?;
     assert_eq!(gas_cost, &gas_status.summary(true));
     Ok(())
 }
@@ -149,20 +151,23 @@ async fn test_native_transfer_insufficient_gas_execution() {
     let budget = total_gas - 1;
     let result = execute_transfer(budget, budget, true).await;
     let effects = result.response.unwrap().signed_effects.unwrap().effects;
-    // The gas balance should be drained.
-    assert_eq!(effects.status.gas_cost_summary().gas_used(), budget);
+    // We won't drain the entire budget because we don't charge for storage if tx failed.
+    assert!(effects.status.gas_cost_summary().gas_used() < budget);
     let gas_object = result
         .authority_state
         .get_object(&result.gas_object_id)
         .await
         .unwrap()
         .unwrap();
     let gas_coin = GasCoin::try_from(&gas_object).unwrap();
-    assert_eq!(gas_coin.value(), 0);
+    assert_eq!(
+        gas_coin.value(),
+        budget - effects.status.gas_cost_summary().gas_used()
+    );
     assert_eq!(
         effects.status.unwrap_err().1,
         SuiError::InsufficientGas {
-            error: "Ran out of gas while deducting computation cost".to_owned()
+            error: "Ran out of gas while deducting storage cost".to_owned()
         }
     );
 }
@@ -212,16 +217,19 @@ async fn test_publish_gas() -> SuiResult {
     };
 
     // Mimic the gas charge behavior and cross check the result with above.
-    let mut gas_status = SuiGasStatus::new_with_budget(*MAX_GAS_BUDGET);
+    let mut gas_status = SuiGasStatus::new_with_budget(*MAX_GAS_BUDGET, 1, 1);
     gas_status.charge_min_tx_gas()?;
     gas_status.charge_storage_read(genesis_objects.iter().map(|o| o.object_data_size()).sum())?;
     gas_status.charge_storage_read(gas_object.object_data_size())?;
     gas_status.charge_publish_package(publish_bytes.iter().map(|v| v.len()).sum())?;
-    gas_status.charge_storage_mutation(0, package.object_data_size())?;
+    gas_status.charge_storage_mutation(0, package.object_data_size(), 0)?;
     // Remember the gas used so far. We will use this to create another failure case latter.
     let gas_used_after_package_creation = gas_status.summary(true).gas_used();
-    gas_status
-        .charge_storage_mutation(gas_object.object_data_size(), gas_object.object_data_size())?;
+    gas_status.charge_storage_mutation(
+        gas_object.object_data_size(),
+        gas_object.object_data_size(),
+        0,
+    )?;
     assert_eq!(gas_cost, &gas_status.summary(true));
 
     // Create a transaction with budget DELTA less than the gas cost required.
@@ -245,16 +253,14 @@ async fn test_publish_gas() -> SuiResult {
     assert_eq!(
         err,
         SuiError::InsufficientGas {
-            error: "Ran out of gas while deducting computation cost".to_owned()
+            error: "Ran out of gas while deducting storage cost".to_owned()
         }
     );
 
     // Make sure that we are not charging storage cost at failure.
     assert_eq!(gas_cost.storage_cost, 0);
     // Upon failure, we should only be charging the expected computation cost.
-    // Since we failed when trying to charge the last piece of computation cost,
-    // the total cost will be DELTA less since it's not enough.
-    assert_eq!(gas_cost.gas_used(), computation_cost - DELTA);
+    assert_eq!(gas_cost.gas_used(), computation_cost);
 
     let gas_object = authority_state.get_object(&gas_object_id).await?.unwrap();
     let expected_gas_balance = expected_gas_balance - gas_cost.gas_used();
@@ -336,7 +342,7 @@ async fn test_move_call_gas() -> SuiResult {
     );
 
     // Mimic the gas charge behavior and cross check the result with above.
-    let mut gas_status = SuiGasStatus::new_with_budget(*MAX_GAS_BUDGET);
+    let mut gas_status = SuiGasStatus::new_with_budget(*MAX_GAS_BUDGET, 1, 1);
     gas_status.charge_min_tx_gas()?;
     let package_object = authority_state
         .get_object(&package_object_ref.0)
@@ -348,27 +354,60 @@ async fn test_move_call_gas() -> SuiResult {
     // The gas cost to execute the function in Move VM.
     // Hard code it here since it's difficult to mock that in test.
     const MOVE_VM_EXEC_COST: u64 = 17;
-    gas_status
-        .charge_storage_mutation(gas_object.object_data_size(), gas_object.object_data_size())?;
+    gas_status.charge_storage_mutation(
+        gas_object.object_data_size(),
+        gas_object.object_data_size(),
+        0,
+    )?;
     let created_object = authority_state
         .get_object(&effects.created[0].0 .0)
         .await?
         .unwrap();
-    gas_status.charge_storage_mutation(0, created_object.object_data_size())?;
+    gas_status.charge_storage_mutation(0, created_object.object_data_size(), 0)?;
 
     let new_cost = gas_status.summary(true);
     assert_eq!(
         gas_cost.computation_cost,
         new_cost.computation_cost + MOVE_VM_EXEC_COST
     );
     assert_eq!(gas_cost.storage_cost, new_cost.storage_cost);
+    // This is the total amount of storage cost paid. We will use this
+    // to check if we get back the same amount of rebate latter.
+    let prev_storage_cost = gas_cost.storage_cost;
+
+    // Execute object deletion, and make sure we have storage rebate.
+    let data = TransactionData::new_move_call(
+        sender,
+        package_object_ref,
+        module.clone(),
+        ident_str!("delete").to_owned(),
+        vec![],
+        gas_object.compute_object_reference(),
+        vec![created_object_ref],
+        vec![],
+        vec![],
+        expected_gas_balance,
+    );
+    let signature = Signature::new(&data, &sender_key);
+    let transaction = Transaction::new(data, signature);
+    let response = send_and_confirm_transaction(&authority_state, transaction).await?;
+    let effects = response.signed_effects.unwrap().effects;
+    assert!(effects.status.is_ok());
+    let gas_cost = effects.status.gas_cost_summary();
+    // storage_cost should be less than rebate because for object deletion, we only
+    // rebate without charging.
+    assert!(gas_cost.storage_cost > 0 && gas_cost.storage_cost < gas_cost.storage_rebate);
+    // Check that we have storage rebate that's the same as previous cost.
+    assert_eq!(gas_cost.storage_rebate, prev_storage_cost);
+    let expected_gas_balance = expected_gas_balance - gas_cost.gas_used() + gas_cost.storage_rebate;
 
     // Create a transaction with gas budget that should run out during Move VM execution.
+    let gas_object = authority_state.get_object(&gas_object_id).await?.unwrap();
     let budget = gas_used_before_vm_exec + 1;
     let data = TransactionData::new_move_call(
         sender,
         package_object_ref,
-        module.clone(),
+        module,
         function,
         Vec::new(),
         gas_object.compute_object_reference(),
@@ -392,34 +431,28 @@ async fn test_move_call_gas() -> SuiResult {
         }
     );
     let gas_object = authority_state.get_object(&gas_object_id).await?.unwrap();
-    let expected_gas_balance = expected_gas_balance - gas_cost.gas_used();
+    let expected_gas_balance = expected_gas_balance - gas_cost.gas_used() + gas_cost.storage_rebate;
     assert_eq!(
         GasCoin::try_from(&gas_object)?.value(),
         expected_gas_balance,
     );
+    Ok(())
+}
 
-    // Execute object deletion, and make sure we have storage rebate.
-    let data = TransactionData::new_move_call(
-        sender,
-        package_object_ref,
-        module,
-        ident_str!("delete").to_owned(),
-        vec![],
-        gas_object.compute_object_reference(),
-        vec![created_object_ref],
-        vec![],
-        vec![],
-        expected_gas_balance,
-    );
-    let signature = Signature::new(&data, &sender_key);
-    let transaction = Transaction::new(data, signature);
-    let response = send_and_confirm_transaction(&authority_state, transaction).await?;
-    let effects = response.signed_effects.unwrap().effects;
-    assert!(effects.status.is_ok());
-    let gas_cost = effects.status.gas_cost_summary();
-    assert_eq!(gas_cost.storage_cost, 0);
-    // Check that we have storage rebate after deletion.
-    assert!(gas_cost.storage_rebate > 0);
+#[tokio::test]
+async fn test_storage_gas_unit_price() -> SuiResult {
+    let mut gas_status1 = SuiGasStatus::new_with_budget(*MAX_GAS_BUDGET, 1, 1);
+    gas_status1.charge_storage_mutation(100, 200, 5)?;
+    let gas_cost1 = gas_status1.summary(true);
+    let mut gas_status2 = SuiGasStatus::new_with_budget(*MAX_GAS_BUDGET, 1, 3);
+    gas_status2.charge_storage_mutation(100, 200, 5)?;
+    let gas_cost2 = gas_status2.summary(true);
+    // Computation unit price is the same, hence computation cost should be the same.
+    assert_eq!(gas_cost1.computation_cost, gas_cost2.computation_cost);
+    // Storage unit prices is 3X, so will be the storage cost.
+    assert_eq!(gas_cost1.storage_cost * 3, gas_cost2.storage_cost);
+    // Storage rebate should not be affected by the price.
+    assert_eq!(gas_cost1.storage_rebate, gas_cost2.storage_rebate);
     Ok(())
 }
 

sui_core/tests/staged/sui.yaml

@@ -273,6 +273,7 @@ Object:
         TYPENAME: Owner
     - previous_transaction:
         TYPENAME: TransactionDigest
+    - storage_rebate: U64
 ObjectDigest:
   NEWTYPESTRUCT: BYTES
 ObjectFormatOptions: