Skip to content

Commit

Permalink
Address comments
Browse files Browse the repository at this point in the history
lxfind committed Jan 5, 2022
1 parent a12c60c commit 1367045
Showing 7 changed files with 114 additions and 52 deletions.
5 changes: 3 additions & 2 deletions fastpay_core/src/authority.rs
Original file line number Diff line number Diff line change
@@ -114,8 +114,9 @@ impl AuthorityState {
FastPayError::IncorrectSigner
);

// The last object in input_objects is always the gas payment object.
// TODO: Add get_gas_object_ref() to Order once Transfer order also has gas object.
if idx + 1 == input_object_cnt {
// The last object in input_objects is always the gas payment object.
check_gas_requirement(&order, &object)?;
}

@@ -211,7 +212,7 @@ impl AuthorityState {
c.type_arguments,
inputs,
c.pure_arguments,
Some(c.gas_budget),
c.gas_budget,
gas_object,
tx_ctx,
) {
13 changes: 13 additions & 0 deletions fastpay_core/src/authority/temporary_store.rs
Original file line number Diff line number Diff line change
@@ -75,6 +75,19 @@ impl AuthorityTemporaryStore {
},
"Object both written and deleted."
);

// Check all mutable inputs are either written or deleted
debug_assert!(
{
use std::collections::HashSet;
let mut used = HashSet::new();
self.written.iter().all(|elt| used.insert(elt.0));
self.deleted.iter().all(|elt| used.insert(elt.0));

self.active_inputs.iter().all(|elt| !used.insert(elt.0))
},
"Mutable input neither written nor deleted."
);
}
}

