Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
805 changes: 764 additions & 41 deletions api/src/tests/multisig_transactions_test.rs

Large diffs are not rendered by default.

40 changes: 33 additions & 7 deletions api/src/transactions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1277,6 +1277,9 @@ impl TransactionsApi {
entry_function,
)?;
},
MultisigTransactionPayload::Script(script) => {
TransactionsApi::validate_script_payload_format(ledger_info, script)?;
},
}
}
},
Expand All @@ -1296,13 +1299,6 @@ impl TransactionsApi {
}) => match executable {
TransactionExecutable::Script(script) => {
TransactionsApi::validate_script(ledger_info, script)?;
if extra_config.is_multisig() {
return Err(SubmitTransactionError::bad_request_with_code(
"Script transaction payload must not be a multisig transaction",
AptosErrorCode::InvalidInput,
ledger_info,
));
}
},
TransactionExecutable::EntryFunction(entry_function) => {
TransactionsApi::validate_entry_function_payload_format(
Expand Down Expand Up @@ -1389,6 +1385,33 @@ impl TransactionsApi {
Ok(())
}

fn validate_script_payload_format(
ledger_info: &LedgerInfo,
script: &Script,
) -> Result<(), SubmitTransactionError> {
if script.code().is_empty() {
return Err(SubmitTransactionError::bad_request_with_code(
"Script payload bytecode must not be empty",
AptosErrorCode::InvalidInput,
ledger_info,
));
}

for arg in script.ty_args() {
let arg = MoveType::from(arg);
arg.verify(0)
.context("Transaction script function type arg invalid")
.map_err(|err| {
SubmitTransactionError::bad_request_with_code(
err,
AptosErrorCode::InvalidInput,
ledger_info,
)
})?;
}
Ok(())
}

/// Parses a batch of signed transactions
fn get_signed_transactions_batch(
&self,
Expand Down Expand Up @@ -1666,6 +1689,9 @@ impl TransactionsApi {
&entry_function.function().into(),
)
},
MultisigTransactionPayload::Script(_) => {
format!("Multisig::Script::{}", txn.committed_hash()).to_string()
},
}
} else {
"Multisig::unknown".to_string()
Expand Down
56 changes: 56 additions & 0 deletions api/test-context/src/test_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,34 @@ impl TestContext {
.await;
}

pub async fn execute_multisig_transaction_with_script_payload(
&mut self,
owner: &mut LocalAccount,
multisig_account: AccountAddress,
bytecode_hex: &str,
type_args: &[&str],
args: &[&str],
expected_status_code: u16,
) {
self.api_execute_txn_expecting(
owner,
json!({
"type": "multisig_payload",
"multisig_address": multisig_account.to_hex_literal(),
"transaction_payload": {
"type": "script_payload",
"code": {
"bytecode": format!("0x{}", bytecode_hex),
},
"type_arguments": type_args,
"arguments": args
}
}),
expected_status_code,
)
.await;
}

pub fn get_indexer_reader(&self) -> Option<&Arc<dyn IndexerReader>> {
self.context.get_indexer_reader()
}
Expand Down Expand Up @@ -1209,6 +1237,34 @@ impl TestContext {
.await
}

pub async fn simulate_multisig_script_transaction(
&mut self,
owner: &LocalAccount,
multisig_account: AccountAddress,
bytecode_hex: &str,
type_args: &[&str],
args: &[&str],
expected_status_code: u16,
) -> Value {
self.simulate_transaction(
owner,
json!({
"type": "multisig_payload",
"multisig_address": multisig_account.to_hex_literal(),
"transaction_payload": {
"type": "script_payload",
"code": {
"bytecode": format!("0x{}", bytecode_hex),
},
"type_arguments": type_args,
"arguments": args
}
}),
expected_status_code,
)
.await
}

pub async fn simulate_transaction(
&mut self,
sender: &LocalAccount,
Expand Down
28 changes: 24 additions & 4 deletions api/types/src/convert.rs
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,10 @@ impl<'a, S: StateView> MoveConverter<'a, S> {
entry_function_payload,
))
},
aptos_types::transaction::MultisigTransactionPayload::Script(script) => {
let script_payload = try_into_script_payload(script)?;
Some(MultisigTransactionPayload::ScriptPayload(script_payload))
},
}
} else {
None
Expand Down Expand Up @@ -371,10 +375,15 @@ impl<'a, S: StateView> MoveConverter<'a, S> {
),
),
}),
aptos_types::transaction::TransactionExecutable::Script(_) => {
bail!(
"Script executable is not supported for multisig transactions"
)
aptos_types::transaction::TransactionExecutable::Script(script) => {
TransactionPayload::MultisigPayload(MultisigPayload {
multisig_address: multisig_address.into(),
transaction_payload: Some(
MultisigTransactionPayload::ScriptPayload(
try_into_script_payload(script)?,
),
),
})
},
aptos_types::transaction::TransactionExecutable::Empty => {
TransactionPayload::MultisigPayload(MultisigPayload {
Expand Down Expand Up @@ -790,6 +799,9 @@ impl<'a, S: StateView> MoveConverter<'a, S> {
) => Executable::EntryFunction(try_into_entry_function(
entry_func_payload,
)?),
MultisigTransactionPayload::ScriptPayload(script) => {
Executable::Script(try_into_script_payload(script)?)
},
}
} else {
Executable::Empty
Expand Down Expand Up @@ -817,6 +829,14 @@ impl<'a, S: StateView> MoveConverter<'a, S> {
),
)
},
MultisigTransactionPayload::ScriptPayload(script) => {
let script = try_into_script_payload(script)?;
Some(
aptos_types::transaction::MultisigTransactionPayload::Script(
script,
),
)
},
}
} else {
None
Expand Down
7 changes: 7 additions & 0 deletions api/types/src/transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1105,6 +1105,7 @@ impl TryFrom<Script> for ScriptPayload {
#[oai(one_of, discriminator_name = "type", rename_all = "snake_case")]
pub enum MultisigTransactionPayload {
EntryFunctionPayload(EntryFunctionPayload),
ScriptPayload(ScriptPayload),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why adding it here as well? We can just move to the new format, that supports orderless as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For existing multisig providers, it's a lot less work to just use the existing rails.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with support in the SDK, is the work any different?

and even if they are creating manually, they can just fully switch to the new format (i.e. even for entry functions), so they don't need any iffs in their code.

this is a good time for them to support orderless transactions? otherwise they'll need to switch again later?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Orderless transactions can be significantly more expensive than non-orderless transactions, and I suspect that it is mostly unnecessary to be used with Multisig transactions.

SDK requires you to basically build some parts for the payload, so I suspect it may change the flows significantly depending on the language SDK used.

}

/// A multisig transaction that allows an owner of a multisig account to execute a pre-approved
Expand All @@ -1129,6 +1130,12 @@ impl VerifyInput for MultisigPayload {
type_arg.verify(0)?;
}
},
MultisigTransactionPayload::ScriptPayload(script_payload) => {
script_payload.verify()?;
for type_arg in script_payload.type_arguments.iter() {
type_arg.verify(0)?;
}
},
}
}

