Automatically upgrade StateValueMetadata

So that the bytes deposit can be tracked as soon as possible

aptos-move/aptos-aggregator/src/delta_change_set.rs

@@ -589,10 +589,16 @@ mod test {
         let sub_op = delta_sub(100, 200);
 
         let add_result = state_view.try_convert_aggregator_v1_delta_into_write_op(&KEY, &add_op);
-        assert_ok_eq!(add_result, WriteOp::Modification(serialize(&200).into()));
+        assert_ok_eq!(
+            add_result,
+            WriteOp::legacy_modification(serialize(&200).into())
+        );
 
         let sub_result = state_view.try_convert_aggregator_v1_delta_into_write_op(&KEY, &sub_op);
-        assert_ok_eq!(sub_result, WriteOp::Modification(serialize(&0).into()));
+        assert_ok_eq!(
+            sub_result,
+            WriteOp::legacy_modification(serialize(&0).into())
+        );
     }
 
     #[test]

aptos-move/aptos-aggregator/src/resolver.rs

@@ -15,7 +15,7 @@ use aptos_types::{
     aggregator::PanicError,
     state_store::{
         state_key::StateKey,
-        state_value::{StateValue, StateValueMetadataKind},
+        state_value::{StateValue, StateValueMetadata},
     },
     write_set::WriteOp,
 };
@@ -64,7 +64,7 @@ pub trait TAggregatorV1View {
     fn get_aggregator_v1_state_value_metadata(
         &self,
         id: &Self::Identifier,
-    ) -> anyhow::Result<Option<StateValueMetadataKind>> {
+    ) -> anyhow::Result<Option<StateValueMetadata>> {
         // When getting state value metadata for aggregator V1, we need to do a
         // precise read.
         let maybe_state_value = self.get_aggregator_v1_state_value(id)?;
@@ -120,7 +120,7 @@ pub trait TAggregatorV1View {
                     )))
                     .into_vm_status()
             })
-            .map(|result| WriteOp::Modification(serialize(&result).into()))
+            .map(|result| WriteOp::legacy_modification(serialize(&result).into()))
     }
 }
 

aptos-move/aptos-gas-meter/src/traits.rs