11 changes: 6 additions & 5 deletions fastpay_core/src/unit_tests/authority_tests.rs
Original file line number Diff line number Diff line change
@@ -7,7 +7,7 @@ use fastx_adapter::genesis;
#[cfg(test)]
use fastx_types::{
base_types::dbg_addr,
gas::{calculate_module_publish_gas, get_gas_balance},
gas::{calculate_module_publish_cost, get_gas_balance},
};
use move_binary_format::{
file_format::{self, AddressIdentifierIndex, IdentifierIndex, ModuleHandle},
@@ -199,6 +199,7 @@ fn make_dependent_module(m: &CompiledModule) -> CompiledModule {
dependent_module
}

#[cfg(test)]
fn check_gas_object(
gas_object: &Object,
expected_balance: u64,
@@ -262,7 +263,7 @@ async fn test_publish_module_no_dependencies_ok() {
let mut module_bytes = Vec::new();
module.serialize(&mut module_bytes).unwrap();
let module_bytes = vec![module_bytes];
let gas_cost = calculate_module_publish_gas(&module_bytes);
let gas_cost = calculate_module_publish_cost(&module_bytes);
let order = Order::new_module(sender, gas_payment_object_ref, module_bytes, &sender_key);
let module_object_id = TxContext::new(order.digest()).fresh_id();
let response = send_and_confirm_order(&mut authority, order).await.unwrap();
@@ -319,7 +320,7 @@ async fn test_handle_move_order() {
let mut genesis_module_objects = genesis.objects.clone();
let module_object_ref = genesis_module_objects
.iter()
.find_map(|o| match o.data.as_module() {
.find_map(|o| match o.data.try_as_module() {
Some(m) => {
if m.self_id().name() == ident_str!("ObjectBasics") {
Some((*m.self_id().address(), SequenceNumber::new()))
@@ -350,7 +351,7 @@ async fn test_handle_move_order() {
MAX_GAS,
&sender_key,
);
let gas_cost = 142;
let gas_cost = 142 + 24; // 142 is for bytecode execution, 24 is for object creation.
let res = send_and_confirm_order(&mut authority_state, order)
.await
.unwrap();
@@ -392,7 +393,7 @@ async fn test_handle_move_order_insufficient_budget() {
let mut genesis_module_objects = genesis.objects.clone();
let module_object_ref = genesis_module_objects
.iter()
.find_map(|o| match o.data.as_module() {
.find_map(|o| match o.data.try_as_module() {
Some(m) if m.self_id().name() == ident_str!("ObjectBasics") => {
Some((*m.self_id().address(), SequenceNumber::new()))
}
64 changes: 43 additions & 21 deletions fastx_programmability/adapter/src/adapter.rs
Original file line number Diff line number Diff line change
@@ -10,7 +10,10 @@ use fastx_types::{
TX_CONTEXT_MODULE_NAME, TX_CONTEXT_STRUCT_NAME,
},
error::{FastPayError, FastPayResult},
gas::{calculate_module_publish_gas, deduct_gas},
gas::{
calculate_module_publish_cost, calculate_object_creation_cost,
calculate_object_deletion_refund, deduct_gas,
},
object::{Data, MoveObject, Object},
storage::Storage,
};
@@ -47,8 +50,8 @@ pub fn execute<E: Debug, S: ResourceResolver<Error = E> + ModuleResolver<Error =
type_args: Vec<TypeTag>,
object_args: Vec<Object>,
pure_args: Vec<Vec<u8>>,
gas_budget: Option<u64>,
gas_object: Object,
gas_budget: u64,
mut gas_object: Object,
ctx: TxContext,
) -> Result<(), FastPayError> {
let TypeCheckSuccess {
@@ -69,7 +72,7 @@ pub fn execute<E: Debug, S: ResourceResolver<Error = E> + ModuleResolver<Error =
.expect("VM creation only fails if natives are invalid, and we created the natives");
// TODO: Update Move gas constants to reflect the gas fee on fastx.
let mut gas_status =
get_gas_status(gas_budget).map_err(|e| FastPayError::GasBudgetTooHigh {
get_gas_status(Some(gas_budget)).map_err(|e| FastPayError::GasBudgetTooHigh {
error: e.to_string(),
})?;
let session = vm.new_session(state_view);
@@ -94,8 +97,6 @@ pub fn execute<E: Debug, S: ResourceResolver<Error = E> + ModuleResolver<Error =
debug_assert!(change_set.accounts().is_empty());
// Input ref parameters we put in should be the same number we get out
debug_assert!(mutable_ref_objects.len() == mutable_ref_values.len());
// Either we consumed gas, or budget is not provided hence no gas metering.
debug_assert!(gas_used > 0 || gas_budget == None);
let mutable_refs = mutable_ref_objects
.into_iter()
.zip(mutable_ref_values.into_iter());
@@ -106,12 +107,20 @@ pub fn execute<E: Debug, S: ResourceResolver<Error = E> + ModuleResolver<Error =
events,
gas_object,
gas_used,
gas_budget,
)?;
Ok(())
}
ExecutionResult::Fail { error, gas_used: _ } => Err(FastPayError::AbortedExecution {
error: error.to_string(),
}),
ExecutionResult::Fail { error, gas_used } => {
// Need to deduct gas even if the execution failed.
deduct_gas(&mut gas_object, gas_used as i128)?;
state_view.write_object(gas_object);
// TODO: Keep track the gas deducted so that we could give them to participants.

Err(FastPayError::AbortedExecution {
error: error.to_string(),
})
}
}
}

@@ -127,9 +136,10 @@ pub fn publish<E: Debug, S: ResourceResolver<Error = E> + ModuleResolver<Error =
error: "Publishing empty list of modules".to_string(),
});
}
let gas_cost = calculate_module_publish_gas(&module_bytes);
deduct_gas(&mut gas_object, gas_cost)?;
let gas_cost = calculate_module_publish_cost(&module_bytes);
deduct_gas(&mut gas_object, gas_cost as i128)?;
state_view.write_object(gas_object);
// TODO: Keep track the gas deducted so that we could give them to participants.

let mut modules = module_bytes
.iter()
@@ -217,7 +227,8 @@ fn process_successful_execution<
mutable_refs: impl Iterator<Item = (Object, Vec<u8>)>,
events: Vec<Event>,
mut gas_object: Object,
gas_used: u64,
mut gas_used: u64,
gas_budget: u64,
) -> Result<(), FastPayError> {
for (mut obj, new_contents) in mutable_refs {
match &mut obj.data {
@@ -259,7 +270,9 @@ fn process_successful_execution<
old_object.next_sequence_number = sequence_number;
old_object
} else {
Object::new_move(move_obj, recipient, SequenceNumber::new())
let obj = Object::new_move(move_obj, recipient, SequenceNumber::new());
gas_used += calculate_object_creation_cost(&obj);
obj
};
new_object.owner = recipient;
state_view.write_object(new_object);
@@ -271,20 +284,29 @@ fn process_successful_execution<
unimplemented!("Processing user events")
}
}
if gas_used > gas_budget {
return Err(FastPayError::InsufficientGas {
error: format!(
"Gas budget is {}, not enough to pay for cost {}",
gas_budget, gas_used
),
});
}
// any object left in `by_value_objects` is an input passed by value that was not transferred or frozen.
// this means that either the object was (1) deleted from the FastX system altogether, or
// (2) wrapped inside another object that is in the FastX object pool
// in either case, we want to delete it
for id in by_value_objects.keys() {
let mut gas_refund: u64 = 0;
for (id, object) in by_value_objects.iter() {
state_view.delete_object(id);
gas_refund += calculate_object_deletion_refund(object);
}

if gas_used > 0 {
// The fact that Move VM succeeded means that there is enough gas.
// Hence deduct_gas cannot fail.
deduct_gas(&mut gas_object, gas_used).unwrap();
state_view.write_object(gas_object);
}
// TODO: In the current approach, we basically can use refund gas to pay for current transaction.
// Is this allowed?
deduct_gas(&mut gas_object, (gas_used as i128) - (gas_refund as i128))?;
state_view.write_object(gas_object);
// TODO: Keep track the gas deducted so that we could give them to participants.

Ok(())
}
@@ -392,7 +414,7 @@ fn resolve_and_type_check(
let mut args = Vec::new();
let mut mutable_ref_objects = Vec::new();
let mut by_value_objects = BTreeMap::new();
#[allow(unused_mut)]
#[cfg(debug_assertions)]
let mut num_immutable_objects = 0;
#[cfg(debug_assertions)]
let num_objects = object_args.len();
30 changes: 17 additions & 13 deletions fastx_programmability/adapter/src/unit_tests/adapter_tests.rs
Original file line number Diff line number Diff line change
@@ -40,7 +40,7 @@ impl InMemoryStorage {
pub fn find_module(&self, name: &str) -> Option<Object> {
let id = Identifier::new(name).unwrap();
for o in self.persistent.values() {
match o.data.as_module() {
match o.data.try_as_module() {
Some(m) if m.self_id().name() == id.as_ident_str() => return Some(o.clone()),
_ => (),
}
@@ -126,7 +126,7 @@ fn call(
native_functions: &NativeFunctionTable,
name: &str,
gas_object: Object,
gas_budget: Option<u64>,
gas_budget: u64,
object_args: Vec<Object>,
pure_args: Vec<Vec<u8>>,
) -> FastPayResult {
@@ -157,7 +157,11 @@ fn test_object_basics() {
let mut storage = InMemoryStorage::new(genesis.objects.clone());

// 0. Create a gas object for gas payment. Note that we won't really use it because we won't be providing a gas budget.
let gas_object = Object::with_id_for_testing(ObjectID::random());
let gas_object = Object::with_id_owner_gas_for_testing(
ObjectID::random(),
base_types::FastPayAddress::default(),
MAX_GAS,
);
storage.write_object(gas_object.clone());
storage.flush();

@@ -175,15 +179,15 @@ fn test_object_basics() {
&native_functions,
"create",
gas_object.clone(),
None,
MAX_GAS,
Vec::new(),
pure_args,
)
.unwrap();

let created = storage.created();
assert_eq!(created.len(), 1);
assert!(storage.updated().is_empty());
assert_eq!(storage.updated().len(), 1); // The gas object
assert!(storage.deleted().is_empty());
let id1 = created
.keys()
@@ -204,14 +208,14 @@ fn test_object_basics() {
&native_functions,
"transfer",
gas_object.clone(),
None,
MAX_GAS,
vec![obj1.clone()],
pure_args,
)
.unwrap();

let updated = storage.updated();
assert_eq!(updated.len(), 1);
assert_eq!(updated.len(), 2);
assert!(storage.created().is_empty());
assert!(storage.deleted().is_empty());
storage.flush();
@@ -232,7 +236,7 @@ fn test_object_basics() {
&native_functions,
"create",
gas_object.clone(),
None,
MAX_GAS,
Vec::new(),
pure_args,
)
@@ -251,13 +255,13 @@ fn test_object_basics() {
&native_functions,
"update",
gas_object.clone(),
None,
MAX_GAS,
vec![obj1.clone(), obj2],
Vec::new(),
)
.unwrap();
let updated = storage.updated();
assert_eq!(updated.len(), 1);
assert_eq!(updated.len(), 2);
assert!(storage.created().is_empty());
assert!(storage.deleted().is_empty());
storage.flush();
@@ -274,15 +278,15 @@ fn test_object_basics() {
&native_functions,
"delete",
gas_object,
None,
MAX_GAS,
vec![obj1],
Vec::new(),
)
.unwrap();
let deleted = storage.deleted();
assert_eq!(deleted.len(), 1);
assert!(storage.created().is_empty());
assert!(storage.updated().is_empty());
assert_eq!(storage.updated().len(), 1);
storage.flush();
assert!(storage.read_object(&id1).is_none())
}
@@ -317,7 +321,7 @@ fn test_move_call_insufficient_gas() {
&native_functions,
"create",
gas_object,
Some(50), // This budget is not enough to execute all bytecode.
50, // This budget is not enough to execute all bytecode.
Vec::new(),
pure_args,
);
Loading

0 comments on commit 1367045

Please sign in to comment.