Expand Down
83 changes: 60 additions & 23 deletions aptos-move/aptos-vm/src/aptos_vm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -877,16 +877,16 @@ impl AptosVM {
Ok((VMStatus::Executed, output))
}

fn validate_and_execute_script<'a>(
fn validate_and_execute_script(
&self,
session: &mut SessionExt<impl AptosMoveResolver>,
serialized_signers: &SerializedSigners,
code_storage: &impl AptosCodeStorage,
// Note: cannot use AptosGasMeter because it is not implemented for
// UnmeteredGasMeter.
gas_meter: &mut impl GasMeter,
traversal_context: &mut TraversalContext<'a>,
serialized_script: &'a Script,
traversal_context: &mut TraversalContext,
serialized_script: &Script,
trace_recorder: &mut impl TraceRecorder,
) -> Result<(), VMStatus> {
if !self
Expand Down Expand Up @@ -1176,7 +1176,7 @@ impl AptosVM {
fn execute_multisig_transaction<'r>(
&self,
resolver: &'r impl AptosMoveResolver,
module_storage: &impl AptosModuleStorage,
code_storage: &impl AptosCodeStorage,
mut session: UserSession<'r>,
serialized_signers: &SerializedSigners,
prologue_session_change_set: &SystemSessionChangeSet,
Expand Down Expand Up @@ -1230,12 +1230,9 @@ impl AptosVM {
bcs::to_bytes::<Vec<u8>>(&vec![]).map_err(|_| invariant_violation_error())?
}
},
TransactionExecutableRef::Script(_) => {
let s = VMStatus::error(
StatusCode::FEATURE_UNDER_GATING,
Some("Multisig transaction does not support script payload".to_string()),
);
return Ok((s, discarded_output(StatusCode::FEATURE_UNDER_GATING)));
TransactionExecutableRef::Script(script) => {
bcs::to_bytes(&MultisigTransactionPayload::Script(script.clone()))
.map_err(|_| invariant_violation_error())?
},
};
// Failures here will be propagated back.
Expand All @@ -1251,7 +1248,7 @@ impl AptosVM {
]),
gas_meter,
traversal_context,
module_storage,
code_storage,
)
})?
.return_values
Expand Down Expand Up @@ -1289,7 +1286,7 @@ impl AptosVM {
MultisigTransactionPayload::EntryFunction(entry_function) => self
.execute_multisig_entry_function(
resolver,
module_storage,
code_storage,
session,
gas_meter,
traversal_context,
Expand All @@ -1298,6 +1295,16 @@ impl AptosVM {
change_set_configs,
trace_recorder,
),
MultisigTransactionPayload::Script(script) => self.execute_multisig_script(
resolver,
code_storage,
session,
gas_meter,
traversal_context,
multisig_address,
&script,
change_set_configs,
),
};

// Step 3: Call post transaction cleanup function in multisig account module with the result
Expand All @@ -1313,7 +1320,7 @@ impl AptosVM {
let epilogue_session = match execution_result {
Err(execution_error) => self.failure_multisig_payload_cleanup(
resolver,
module_storage,
code_storage,
prologue_session_change_set,
execution_error,
txn_data,
Expand All @@ -1327,7 +1334,7 @@ impl AptosVM {
let mut epilogue_session = self.charge_change_set_and_respawn_session(
user_session_change_set,
resolver,
module_storage,
code_storage,
gas_meter,
txn_data,
)?;
Expand All @@ -1340,7 +1347,7 @@ impl AptosVM {
cleanup_args,
&mut UnmeteredGasMeter,
traversal_context,
module_storage,
code_storage,
)
.map_err(|e| e.into_vm_status())
})?;
Expand All @@ -1351,7 +1358,7 @@ impl AptosVM {
// TODO(Gas): Charge for aggregator writes
self.success_transaction_cleanup(
epilogue_session,
module_storage,
code_storage,
serialized_signers,
gas_meter,
txn_data,
Expand Down Expand Up @@ -1399,6 +1406,43 @@ impl AptosVM {
)
}

fn execute_multisig_script(
&self,
resolver: &impl AptosMoveResolver,
code_storage: &impl AptosCodeStorage,
mut session: UserSession,
gas_meter: &mut impl AptosGasMeter,
traversal_context: &mut TraversalContext,
multisig_address: AccountAddress,
payload: &Script,
change_set_configs: &ChangeSetConfigs,
) -> Result<UserSessionChangeSet, VMStatus> {
// If txn args are not valid, we'd still consider the transaction as executed but
// failed. This is primarily because it's unrecoverable at this point.
session.execute(|session| {
self.validate_and_execute_script(
session,
&SerializedSigners::new(vec![serialized_signer(&multisig_address)], None),
code_storage,
gas_meter,
traversal_context,
payload,
&mut NoOpTraceRecorder,
)
})?;

// Resolve any pending module publishes in case the multisig transaction is deploying
// modules.
self.resolve_pending_code_publish_and_finish_user_session(
session,
resolver,
code_storage,
gas_meter,
traversal_context,
change_set_configs,
)
}

fn failure_multisig_payload_cleanup<'r>(
&self,
resolver: &'r impl AptosMoveResolver,
Expand Down Expand Up @@ -2836,13 +2880,6 @@ impl AptosVM {
));
}

if executable.is_script() && extra_config.is_multisig() {
return Err(VMStatus::error(
StatusCode::FEATURE_UNDER_GATING,
Some("Script payload not yet supported for multisig transactions".to_string()),
));
}

// Runs script prologue for all transaction types including multisig
transaction_validation::run_script_prologue(
session,
Expand Down
Loading
Loading