Skip to content

Commit

Permalink
[framework] improve transaction payload generation with module names
Browse files Browse the repository at this point in the history
before there were no module names, a function name would be claimed by
the first written transaction. That leads to really weird issues where
claim, withdraw, deposit, transfer can only live in one module.

There was also a lot of excessive verbosity with _script_function that
waas unnecessary.
  • Loading branch information
davidiw committed May 3, 2022
1 parent b023013 commit da86b2f
Show file tree
Hide file tree
Showing 44 changed files with 304 additions and 235 deletions.
144 changes: 80 additions & 64 deletions Cargo.lock

Large diffs are not rendered by default.

13 changes: 5 additions & 8 deletions aptos-move/e2e-tests/src/common_transactions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,7 @@
//! Support for encoding transactions for common situations.
use crate::account::Account;
use aptos_transaction_builder::aptos_stdlib::{
encode_create_account_script_function, encode_rotate_authentication_key_script_function,
encode_transfer_script_function,
};
use aptos_transaction_builder::aptos_stdlib;
use aptos_types::transaction::{RawTransaction, Script, SignedTransaction};
use move_ir_compiler::Compiler;
use once_cell::sync::Lazy;
Expand Down Expand Up @@ -49,7 +46,7 @@ pub fn create_account_txn(
) -> SignedTransaction {
sender
.transaction()
.payload(encode_create_account_script_function(
.payload(aptos_stdlib::encode_account_create_account(
*new_account.address(),
))
.sequence_number(seq_num)
Expand All @@ -67,7 +64,7 @@ pub fn peer_to_peer_txn(
// get a SignedTransaction
sender
.transaction()
.payload(encode_transfer_script_function(
.payload(aptos_stdlib::encode_test_coin_transfer(
*receiver.address(),
transfer_amount,
))
Expand All @@ -79,7 +76,7 @@ pub fn peer_to_peer_txn(
pub fn rotate_key_txn(sender: &Account, new_key_hash: Vec<u8>, seq_num: u64) -> SignedTransaction {
sender
.transaction()
.payload(encode_rotate_authentication_key_script_function(
.payload(aptos_stdlib::encode_account_rotate_authentication_key(
new_key_hash,
))
.sequence_number(seq_num)
Expand All @@ -90,7 +87,7 @@ pub fn rotate_key_txn(sender: &Account, new_key_hash: Vec<u8>, seq_num: u64) ->
pub fn raw_rotate_key_txn(sender: &Account, new_key_hash: Vec<u8>, seq_num: u64) -> RawTransaction {
sender
.transaction()
.payload(encode_rotate_authentication_key_script_function(
.payload(aptos_stdlib::encode_account_rotate_authentication_key(
new_key_hash,
))
.sequence_number(seq_num)
Expand Down
4 changes: 2 additions & 2 deletions aptos-move/e2e-tests/src/on_chain_configs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@
// SPDX-License-Identifier: Apache-2.0

use crate::{account::Account, executor::FakeExecutor};
use aptos_transaction_builder::aptos_stdlib::encode_set_version_script_function;
use aptos_transaction_builder::aptos_stdlib;
use aptos_types::on_chain_config::Version;
use aptos_vm::AptosVM;

pub fn set_aptos_version(executor: &mut FakeExecutor, version: Version) {
let account = Account::new_genesis_account(aptos_types::on_chain_config::config_address());
let txn = account
.transaction()
.payload(encode_set_version_script_function(version.major))
.payload(aptos_stdlib::encode_version_set_version(version.major))
.sequence_number(0)
.sign();
executor.new_block();
Expand Down
4 changes: 2 additions & 2 deletions aptos-move/e2e-tests/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

#![forbid(unsafe_code)]
use crate::{account::Account, compile, executor::FakeExecutor};
use aptos_transaction_builder::aptos_stdlib::encode_set_version_script_function;
use aptos_transaction_builder::aptos_stdlib;
use move_binary_format::file_format::CompiledModule;

pub fn close_module_publishing(
Expand Down Expand Up @@ -61,7 +61,7 @@ pub fn upgrade_df(
executor.execute_and_apply(
dr_account
.transaction()
.payload(encode_set_version_script_function(version_number))
.payload(aptos_stdlib::encode_version_set_version(version_number))
.sequence_number(*dr_seqno)
.sign(),
);
Expand Down
4 changes: 2 additions & 2 deletions aptos-move/e2e-testsuite/src/tests/mint.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright (c) Aptos
// SPDX-License-Identifier: Apache-2.0

use aptos_transaction_builder::aptos_stdlib::encode_mint_script_function;
use aptos_transaction_builder::aptos_stdlib;
use aptos_types::transaction::{ExecutionStatus, TransactionStatus};
use language_e2e_tests::{
gas_costs::TXN_RESERVED, test_with_different_versions, versioning::CURRENT_RELEASE_VERSIONS,
Expand All @@ -21,7 +21,7 @@ fn mint_to_new_account() {
let mint_amount = TXN_RESERVED;
let output = executor.execute_transaction(
root.transaction()
.payload(encode_mint_script_function(
.payload(aptos_stdlib::encode_test_coin_mint(
*new_account.address(),
mint_amount,
))
Expand Down
6 changes: 3 additions & 3 deletions aptos-move/e2e-testsuite/src/tests/on_chain_configs.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright (c) Aptos
// SPDX-License-Identifier: Apache-2.0

use aptos_transaction_builder::aptos_stdlib::encode_set_version_script_function;
use aptos_transaction_builder::aptos_stdlib;
use aptos_types::{on_chain_config::Version, transaction::TransactionStatus};
use aptos_vm::AptosVM;
use language_e2e_tests::{
Expand All @@ -24,7 +24,7 @@ fn initial_aptos_version() {
let account = test_env.dr_account;
let txn = account
.transaction()
.payload(encode_set_version_script_function(test_env.version_number + 1))
.payload(aptos_stdlib::encode_version_set_version(test_env.version_number + 1))
.sequence_number(test_env.dr_sequence_number)
.sign();
executor.new_block();
Expand Down Expand Up @@ -53,7 +53,7 @@ fn drop_txn_after_reconfiguration() {
let account = test_env.dr_account;
let txn = account
.transaction()
.payload(encode_set_version_script_function(test_env.version_number + 1))
.payload(aptos_stdlib::encode_version_set_version(test_env.version_number + 1))
.sequence_number(test_env.dr_sequence_number)
.sign();
executor.new_block();
Expand Down
8 changes: 4 additions & 4 deletions aptos-move/e2e-testsuite/src/tests/verify_txn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// SPDX-License-Identifier: Apache-2.0

use aptos_crypto::{ed25519::Ed25519PrivateKey, PrivateKey, Uniform};
use aptos_transaction_builder::aptos_stdlib::encode_transfer_script_function;
use aptos_transaction_builder::aptos_stdlib;
use aptos_types::{
account_address::AccountAddress,
account_config,
Expand Down Expand Up @@ -34,7 +34,7 @@ fn verify_signature() {
executor.add_account_data(&sender);
// Generate a new key pair to try and sign things with.
let private_key = Ed25519PrivateKey::generate_for_testing();
let program = encode_transfer_script_function(
let program = aptos_stdlib::encode_test_coin_transfer(
*sender.address(),
100,
);
Expand Down Expand Up @@ -166,7 +166,7 @@ fn verify_reserved_sender() {
executor.add_account_data(&sender);
// Generate a new key pair to try and sign things with.
let private_key = Ed25519PrivateKey::generate_for_testing();
let program = encode_transfer_script_function(
let program = aptos_stdlib::encode_test_coin_transfer(
*sender.address(),
100,
);
Expand Down Expand Up @@ -206,7 +206,7 @@ fn verify_simple_payment() {
let txn = sender
.account()
.transaction()
.payload(encode_transfer_script_function(*receiver.address(), transfer_amount))
.payload(aptos_stdlib::encode_test_coin_transfer(*receiver.address(), transfer_amount))
.sequence_number(10)
.sign();
assert_eq!(executor.verify_transaction(txn).status(), None);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// SPDX-License-Identifier: Apache-2.0

use aptos_types::AccountAddress;
use framework::{encode_transfer_script_function, ScriptFunctionCall};
use framework::{encode_test_coin_transfer, ScriptFunctionCall};

fn demo_p2p_script_function() {
let payee = AccountAddress([
Expand All @@ -13,10 +13,10 @@ fn demo_p2p_script_function() {
let amount = 1234567;

// Now encode and decode a peer to peer transaction script function.
let payload = encode_transfer_script_function(payee.clone(), amount);
let payload = encode_test_coin_transfer(payee.clone(), amount);
let function_call = ScriptFunctionCall::decode(&payload);
match function_call {
Some(ScriptFunctionCall::Transfer { amount: a, to: p }) => {
Some(ScriptFunctionCall::TestCoinTransfer { amount: a, to: p }) => {
assert_eq!(a, amount);
assert_eq!(p, payee.clone());
}
Expand Down
29 changes: 29 additions & 0 deletions aptos-move/transaction-builder-generator/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,35 @@ pub(crate) fn make_abi_enum_container(abis: &[ScriptABI]) -> ContainerFormat {
ContainerFormat::Enum(variants)
}

pub(crate) fn make_namespaced_abi_enum_container(abis: &[ScriptABI]) -> ContainerFormat {
let mut variants = BTreeMap::new();
for (index, abi) in abis.iter().enumerate() {
let mut fields = Vec::new();
for ty_arg in abi.ty_args() {
fields.push(quote_type_parameter_as_field(ty_arg));
}
for arg in abi.args() {
fields.push(quote_parameter_as_field(arg));
}

let name = match abi {
ScriptABI::ScriptFunction(sf) => {
format!("{}{}", sf.module_name().name(), abi.name().to_camel_case())
}
_ => abi.name().to_camel_case(),
};

variants.insert(
index as u32,
Named {
name,
value: VariantFormat::Struct(fields),
},
);
}
ContainerFormat::Enum(variants)
}

pub(crate) fn mangle_type(type_tag: &TypeTag) -> String {
use TypeTag::*;
match type_tag {
Expand Down
20 changes: 18 additions & 2 deletions aptos-move/transaction-builder-generator/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,11 @@ fn get_abi_paths(dir: &Path) -> std::io::Result<Vec<String>> {
abi_paths.append(&mut get_abi_paths(&path)?);
} else if let Some("abi") = path.extension().and_then(OsStr::to_str) {
// not read Genesis abi (script builder doesn't work with the script function there)
if !path.to_str().map(|s| s.contains("Genesis")).unwrap() {
if !path
.to_str()
.map(|s| s.contains("/Genesis/") || s.contains("/Coin/"))
.unwrap()
{
abi_paths.push(path.to_str().unwrap().to_string());
}
}
Expand All @@ -57,7 +61,19 @@ pub fn read_abis(dir_paths: &[impl AsRef<Path>]) -> anyhow::Result<Vec<ScriptABI
}
// Sort scripts by alphabetical order.
#[allow(clippy::unnecessary_sort_by)]
abis.sort_by(|a, b| a.name().cmp(b.name()));
abis.sort_by(|a, b| {
let a0 = if let ScriptABI::ScriptFunction(sf) = a {
sf.module_name().name().to_string()
} else {
"".to_owned()
};
let b0 = if let ScriptABI::ScriptFunction(sf) = b {
sf.module_name().name().to_string()
} else {
"".to_owned()
};
(a0, a.name()).cmp(&(b0, b.name()))
});
Ok(abis)
}

Expand Down
42 changes: 30 additions & 12 deletions aptos-move/transaction-builder-generator/src/rust.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use serde_generate::{
rust, CodeGeneratorConfig,
};

use heck::{CamelCase, ShoutySnakeCase};
use heck::{CamelCase, ShoutySnakeCase, SnakeCase};
use serde_reflection::ContainerFormat;
use std::{
collections::BTreeMap,
Expand Down Expand Up @@ -149,7 +149,7 @@ where
};
let mut script_function_registry: BTreeMap<_, _> = vec![(
"ScriptFunctionCall".to_string(),
common::make_abi_enum_container(script_fun_abis.as_slice()),
common::make_namespaced_abi_enum_container(script_fun_abis.as_slice()),
)]
.into_iter()
.collect();
Expand All @@ -166,7 +166,12 @@ where
"ScriptFunctionCall"
}
.to_string(),
abi.name().to_camel_case(),
match abi {
ScriptABI::ScriptFunction(sf) => {
format!("{}{}", sf.module_name().name(), abi.name().to_camel_case())
}
_ => abi.name().to_camel_case(),
},
],
common::prepare_doc_string(abi.doc()),
)
Expand Down Expand Up @@ -277,7 +282,7 @@ pub fn encode(self) -> Script {{"#
writeln!(self.out, "use ScriptCall::*;\nmatch self {{")?;
self.out.indent();
for abi in abis {
self.output_variant_encoder(&ScriptABI::TransactionScript(abi.clone()), false)?;
self.output_variant_encoder(&ScriptABI::TransactionScript(abi.clone()))?;
}
self.out.unindent();
writeln!(self.out, "}}")?;
Expand All @@ -296,27 +301,35 @@ pub fn encode(self) -> TransactionPayload {{"#
writeln!(self.out, "use ScriptFunctionCall::*;\nmatch self {{")?;
self.out.indent();
for abi in abis {
self.output_variant_encoder(&ScriptABI::ScriptFunction(abi.clone()), true)?;
self.output_variant_encoder(&ScriptABI::ScriptFunction(abi.clone()))?;
}
self.out.unindent();
writeln!(self.out, "}}")?;
self.out.unindent();
writeln!(self.out, "}}\n")
}

fn output_variant_encoder(&mut self, abi: &ScriptABI, is_script_fun: bool) -> Result<()> {
fn output_variant_encoder(&mut self, abi: &ScriptABI) -> Result<()> {
let params = std::iter::empty()
.chain(abi.ty_args().iter().map(TypeArgumentABI::name))
.chain(abi.args().iter().map(ArgumentABI::name))
.collect::<Vec<_>>()
.join(", ");

let prefix = if let ScriptABI::ScriptFunction(sf) = abi {
sf.module_name().name().to_string()
} else {
String::new()
};
writeln!(
self.out,
"{0}{{{2}}} => encode_{1}_script{3}({2}),",
"{5}{0}{{{2}}} => encode_{3}{4}{1}({2}),",
abi.name().to_camel_case(),
abi.name(),
params,
if is_script_fun { "_function" } else { "" },
prefix.to_snake_case(),
if prefix.is_empty() { "" } else { "_" },
prefix,
)
}

Expand Down Expand Up @@ -452,7 +465,8 @@ Script {{
fn emit_script_function_encoder_function(&mut self, abi: &ScriptFunctionABI) -> Result<()> {
write!(
self.out,
"pub fn encode_{}_script_function({}) -> TransactionPayload {{",
"pub fn encode_{}_{}({}) -> TransactionPayload {{",
abi.module_name().name().to_string().to_snake_case(),
abi.name(),
[
Self::quote_type_parameters(abi.ty_args()),
Expand Down Expand Up @@ -515,9 +529,11 @@ TransactionPayload::ScriptFunction(ScriptFunction {{

fn emit_script_function_decoder_function(&mut self, abi: &ScriptFunctionABI) -> Result<()> {
// `payload` is always used, so don't need to fix warning "unused variable" by prefixing with "_"
//
writeln!(
self.out,
"\nfn decode_{}_script_function(payload: &TransactionPayload) -> Option<ScriptFunctionCall> {{",
"\nfn decode_{}_{}(payload: &TransactionPayload) -> Option<ScriptFunctionCall> {{",
abi.module_name().name().to_string().to_snake_case(),
abi.name(),
)?;
self.out.indent();
Expand All @@ -534,7 +550,8 @@ TransactionPayload::ScriptFunction(ScriptFunction {{
self.out.indent();
writeln!(
self.out,
"Some(ScriptFunctionCall::{} {{",
"Some(ScriptFunctionCall::{}{} {{",
abi.module_name().name(),
abi.name().to_camel_case(),
)?;
self.out.indent();
Expand Down Expand Up @@ -661,9 +678,10 @@ static SCRIPT_FUNCTION_DECODER_MAP: once_cell::sync::Lazy<ScriptFunctionDecoderM
for abi in abis {
writeln!(
self.out,
"map.insert(\"{}{}\".to_string(), Box::new(decode_{}_script_function));",
"map.insert(\"{}{}\".to_string(), Box::new(decode_{}_{}));",
abi.module_name().name(),
abi.name(),
abi.module_name().name().to_string().to_snake_case(),
abi.name()
)?;
}
Expand Down
7 changes: 3 additions & 4 deletions config/management/operational/src/account_resource.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,10 +104,9 @@ impl RotateOperatorKey {
let sequence_number = client.sequence_number(operator_account).await?;

// Build the operator rotation transaction
let rotate_key_script =
transaction_builder::encode_rotate_authentication_key_script_function(
AuthenticationKey::ed25519(&new_storage_key).to_vec(),
);
let rotate_key_script = transaction_builder::encode_account_rotate_authentication_key(
AuthenticationKey::ed25519(&new_storage_key).to_vec(),
);
let rotate_key_txn = build_raw_transaction(
config.chain_id,
operator_account,
Expand Down
Loading

0 comments on commit da86b2f

Please sign in to comment.