[move traits] Move VM MoveStorage trait to move-core-types

Increasingly, Move tools are relying on the MoveStorage trait for fetching modules and resources from on-chain. This trait lives in the VM, but tooling should not need to pull in the VM as a dependency just to use this trait. This PR moves the trait to move-core-types and does some renaming to disambiguate from the MoveStorage trait in storage-interface. Additionally, it makes the error type generic instead of forcing implementers to return VM-internal errors. One thing we might want to look at in the future is fixing the inconsistency between `get_module` (which takes a `ModuleId`) and `get_resource` (which takes an address and type). I think we should either change `get_module` to take an address and a name or change `get_resource` to take a `ResourceKey`). Closes: aptos-labs#8886
gpashu87 · Aug 11, 2021 · 844c4a7 · 844c4a7
1 parent cd43a03
commit 844c4a7
Show file tree

Hide file tree

Showing 33 changed files with 275 additions and 202 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/language/diem-tools/diem-read-write-set/Cargo.toml b/language/diem-tools/diem-read-write-set/Cargo.toml
@@ -15,5 +15,4 @@ diem-workspace-hack = { path = "../../../common/workspace-hack" }
 diem-types = { path = "../../../types" }
 diem-vm = { path = "../../diem-vm" }
 move-core-types = { path = "../../move-core/types" }
-move-vm-runtime = { path = "../../move-vm/runtime" }
 read-write-set = { path = "../../tools/read-write-set" }
diff --git a/language/diem-tools/diem-read-write-set/src/lib.rs b/language/diem-tools/diem-read-write-set/src/lib.rs
@@ -11,9 +11,9 @@ use move_core_types::{
     ident_str,
     identifier::IdentStr,
     language_storage::{ResourceKey, StructTag},
+    resolver::MoveResolver,
     value::{serialize_values, MoveValue},
 };
-use move_vm_runtime::data_cache::MoveStorage;
 use std::ops::Deref;
 
 pub struct ReadWriteSetAnalysis(read_write_set::ReadWriteSetAnalysis);
