Addressed review comments and added tests

- Introduced a new type PayloadTypeReference which will describe user-transaction payload type and reference payload if required. - Updated TransactionMetadata and UserTransctionContext to utilize PayloadTypeReference enabling easy extension of any new payload type when needed - Updated transaction_arg_validation::validate_combine_signer_and_txn_args to accept GasMeter as input argument. - Updated automation inner payload validation to charge gas for it - Added initial set of e2e tests to check Automation transaction flow.
Entropy-Foundation · Dec 6, 2024 · 71c43e9 · 71c43e9
1 parent af7d278
commit 71c43e9
Show file tree

Hide file tree

Showing 18 changed files with 1,456 additions and 109 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/aptos-move/aptos-vm/src/aptos_vm.rs b/aptos-move/aptos-vm/src/aptos_vm.rs
@@ -731,6 +731,7 @@ impl AptosVM {
 
         let args = verifier::transaction_arg_validation::validate_combine_signer_and_txn_args(
             session,
+            &mut UnmeteredGasMeter,
             senders,
             convert_txn_args(script.args()),
             &func,
@@ -788,6 +789,7 @@ impl AptosVM {
             self.features().is_enabled(FeatureFlag::STRUCT_CONSTRUCTORS);
         let args = verifier::transaction_arg_validation::validate_combine_signer_and_txn_args(
             session,
+            &mut UnmeteredGasMeter,
             senders,
             entry_fn.args().to_vec(),
             &function,
@@ -2542,7 +2544,7 @@ impl AptosVM {
     fn validate_automation_txn_inner_payload(
         &self,
         session: &mut SessionExt,
-        _gas_meter: &mut impl AptosGasMeter,
+        gas_meter: &mut impl AptosGasMeter,
         senders: Vec<AccountAddress>,
         automation_entry_fn: &EntryFunction,
     ) -> Result<(), VMStatus> {
@@ -2552,10 +2554,15 @@ impl AptosVM {
                 Some("Automation transaction payload arguments are empty".to_string()),
             ));
         }
-        let maybe_inner_payload_bytes = &automation_entry_fn.args()[0];
-        let inner_entry_function = bcs::from_bytes::<EntryFunction>(maybe_inner_payload_bytes).map_err(|e| {
-            VMStatus::error(StatusCode::FAILED_TO_DESERIALIZE_ARGUMENT,
-            Some(format!("Automation transaction payload first argument is expected to be BCS bytes of entry-function {}", e)))
+        // first we should deserialize actual parameter of Vec<u8> which represents bytes of the EntryFunction.
+        let param_bytes = &automation_entry_fn.args()[0];
+        let inner_entry_function = bcs::from_bytes::<Vec<u8>>(param_bytes)
+            .and_then(|payload_bytes| bcs::from_bytes::<EntryFunction>(&payload_bytes))
+            .map_err(|e| {
+                VMStatus::error(StatusCode::FAILED_TO_DESERIALIZE_ARGUMENT,
+                                Some(
+                                    format!("Automation transaction payload first argument is \
+                                    expected to be BCS bytes of entry-function {}", e)))
         })?;
 
         let function = session.load_function(
@@ -2570,6 +2577,7 @@ impl AptosVM {
         // task will not fail due to invalid arguments passed
         verifier::transaction_arg_validation::validate_combine_signer_and_txn_args(
             session,
+            gas_meter,
             senders,
             actual_args,
             &function,

diff --git a/aptos-move/aptos-vm/src/transaction_metadata.rs b/aptos-move/aptos-vm/src/transaction_metadata.rs
@@ -7,7 +7,6 @@
 use aptos_crypto::HashValue;
 use aptos_gas_algebra::{FeePerGasUnit, Gas, NumBytes};
 use aptos_types::transaction::automated_transaction::AutomatedTransaction;
-use aptos_types::transaction::automation::AutomationTransactionPayload;
 use aptos_types::{
     account_address::AccountAddress,
     chain_id::ChainId,
@@ -16,7 +15,9 @@ use aptos_types::{
         SignedTransaction, TransactionPayload,
     },
 };
+use aptos_types::transaction::user_transaction_context::{PayloadTypeReference, PayloadTypeReferenceContext};
 
+pub type PayloadTypeReferenceMeta = PayloadTypeReference<EntryFunction, Multisig>;
 pub struct TransactionMetadata {
     pub sender: AccountAddress,
     pub authentication_key: Vec<u8>,
@@ -33,14 +34,19 @@ pub struct TransactionMetadata {
     pub script_hash: Vec<u8>,
     pub script_size: NumBytes,
     pub is_keyless: bool,
-    pub entry_function_payload: Option<EntryFunction>,
-    pub multisig_payload: Option<Multisig>,
-    pub automation_payload: Option<AutomationTransactionPayload>,
+    pub payload_type_reference: PayloadTypeReferenceMeta,
     pub txn_app_hash: Vec<u8>,
 }
 
 impl TransactionMetadata {
     pub fn new(txn: &SignedTransaction) -> Self {
+        let payload_type_reference = match txn.payload() {
+            TransactionPayload::Script(_) |
+            TransactionPayload::ModuleBundle(_) => PayloadTypeReferenceMeta::Other,
+            TransactionPayload::EntryFunction(e) => PayloadTypeReferenceMeta::UserEntryFunction(e.clone()),
+            TransactionPayload::Multisig(m) => PayloadTypeReferenceMeta::Multisig(m.clone()),
+            TransactionPayload::Automation(_) => PayloadTypeReferenceMeta::Automation,
+        };
         Self {
             sender: txn.sender(),
             authentication_key: txn.authenticator().sender().authentication_key().to_vec(),
@@ -79,18 +85,7 @@ impl TransactionMetadata {
             is_keyless: aptos_types::keyless::get_authenticators(txn)
                 .map(|res| !res.is_empty())
                 .unwrap_or(false),
-            entry_function_payload: match txn.payload() {
-                TransactionPayload::EntryFunction(e) => Some(e.clone()),
-                _ => None,
-            },
-            multisig_payload: match txn.payload() {
-                TransactionPayload::Multisig(m) => Some(m.clone()),
-                _ => None,
-            },
-            automation_payload: match txn.payload() {
-                TransactionPayload::Automation(a) => Some(a.clone()),
-                _ => None,
-            },
+            payload_type_reference,
             txn_app_hash: HashValue::sha3_256_of(
                 &bcs::to_bytes(&txn).expect("Unable to serialize SignedTransaction"),
             )
@@ -157,28 +152,28 @@ impl TransactionMetadata {
     }
 
     pub fn entry_function_payload(&self) -> Option<EntryFunction> {
-        self.entry_function_payload.clone()
+        self.payload_type_reference.entry_function_payload()
     }
 
     pub fn multisig_payload(&self) -> Option<Multisig> {
-        self.multisig_payload.clone()
+        self.payload_type_reference.multisig_payload()
     }
 
     pub fn as_user_transaction_context(&self) -> UserTransactionContext {
+        let payload_type_reference = match &self.payload_type_reference {
+            PayloadTypeReferenceMeta::Other => PayloadTypeReferenceContext::Other,
+            PayloadTypeReferenceMeta::UserEntryFunction(e) => PayloadTypeReferenceContext::UserEntryFunction(e.as_entry_function_payload()),
+            PayloadTypeReferenceMeta::Multisig(m) => PayloadTypeReferenceContext::Multisig(m.as_multisig_payload()),
+            PayloadTypeReferenceMeta::Automation => PayloadTypeReferenceContext::Automation,
+        };
         UserTransactionContext::new(
             self.sender,
             self.secondary_signers.clone(),
             self.fee_payer.unwrap_or(self.sender),
             self.max_gas_amount.into(),
             self.gas_unit_price.into(),
             self.chain_id.id(),
-            self.entry_function_payload()
-                .map(|entry_func| entry_func.as_entry_function_payload()),
-            self.multisig_payload()
-                .map(|multisig| multisig.as_multisig_payload()),
-            self.automation_payload
-                .as_ref()
-                .and_then(|e| e.as_entry_function_payload()),
+            payload_type_reference,
             self.txn_app_hash.clone(),
         )
     }
@@ -202,12 +197,7 @@ impl From<&AutomatedTransaction> for TransactionMetadata {
             script_hash: vec![],
             script_size: NumBytes::zero(),
             is_keyless: false,
-            entry_function_payload: match txn.payload() {
-                TransactionPayload::EntryFunction(e) => Some(e.clone()),
-                _ => None,
-            },
-            multisig_payload: None,
-            automation_payload: None,
+            payload_type_reference: PayloadTypeReferenceMeta::Automation,
             txn_app_hash: txn.hash().to_vec(),
         }
     }

diff --git a/aptos-move/aptos-vm/src/verifier/transaction_arg_validation.rs b/aptos-move/aptos-vm/src/verifier/transaction_arg_validation.rs
@@ -24,10 +24,7 @@ use move_vm_runtime::{
     module_traversal::{TraversalContext, TraversalStorage},
     LoadedFunction,
 };
-use move_vm_types::{
-    gas::{GasMeter, UnmeteredGasMeter},
-    loaded_data::runtime_types::Type,
-};
+use move_vm_types::{gas::GasMeter, loaded_data::runtime_types::Type};
 use once_cell::sync::Lazy;
 use std::{
     collections::BTreeMap,
@@ -41,43 +38,70 @@ pub(crate) struct FunctionId {
 
 type ConstructorMap = Lazy<BTreeMap<String, FunctionId>>;
 static OLD_ALLOWED_STRUCTS: ConstructorMap = Lazy::new(|| {
-    [("0x1::string::String", FunctionId {
-        module_id: ModuleId::new(AccountAddress::ONE, Identifier::from(ident_str!("string"))),
-        func_name: ident_str!("utf8"),
-    })]
+    [(
+        "0x1::string::String",
+        FunctionId {
+            module_id: ModuleId::new(AccountAddress::ONE, Identifier::from(ident_str!("string"))),
+            func_name: ident_str!("utf8"),
+        },
+    )]
     .into_iter()
     .map(|(s, validator)| (s.to_string(), validator))
     .collect()
 });
 
 static NEW_ALLOWED_STRUCTS: ConstructorMap = Lazy::new(|| {
     [
-        ("0x1::string::String", FunctionId {
-            module_id: ModuleId::new(AccountAddress::ONE, Identifier::from(ident_str!("string"))),
-            func_name: ident_str!("utf8"),
-        }),
-        ("0x1::object::Object", FunctionId {
-            module_id: ModuleId::new(AccountAddress::ONE, Identifier::from(ident_str!("object"))),
-            func_name: ident_str!("address_to_object"),
-        }),
-        ("0x1::option::Option", FunctionId {
-            module_id: ModuleId::new(AccountAddress::ONE, Identifier::from(ident_str!("option"))),
-            func_name: ident_str!("from_vec"),
-        }),
-        ("0x1::fixed_point32::FixedPoint32", FunctionId {
-            module_id: ModuleId::new(
-                AccountAddress::ONE,
-                Identifier::from(ident_str!("fixed_point32")),
-            ),
-            func_name: ident_str!("create_from_raw_value"),
-        }),
-        ("0x1::fixed_point64::FixedPoint64", FunctionId {
-            module_id: ModuleId::new(
-                AccountAddress::ONE,
-                Identifier::from(ident_str!("fixed_point64")),
-            ),
-            func_name: ident_str!("create_from_raw_value"),
-        }),
+        (
+            "0x1::string::String",
+            FunctionId {
+                module_id: ModuleId::new(
+                    AccountAddress::ONE,
+                    Identifier::from(ident_str!("string")),
+                ),
+                func_name: ident_str!("utf8"),
+            },
+        ),
+        (
+            "0x1::object::Object",
+            FunctionId {
+                module_id: ModuleId::new(
+                    AccountAddress::ONE,
+                    Identifier::from(ident_str!("object")),
+                ),
+                func_name: ident_str!("address_to_object"),
+            },
+        ),
+        (
+            "0x1::option::Option",
+            FunctionId {
+                module_id: ModuleId::new(
+                    AccountAddress::ONE,
+                    Identifier::from(ident_str!("option")),
+                ),
+                func_name: ident_str!("from_vec"),
+            },
+        ),
+        (
+            "0x1::fixed_point32::FixedPoint32",
+            FunctionId {
+                module_id: ModuleId::new(
+                    AccountAddress::ONE,
+                    Identifier::from(ident_str!("fixed_point32")),
+                ),
+                func_name: ident_str!("create_from_raw_value"),
+            },
+        ),
+        (
+            "0x1::fixed_point64::FixedPoint64",
+            FunctionId {
+                module_id: ModuleId::new(
+                    AccountAddress::ONE,
+                    Identifier::from(ident_str!("fixed_point64")),
+                ),
+                func_name: ident_str!("create_from_raw_value"),
+            },
+        ),
     ]
     .into_iter()
     .map(|(s, validator)| (s.to_string(), validator))
@@ -103,6 +127,7 @@ pub(crate) fn get_allowed_structs(
 /// after validation, add senders and non-signer arguments to generate the final args
 pub fn validate_combine_signer_and_txn_args(
     session: &mut SessionExt,
+    gas_meter: &mut impl GasMeter,
     senders: Vec<AccountAddress>,
     args: Vec<Vec<u8>>,
     func: &LoadedFunction,
@@ -173,6 +198,7 @@ pub fn validate_combine_signer_and_txn_args(
     // FAILED_TO_DESERIALIZE_ARGUMENT error.
     let args = construct_args(
         session,
+        gas_meter,
         &func.param_tys()[signer_param_cnt..],
         args,
         func.ty_args(),
@@ -219,14 +245,13 @@ pub(crate) fn is_valid_txn_arg(
 // TODO: This needs a more solid story and a tighter integration with the VM.
 pub(crate) fn construct_args(
     session: &mut SessionExt,
+    gas_meter: &mut impl GasMeter,
     types: &[Type],
     args: Vec<Vec<u8>>,
     ty_args: &[Type],
     allowed_structs: &ConstructorMap,
     is_view: bool,
 ) -> Result<Vec<Vec<u8>>, VMStatus> {
-    // Perhaps in a future we should do proper gas metering here
-    let mut gas_meter = UnmeteredGasMeter;
     let mut res_args = vec![];
     if types.len() != args.len() {
         return Err(invalid_signature());
@@ -241,7 +266,7 @@ pub(crate) fn construct_args(
             subst_res.map_err(|e| e.finish(Location::Undefined).into_vm_status())?
         };
 
-        let arg = construct_arg(session, &ty, allowed_structs, arg, &mut gas_meter, is_view)?;
+        let arg = construct_arg(session, &ty, allowed_structs, arg, gas_meter, is_view)?;
         res_args.push(arg);
     }
     Ok(res_args)

diff --git a/aptos-move/aptos-vm/src/verifier/view_function.rs b/aptos-move/aptos-vm/src/verifier/view_function.rs
@@ -9,6 +9,7 @@ use aptos_framework::RuntimeModuleMetadataV1;
 use move_binary_format::errors::{PartialVMError, PartialVMResult};
 use move_core_types::{identifier::IdentStr, vm_status::StatusCode};
 use move_vm_runtime::LoadedFunction;
+use move_vm_types::gas::UnmeteredGasMeter;
 
 /// Based on the function attributes in the module metadata, determine whether a
 /// function is a view function.
@@ -56,6 +57,7 @@ pub(crate) fn validate_view_function(
     let allowed_structs = get_allowed_structs(struct_constructors_feature);
     let args = transaction_arg_validation::construct_args(
         session,
+        &mut UnmeteredGasMeter,
         func.param_tys(),
         args,
         func.ty_args(),

diff --git a/aptos-move/e2e-tests/src/executor.rs b/aptos-move/e2e-tests/src/executor.rs
@@ -1131,6 +1131,7 @@ impl FakeExecutor {
             session.load_function(entry_fn.module(), entry_fn.function(), entry_fn.ty_args())?;
         let args = verifier::transaction_arg_validation::validate_combine_signer_and_txn_args(
             &mut session,
+            &mut UnmeteredGasMeter,
             senders,
             entry_fn.args().to_vec(),
             &func,

diff --git a/aptos-move/e2e-testsuite/Cargo.toml b/aptos-move/e2e-testsuite/Cargo.toml
@@ -29,6 +29,7 @@ move-bytecode-verifier = { workspace = true }
 move-core-types = { workspace = true }
 move-ir-compiler = { workspace = true }
 proptest = { workspace = true }
+bcs = { workspace = true }
 
 [features]
 default = [