From 65b0a2a4b863b95ccc14e1a84dfd644f14dfc3d5 Mon Sep 17 00:00:00 2001 From: austinabell Date: Mon, 20 Jan 2020 10:26:21 -0500 Subject: [PATCH 1/2] refactor message params, change serialization encoding format, update from changing spec --- vm/Cargo.toml | 3 ++ vm/actor/src/builtin/account.rs | 9 ++--- vm/actor/src/builtin/cron.rs | 18 ++++----- vm/actor/src/builtin/init.rs | 27 +++++--------- vm/actor/src/builtin/reward.rs | 4 +- vm/actor/src/builtin/storage_power.rs | 11 ++---- vm/message/src/lib.rs | 4 +- vm/message/src/signed_message.rs | 4 +- vm/message/src/unsigned_message.rs | 10 ++--- vm/message/tests/builder_test.rs | 6 +-- vm/runtime/src/actor_code.rs | 20 +--------- vm/src/invoc.rs | 4 +- vm/src/method.rs | 53 ++++++++++++--------------- vm/tests/params_test.rs | 38 +++++++++++++------ 14 files changed, 98 insertions(+), 113 deletions(-) diff --git a/vm/Cargo.toml b/vm/Cargo.toml index f4678d9de1fd..0a9411b773ed 100644 --- a/vm/Cargo.toml +++ b/vm/Cargo.toml @@ -9,3 +9,6 @@ num-bigint = "0.2.3" address = {path = "./address"} encoding = {path = "../encoding"} serde = {version = "1.0", features = ["derive"]} + +[dev-dependencies] +hex = "0.4.0" \ No newline at end of file diff --git a/vm/actor/src/builtin/account.rs b/vm/actor/src/builtin/account.rs index 92b44b21fee6..4276c15fae1a 100644 --- a/vm/actor/src/builtin/account.rs +++ b/vm/actor/src/builtin/account.rs @@ -4,8 +4,8 @@ use address::Address; use num_derive::FromPrimitive; use num_traits::FromPrimitive; -use runtime::{arg_end, ActorCode, Runtime}; -use vm::{ExitCode, InvocOutput, MethodNum, MethodParams, SysCode, METHOD_CONSTRUCTOR}; +use runtime::{ActorCode, Runtime}; +use vm::{ExitCode, InvocOutput, MethodNum, Serialized, SysCode, METHOD_CONSTRUCTOR}; /// AccountActorState includes the address for the actor pub struct AccountActorState { @@ -40,12 +40,11 @@ impl ActorCode for AccountActorCode { &self, rt: &RT, method: MethodNum, - params: &MethodParams, + _params: &Serialized, ) -> InvocOutput { match AccountMethod::from_method_num(method) { Some(AccountMethod::Constructor) => { - // Assert no parameters passed - arg_end(params, rt); + // TODO unfinished spec Self::constructor(rt) } _ => { diff --git a/vm/actor/src/builtin/cron.rs b/vm/actor/src/builtin/cron.rs index 3fb44b3440a2..9cf31e151614 100644 --- a/vm/actor/src/builtin/cron.rs +++ b/vm/actor/src/builtin/cron.rs @@ -2,14 +2,14 @@ // SPDX-License-Identifier: Apache-2.0 use vm::{ - ExitCode, InvocInput, InvocOutput, MethodNum, MethodParams, SysCode, TokenAmount, + ExitCode, InvocInput, InvocOutput, MethodNum, Serialized, SysCode, TokenAmount, METHOD_CONSTRUCTOR, METHOD_CRON, }; use address::Address; use num_derive::FromPrimitive; use num_traits::FromPrimitive; -use runtime::{arg_end, ActorCode, Runtime}; +use runtime::{ActorCode, Runtime}; /// CronActorState has no internal state #[derive(Default)] @@ -55,11 +55,11 @@ impl CronActorCode { fn epoch_tick(&self, rt: &RT) -> InvocOutput { // self.entries is basically a static registry for now, loaded // in the interpreter static registry. - for entry in self.entries.clone() { + for entry in &self.entries { let res = rt.send_catching_errors(InvocInput { - to: entry.to_addr, + to: entry.to_addr.clone(), method: entry.method_num, - params: MethodParams::default(), + params: Serialized::default(), value: TokenAmount::new(0), }); if let Err(e) = res { @@ -76,17 +76,15 @@ impl ActorCode for CronActorCode { &self, rt: &RT, method: MethodNum, - params: &MethodParams, + _params: &Serialized, ) -> InvocOutput { match CronMethod::from_method_num(method) { Some(CronMethod::Constructor) => { - // Assert no parameters passed - arg_end(params, rt); + // TODO unfinished spec Self::constructor(rt) } Some(CronMethod::Cron) => { - // Assert no parameters passed - arg_end(params, rt); + // TODO unfinished spec self.epoch_tick(rt) } _ => { diff --git a/vm/actor/src/builtin/init.rs b/vm/actor/src/builtin/init.rs index b80e3f74d5fa..676b1e90fc60 100644 --- a/vm/actor/src/builtin/init.rs +++ b/vm/actor/src/builtin/init.rs @@ -3,14 +3,14 @@ use crate::{ActorID, CodeID}; use vm::{ - ExitCode, InvocOutput, MethodNum, MethodParams, SysCode, METHOD_CONSTRUCTOR, METHOD_PLACEHOLDER, + ExitCode, InvocOutput, MethodNum, Serialized, SysCode, METHOD_CONSTRUCTOR, METHOD_PLACEHOLDER, }; use address::Address; -use encoding::{from_slice, Cbor}; +use encoding::Cbor; use num_derive::FromPrimitive; use num_traits::FromPrimitive; -use runtime::{arg_end, arg_pop, check_args, ActorCode, Runtime}; +use runtime::{ActorCode, Runtime}; use std::collections::HashMap; /// InitActorState is reponsible for creating @@ -52,7 +52,7 @@ impl InitActorCode { rt.success_return() } - fn exec(rt: &RT, _code: CodeID, _params: &MethodParams) -> InvocOutput { + fn exec(rt: &RT, _code: CodeID, _params: &Serialized) -> InvocOutput { // TODO let addr = Address::new_id(0).unwrap(); rt.value_return(addr.marshal_cbor().unwrap()) @@ -68,33 +68,26 @@ impl ActorCode for InitActorCode { &self, rt: &RT, method: MethodNum, - params: &MethodParams, + params: &Serialized, ) -> InvocOutput { // Create mutable copy of params for usage in functions - let params: &mut MethodParams = &mut params.clone(); + let params: &mut Serialized = &mut params.clone(); match InitMethod::from_method_num(method) { Some(InitMethod::Constructor) => { - // validate no arguments passed in - arg_end(params, rt); + // TODO unfinished spec Self::constructor(rt) } Some(InitMethod::Exec) => { // TODO deserialize CodeID on finished spec - let _ = arg_pop(params, rt); - check_args(params, rt, true); Self::exec(rt, CodeID::Init, params) } Some(InitMethod::GetActorIDForAddress) => { - // Pop and unmarshall address parameter - let addr_res = from_slice(&arg_pop(params, rt).bytes()); - - // validate addr deserialization and parameters - check_args(params, rt, addr_res.is_ok()); - arg_end(params, rt); + // Unmarshall address parameter + // TODO unfinished spec // Errors checked, get actor by address - Self::get_actor_id_for_address(rt, addr_res.unwrap()) + Self::get_actor_id_for_address(rt, Address::default()) } _ => { // Method number does not match available, abort in runtime diff --git a/vm/actor/src/builtin/reward.rs b/vm/actor/src/builtin/reward.rs index 5c22750f4704..6aa4c8b20b58 100644 --- a/vm/actor/src/builtin/reward.rs +++ b/vm/actor/src/builtin/reward.rs @@ -8,7 +8,7 @@ use num_traits::FromPrimitive; use runtime::{ActorCode, Runtime}; use std::collections::HashMap; use vm::{ - ExitCode, InvocOutput, MethodNum, MethodParams, SysCode, TokenAmount, METHOD_CONSTRUCTOR, + ExitCode, InvocOutput, MethodNum, Serialized, SysCode, TokenAmount, METHOD_CONSTRUCTOR, METHOD_PLACEHOLDER, }; @@ -70,7 +70,7 @@ impl ActorCode for RewardActorCode { &self, rt: &RT, method: MethodNum, - _params: &MethodParams, + _params: &Serialized, ) -> InvocOutput { match RewardMethod::from_method_num(method) { // TODO determine parameters for each method on finished spec diff --git a/vm/actor/src/builtin/storage_power.rs b/vm/actor/src/builtin/storage_power.rs index 74a92a35ce07..01feaad1dfba 100644 --- a/vm/actor/src/builtin/storage_power.rs +++ b/vm/actor/src/builtin/storage_power.rs @@ -4,8 +4,8 @@ use num_bigint::BigUint; use num_derive::FromPrimitive; use num_traits::FromPrimitive; -use runtime::{arg_end, ActorCode, Runtime}; -use vm::{ExitCode, InvocOutput, MethodNum, MethodParams, SysCode, METHOD_CONSTRUCTOR}; +use runtime::{ActorCode, Runtime}; +use vm::{ExitCode, InvocOutput, MethodNum, Serialized, SysCode, METHOD_CONSTRUCTOR}; /// State of storage power actor pub struct StoragePowerActorState { @@ -51,15 +51,12 @@ impl ActorCode for StoragePowerActorCode { &self, rt: &RT, method: MethodNum, - params: &MethodParams, + _params: &Serialized, ) -> InvocOutput { match StoragePowerMethod::from_method_num(method) { // TODO determine parameters for each method on finished spec Some(StoragePowerMethod::Constructor) => Self::constructor(rt), - Some(StoragePowerMethod::GetTotalStorage) => { - arg_end(params, rt); - Self::get_total_storage(rt) - } + Some(StoragePowerMethod::GetTotalStorage) => Self::get_total_storage(rt), _ => { rt.abort( ExitCode::SystemErrorCode(SysCode::InvalidMethod), diff --git a/vm/message/src/lib.rs b/vm/message/src/lib.rs index 0ac74a6728c6..5a51eec6477a 100644 --- a/vm/message/src/lib.rs +++ b/vm/message/src/lib.rs @@ -11,7 +11,7 @@ pub use unsigned_message::*; use address::Address; use num_bigint::BigUint; -use vm::{MethodNum, MethodParams, TokenAmount}; +use vm::{MethodNum, Serialized, TokenAmount}; pub trait Message { /// from returns the from address of the message @@ -25,7 +25,7 @@ pub trait Message { /// method_num returns the method number to be called fn method_num(&self) -> &MethodNum; /// params returns the encoded parameters for the method call - fn params(&self) -> &MethodParams; + fn params(&self) -> &Serialized; /// gas_price returns gas price for the message fn gas_price(&self) -> &BigUint; /// gas_limit returns the gas limit for the message diff --git a/vm/message/src/signed_message.rs b/vm/message/src/signed_message.rs index 52a69f56d902..d01d52f2636e 100644 --- a/vm/message/src/signed_message.rs +++ b/vm/message/src/signed_message.rs @@ -7,7 +7,7 @@ use crypto::{Error as CryptoError, Signature, Signer}; use encoding::Cbor; use num_bigint::BigUint; use serde::{Deserialize, Serialize}; -use vm::{MethodNum, MethodParams, TokenAmount}; +use vm::{MethodNum, Serialized, TokenAmount}; /// SignedMessage represents a wrapped message with signature bytes #[derive(PartialEq, Clone, Debug, Serialize, Deserialize)] @@ -60,7 +60,7 @@ impl Message for SignedMessage { self.message.method_num() } /// params returns the encoded parameters for the method call - fn params(&self) -> &MethodParams { + fn params(&self) -> &Serialized { self.message.params() } /// gas_price returns gas price for the message diff --git a/vm/message/src/unsigned_message.rs b/vm/message/src/unsigned_message.rs index d5b42807e2df..495980cfffde 100644 --- a/vm/message/src/unsigned_message.rs +++ b/vm/message/src/unsigned_message.rs @@ -3,7 +3,7 @@ use super::Message; use crate::TokenAmount; -use crate::{MethodNum, MethodParams}; +use crate::{MethodNum, Serialized}; use address::Address; use derive_builder::Builder; @@ -16,7 +16,7 @@ use serde::{Deserialize, Serialize}; /// Usage: /// ``` /// use message::{UnsignedMessage, Message}; -/// use vm::{TokenAmount, MethodParams, MethodNum}; +/// use vm::{TokenAmount, Serialized, MethodNum}; /// use num_bigint::BigUint; /// use address::Address; /// @@ -27,7 +27,7 @@ use serde::{Deserialize, Serialize}; /// .sequence(0) // optional /// .value(TokenAmount::new(0)) // optional /// .method_num(MethodNum::default()) // optional -/// .params(MethodParams::default()) // optional +/// .params(Serialized::default()) // optional /// .gas_limit(BigUint::default()) // optional /// .gas_price(BigUint::default()) // optional /// .build() @@ -53,7 +53,7 @@ pub struct UnsignedMessage { #[builder(default)] method_num: MethodNum, #[builder(default)] - params: MethodParams, + params: Serialized, #[builder(default)] gas_price: BigUint, #[builder(default)] @@ -91,7 +91,7 @@ impl Message for UnsignedMessage { &self.method_num } /// params returns the encoded parameters for the method call - fn params(&self) -> &MethodParams { + fn params(&self) -> &Serialized { &self.params } /// gas_price returns gas price for the message diff --git a/vm/message/tests/builder_test.rs b/vm/message/tests/builder_test.rs index 96f8e4b5fbae..e60bc29fff2c 100644 --- a/vm/message/tests/builder_test.rs +++ b/vm/message/tests/builder_test.rs @@ -6,7 +6,7 @@ use crypto::{Signature, Signer}; use message::{Message, SignedMessage, UnsignedMessage}; use num_bigint::BigUint; use std::error::Error; -use vm::{MethodNum, MethodParams, TokenAmount}; +use vm::{MethodNum, Serialized, TokenAmount}; const DUMMY_SIG: [u8; 1] = [0u8]; @@ -28,7 +28,7 @@ fn unsigned_message_builder() { .sequence(0) .value(TokenAmount::new(0)) .method_num(MethodNum::default()) - .params(MethodParams::default()) + .params(Serialized::default()) .gas_limit(BigUint::default()) .gas_price(BigUint::default()) .build() @@ -37,7 +37,7 @@ fn unsigned_message_builder() { assert_eq!(message.to(), &to_addr.clone()); assert_eq!(message.sequence(), 0); assert_eq!(message.method_num(), &MethodNum::default()); - assert_eq!(message.params(), &MethodParams::default()); + assert_eq!(message.params(), &Serialized::default()); assert_eq!(message.value(), &TokenAmount::new(0)); assert_eq!(message.gas_price(), &BigUint::default()); assert_eq!(message.gas_limit(), &BigUint::default()); diff --git a/vm/runtime/src/actor_code.rs b/vm/runtime/src/actor_code.rs index bbefc62ec296..5438e697afc2 100644 --- a/vm/runtime/src/actor_code.rs +++ b/vm/runtime/src/actor_code.rs @@ -2,7 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 use crate::Runtime; -use vm::{InvocOutput, MethodNum, MethodParams, Serialized}; +use vm::{InvocOutput, MethodNum, Serialized}; pub trait ActorCode { /// Invokes method with runtime on the actor's code @@ -10,22 +10,6 @@ pub trait ActorCode { &self, rt: &RT, method: MethodNum, - params: &MethodParams, + params: &Serialized, ) -> InvocOutput; } - -pub fn check_args(_params: &MethodParams, rt: &RT, cond: bool) { - if !cond { - rt.abort_arg(); - } - // TODO assume params validation on finished spec -} - -pub fn arg_pop(params: &mut MethodParams, rt: &RT) -> Serialized { - check_args(params, rt, !params.is_empty()); - params.remove(0) -} - -pub fn arg_end(params: &MethodParams, rt: &RT) { - check_args(params, rt, params.is_empty()) -} diff --git a/vm/src/invoc.rs b/vm/src/invoc.rs index a64c86a16282..7f721ad7db5d 100644 --- a/vm/src/invoc.rs +++ b/vm/src/invoc.rs @@ -1,7 +1,7 @@ // Copyright 2020 ChainSafe Systems // SPDX-License-Identifier: Apache-2.0 -use crate::{ExitCode, MethodNum, MethodParams, TokenAmount}; +use crate::{ExitCode, MethodNum, Serialized, TokenAmount}; use address::Address; @@ -9,7 +9,7 @@ use address::Address; pub struct InvocInput { pub to: Address, pub method: MethodNum, - pub params: MethodParams, + pub params: Serialized, pub value: TokenAmount, } diff --git a/vm/src/method.rs b/vm/src/method.rs index 7d5993a1bf29..50a2d09805ef 100644 --- a/vm/src/method.rs +++ b/vm/src/method.rs @@ -1,12 +1,12 @@ // Copyright 2020 ChainSafe Systems // SPDX-License-Identifier: Apache-2.0 -use encoding::{ser, to_vec, Error as EncodingError}; +use encoding::{de, ser, serde_bytes, to_vec, Error as EncodingError}; use serde::{Deserialize, Serialize}; -use std::ops::{Deref, DerefMut}; +use std::ops::Deref; /// Method number indicator for calling actor methods -#[derive(Default, Clone, PartialEq, Debug, Serialize, Deserialize)] +#[derive(Default, Clone, Copy, PartialEq, Debug, Serialize, Deserialize)] pub struct MethodNum(i32); // TODO: add constraints to this // TODO verify format or implement custom serialize/deserialize function (if necessary): @@ -36,14 +36,31 @@ pub const METHOD_CRON: isize = 2; // TODO revisit on complete spec pub const METHOD_PLACEHOLDER: isize = 3; -/// Serialized bytes to be used as individual parameters into actor methods -#[derive(Default, Clone, PartialEq, Debug, Serialize, Deserialize)] +/// Serialized bytes to be used as parameters into actor methods +#[derive(Default, Clone, PartialEq, Debug)] pub struct Serialized { bytes: Vec, } -// TODO verify format or implement custom serialize/deserialize function (if necessary): -// https://github.com/ChainSafe/ferret/issues/143 +impl ser::Serialize for Serialized { + fn serialize(&self, s: S) -> Result + where + S: ser::Serializer, + { + let value = serde_bytes::Bytes::new(&self.bytes); + serde_bytes::Serialize::serialize(value, s) + } +} + +impl<'de> de::Deserialize<'de> for Serialized { + fn deserialize(deserializer: D) -> Result + where + D: de::Deserializer<'de>, + { + let bz: Vec = serde_bytes::Deserialize::deserialize(deserializer)?; + Ok(Serialized::new(bz)) + } +} impl Deref for Serialized { type Target = Vec; @@ -70,25 +87,3 @@ impl Serialized { self.bytes.clone() } } - -/// Method parameters used in Actor execution -#[derive(Default, Clone, PartialEq, Debug, Serialize, Deserialize)] -pub struct MethodParams { - params: Vec, -} - -// TODO verify format or implement custom serialize/deserialize function (if necessary): -// https://github.com/ChainSafe/ferret/issues/143 - -impl Deref for MethodParams { - type Target = Vec; - fn deref(&self) -> &Self::Target { - &self.params - } -} - -impl DerefMut for MethodParams { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.params - } -} diff --git a/vm/tests/params_test.rs b/vm/tests/params_test.rs index af2ff7894ddd..ea024c4e2d6f 100644 --- a/vm/tests/params_test.rs +++ b/vm/tests/params_test.rs @@ -2,25 +2,41 @@ // SPDX-License-Identifier: Apache-2.0 use address::Address; -use encoding::from_slice; -use vm::{MethodNum, MethodParams, Serialized}; +use encoding::{from_slice, to_vec}; +use vm::{MethodNum, Serialized}; #[test] -fn params_usage() { - // Test to make sure a vector of bytes can be added and removed from params - let mut params = MethodParams::default(); - params.push(Serialized::new(vec![1, 2])); - assert_eq!(params.pop().unwrap().bytes(), vec![1, 2]); +fn serialized_deserialize() { + // Bytes taken from filecoin network encoded parameters from a message encoding + let valid_bytes: &[u8] = &[ + 88, 122, 129, 129, 137, 88, 32, 253, 47, 195, 200, 241, 49, 105, 17, 23, 102, 198, 44, 98, + 146, 98, 117, 43, 43, 228, 104, 245, 49, 207, 200, 140, 11, 71, 209, 172, 19, 198, 46, 27, + 0, 0, 0, 7, 240, 0, 0, 0, 88, 49, 3, 147, 181, 28, 1, 26, 25, 116, 166, 43, 56, 78, 42, + 134, 132, 42, 191, 56, 201, 240, 167, 211, 246, 121, 196, 206, 153, 40, 118, 180, 128, 107, + 133, 251, 45, 22, 108, 103, 132, 111, 147, 187, 97, 172, 132, 190, 26, 162, 166, 67, 0, + 135, 8, 27, 255, 255, 255, 255, 255, 255, 255, 255, 27, 127, 255, 255, 255, 255, 255, 255, + 255, 64, 64, 246, + ]; + let invalid_bytes: &[u8] = b"auefbfd7aNasdjfiA"; + + // Check deserialization of valid and invalid parameters + let des_params: Serialized = + from_slice(valid_bytes).expect("valid parameter bytes should be able to be deserialized"); + assert!(from_slice::(invalid_bytes).is_err()); + + // Check to make sure bytes can be serialized + let enc_params = to_vec(&des_params).unwrap(); + + // Assert symmetric serialization + assert_eq!(enc_params.as_slice(), valid_bytes); } #[test] fn cbor_params() { // Test cbor encodable objects can be added and removed from parameters - let mut params = MethodParams::default(); let addr = Address::new_id(1).unwrap(); - params.insert(0, Serialized::serialize(addr.clone()).unwrap()); - let encoded_addr = params.remove(0); - assert_eq!(from_slice::
(&encoded_addr).unwrap(), addr); + let params = Serialized::serialize(addr.clone()).unwrap(); + assert_eq!(from_slice::
(¶ms).unwrap(), addr); } #[test] From 780bedb03b406bfaac693e1053db185a5bf80f78 Mon Sep 17 00:00:00 2001 From: austinabell Date: Mon, 20 Jan 2020 10:36:31 -0500 Subject: [PATCH 2/2] remove now unneeded dev dependency --- vm/Cargo.toml | 3 --- 1 file changed, 3 deletions(-) diff --git a/vm/Cargo.toml b/vm/Cargo.toml index 0a9411b773ed..f4678d9de1fd 100644 --- a/vm/Cargo.toml +++ b/vm/Cargo.toml @@ -9,6 +9,3 @@ num-bigint = "0.2.3" address = {path = "./address"} encoding = {path = "../encoding"} serde = {version = "1.0", features = ["derive"]} - -[dev-dependencies] -hex = "0.4.0" \ No newline at end of file