@@ -34,7 +34,7 @@ impl ReadWriteSetAnalysis {
     pub fn get_keys_written(
         &self,
         tx: &SignedTransaction,
-        blockchain_view: &dyn MoveStorage,
+        blockchain_view: &impl MoveResolver,
     ) -> Result<Vec<ResourceKey>> {
         self.get_concretized_keys_tx(tx, blockchain_view, true)
     }
@@ -46,15 +46,15 @@ impl ReadWriteSetAnalysis {
     pub fn get_keys_read(
         &self,
         tx: &SignedTransaction,
-        blockchain_view: &dyn MoveStorage,
+        blockchain_view: &impl MoveResolver,
     ) -> Result<Vec<ResourceKey>> {
         self.get_concretized_keys_tx(tx, blockchain_view, false)
     }
 
     fn get_concretized_keys_tx(
         &self,
         tx: &SignedTransaction,
-        blockchain_view: &dyn MoveStorage,
+        blockchain_view: &impl MoveResolver,
         is_write: bool,
     ) -> Result<Vec<ResourceKey>> {
         match tx.payload() {

diff --git a/language/diem-tools/writeset-transaction-generator/src/writeset_builder.rs b/language/diem-tools/writeset-transaction-generator/src/writeset_builder.rs
@@ -12,15 +12,16 @@ use diem_vm::{convert_changeset_and_events, data_cache::RemoteStorage};
 use move_core_types::{
     identifier::Identifier,
     language_storage::{ModuleId, TypeTag},
+    resolver::MoveResolver,
     transaction_argument::convert_txn_args,
     value::{serialize_values, MoveValue},
 };
-use move_vm_runtime::{data_cache::MoveStorage, move_vm::MoveVM, session::Session};
+use move_vm_runtime::{move_vm::MoveVM, session::Session};
 use move_vm_types::gas_schedule::GasStatus;
 
 pub struct GenesisSession<'r, 'l, S>(Session<'r, 'l, S>);
 
-impl<'r, 'l, S: MoveStorage> GenesisSession<'r, 'l, S> {
+impl<'r, 'l, S: MoveResolver> GenesisSession<'r, 'l, S> {
     pub fn exec_func(
         &mut self,
         module_name: &str,

diff --git a/language/diem-vm/src/data_cache.rs b/language/diem-vm/src/data_cache.rs
@@ -18,8 +18,8 @@ use move_binary_format::errors::*;
 use move_core_types::{
     account_address::AccountAddress,
     language_storage::{ModuleId, StructTag},
+    resolver::{ModuleResolver, ResourceResolver},
 };
-use move_vm_runtime::data_cache::MoveStorage;
 use std::collections::btree_map::BTreeMap;
 
 /// A local cache for a given a `StateView`. The cache is private to the Diem layer
@@ -107,16 +107,22 @@ impl<'block> StateView for StateViewCache<'block> {
     }
 }
 
-impl<'block> MoveStorage for StateViewCache<'block> {
-    fn get_module(&self, module_id: &ModuleId) -> VMResult<Option<Vec<u8>>> {
+impl<'block> ModuleResolver for StateViewCache<'block> {
+    type Error = VMError;
+
+    fn get_module(&self, module_id: &ModuleId) -> Result<Option<Vec<u8>>, Self::Error> {
         RemoteStorage::new(self).get_module(module_id)
     }
+}
+
+impl<'block> ResourceResolver for StateViewCache<'block> {
+    type Error = VMError;
 
     fn get_resource(
         &self,
         address: &AccountAddress,
         tag: &StructTag,
-    ) -> PartialVMResult<Option<Vec<u8>>> {
+    ) -> Result<Option<Vec<u8>>, Self::Error> {
         RemoteStorage::new(self).get_resource(address, tag)
     }
 }
@@ -142,20 +148,26 @@ impl<'a, S: StateView> RemoteStorage<'a, S> {
     }
 }
 
-impl<'a, S: StateView> MoveStorage for RemoteStorage<'a, S> {
-    fn get_module(&self, module_id: &ModuleId) -> VMResult<Option<Vec<u8>>> {
+impl<'a, S: StateView> ModuleResolver for RemoteStorage<'a, S> {
+    type Error = VMError;
+
+    fn get_module(&self, module_id: &ModuleId) -> Result<Option<Vec<u8>>, Self::Error> {
         // REVIEW: cache this?
         let ap = AccessPath::from(module_id);
         self.get(&ap).map_err(|e| e.finish(Location::Undefined))
     }
+}
+
+impl<'a, S: StateView> ResourceResolver for RemoteStorage<'a, S> {
+    type Error = VMError;
 
     fn get_resource(
         &self,
         address: &AccountAddress,
         struct_tag: &StructTag,
-    ) -> PartialVMResult<Option<Vec<u8>>> {
+    ) -> Result<Option<Vec<u8>>, Self::Error> {
         let ap = create_access_path(*address, struct_tag.clone());
-        self.get(&ap)
+        self.get(&ap).map_err(|e| e.finish(Location::Undefined))
     }
 }
 

diff --git a/language/diem-vm/src/diem_transaction_executor.rs b/language/diem-vm/src/diem_transaction_executor.rs
@@ -35,10 +35,11 @@ use move_core_types::{
     account_address::AccountAddress,
     gas_schedule::GasAlgebra,
     identifier::IdentStr,
+    resolver::MoveResolver,
     transaction_argument::convert_txn_args,
     value::{serialize_values, MoveValue},
 };
-use move_vm_runtime::{data_cache::MoveStorage, session::Session};
+use move_vm_runtime::session::Session;
 use move_vm_types::gas_schedule::GasStatus;
 use rayon::prelude::*;
 use std::{
@@ -59,7 +60,7 @@ impl DiemVM {
 
     /// Generates a transaction output for a transaction that encountered errors during the
     /// execution process. This is public for now only for tests.
-    pub fn failed_transaction_cleanup<S: MoveStorage>(
+    pub fn failed_transaction_cleanup<S: MoveResolver>(
         &self,
         error_code: VMStatus,
         gas_status: &mut GasStatus,
@@ -79,7 +80,7 @@ impl DiemVM {
         .1
     }
 
-    fn failed_transaction_cleanup_and_keep_vm_status<S: MoveStorage>(
+    fn failed_transaction_cleanup_and_keep_vm_status<S: MoveResolver>(
         &self,
         error_code: VMStatus,
         gas_status: &mut GasStatus,
@@ -124,7 +125,7 @@ impl DiemVM {
         }
     }
 
-    fn success_transaction_cleanup<S: MoveStorage>(
+    fn success_transaction_cleanup<S: MoveResolver>(
         &self,
         mut session: Session<S>,
         gas_status: &mut GasStatus,
@@ -153,7 +154,7 @@ impl DiemVM {
         ))
     }
 
-    fn execute_script_or_script_function<S: MoveStorage>(
+    fn execute_script_or_script_function<S: MoveResolver>(
         &self,
         mut session: Session<S>,
         gas_status: &mut GasStatus,
@@ -240,7 +241,7 @@ impl DiemVM {
         }
     }
 
-    fn execute_module<S: MoveStorage>(
+    fn execute_module<S: MoveResolver>(
         &self,
         mut session: Session<S>,
         gas_status: &mut GasStatus,
@@ -281,7 +282,7 @@ impl DiemVM {
         )
     }
 
-    fn execute_user_transaction<S: MoveStorage>(
+    fn execute_user_transaction<S: MoveResolver>(
         &self,
         storage: &S,
         txn: &SignatureCheckedTransaction,
@@ -366,7 +367,7 @@ impl DiemVM {
         }
     }
 
-    fn execute_writeset<S: MoveStorage>(
+    fn execute_writeset<S: MoveResolver>(
         &self,
         storage: &S,
         writeset_payload: &WriteSetPayload,
@@ -439,7 +440,7 @@ impl DiemVM {
         Ok(())
     }
 
-    fn process_waypoint_change_set<S: MoveStorage + StateView>(
+    fn process_waypoint_change_set<S: MoveResolver + StateView>(
         &self,
         storage: &S,
         writeset_payload: WriteSetPayload,
@@ -457,7 +458,7 @@ impl DiemVM {
         ))
     }
 
-    fn process_block_prologue<S: MoveStorage>(
+    fn process_block_prologue<S: MoveResolver>(
         &self,
         storage: &S,
         block_metadata: BlockMetadata,
@@ -508,7 +509,7 @@ impl DiemVM {
         Ok((VMStatus::Executed, output))
     }
 
-    fn process_writeset_transaction<S: MoveStorage + StateView>(
+    fn process_writeset_transaction<S: MoveResolver + StateView>(
         &self,
         storage: &S,
         txn: &SignatureCheckedTransaction,
@@ -551,7 +552,7 @@ impl DiemVM {
         )
     }
 
-    pub fn execute_writeset_transaction<S: MoveStorage + StateView>(
+    pub fn execute_writeset_transaction<S: MoveResolver + StateView>(
         &self,
         storage: &S,
         writeset_payload: &WriteSetPayload,
@@ -750,7 +751,7 @@ impl DiemVM {
         Ok(result)
     }
 
-    pub(crate) fn execute_single_transaction<S: MoveStorage + StateView>(
+    pub(crate) fn execute_single_transaction<S: MoveResolver + StateView>(
         &self,
         txn: &PreprocessedTransaction,
         data_cache: &S,

diff --git a/language/diem-vm/src/diem_transaction_validator.rs b/language/diem-vm/src/diem_transaction_validator.rs
@@ -20,8 +20,9 @@ use diem_types::{
 use move_core_types::{
     identifier::{IdentStr, Identifier},
     move_resource::MoveStructType,
+    resolver::MoveResolver,
 };
-use move_vm_runtime::{data_cache::MoveStorage, session::Session};
+use move_vm_runtime::session::Session;
 
 use crate::logging::AdapterLogSchema;
 
@@ -118,7 +119,7 @@ fn get_account_role(sender: AccountAddress, remote_cache: &StateViewCache) -> Go
     GovernanceRole::NonGovernanceRole
 }
 
-pub(crate) fn validate_signature_checked_transaction<S: MoveStorage>(
+pub(crate) fn validate_signature_checked_transaction<S: MoveResolver>(
     vm: &DiemVMImpl,
     mut session: &mut Session<S>,
     transaction: &SignatureCheckedTransaction,
@@ -185,7 +186,7 @@ pub(crate) fn validate_signature_checked_transaction<S: MoveStorage>(
     Ok((normalized_gas_price, currency_code))
 }
 
-fn get_currency_info<S: MoveStorage>(
+fn get_currency_info<S: MoveResolver>(
     currency_code: &IdentStr,
     remote_cache: &S,
 ) -> Result<CurrencyInfoResource, VMStatus> {