Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[script-composer] Add infer functionality, fix multiple return values #15438

Merged
merged 1 commit into from
Dec 4, 2024
Merged

[script-composer] Add infer functionality, fix multiple return values #15438

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
39 changes: 26 additions & 13 deletions aptos-move/script-composer/src/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,14 +317,35 @@ impl TransactionComposer {
.collect::<Vec<_>>()
};

for return_ty in expected_returns_ty {
let mut returned_arguments = vec![];
let num_of_calls = self.calls.len() as u16;

for (idx, return_ty) in expected_returns_ty.into_iter().enumerate() {
let ability = BinaryIndexedView::Script(self.builder.as_script())
.abilities(&return_ty, &[])
.map_err(|_| anyhow!("Failed to calculate ability for type"))?;

let local_idx = self.locals_ty.len() as u16;
self.locals_ty.push(return_ty);
self.locals_availability.push(true);
returns.push(local_idx);

// For values that has drop and copy ability, use copy by default to avoid calling copy manually
// on the client side.
returned_arguments.push(CallArgument::PreviousResult(PreviousResult {
operation_type: if ability.has_drop() && ability.has_copy() {
ArgumentOperation::Copy
} else {
ArgumentOperation::Move
},
call_idx: num_of_calls,
return_idx: idx as u16,
}));
}

let num_of_calls = self.calls.len() as u16;
if self.parameters.len() + self.locals_ty.len() > u8::MAX as usize {
bail!("Too many locals being allocated, please truncate the transaction");
}

self.calls.push(BuilderCall {
type_args: type_arguments,
Expand All @@ -335,15 +356,7 @@ impl TransactionComposer {
type_tags: ty_args,
});

Ok((0..returns.len())
.map(|idx| {
CallArgument::PreviousResult(PreviousResult {
operation_type: ArgumentOperation::Move,
call_idx: num_of_calls,
return_idx: idx as u16,
})
})
.collect())
Ok(returned_arguments)
}

fn check_drop_at_end(&self) -> anyhow::Result<()> {
Expand Down Expand Up @@ -390,11 +403,11 @@ impl TransactionComposer {
}

// Storing return values
for arg in call.returns {
for arg in call.returns.iter().rev() {
script
.code
.code
.push(Bytecode::StLoc((arg + parameters_count) as u8));
.push(Bytecode::StLoc((*arg + parameters_count) as u8));
Copy link
Contributor

Choose a reason for hiding this comment

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

Although not part of this PR, do we check for "returns + parameters < 255", or does that happen later when running the script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be checked by the bytecode verifier. But I added a check so that the user can be prompted earlier and not having to see the lengthy bytecode verifier error.

Copy link
Contributor

Choose a reason for hiding this comment

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

@runtian-zhou There are a bunch of as u* casts in this file, but it is not clear if all of those are safe casts. E.g., num_of_calls unconditionally cast to u16.

For example, in the case above, without the check for u8::MAX, the value would get silently truncated and have incorrect code generated. It is likely that something downstream (like the bytecode verifier) would catch it, but might be good to add checks here in the builder (unless some of these are checked earlier somewhere).

}
}
script.code.code.push(Bytecode::Ret);
Expand Down
74 changes: 72 additions & 2 deletions aptos-move/script-composer/src/tests/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,9 @@ fn test_module() {

run_txn(builder, &mut h);

// Create a copyable value and move it twice
// Create a droppable and copyable value and move it twice. This is ok because the builder
// will use copy instruction instead of move instruction for values that are both copyable
// and droppable.
let mut builder = TransactionComposer::single_signer();
load_module(&mut builder, &h, "0x1::batched_execution");
let returns_1 = builder
Expand Down Expand Up @@ -329,7 +331,7 @@ fn test_module() {
)
.unwrap();

assert!(builder
builder
.add_batched_call(
"0x1::batched_execution".to_string(),
"consume_copyable_value".to_string(),
Expand All @@ -339,6 +341,47 @@ fn test_module() {
CallArgument::new_bytes(MoveValue::U8(10).simple_serialize().unwrap()),
],
)
.unwrap();

run_txn(builder, &mut h);

// Create a droppable value and move it twice
let mut builder = TransactionComposer::single_signer();
load_module(&mut builder, &h, "0x1::batched_execution");
let returns_1 = builder
.add_batched_call(
"0x1::batched_execution".to_string(),
"create_droppable_value".to_string(),
vec![],
vec![CallArgument::new_bytes(
MoveValue::U8(10).simple_serialize().unwrap(),
)],
)
.unwrap()
.pop()
.unwrap();

builder
.add_batched_call(
"0x1::batched_execution".to_string(),
"consume_droppable_value".to_string(),
vec![],
vec![
returns_1.clone(),
CallArgument::new_bytes(MoveValue::U8(10).simple_serialize().unwrap()),
],
)
.unwrap();
assert!(builder
.add_batched_call(
"0x1::batched_execution".to_string(),
"consume_droppable_value".to_string(),
vec![],
vec![
returns_1,
CallArgument::new_bytes(MoveValue::U8(10).simple_serialize().unwrap()),
],
)
.is_err());

// Copying a non-copyable value should return error on call.
Expand Down Expand Up @@ -635,4 +678,31 @@ fn test_module() {
],
)
.is_err());

// Test functions with multiple return values.
// Create a copyable value and copy it twice
let mut builder = TransactionComposer::single_signer();
load_module(&mut builder, &h, "0x1::batched_execution");
let returns = builder
.add_batched_call(
"0x1::batched_execution".to_string(),
"multiple_returns".to_string(),
vec![],
vec![],
)
.unwrap();

builder
.add_batched_call(
"0x1::batched_execution".to_string(),
"consume_non_droppable_value".to_string(),
vec![],
vec![
returns[1].clone(),
CallArgument::new_bytes(MoveValue::U8(1).simple_serialize().unwrap()),
],
)
.unwrap();

run_txn(builder, &mut h);
}
6 changes: 5 additions & 1 deletion aptos-move/script-composer/src/tests/test_modules/sources/test.move
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ module 0x1::batched_execution {
val: u8,
}

struct CopyableValue has copy {
struct CopyableValue has drop, copy {
val: u8,
}

Expand Down Expand Up @@ -73,4 +73,8 @@ module 0x1::batched_execution {
let GenericNonDroppableValue { val } = v;
assert!(val == expected_val, 10);
}

public fun multiple_returns(): (DroppableValue, NonDroppableValue) {
return (DroppableValue { val: 0 }, NonDroppableValue { val: 1} )
}
}
Loading