@@ -144,13 +144,9 @@ pub trait AptosGasMeter: MoveGasMeter {
 
         // Write set
         let mut writeset_charge_and_refund = ChargeAndRefund::zero();
-        for (key, op_size, metadata_opt) in change_set.write_set_iter_mut() {
-            writeset_charge_and_refund.combine(pricing.charge_refund_write_op(
-                params,
-                key,
-                &op_size,
-                metadata_opt,
-            ));
+        for (key, op_size, metadata) in change_set.write_set_iter_mut() {
+            writeset_charge_and_refund
+                .combine(pricing.charge_refund_write_op(params, key, &op_size, metadata));
         }
         let ChargeAndRefund {
             non_discountable,

aptos-move/aptos-vm-types/src/abstract_write_op.rs

@@ -5,7 +5,6 @@ use aptos_types::{
     state_store::state_value::StateValueMetadata,
     write_set::{TransactionWrite, WriteOp, WriteOpSize},
 };
-use claims::assert_none;
 use move_core_types::{language_storage::StructTag, value::MoveTypeLayout};
 use std::{collections::BTreeMap, sync::Arc};
 
@@ -55,15 +54,13 @@ impl AbstractResourceWriteOp {
             }) => {
                 use WriteOp::*;
                 match write_op {
-                    Creation(_) | CreationWithMetadata { .. } => WriteOpSize::Creation {
+                    Creation { .. } => WriteOpSize::Creation {
                         write_len: materialized_size.expect("Creation must have size"),
                     },
-                    Modification(_) | ModificationWithMetadata { .. } => {
-                        WriteOpSize::Modification {
-                            write_len: materialized_size.expect("Modification must have size"),
-                        }
+                    Modification { .. } => WriteOpSize::Modification {
+                        write_len: materialized_size.expect("Modification must have size"),
                     },
-                    Deletion | DeletionWithMetadata { .. } => WriteOpSize::Deletion,
+                    Deletion { .. } => WriteOpSize::Deletion,
                 }
             },
             InPlaceDelayedFieldChange(InPlaceDelayedFieldChangeOp {
@@ -80,7 +77,7 @@ impl AbstractResourceWriteOp {
 
     /// Deposit amount is inserted into metadata at a different time than the WriteOp is created.
     /// So this method is needed to be able to update metadata generically across different variants.
-    pub fn get_metadata_mut(&mut self) -> Option<&mut StateValueMetadata> {
+    pub fn get_metadata_mut(&mut self) -> &mut StateValueMetadata {
         use AbstractResourceWriteOp::*;
         match self {
             Write(write_op)
@@ -89,7 +86,10 @@ impl AbstractResourceWriteOp {
                 metadata_op: write_op,
                 ..
             }) => write_op.get_metadata_mut(),
-            InPlaceDelayedFieldChange(_) | ResourceGroupInPlaceDelayedFieldChange(_) => None,
+            InPlaceDelayedFieldChange(_) | ResourceGroupInPlaceDelayedFieldChange(_) => {
+                // FIXME(aldenhu): confirm this is correct
+                unreachable!("No metadata for delayed field changes")
+            },
         }
     }
 
@@ -148,7 +148,10 @@ impl GroupWrite {
             metadata_op
         );
         for (_tag, (v, _layout)) in &inner_ops {
-            assert_none!(v.metadata(), "Group inner ops must have no metadata");
+            assert!(
+                v.metadata().is_dummy(),
+                "Group inner ops must have no metadata"
+            )
         }
 
         let maybe_group_op_size = (!metadata_op.is_deletion()).then_some(group_size);

aptos-move/aptos-vm-types/src/change_set.rs

@@ -341,7 +341,7 @@ impl VMChangeSet {
     /// So this method is needed to be able to update metadata generically across different variants.
     pub fn write_set_iter_mut(
         &mut self,
-    ) -> impl Iterator<Item = (&StateKey, WriteOpSize, Option<&mut StateValueMetadata>)> {
+    ) -> impl Iterator<Item = (&StateKey, WriteOpSize, &mut StateValueMetadata)> {
         self.resource_write_set
             .iter_mut()
             .map(|(k, v)| (k, v.materialized_size(), v.get_metadata_mut()))
@@ -477,10 +477,7 @@ impl VMChangeSet {
             if let Some(write_op) = aggregator_v1_write_set.get_mut(&state_key) {
                 // In this case, delta follows a write op.
                 match write_op {
-                    Creation(data)
-                    | Modification(data)
-                    | CreationWithMetadata { data, .. }
-                    | ModificationWithMetadata { data, .. } => {
+                    Creation { data, .. } | Modification { data, .. } => {
                         // Apply delta on top of creation or modification.
                         // TODO[agg_v1](cleanup): This will not be needed anymore once aggregator
                         // change sets carry non-serialized information.
@@ -492,7 +489,7 @@ impl VMChangeSet {
                             .map_err(|e| e.finish(Location::Undefined).into_vm_status())?;
                         *data = serialize(&value).into();
                     },
-                    Deletion | DeletionWithMetadata { .. } => {
+                    Deletion { .. } => {
                         // This case (applying a delta to deleted item) should
                         // never happen. Let's still return an error instead of
                         // panicking.

aptos-move/aptos-vm-types/src/resolver.rs

@@ -10,7 +10,7 @@ use aptos_types::{
     state_store::{
         state_key::StateKey,
         state_storage_usage::StateStorageUsage,
-        state_value::{StateValue, StateValueMetadataKind},
+        state_value::{StateValue, StateValueMetadata},
     },
     write_set::WriteOp,
 };
@@ -45,7 +45,7 @@ pub trait TResourceView {
     fn get_resource_state_value_metadata(
         &self,
         state_key: &Self::Key,
-    ) -> anyhow::Result<Option<StateValueMetadataKind>> {
+    ) -> anyhow::Result<Option<StateValueMetadata>> {
         // For metadata, layouts are not important.
         self.get_resource_state_value(state_key, None)
             .map(|maybe_state_value| maybe_state_value.map(StateValue::into_metadata))
@@ -151,7 +151,7 @@ pub trait TModuleView {
     fn get_module_state_value_metadata(
         &self,
         state_key: &Self::Key,
-    ) -> anyhow::Result<Option<StateValueMetadataKind>> {
+    ) -> anyhow::Result<Option<StateValueMetadata>> {
         let maybe_state_value = self.get_module_state_value(state_key)?;
         Ok(maybe_state_value.map(StateValue::into_metadata))
     }
@@ -275,13 +275,13 @@ pub trait StateValueMetadataResolver {
     fn get_module_state_value_metadata(
         &self,
         state_key: &StateKey,
-    ) -> anyhow::Result<Option<StateValueMetadataKind>>;
+    ) -> anyhow::Result<Option<StateValueMetadata>>;
 
     /// Can also be used to get the metadata of a resource group at a provided group key.
     fn get_resource_state_value_metadata(
         &self,
         state_key: &StateKey,
-    ) -> anyhow::Result<Option<StateValueMetadataKind>>;
+    ) -> anyhow::Result<Option<StateValueMetadata>>;
 }
 
 #[derive(Clone, Copy, Debug, Eq, PartialEq)]

aptos-move/aptos-vm-types/src/storage/space_pricing.rs

@@ -71,7 +71,7 @@ impl DiskSpacePricing {
         params: &TransactionGasParameters,
         key: &StateKey,
         op_size: &WriteOpSize,
-        metadata: Option<&mut StateValueMetadata>,
+        metadata: &mut StateValueMetadata,
     ) -> ChargeAndRefund {
         match self {
             Self::V1 => Self::charge_refund_write_op_v1(params, key, op_size, metadata),
@@ -160,7 +160,7 @@ impl DiskSpacePricing {
         params: &TransactionGasParameters,
         key: &StateKey,
         op_size: &WriteOpSize,
-        metadata: Option<&mut StateValueMetadata>,
+        metadata: &mut StateValueMetadata,
     ) -> ChargeAndRefund {
         use WriteOpSize::*;
 
@@ -170,8 +170,8 @@ impl DiskSpacePricing {
                 let bytes_fee = Self::discounted_write_op_size_for_v1(params, key, *write_len)
                     * params.legacy_storage_fee_per_excess_state_byte;
 
-                if let Some(m) = metadata {
-                    m.set_slot_deposit(slot_fee.into())
+                if !metadata.is_dummy() {
+                    metadata.set_slot_deposit(slot_fee.into())
                 }
 
                 ChargeAndRefund {
@@ -190,18 +190,10 @@ impl DiskSpacePricing {
                     refund: 0.into(),
                 }
             },
-            Deletion => {
-                let refund = match metadata {
-                    None => 0,
-                    Some(m) => m.total_deposit(),
-                }
-                .into();
-
-                ChargeAndRefund {
-                    non_discountable: 0.into(),
-                    discountable: 0.into(),
-                    refund,
-                }
+            Deletion => ChargeAndRefund {
+                non_discountable: 0.into(),
+                discountable: 0.into(),
+                refund: metadata.total_deposit().into(),
             },
         }
     }
@@ -210,7 +202,7 @@ impl DiskSpacePricing {
         params: &TransactionGasParameters,
         key: &StateKey,
         op_size: &WriteOpSize,
-        metadata: Option<&mut StateValueMetadata>,
+        metadata: &mut StateValueMetadata,
     ) -> ChargeAndRefund {
         use WriteOpSize::*;
 
@@ -227,11 +219,9 @@ impl DiskSpacePricing {
                 let slot_deposit = params.storage_fee_per_state_slot_refundable * NumSlots::new(1);
                 let bytes_deposit = num_bytes * params.storage_fee_per_state_byte_refundable;
 
-                if let Some(m) = metadata {
-                    m.set_deposits(slot_deposit.into(), bytes_deposit.into())
-                } else {
-                    // FIXME(aldenhu): this shouldn't happen
-                }
+                metadata.assert_v1();
+                metadata.set_slot_deposit(slot_deposit.into());
+                metadata.set_bytes_deposit(bytes_deposit.into());
 
                 ChargeAndRefund {
                     non_discountable: slot_deposit + bytes_deposit,
@@ -243,7 +233,7 @@ impl DiskSpacePricing {
                 // change of slot size or per byte price can result in a charge or refund of permanent bytes fee
                 let num_bytes = NumBytes::new(key.size() as u64) + NumBytes::new(*write_len);
                 let target_bytes_deposit = num_bytes * params.storage_fee_per_state_byte_refundable;
-                let old_bytes_deposit = metadata.as_ref().map_or(0, |m| m.bytes_deposit()).into();
+                let old_bytes_deposit = Fee::new(metadata.bytes_deposit());
                 let (state_bytes_charge, state_bytes_refund) = if target_bytes_deposit
                     > old_bytes_deposit
                 {
@@ -255,29 +245,20 @@ impl DiskSpacePricing {
                     (0.into(), bytes_refund)
                 };
 
-                // FIXME(aldenhu): upgrade to new format automatically, otherwise old slots will be heavily punished whenever modified.
-                if let Some(m) = metadata {
-                    m.set_bytes_deposit(target_bytes_deposit.into())
-                }
+                metadata
+                    .upgrade()
+                    .set_bytes_deposit(target_bytes_deposit.into());
 
                 ChargeAndRefund {
                     non_discountable: state_bytes_charge,
                     discountable,
                     refund: state_bytes_refund,
                 }
             },
-            Deletion => {
-                let refund = match metadata {
-                    None => 0,
-                    Some(m) => m.total_deposit(),
-                }
-                .into();
-
-                ChargeAndRefund {
-                    non_discountable: 0.into(),
-                    discountable,
-                    refund,
-                }
+            Deletion => ChargeAndRefund {
+                non_discountable: 0.into(),
+                discountable,
+                refund: metadata.total_deposit().into(),
             },
         }
     }