From a177db0a33a82fb0e5bb2551809cc914ecf0eeec Mon Sep 17 00:00:00 2001 From: aldenhu Date: Tue, 5 Nov 2024 23:08:35 +0000 Subject: [PATCH] WriteOp carries StateValue --- .../aptos-vm-types/src/abstract_write_op.rs | 4 +- aptos-move/aptos-vm-types/src/change_set.rs | 12 +- .../aptos-vm-types/src/module_write_set.rs | 2 +- .../src/tests/test_change_set.rs | 38 +--- .../src/move_vm_ext/write_op_converter.rs | 22 +- .../src/tests/storage_refund.rs | 2 +- crates/aptos-rosetta/src/test/mod.rs | 8 +- .../src/stream_coordinator.rs | 2 +- testsuite/generate-format/src/api.rs | 4 +- testsuite/generate-format/src/aptos.rs | 9 +- testsuite/generate-format/src/consensus.rs | 9 +- types/src/state_store/state_value.rs | 17 ++ types/src/write_set.rs | 198 ++++++++---------- 13 files changed, 134 insertions(+), 193 deletions(-) diff --git a/aptos-move/aptos-vm-types/src/abstract_write_op.rs b/aptos-move/aptos-vm-types/src/abstract_write_op.rs index c73dc7af2cacd..381228f272940 100644 --- a/aptos-move/aptos-vm-types/src/abstract_write_op.rs +++ b/aptos-move/aptos-vm-types/src/abstract_write_op.rs @@ -111,7 +111,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) -> &mut StateValueMetadata { + pub fn metadata_mut(&mut self) -> &mut StateValueMetadata { use AbstractResourceWriteOp::*; match self { Write(write_op) @@ -119,7 +119,7 @@ impl AbstractResourceWriteOp { | WriteResourceGroup(GroupWrite { metadata_op: write_op, .. - }) => write_op.get_metadata_mut(), + }) => write_op.metadata_mut(), InPlaceDelayedFieldChange(InPlaceDelayedFieldChangeOp { metadata, .. }) | ResourceGroupInPlaceDelayedFieldChange(ResourceGroupInPlaceDelayedFieldChangeOp { metadata, diff --git a/aptos-move/aptos-vm-types/src/change_set.rs b/aptos-move/aptos-vm-types/src/change_set.rs index 5f6029dd2f984..74e609df51abf 100644 --- a/aptos-move/aptos-vm-types/src/change_set.rs +++ b/aptos-move/aptos-vm-types/src/change_set.rs @@ -411,18 +411,18 @@ 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, .. } => { + Creation(v) | Modification(v) => { // 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. - let base: u128 = bcs::from_bytes(data) + let base: u128 = bcs::from_bytes(v.bytes()) .expect("Deserializing into an aggregator value always succeeds"); let value = additional_delta_op .apply_to(base) .map_err(PartialVMError::from)?; - *data = serialize(&value).into(); + v.set_bytes(serialize(&value).into()) }, - Deletion { .. } => { + Deletion(..) => { // This case (applying a delta to deleted item) should // never happen. Let's still return an error instead of // panicking. @@ -883,7 +883,7 @@ impl ChangeSetInterface for VMChangeSet { key, op_size: op.materialized_size(), prev_size: op.prev_materialized_size(key, executor_view)?, - metadata_mut: op.get_metadata_mut(), + metadata_mut: op.metadata_mut(), }) }); let v1_aggregators = self.aggregator_v1_write_set.iter_mut().map(|(key, op)| { @@ -893,7 +893,7 @@ impl ChangeSetInterface for VMChangeSet { prev_size: executor_view .get_aggregator_v1_state_value_size(key)? .unwrap_or(0), - metadata_mut: op.get_metadata_mut(), + metadata_mut: op.metadata_mut(), }) }); diff --git a/aptos-move/aptos-vm-types/src/module_write_set.rs b/aptos-move/aptos-vm-types/src/module_write_set.rs index be236a46a674d..c6d3d634e7b5c 100644 --- a/aptos-move/aptos-vm-types/src/module_write_set.rs +++ b/aptos-move/aptos-vm-types/src/module_write_set.rs @@ -123,7 +123,7 @@ impl ModuleWriteSet { key, op_size: write.write_op().write_op_size(), prev_size, - metadata_mut: write.write_op_mut().get_metadata_mut(), + metadata_mut: write.write_op_mut().metadata_mut(), }) }) } diff --git a/aptos-move/aptos-vm-types/src/tests/test_change_set.rs b/aptos-move/aptos-vm-types/src/tests/test_change_set.rs index 2d887e12e5c80..ff5f0c16512c8 100644 --- a/aptos-move/aptos-vm-types/src/tests/test_change_set.rs +++ b/aptos-move/aptos-vm-types/src/tests/test_change_set.rs @@ -436,10 +436,7 @@ fn test_aggregator_v2_snapshots_and_derived() { #[test] fn test_resource_groups_squashing() { - let modification_metadata = WriteOp::Modification { - data: Bytes::new(), - metadata: raw_metadata(2000), - }; + let modification_metadata = WriteOp::modification(Bytes::new(), raw_metadata(2000)); macro_rules! as_create_op { ($val:expr) => { @@ -593,10 +590,7 @@ fn test_write_and_read_discrepancy_caught() { )]) .try_build()); - let metadata_op = WriteOp::Modification { - data: Bytes::new(), - metadata: raw_metadata(1000), - }; + let metadata_op = WriteOp::modification(Bytes::new(), raw_metadata(1000)); let group_size = ResourceGroupSize::Combined { num_tagged_resources: 1, all_tagged_resources_size: 14, @@ -637,17 +631,9 @@ mod tests { pub(crate) fn write_op_with_metadata(type_idx: u8, v: u128) -> WriteOp { match type_idx { - CREATION => WriteOp::Creation { - data: vec![].into(), - metadata: raw_metadata(v as u64), - }, - MODIFICATION => WriteOp::Modification { - data: vec![].into(), - metadata: raw_metadata(v as u64), - }, - DELETION => WriteOp::Deletion { - metadata: raw_metadata(v as u64), - }, + CREATION => WriteOp::creation(vec![].into(), raw_metadata(v as u64)), + MODIFICATION => WriteOp::modification(vec![].into(), raw_metadata(v as u64)), + DELETION => WriteOp::deletion(raw_metadata(v as u64)), _ => unreachable!("Wrong type index for test"), } } @@ -694,9 +680,7 @@ mod tests { None ); assert_group_write_size!( - WriteOp::Deletion { - metadata: raw_metadata(10) - }, + WriteOp::deletion(raw_metadata(10)), ResourceGroupSize::zero_combined(), None ); @@ -729,10 +713,7 @@ mod tests { Some(sizes[0]) ); assert_group_write_size!( - WriteOp::Creation { - data: Bytes::new(), - metadata: raw_metadata(20) - }, + WriteOp::creation(Bytes::new(), raw_metadata(20)), sizes[1], Some(sizes[1]) ); @@ -742,10 +723,7 @@ mod tests { Some(sizes[2]) ); assert_group_write_size!( - WriteOp::Modification { - data: Bytes::new(), - metadata: raw_metadata(30) - }, + WriteOp::modification(Bytes::new(), raw_metadata(30)), sizes[3], Some(sizes[3]) ); diff --git a/aptos-move/aptos-vm/src/move_vm_ext/write_op_converter.rs b/aptos-move/aptos-vm/src/move_vm_ext/write_op_converter.rs index 353a4dd202b15..1d54be49fe16f 100644 --- a/aptos-move/aptos-vm/src/move_vm_ext/write_op_converter.rs +++ b/aptos-move/aptos-vm/src/move_vm_ext/write_op_converter.rs @@ -210,7 +210,6 @@ impl<'r> WriteOpConverter<'r> { legacy_creation_as_modification: bool, ) -> PartialVMResult { use MoveStorageOp::*; - use WriteOp::*; let write_op = match (state_value_metadata, move_storage_op) { (None, Modify(_) | Delete) => { // Possible under speculative execution, returning speculative error waiting for re-execution. @@ -238,18 +237,12 @@ impl<'r> WriteOpConverter<'r> { WriteOp::legacy_creation(data) } }, - Some(metadata) => Creation { - data, - metadata: metadata.clone(), - }, - }, - (Some(metadata), Modify(data)) => { - // Inherit metadata even if the feature flags is turned off, for compatibility. - Modification { data, metadata } + Some(metadata) => WriteOp::creation(data, metadata.clone()), }, + (Some(metadata), Modify(data)) => WriteOp::modification(data, metadata), (Some(metadata), Delete) => { // Inherit metadata even if the feature flags is turned off, for compatibility. - Deletion { metadata } + WriteOp::deletion(metadata) }, }; Ok(write_op) @@ -270,13 +263,10 @@ impl<'r> WriteOpConverter<'r> { match &self.new_slot_metadata { // n.b. Aggregator writes historically did not distinguish Create vs Modify. None => WriteOp::legacy_modification(data), - Some(metadata) => WriteOp::Creation { - data, - metadata: metadata.clone(), - }, + Some(metadata) => WriteOp::creation(data, metadata.clone()), } }, - Some(metadata) => WriteOp::Modification { data, metadata }, + Some(metadata) => WriteOp::modification(data, metadata), }; Ok(op) @@ -623,7 +613,7 @@ mod tests { // Deletion should still contain the metadata - for storage refunds. assert_eq!(group_write.metadata_op().metadata(), &metadata); - assert_eq!(group_write.metadata_op(), &WriteOp::Deletion { metadata }); + assert_eq!(group_write.metadata_op(), &WriteOp::deletion(metadata)); assert_none!(group_write.metadata_op().bytes()); } } diff --git a/aptos-move/e2e-move-tests/src/tests/storage_refund.rs b/aptos-move/e2e-move-tests/src/tests/storage_refund.rs index ff510308a8b6b..15c1e536eed11 100644 --- a/aptos-move/e2e-move-tests/src/tests/storage_refund.rs +++ b/aptos-move/e2e-move-tests/src/tests/storage_refund.rs @@ -159,7 +159,7 @@ fn assert_result( for (_state_key, write_op) in txn_out.write_set() { match write_op { WriteOp::Creation { .. } => creates += 1, - WriteOp::Deletion { metadata } => { + WriteOp::Deletion(metadata) => { if metadata.is_none() { panic!("This test expects all deletions to have metadata") } diff --git a/crates/aptos-rosetta/src/test/mod.rs b/crates/aptos-rosetta/src/test/mod.rs index 173b55935e574..4dc6873870005 100644 --- a/crates/aptos-rosetta/src/test/mod.rs +++ b/crates/aptos-rosetta/src/test/mod.rs @@ -98,10 +98,10 @@ fn resource_group_modification_write_op( ) -> (StateKey, WriteOp) { let encoded = bcs::to_bytes(input).unwrap(); let state_key = StateKey::resource_group(address, resource); - let write_op = WriteOp::Modification { - data: encoded.into(), - metadata: StateValueMetadata::new(0, 0, &CurrentTimeMicroseconds { microseconds: 0 }), - }; + let write_op = WriteOp::modification( + encoded.into(), + StateValueMetadata::new(0, 0, &CurrentTimeMicroseconds { microseconds: 0 }), + ); (state_key, write_op) } diff --git a/ecosystem/indexer-grpc/indexer-grpc-fullnode/src/stream_coordinator.rs b/ecosystem/indexer-grpc/indexer-grpc-fullnode/src/stream_coordinator.rs index ce190dc2d2e4b..fed09104aa89f 100644 --- a/ecosystem/indexer-grpc/indexer-grpc-fullnode/src/stream_coordinator.rs +++ b/ecosystem/indexer-grpc/indexer-grpc-fullnode/src/stream_coordinator.rs @@ -475,7 +475,7 @@ impl IndexerStreamCoordinator { .iter() .map(|(state_key, write_op)| WriteOpSizeInfo { key_bytes: Self::ser_size_u32(state_key), - value_bytes: write_op.size() as u32, + value_bytes: write_op.bytes_size() as u32, }) .collect(), } diff --git a/testsuite/generate-format/src/api.rs b/testsuite/generate-format/src/api.rs index e9290bb7fdd5e..4be4dfcb71592 100644 --- a/testsuite/generate-format/src/api.rs +++ b/testsuite/generate-format/src/api.rs @@ -95,9 +95,7 @@ pub fn get_registry() -> Result { // 1. Record samples for types with custom deserializers. trace_crypto_values(&mut tracer, &mut samples)?; tracer.trace_value(&mut samples, &event::EventKey::random())?; - tracer.trace_value(&mut samples, &write_set::WriteOp::Deletion { - metadata: StateValueMetadata::none(), - })?; + tracer.trace_value(&mut samples, &write_set::WriteOp::legacy_deletion())?; // 2. Trace the main entry point(s) + every enum separately. // stdlib types diff --git a/testsuite/generate-format/src/aptos.rs b/testsuite/generate-format/src/aptos.rs index 0fa55466b8cdc..0c4e5fc70da05 100644 --- a/testsuite/generate-format/src/aptos.rs +++ b/testsuite/generate-format/src/aptos.rs @@ -14,10 +14,7 @@ use aptos_crypto_derive::{BCSCryptoHash, CryptoHasher}; use aptos_types::{ block_metadata_ext::BlockMetadataExt, contract_event, event, - state_store::{ - state_key::StateKey, - state_value::{PersistedStateValueMetadata, StateValueMetadata}, - }, + state_store::{state_key::StateKey, state_value::PersistedStateValueMetadata}, transaction, transaction::block_epilogue::BlockEpiloguePayload, validator_txn::ValidatorTransaction, @@ -90,9 +87,7 @@ pub fn get_registry() -> Result { // 1. Record samples for types with custom deserializers. trace_crypto_values(&mut tracer, &mut samples)?; tracer.trace_value(&mut samples, &event::EventKey::random())?; - tracer.trace_value(&mut samples, &write_set::WriteOp::Deletion { - metadata: StateValueMetadata::none(), - })?; + tracer.trace_value(&mut samples, &write_set::WriteOp::legacy_deletion())?; // 2. Trace the main entry point(s) + every enum separately. tracer.trace_type::(&samples)?; diff --git a/testsuite/generate-format/src/consensus.rs b/testsuite/generate-format/src/consensus.rs index 5ef14eb37c69b..17c079dfcbbfc 100644 --- a/testsuite/generate-format/src/consensus.rs +++ b/testsuite/generate-format/src/consensus.rs @@ -14,10 +14,7 @@ use aptos_crypto_derive::{BCSCryptoHash, CryptoHasher}; use aptos_types::{ block_metadata_ext::BlockMetadataExt, contract_event, event, - state_store::{ - state_key::StateKey, - state_value::{PersistedStateValueMetadata, StateValueMetadata}, - }, + state_store::{state_key::StateKey, state_value::PersistedStateValueMetadata}, transaction, transaction::block_epilogue::BlockEpiloguePayload, validator_txn::ValidatorTransaction, @@ -89,9 +86,7 @@ pub fn get_registry() -> Result { &aptos_consensus_types::block::Block::make_genesis_block(), )?; tracer.trace_value(&mut samples, &event::EventKey::random())?; - tracer.trace_value(&mut samples, &write_set::WriteOp::Deletion { - metadata: StateValueMetadata::none(), - })?; + tracer.trace_value(&mut samples, &write_set::WriteOp::legacy_deletion())?; // 2. Trace the main entry point(s) + every enum separately. tracer.trace_type::(&samples)?; diff --git a/types/src/state_store/state_value.rs b/types/src/state_store/state_value.rs index d69b8b3fc8ec5..5db5a22b088b2 100644 --- a/types/src/state_store/state_value.rs +++ b/types/src/state_store/state_value.rs @@ -280,6 +280,23 @@ impl StateValue { )) } + pub fn into_bytes(self) -> Bytes { + self.inner.data + } + + pub fn set_bytes(&mut self, data: Bytes) { + self.inner.data = data; + self.hash = OnceCell::new(); + } + + pub fn metadata(&self) -> &StateValueMetadata { + &self.inner.metadata + } + + pub fn metadata_mut(&mut self) -> &mut StateValueMetadata { + &mut self.inner.metadata + } + pub fn into_metadata(self) -> StateValueMetadata { self.inner.metadata } diff --git a/types/src/write_set.rs b/types/src/write_set.rs index d579f5cc951f7..41b5435badeb0 100644 --- a/types/src/write_set.rs +++ b/types/src/write_set.rs @@ -5,12 +5,9 @@ //! For each transaction the VM executes, the VM will output a `WriteSet` that contains each access //! path it updates. For each access path, the VM can either give its new value or delete it. -use crate::{ - state_store::{ - state_key::StateKey, - state_value::{PersistedStateValueMetadata, StateValue, StateValueMetadata}, - }, - write_set::WriteOp::{Creation, Deletion, Modification}, +use crate::state_store::{ + state_key::StateKey, + state_value::{PersistedStateValueMetadata, StateValue, StateValueMetadata}, }; use anyhow::{bail, ensure, Result}; use aptos_crypto_derive::{BCSCryptoHash, CryptoHasher}; @@ -68,45 +65,25 @@ impl PersistedWriteOp { use PersistedWriteOp::*; match self { - Creation(data) => WriteOp::Creation { - data, - metadata: StateValueMetadata::none(), - }, - Modification(data) => WriteOp::Modification { - data, - metadata: StateValueMetadata::none(), - }, - Deletion => WriteOp::Deletion { - metadata: StateValueMetadata::none(), - }, - CreationWithMetadata { data, metadata } => WriteOp::Creation { - data, - metadata: metadata.into_in_mem_form(), - }, - ModificationWithMetadata { data, metadata } => WriteOp::Modification { - data, - metadata: metadata.into_in_mem_form(), + Creation(data) => WriteOp::legacy_creation(data), + Modification(data) => WriteOp::legacy_modification(data), + Deletion => WriteOp::legacy_deletion(), + CreationWithMetadata { data, metadata } => { + WriteOp::creation(data, metadata.into_in_mem_form()) }, - DeletionWithMetadata { metadata } => WriteOp::Deletion { - metadata: metadata.into_in_mem_form(), + ModificationWithMetadata { data, metadata } => { + WriteOp::modification(data, metadata.into_in_mem_form()) }, + DeletionWithMetadata { metadata } => WriteOp::deletion(metadata.into_in_mem_form()), } } } #[derive(Clone, Eq, PartialEq)] pub enum WriteOp { - Creation { - data: Bytes, - metadata: StateValueMetadata, - }, - Modification { - data: Bytes, - metadata: StateValueMetadata, - }, - Deletion { - metadata: StateValueMetadata, - }, + Creation(StateValue), + Modification(StateValue), + Deletion(StateValueMetadata), } impl WriteOp { @@ -116,17 +93,17 @@ impl WriteOp { let metadata = self.metadata().clone().into_persistable(); match metadata { None => match self { - WriteOp::Creation { data, .. } => Creation(data.clone()), - WriteOp::Modification { data, .. } => Modification(data.clone()), + WriteOp::Creation(v) => Creation(v.bytes().clone()), + WriteOp::Modification(v) => Modification(v.bytes().clone()), WriteOp::Deletion { .. } => Deletion, }, Some(metadata) => match self { - WriteOp::Creation { data, .. } => CreationWithMetadata { - data: data.clone(), + WriteOp::Creation(v) => CreationWithMetadata { + data: v.bytes().clone(), metadata, }, - WriteOp::Modification { data, .. } => ModificationWithMetadata { - data: data.clone(), + WriteOp::Modification(v) => ModificationWithMetadata { + data: v.bytes().clone(), metadata, }, WriteOp::Deletion { .. } => DeletionWithMetadata { metadata }, @@ -147,43 +124,32 @@ impl WriteOp { => { bail!("The given change sets cannot be squashed") }, - (Creation {metadata: old_meta, .. } , Modification {data, metadata}) => { - Self::ensure_metadata_compatible(old_meta, &metadata)?; + (Creation(c) , Modification(m)) => { + Self::ensure_metadata_compatible(c.metadata(), m.metadata())?; - *op = Creation { - data, - metadata, - }; + *op = Creation(m) }, - (Modification{metadata: old_meta, .. } , Modification {data, metadata}) => { - Self::ensure_metadata_compatible(old_meta, &metadata)?; + (Modification(c) , Modification(m)) => { + Self::ensure_metadata_compatible(c.metadata(), m.metadata())?; - *op = Modification { - data, - metadata, - }; + *op = Modification(m); }, - (Modification {metadata: old_meta, ..}, Deletion {metadata}) => { - Self::ensure_metadata_compatible(old_meta, &metadata)?; + (Modification(m), Deletion(d_meta)) => { + Self::ensure_metadata_compatible(m.metadata(), &d_meta)?; - *op = Deletion { - metadata, - } + *op = Deletion(d_meta) }, - (Deletion {metadata}, Creation {data, ..}) => { + (Deletion(d_meta), Creation(c)) => { // n.b. With write sets from multiple sessions being squashed together, it's possible // to see two ops carrying different metadata (or one with it the other without) // due to deleting in one session and recreating in another. The original metadata // shouldn't change due to the squash. // And because the deposit or refund happens after all squashing is finished, it's // not a concern of fairness. - *op = Modification { - data, - metadata: metadata.clone(), - } + *op = Modification(StateValue::new_with_metadata(c.into_bytes(), d_meta.clone())) }, - (Creation { metadata: old_meta, .. }, Deletion { metadata }) => { - Self::ensure_metadata_compatible(old_meta, &metadata)?; + (Creation(c), Deletion(d_meta)) => { + Self::ensure_metadata_compatible(c.metadata(), &d_meta)?; return Ok(false) }, @@ -204,70 +170,73 @@ impl WriteOp { Ok(()) } - pub fn bytes(&self) -> Option<&Bytes> { + pub fn state_value_ref(&self) -> Option<&StateValue> { use WriteOp::*; match self { - Creation { data, .. } | Modification { data, .. } => Some(data), - Deletion { .. } => None, + Creation(v) | Modification(v) => Some(v), + Deletion(..) => None, } } - pub fn size(&self) -> usize { - use WriteOp::*; + pub fn bytes(&self) -> Option<&Bytes> { + self.state_value_ref().map(StateValue::bytes) + } - match self { - Creation { data, .. } | Modification { data, .. } => data.len(), - Deletion { .. } => 0, - } + /// Size not counting metadata. + pub fn bytes_size(&self) -> usize { + self.bytes().map_or(0, Bytes::len) } pub fn metadata(&self) -> &StateValueMetadata { use WriteOp::*; match self { - Creation { metadata, .. } | Modification { metadata, .. } | Deletion { metadata } => { - metadata - }, + Creation(v) | Modification(v) => v.metadata(), + Deletion(meta) => meta, } } - pub fn get_metadata_mut(&mut self) -> &mut StateValueMetadata { + pub fn metadata_mut(&mut self) -> &mut StateValueMetadata { use WriteOp::*; match self { - Creation { metadata, .. } | Modification { metadata, .. } | Deletion { metadata } => { - metadata - }, + Creation(v) | Modification(v) => v.metadata_mut(), + Deletion(meta) => meta, } } pub fn into_metadata(self) -> StateValueMetadata { + use WriteOp::*; + match self { - Creation { metadata, .. } | Modification { metadata, .. } | Deletion { metadata } => { - metadata - }, + Creation(v) | Modification(v) => v.into_metadata(), + Deletion(meta) => meta, } } + pub fn creation(data: Bytes, metadata: StateValueMetadata) -> Self { + Self::Creation(StateValue::new_with_metadata(data, metadata)) + } + + pub fn modification(data: Bytes, metadata: StateValueMetadata) -> Self { + Self::Modification(StateValue::new_with_metadata(data, metadata)) + } + + pub fn deletion(metadata: StateValueMetadata) -> Self { + Self::Deletion(metadata) + } + pub fn legacy_creation(data: Bytes) -> Self { - Self::Creation { - data, - metadata: StateValueMetadata::none(), - } + Self::Creation(StateValue::new_legacy(data)) } pub fn legacy_modification(data: Bytes) -> Self { - Self::Modification { - data, - metadata: StateValueMetadata::none(), - } + Self::Modification(StateValue::new_legacy(data)) } pub fn legacy_deletion() -> Self { - Self::Deletion { - metadata: StateValueMetadata::none(), - } + Self::Deletion(StateValueMetadata::none()) } } @@ -316,8 +285,7 @@ pub trait TransactionWrite: Debug { // Provided as a separate method to avoid the clone in as_state_value method // (although default implementation below does just that). fn as_state_value_metadata(&self) -> Option { - self.as_state_value() - .map(|state_value| state_value.into_metadata()) + self.as_state_value().map(StateValue::into_metadata) } // Often, the contents of W:TransactionWrite are converted to Option, e.g. @@ -376,23 +344,19 @@ impl TransactionWrite for WriteOp { } fn as_state_value(&self) -> Option { - self.bytes() - .map(|bytes| StateValue::new_with_metadata(bytes.clone(), self.metadata().clone())) + self.state_value_ref().cloned() } // Note that even if WriteOp is DeletionWithMetadata, the method returns None, as a later // read would not read the metadata of the deletion op. fn as_state_value_metadata(&self) -> Option { - self.bytes().map(|_| self.metadata().clone()) + self.state_value_ref().map(StateValue::metadata).cloned() } fn from_state_value(maybe_state_value: Option) -> Self { match maybe_state_value { None => Self::legacy_deletion(), - Some(state_value) => { - let (metadata, data) = state_value.unpack(); - Self::Modification { data, metadata } - }, + Some(state_value) => Self::Modification(state_value), } } @@ -409,33 +373,37 @@ impl TransactionWrite for WriteOp { use WriteOp::*; match self { - Creation { data, .. } | Modification { data, .. } => *data = bytes, + Creation(v) | Modification(v) => v.set_bytes(bytes), Deletion { .. } => (), } } } #[allow(clippy::format_collect)] -impl std::fmt::Debug for WriteOp { +impl Debug for WriteOp { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + use WriteOp::*; + match self { - Creation { data, metadata } => write!( + Creation(v) => write!( f, "Creation({}, metadata:{:?})", - data.iter() + v.bytes() + .iter() .map(|byte| format!("{:02x}", byte)) .collect::(), - metadata, + v.metadata(), ), - Modification { data, metadata } => write!( + Modification(v) => write!( f, "Modification({}, metadata:{:?})", - data.iter() + v.bytes() + .iter() .map(|byte| format!("{:02x}", byte)) .collect::(), - metadata, + v.metadata(), ), - Deletion { metadata } => { + Deletion(metadata) => { write!(f, "Deletion(metadata:{:?})", metadata,) }, }