From 1acf4ec9997244eff92309398e743f70940766d6 Mon Sep 17 00:00:00 2001 From: Xun Li Date: Tue, 15 Mar 2022 11:47:03 -0700 Subject: [PATCH] Structured publish response (#830) --- sui/src/unit_tests/cli_tests.rs | 65 +++------ sui/src/wallet_commands.rs | 17 ++- sui_core/src/authority_server.rs | 2 +- sui_core/src/gateway_state.rs | 64 ++++++++- .../src/gateway_state/gateway_responses.rs | 37 +++++ sui_core/src/unit_tests/client_tests.rs | 136 ++++-------------- sui_core/tests/staged/sui.yaml | 10 +- sui_types/src/error.rs | 12 +- 8 files changed, 155 insertions(+), 188 deletions(-) diff --git a/sui/src/unit_tests/cli_tests.rs b/sui/src/unit_tests/cli_tests.rs index f011d9271cfdc..34dbbc94ea887 100644 --- a/sui/src/unit_tests/cli_tests.rs +++ b/sui/src/unit_tests/cli_tests.rs @@ -923,70 +923,41 @@ async fn test_package_publish_command() -> Result<(), anyhow::Error> { // Print it out to CLI/logs resp.print(true); - let dumy_obj = Object::with_id_owner_for_testing(ObjectID::random(), address); - // Get the created objects - let (mut created_obj1, mut created_obj2) = ( - dumy_obj.compute_object_reference(), - dumy_obj.compute_object_reference(), - ); - - if let WalletCommandResult::Publish( - _, - TransactionEffects { - created: new_objs, .. - }, - ) = resp - { - (created_obj1, created_obj2) = (new_objs.get(0).unwrap().0, new_objs.get(1).unwrap().0); + let (package, created_obj) = if let WalletCommandResult::Publish(resppnse) = resp { + ( + resppnse.package, + resppnse.created_objects[0].compute_object_reference(), + ) } else { - // Fail this way because Panic! causes test issues - assert!(false); + unreachable!("Invaldi response"); }; // One is the actual module, while the other is the object created at init retry_assert!( - logs_contain(&format!("{:02X}", created_obj1.0)), + logs_contain(&format!("{}", package.0)), Duration::from_millis(5000) ); retry_assert!( - logs_contain(&format!("{:02X}", created_obj2.0)), + logs_contain(&format!("{}", created_obj.0)), Duration::from_millis(5000) ); // Check the objects - // Init with some value to satisfy the type checker - let mut cr_obj1 = dumy_obj.clone(); - - let resp = WalletCommands::Object { id: created_obj1.0 } + let resp = WalletCommands::Object { id: package.0 } .execute(&mut context) .await?; - if let WalletCommandResult::Object(ObjectRead::Exists(_, object, _)) = resp { - cr_obj1 = object; - } else { - // Fail this way because Panic! causes test issues - assert!(false) - }; + assert!(matches!( + resp, + WalletCommandResult::Object(ObjectRead::Exists(..)) + )); - let mut cr_obj2 = dumy_obj; - - let resp = WalletCommands::Object { id: created_obj2.0 } + let resp = WalletCommands::Object { id: created_obj.0 } .execute(&mut context) .await?; - if let WalletCommandResult::Object(ObjectRead::Exists(_, object, _)) = resp { - cr_obj2 = object; - } else { - // Fail this way because Panic! causes test issues - assert!(false) - }; - - let (pkg, obj) = if cr_obj1.is_package() { - (cr_obj1, cr_obj2) - } else { - (cr_obj2, cr_obj1) - }; - - assert!(pkg.is_package()); - assert!(!obj.is_package()); + assert!(matches!( + resp, + WalletCommandResult::Object(ObjectRead::Exists(..)) + )); network.abort(); Ok(()) diff --git a/sui/src/wallet_commands.rs b/sui/src/wallet_commands.rs index b2f1f4d6370b6..be544624097c8 100644 --- a/sui/src/wallet_commands.rs +++ b/sui/src/wallet_commands.rs @@ -20,7 +20,9 @@ use structopt::clap::AppSettings; use structopt::StructOpt; use tracing::info; -use sui_core::gateway_state::gateway_responses::{MergeCoinResponse, SplitCoinResponse}; +use sui_core::gateway_state::gateway_responses::{ + MergeCoinResponse, PublishResponse, SplitCoinResponse, +}; use sui_core::gateway_state::{AsyncTransactionSigner, GatewayClient}; use sui_framework::build_move_package_to_bytes; use sui_types::base_types::{decode_bytes_hex, ObjectID, ObjectRef, SuiAddress}; @@ -222,7 +224,7 @@ impl WalletCommands { let gas_obj_ref = gas_object.compute_object_reference(); let compiled_modules = build_move_package_to_bytes(Path::new(path))?; - let (cert, effects) = context + let response = context .gateway .publish( sender, @@ -233,10 +235,7 @@ impl WalletCommands { ) .await?; - if matches!(effects.status, ExecutionStatus::Failure { .. }) { - return Err(anyhow!("Error publishing module: {:#?}", effects.status)); - }; - WalletCommandResult::Publish(cert, effects) + WalletCommandResult::Publish(response) } WalletCommands::Object { id } => { @@ -459,8 +458,8 @@ impl Display for WalletCommandResult { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { let mut writer = String::new(); match self { - WalletCommandResult::Publish(cert, effects) => { - write!(writer, "{}", write_cert_and_effects(cert, effects)?)?; + WalletCommandResult::Publish(response) => { + write!(writer, "{}", response)?; } WalletCommandResult::Object(object_read) => { let object = object_read.object().map_err(fmt::Error::custom)?; @@ -569,7 +568,7 @@ impl WalletCommandResult { #[derive(Serialize)] #[serde(untagged)] pub enum WalletCommandResult { - Publish(CertifiedTransaction, TransactionEffects), + Publish(PublishResponse), Object(ObjectRead), Call(CertifiedTransaction, TransactionEffects), Transfer( diff --git a/sui_core/src/authority_server.rs b/sui_core/src/authority_server.rs index 2cf46a302b25d..95bf32e62c8fe 100644 --- a/sui_core/src/authority_server.rs +++ b/sui_core/src/authority_server.rs @@ -149,7 +149,7 @@ impl AuthorityServer { Err(RecvError::Lagged(number_skipped)) => { // We tell the client they are too slow to consume, and // stop. - return Err(SuiError::SubscriptionItemsDropedError(number_skipped)); + return Err(SuiError::SubscriptionItemsDroppedError(number_skipped)); } } } diff --git a/sui_core/src/gateway_state.rs b/sui_core/src/gateway_state.rs index f15df15191e36..ff269ad6ce2e0 100644 --- a/sui_core/src/gateway_state.rs +++ b/sui_core/src/gateway_state.rs @@ -32,7 +32,7 @@ use std::{ pin::Pin, }; -use self::gateway_responses::{MergeCoinResponse, SplitCoinResponse}; +use self::gateway_responses::*; /// A trait for supplying transaction signature asynchronously. /// @@ -191,7 +191,7 @@ pub trait GatewayAPI { gas_object_ref: ObjectRef, gas_budget: u64, tx_signer: StableSyncTransactionSigner, - ) -> Result<(CertifiedTransaction, TransactionEffects), anyhow::Error>; + ) -> Result; /// Split the coin object (identified by `coin_object_ref`) into /// multiple new coins. The amount of each new coin is specified in @@ -731,12 +731,56 @@ where gas_object_ref: ObjectRef, gas_budget: u64, tx_signer: StableSyncTransactionSigner, - ) -> Result<(CertifiedTransaction, TransactionEffects), anyhow::Error> { + ) -> Result { let data = TransactionData::new_module(signer, gas_object_ref, package_bytes, gas_budget); let signature = tx_signer.sign(&signer, data.clone()).await?; - self.execute_transaction(Transaction::new(data, signature)) - .await + let (certificate, effects) = self + .execute_transaction(Transaction::new(data, signature)) + .await?; + if let ExecutionStatus::Failure { gas_used: _, error } = effects.status { + return Err(error.into()); + } + fp_ensure!( + effects.mutated.len() == 1, + SuiError::InconsistentGatewayResult { + error: format!( + "Expecting only one object mutated (the gas), seeing {} mutated", + effects.mutated.len() + ), + } + .into() + ); + let updated_gas = self + .get_object_info(gas_object_ref.0) + .await? + .into_object()?; + let mut package = None; + let mut created_objects = vec![]; + for (obj_ref, _) in effects.created { + let object = self.get_object_info(obj_ref.0).await?.into_object()?; + if object.is_package() { + fp_ensure!( + package.is_none(), + SuiError::InconsistentGatewayResult { + error: "More than one package created".to_owned(), + } + .into() + ); + package = Some(obj_ref); + } else { + created_objects.push(object); + } + } + let package = package.ok_or(SuiError::InconsistentGatewayResult { + error: "No package created".to_owned(), + })?; + Ok(PublishResponse { + certificate, + package, + created_objects, + updated_gas, + }) } async fn split_coin( @@ -783,7 +827,10 @@ where effects.mutated.len() == 2 // coin and gas && created.len() == split_amounts.len() && created.iter().all(|(_, owner)| owner == &signer), - SuiError::IncorrectGasSplit.into() + SuiError::InconsistentGatewayResult { + error: "Unexpected split outcome".to_owned() + } + .into() ); let updated_coin = self .get_object_info(coin_object_ref.0) @@ -843,7 +890,10 @@ where } fp_ensure!( effects.mutated.len() == 2, // coin and gas - SuiError::IncorrectGasMerge.into() + SuiError::InconsistentGatewayResult { + error: "Unexpected split outcome".to_owned() + } + .into() ); let updated_coin = self.get_object_info(primary_coin.0).await?.into_object()?; let updated_gas = self.get_object_info(gas_payment.0).await?.into_object()?; diff --git a/sui_core/src/gateway_state/gateway_responses.rs b/sui_core/src/gateway_state/gateway_responses.rs index 961283c0a73ef..0e91460fe0bf2 100644 --- a/sui_core/src/gateway_state/gateway_responses.rs +++ b/sui_core/src/gateway_state/gateway_responses.rs @@ -6,6 +6,7 @@ use serde::Serialize; use std::fmt; use std::fmt::Write; use std::fmt::{Display, Formatter}; +use sui_types::base_types::ObjectRef; use sui_types::gas_coin::GasCoin; use sui_types::messages::CertifiedTransaction; use sui_types::object::Object; @@ -71,3 +72,39 @@ impl Display for MergeCoinResponse { write!(f, "{}", writer) } } + +#[derive(Serialize)] +pub struct PublishResponse { + /// Certificate of the transaction + pub certificate: CertifiedTransaction, + /// The newly published package object reference. + pub package: ObjectRef, + /// List of Move objects created as part of running the module initializers in the package + pub created_objects: Vec, + /// The updated gas payment object after deducting payment + pub updated_gas: Object, +} + +impl Display for PublishResponse { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + let mut writer = String::new(); + writeln!(writer, "----- Certificate ----")?; + write!(writer, "{}", self.certificate)?; + writeln!(writer, "----- Publish Results ----")?; + writeln!( + writer, + "The newly published package object: {:?}", + self.package + )?; + writeln!( + writer, + "List of objects created by running module initializers:" + )?; + for obj in &self.created_objects { + writeln!(writer, "{}", obj)?; + } + let gas_coin = GasCoin::try_from(&self.updated_gas).map_err(fmt::Error::custom)?; + writeln!(writer, "Updated Gas : {}", gas_coin)?; + write!(f, "{}", writer) + } +} diff --git a/sui_core/src/unit_tests/client_tests.rs b/sui_core/src/unit_tests/client_tests.rs index c226c0d423c71..0e1865ebbafe9 100644 --- a/sui_core/src/unit_tests/client_tests.rs +++ b/sui_core/src/unit_tests/client_tests.rs @@ -22,7 +22,7 @@ use sui_framework::build_move_package_to_bytes; use sui_types::crypto::Signature; use sui_types::crypto::{get_key_pair, KeyPair}; use sui_types::gas_coin::GasCoin; -use sui_types::object::{Data, Object, Owner, GAS_VALUE_FOR_TESTING, OBJECT_START_VERSION}; +use sui_types::object::{Data, Object, Owner, GAS_VALUE_FOR_TESTING}; use typed_store::Map; use signature::{Error, Signer}; @@ -1141,22 +1141,6 @@ async fn test_move_calls_object_delete() { } } -async fn get_package_obj( - client: &mut GatewayState, - objects: &[(ObjectRef, Owner)], - gas_object_ref: &ObjectRef, -) -> Option { - let mut pkg_obj_opt = None; - for (new_obj_ref, _) in objects { - assert_ne!(gas_object_ref, new_obj_ref); - let new_obj = client.get_object_info(new_obj_ref.0).await.unwrap(); - if let Data::Package(_) = new_obj.object().unwrap().data { - pkg_obj_opt = Some(new_obj); - } - } - pkg_obj_opt -} - #[tokio::test] async fn test_module_publish_and_call_good() { // Init the states @@ -1180,7 +1164,7 @@ async fn test_module_publish_and_call_good() { hero_path.push_str("/src/unit_tests/data/hero/"); let compiled_modules = build_move_package_to_bytes(Path::new(&hero_path)).unwrap(); - let pub_res = client + let response = client .publish( addr1, compiled_modules, @@ -1188,48 +1172,21 @@ async fn test_module_publish_and_call_good() { GAS_VALUE_FOR_TESTING / 2, signature_callback(&key1), ) - .await; - - let (_, published_effects) = pub_res.unwrap(); - - assert!(matches!( - published_effects.status, - ExecutionStatus::Success { .. } - )); - - // A package obj and two objects resulting from two - // initializer runs in different modules should be created. - assert_eq!(published_effects.created.len(), 3); - - // Verify gas obj - assert_eq!(published_effects.gas_object.0 .0, gas_object_ref.0); - - for (new_obj_ref, _) in &published_effects.created { - assert_ne!(gas_object_ref, *new_obj_ref); - } - - // find the package object and inspect it - - let new_obj = get_package_obj(&mut client, &published_effects.created, &gas_object_ref) .await .unwrap(); - // Version should be 1 for all modules - assert_eq!(new_obj.object().unwrap().version(), OBJECT_START_VERSION); - // Must be immutable - assert!(new_obj.object().unwrap().is_read_only()); + // Two objects resulting from two initializer runs in different modules should be created. + assert_eq!(response.created_objects.len(), 2); - // StructTag type is not defined for package - assert!(new_obj.object().unwrap().type_().is_none()); + // Verify gas obj + assert_eq!(response.updated_gas.id(), gas_object_ref.0); - // Data should be castable as a package - assert!(new_obj.object().unwrap().data.try_as_package().is_some()); + let package = response.package; // This gets the treasury cap for the coin and gives it to the sender let mut tres_cap_opt = None; - for (new_obj_ref, _) in &published_effects.created { - let new_obj = client.get_object_info(new_obj_ref.0).await.unwrap(); - if let Data::Move(move_obj) = &new_obj.object().unwrap().data { + for new_obj in &response.created_objects { + if let Data::Move(move_obj) = &new_obj.data { if move_obj.type_.module == Identifier::new("Coin").unwrap() && move_obj.type_.name == Identifier::new("TreasuryCap").unwrap() { @@ -1244,21 +1201,18 @@ async fn test_module_publish_and_call_good() { let (gas_object_ref, gas_object) = client_object(&mut client, gas_object_id).await; // Confirm we own this object - assert_eq!(tres_cap_obj_info.object().unwrap().owner, gas_object.owner); + assert_eq!(tres_cap_obj_info.owner, gas_object.owner); //Try to call a function in TrustedCoin module let call_resp = client .move_call( addr1, - new_obj.object().unwrap().compute_object_reference(), + package, ident_str!("TrustedCoin").to_owned(), ident_str!("mint").to_owned(), vec![], gas_object_ref, - vec![tres_cap_obj_info - .object() - .unwrap() - .compute_object_reference()], + vec![tres_cap_obj_info.compute_object_reference()], vec![], vec![42u64.to_le_bytes().to_vec()], 1000, @@ -1311,7 +1265,7 @@ async fn test_module_publish_file_path() { hero_path.push_str("/src/unit_tests/data/hero/Hero.move"); let compiled_modules = build_move_package_to_bytes(Path::new(&hero_path)).unwrap(); - let pub_resp = client + let response = client .publish( addr1, compiled_modules, @@ -1319,45 +1273,19 @@ async fn test_module_publish_file_path() { GAS_VALUE_FOR_TESTING / 2, signature_callback(&key1), ) - .await; - - let (_, published_effects) = pub_resp.unwrap(); - - assert!(matches!( - published_effects.status, - ExecutionStatus::Success { .. } - )); + .await + .unwrap(); // Even though we provided a path to Hero.move, the builder is // able to find the package root build all in the package, // including TrustedCoin module // - // Consequently,a package obj and two objects resulting from two + // Consequently, two objects resulting from two // initializer runs in different modules should be created. - assert_eq!(published_effects.created.len(), 3); + assert_eq!(response.created_objects.len(), 2); // Verify gas - assert_eq!(published_effects.gas_object.0 .0, gas_object_ref.0); - - for (new_obj_ref, _) in &published_effects.created { - assert_ne!(gas_object_ref, *new_obj_ref); - } - // find the package object and inspect it - - let new_obj = get_package_obj(&mut client, &published_effects.created, &gas_object_ref) - .await - .unwrap(); - - // Version should be 1 for all modules - assert_eq!(new_obj.object().unwrap().version(), OBJECT_START_VERSION); - // Must be immutable - assert!(new_obj.object().unwrap().is_read_only()); - - // StructTag type is not defined for package - assert!(new_obj.object().unwrap().type_().is_none()); - - // Data should be castable as a package - assert!(new_obj.object().unwrap().data.try_as_package().is_some()); + assert_eq!(response.updated_gas.id(), gas_object_ref.0); } #[tokio::test] @@ -1570,7 +1498,7 @@ async fn test_object_store() { hero_path.push_str("/src/unit_tests/data/hero/"); let compiled_modules = build_move_package_to_bytes(Path::new(&hero_path)).unwrap(); - let pub_res = client + let response = client .publish( addr1, compiled_modules, @@ -1578,31 +1506,15 @@ async fn test_object_store() { GAS_VALUE_FOR_TESTING / 2, signature_callback(&key1), ) - .await; - - let (_, published_effects) = pub_res.as_ref().unwrap(); - - assert!(matches!( - published_effects.status, - ExecutionStatus::Success { .. } - )); + .await + .unwrap(); - // A package obj and two objects resulting from two + // Two objects resulting from two // initializer runs in different modules should be created. - assert_eq!(published_effects.created.len(), 3); + assert_eq!(response.created_objects.len(), 2); // Verify gas obj - assert_eq!(published_effects.gas_object.0 .0, gas_object_ref.0); - - for (new_obj_ref, _) in &published_effects.created { - assert_ne!(gas_object_ref, *new_obj_ref); - } - - // find the package object and inspect it - - let _new_obj = get_package_obj(&mut client, &published_effects.created, &gas_object_ref) - .await - .unwrap(); + assert_eq!(response.updated_gas.id(), gas_object_ref.0); // New gas object should be in storage, so 1 new items, plus 3 from before // The published package is not in the store because it's not owned by anyone. diff --git a/sui_core/tests/staged/sui.yaml b/sui_core/tests/staged/sui.yaml index 3599e82dd49d5..e3eb279f091e7 100644 --- a/sui_core/tests/staged/sui.yaml +++ b/sui_core/tests/staged/sui.yaml @@ -418,7 +418,7 @@ SuiError: TYPENAME: ObjectID - err: STR 23: - MissingEalierConfirmations: + MissingEarlierConfirmations: STRUCT: - object_id: TYPENAME: ObjectID @@ -487,7 +487,7 @@ SuiError: 46: CannotSendClientMessageError: UNIT 47: - SubscriptionItemsDropedError: + SubscriptionItemsDroppedError: NEWTYPE: U64 48: SubscriptionServiceClosed: UNIT @@ -630,9 +630,9 @@ SuiError: 86: TooManyIncorrectAuthorities: UNIT 87: - IncorrectGasSplit: UNIT - 88: - IncorrectGasMerge: UNIT + InconsistentGatewayResult: + STRUCT: + - error: STR Transaction: STRUCT: - data: diff --git a/sui_types/src/error.rs b/sui_types/src/error.rs index 6f5f847d16b69..873e7fab17bcd 100644 --- a/sui_types/src/error.rs +++ b/sui_types/src/error.rs @@ -89,7 +89,7 @@ pub enum SuiError { #[error("Object fetch failed for {object_id:?}, err {err:?}.")] ObjectFetchFailed { object_id: ObjectID, err: String }, #[error("Object {object_id:?} at old version: {current_sequence_number:?}")] - MissingEalierConfirmations { + MissingEarlierConfirmations { object_id: ObjectID, current_sequence_number: VersionNumber, }, @@ -149,12 +149,12 @@ pub enum SuiError { TooManyItemsError(u64), #[error("The range specified is invalid.")] InvalidSequenceRangeError, - #[error("No batches mached the range requested.")] + #[error("No batches matched the range requested.")] NoBatchesFoundError, #[error("The channel to repond to the client returned an error.")] CannotSendClientMessageError, #[error("Subscription service had to drop {0} items")] - SubscriptionItemsDropedError(u64), + SubscriptionItemsDroppedError(u64), #[error("Subscription service closed.")] SubscriptionServiceClosed, @@ -258,10 +258,8 @@ pub enum SuiError { IncorrectRecipientError, #[error("Too many authority errors were detected.")] TooManyIncorrectAuthorities, - #[error("Inconsistent gas coin split result.")] - IncorrectGasSplit, - #[error("Inconsistent gas coin merge result.")] - IncorrectGasMerge, + #[error("Inconsistent results observed in the Gateway. This should not happen and typically means there is a bug in the Sui implementation. Details: {error:?}")] + InconsistentGatewayResult { error: String }, } pub type SuiResult = Result;