From d5e74fca6b20a7bdda877f49d5712c5c053ffae9 Mon Sep 17 00:00:00 2001 From: Ahmed Sagdati <37515857+segfault-magnet@users.noreply.github.com> Date: Fri, 10 May 2024 19:28:44 +0200 Subject: [PATCH] bug: fix revert id (#1368) simulating now propagates the correct revert reason --- packages/fuels-accounts/Cargo.toml | 9 +- packages/fuels-accounts/src/provider.rs | 33 +- packages/fuels-core/Cargo.toml | 1 + packages/fuels-core/src/types/tx_status.rs | 27 ++ packages/fuels/tests/logs.rs | 336 ++++++++++++--------- 5 files changed, 240 insertions(+), 166 deletions(-) diff --git a/packages/fuels-accounts/Cargo.toml b/packages/fuels-accounts/Cargo.toml index ed37c9a8d0..d6ddadc58c 100644 --- a/packages/fuels-accounts/Cargo.toml +++ b/packages/fuels-accounts/Cargo.toml @@ -13,7 +13,7 @@ description = "Fuel Rust SDK accounts." async-trait = { workspace = true, default-features = false } chrono = { workspace = true } elliptic-curve = { workspace = true, default-features = false } -eth-keystore = { workspace = true, optional = true } +eth-keystore = { workspace = true, optional = true } fuel-core-client = { workspace = true, optional = true } fuel-core-types = { workspace = true } fuel-crypto = { workspace = true, features = ["random"] } @@ -34,4 +34,9 @@ tokio = { workspace = true, features = ["test-util"] } [features] default = ["std"] coin-cache = ["tokio?/time"] -std = ["fuels-core/std", "dep:tokio", "fuel-core-client/default", "dep:eth-keystore"] +std = [ + "fuels-core/std", + "dep:tokio", + "fuel-core-client/default", + "dep:eth-keystore", +] diff --git a/packages/fuels-accounts/src/provider.rs b/packages/fuels-accounts/src/provider.rs index c5bbcac218..761545e5e4 100644 --- a/packages/fuels-accounts/src/provider.rs +++ b/packages/fuels-accounts/src/provider.rs @@ -16,7 +16,6 @@ use fuel_core_client::client::{ gas_price::{EstimateGasPrice, LatestGasPrice}, }, }; -use fuel_core_types::services::executor::{TransactionExecutionResult, TransactionExecutionStatus}; use fuel_tx::{ AssetId, ConsensusParameters, Receipt, Transaction as FuelTransaction, TxId, UtxoId, }; @@ -256,12 +255,12 @@ impl Provider { } pub async fn dry_run(&self, tx: impl Transaction) -> Result { - let [(_, tx_status)] = self + let [tx_status] = self .client .dry_run(Transactions::new().insert(tx).as_slice()) .await? .into_iter() - .map(Self::tx_status_from_execution_status) + .map(Into::into) .collect::>() .try_into() .expect("should have only one element"); @@ -278,37 +277,17 @@ impl Provider { .dry_run(transactions.as_slice()) .await? .into_iter() - .map(Self::tx_status_from_execution_status) + .map(|execution_status| (execution_status.id, execution_status.into())) .collect()) } - fn tx_status_from_execution_status( - tx_execution_status: TransactionExecutionStatus, - ) -> (TxId, TxStatus) { - ( - tx_execution_status.id, - match tx_execution_status.result { - TransactionExecutionResult::Success { receipts, .. } => { - TxStatus::Success { receipts } - } - TransactionExecutionResult::Failed { - receipts, result, .. - } => TxStatus::Revert { - reason: TransactionExecutionResult::reason(&receipts, &result), - receipts, - revert_id: 0, - }, - }, - ) - } - pub async fn dry_run_no_validation(&self, tx: impl Transaction) -> Result { - let [(_, tx_status)] = self + let [tx_status] = self .client .dry_run_opt(Transactions::new().insert(tx).as_slice(), Some(false)) .await? .into_iter() - .map(Self::tx_status_from_execution_status) + .map(Into::into) .collect::>() .try_into() .expect("should have only one element"); @@ -325,7 +304,7 @@ impl Provider { .dry_run_opt(transactions.as_slice(), Some(false)) .await? .into_iter() - .map(Self::tx_status_from_execution_status) + .map(|execution_status| (execution_status.id, execution_status.into())) .collect()) } diff --git a/packages/fuels-core/Cargo.toml b/packages/fuels-core/Cargo.toml index 2460673dca..a86ee51dd9 100644 --- a/packages/fuels-core/Cargo.toml +++ b/packages/fuels-core/Cargo.toml @@ -20,6 +20,7 @@ fuel-core-client = { workspace = true, optional = true } fuel-crypto = { workspace = true } fuel-tx = { workspace = true } fuel-types = { workspace = true, features = ["default"] } +fuel-core-types = { workspace = true } fuel-vm = { workspace = true } fuels-macros = { workspace = true } hex = { workspace = true, features = ["std"] } diff --git a/packages/fuels-core/src/types/tx_status.rs b/packages/fuels-core/src/types/tx_status.rs index 472f62e007..7f90ac478d 100644 --- a/packages/fuels-core/src/types/tx_status.rs +++ b/packages/fuels-core/src/types/tx_status.rs @@ -4,6 +4,8 @@ use fuel_abi_types::error_codes::{ }; #[cfg(feature = "std")] use fuel_core_client::client::types::TransactionStatus as ClientTransactionStatus; +#[cfg(feature = "std")] +use fuel_core_types::services::executor::{TransactionExecutionResult, TransactionExecutionStatus}; use fuel_tx::Receipt; #[cfg(feature = "std")] use fuel_vm::state::ProgramState; @@ -118,3 +120,28 @@ impl From for TxStatus { } } } + +#[cfg(feature = "std")] +impl From for TxStatus { + fn from(value: TransactionExecutionStatus) -> Self { + match value.result { + TransactionExecutionResult::Success { receipts, .. } => Self::Success { receipts }, + TransactionExecutionResult::Failed { + result, receipts, .. + } => { + let revert_id = result + .and_then(|result| match result { + ProgramState::Revert(revert_id) => Some(revert_id), + _ => None, + }) + .expect("Transaction failed without a `revert_id`"); + let reason = TransactionExecutionResult::reason(&receipts, &result); + Self::Revert { + receipts, + reason, + revert_id, + } + } + } + } +} diff --git a/packages/fuels/tests/logs.rs b/packages/fuels/tests/logs.rs index f23cbfb12f..801e93e60b 100644 --- a/packages/fuels/tests/logs.rs +++ b/packages/fuels/tests/logs.rs @@ -446,43 +446,34 @@ async fn test_require_log() -> Result<()> { ), ); - let contract_methods = contract_instance.methods(); - { - let error = contract_methods - .require_primitive() - .call() - .await - .expect_err("should return a revert error"); - - assert_revert_containing_msg("42", error); + macro_rules! reverts_with_msg { + ($method:ident, $execution: ident, $msg:expr) => { + let error = contract_instance + .methods() + .$method() + .$execution() + .await + .expect_err("should return a revert error"); + + assert_revert_containing_msg($msg, error); + }; } - { - let error = contract_methods - .require_string() - .call() - .await - .expect_err("should return a revert error"); - assert_revert_containing_msg("fuel", error); - } - { - let error = contract_methods - .require_custom_generic() - .call() - .await - .expect_err("should return a revert error"); + reverts_with_msg!(require_primitive, call, "42"); + reverts_with_msg!(require_primitive, simulate, "42"); - assert_revert_containing_msg("StructDeeplyNestedGeneric", error); - } - { - let error = contract_methods - .require_with_additional_logs() - .call() - .await - .expect_err("should return a revert error"); + reverts_with_msg!(require_string, call, "fuel"); + reverts_with_msg!(require_string, simulate, "fuel"); - assert_revert_containing_msg("64", error); - } + reverts_with_msg!(require_custom_generic, call, "StructDeeplyNestedGeneric"); + reverts_with_msg!( + require_custom_generic, + simulate, + "StructDeeplyNestedGeneric" + ); + + reverts_with_msg!(require_with_additional_logs, call, "64"); + reverts_with_msg!(require_with_additional_logs, simulate, "64"); Ok(()) } @@ -516,6 +507,13 @@ async fn test_multi_call_require_log_single_contract() -> Result<()> { .add_call(call_handler_1) .add_call(call_handler_2); + let error = multi_call_handler + .simulate::<((), ())>() + .await + .expect_err("should return a revert error"); + + assert_revert_containing_msg("fuel", error); + let error = multi_call_handler .call::<((), ())>() .await @@ -533,6 +531,13 @@ async fn test_multi_call_require_log_single_contract() -> Result<()> { .add_call(call_handler_1) .add_call(call_handler_2); + let error = multi_call_handler + .simulate::<((), ())>() + .await + .expect_err("should return a revert error"); + + assert_revert_containing_msg("StructDeeplyNestedGeneric", error); + let error = multi_call_handler .call::<((), ())>() .await @@ -579,6 +584,13 @@ async fn test_multi_call_require_log_multi_contract() -> Result<()> { .add_call(call_handler_1) .add_call(call_handler_2); + let error = multi_call_handler + .simulate::<((), ())>() + .await + .expect_err("should return a revert error"); + + assert_revert_containing_msg("fuel", error); + let error = multi_call_handler .call::<((), ())>() .await @@ -596,6 +608,13 @@ async fn test_multi_call_require_log_multi_contract() -> Result<()> { .add_call(call_handler_1) .add_call(call_handler_2); + let error = multi_call_handler + .simulate::<((), ())>() + .await + .expect_err("should return a revert error"); + + assert_revert_containing_msg("StructDeeplyNestedGeneric", error); + let error = multi_call_handler .call::<((), ())>() .await @@ -897,42 +916,36 @@ async fn test_script_require_log() -> Result<()> { ) ); - { - let error = script_instance - .main(MatchEnum::RequirePrimitive) - .call() - .await - .expect_err("should return a revert error"); - - assert_revert_containing_msg("42", error); + macro_rules! reverts_with_msg { + ($arg:expr, $execution:ident, $msg:expr) => { + let error = script_instance + .main($arg) + .$execution() + .await + .expect_err("should return a revert error"); + assert_revert_containing_msg($msg, error); + }; } - { - let error = script_instance - .main(MatchEnum::RequireString) - .call() - .await - .expect_err("should return a revert error"); - assert_revert_containing_msg("fuel", error); - } - { - let error = script_instance - .main(MatchEnum::RequireCustomGeneric) - .call() - .await - .expect_err("should return a revert error"); + reverts_with_msg!(MatchEnum::RequirePrimitive, call, "42"); + reverts_with_msg!(MatchEnum::RequirePrimitive, simulate, "42"); - assert_revert_containing_msg("StructDeeplyNestedGeneric", error); - } - { - let error = script_instance - .main(MatchEnum::RequireWithAdditionalLogs) - .call() - .await - .expect_err("should return a revert error"); + reverts_with_msg!(MatchEnum::RequireString, call, "fuel"); + reverts_with_msg!(MatchEnum::RequireString, simulate, "fuel"); - assert_revert_containing_msg("64", error); - } + reverts_with_msg!( + MatchEnum::RequireCustomGeneric, + call, + "StructDeeplyNestedGeneric" + ); + reverts_with_msg!( + MatchEnum::RequireCustomGeneric, + simulate, + "StructDeeplyNestedGeneric" + ); + + reverts_with_msg!(MatchEnum::RequireWithAdditionalLogs, call, "64"); + reverts_with_msg!(MatchEnum::RequireWithAdditionalLogs, simulate, "64"); Ok(()) } @@ -1099,28 +1112,38 @@ async fn test_contract_asserts_log() -> Result<()> { wallet = "wallet" ), ); - let contract_methods = contract_instance.methods(); - { - let a = 32; - let b = 64; - let error = contract_methods - .assert_primitive(a, b) - .call() - .await - .expect_err("should return a revert error"); - assert_revert_containing_msg("assertion failed", error); + macro_rules! reverts_with_msg { + (($($arg: expr,)*), $method:ident, $execution: ident, $msg:expr) => { + let error = contract_instance + .methods() + .$method($($arg,)*) + .$execution() + .await + .expect_err("should return a revert error"); + assert_revert_containing_msg($msg, error); + }; } { - let a = 32; - let b = 64; + reverts_with_msg!((32, 64,), assert_primitive, call, "assertion failed"); + reverts_with_msg!((32, 64,), assert_primitive, simulate, "assertion failed"); + } - let error = contract_methods - .assert_eq_primitive(a, b) - .call() - .await - .expect_err("should return a revert error"); - assert_assert_eq_containing_msg(a, b, error); + macro_rules! reverts_with_assert_eq_msg { + (($($arg: expr,)*), $method:ident, $execution: ident, $msg:expr) => { + let error = contract_instance + .methods() + .$method($($arg,)*) + .call() + .await + .expect_err("should return a revert error"); + assert_assert_eq_containing_msg($($arg,)* error); + } + } + + { + reverts_with_assert_eq_msg!((32, 64,), assert_eq_primitive, call, "assertion failed"); + reverts_with_assert_eq_msg!((32, 64,), assert_eq_primitive, simulate, "assertion failed"); } { let test_struct = TestStruct { @@ -1133,24 +1156,36 @@ async fn test_contract_asserts_log() -> Result<()> { field_2: 32, }; - let error = contract_methods - .assert_eq_struct(test_struct.clone(), test_struct2.clone()) - .call() - .await - .expect_err("should return a revert error"); - assert_assert_eq_containing_msg(test_struct, test_struct2, error); + reverts_with_assert_eq_msg!( + (test_struct.clone(), test_struct2.clone(),), + assert_eq_struct, + call, + "assertion failed" + ); + + reverts_with_assert_eq_msg!( + (test_struct.clone(), test_struct2.clone(),), + assert_eq_struct, + simulate, + "assertion failed" + ); } { let test_enum = TestEnum::VariantOne; let test_enum2 = TestEnum::VariantTwo; + reverts_with_assert_eq_msg!( + (test_enum.clone(), test_enum2.clone(),), + assert_eq_enum, + call, + "assertion failed" + ); - let error = contract_methods - .assert_eq_enum(test_enum.clone(), test_enum2.clone()) - .call() - .await - .expect_err("should return a revert error"); - - assert_assert_eq_containing_msg(test_enum, test_enum2, error); + reverts_with_assert_eq_msg!( + (test_enum.clone(), test_enum2.clone(),), + assert_eq_enum, + simulate, + "assertion failed" + ); } Ok(()) @@ -1170,29 +1205,50 @@ async fn test_script_asserts_log() -> Result<()> { wallet = "wallet" ) ); - { - let a = 32; - let b = 64; - - let error = script_instance - .main(MatchEnum::AssertPrimitive((a, b))) - .call() - .await - .expect_err("should return a revert error"); + macro_rules! reverts_with_msg { + ($arg:expr, $execution:ident, $msg:expr) => { + let error = script_instance + .main($arg) + .$execution() + .await + .expect_err("should return a revert error"); + assert_revert_containing_msg($msg, error); + }; + } - assert_revert_containing_msg("assertion failed", error); + macro_rules! reverts_with_assert_eq_msg { + ($arg:expr, $execution:ident, $msg:expr) => { + let error = script_instance + .main($arg) + .$execution() + .await + .expect_err("should return a revert error"); + assert_revert_containing_msg($msg, error); + }; } { - let a = 32; - let b = 64; - - let error = script_instance - .main(MatchEnum::AssertEqPrimitive((a, b))) - .call() - .await - .expect_err("should return a revert error"); - - assert_assert_eq_containing_msg(a, b, error); + reverts_with_msg!( + MatchEnum::AssertPrimitive((32, 64)), + call, + "assertion failed" + ); + reverts_with_msg!( + MatchEnum::AssertPrimitive((32, 64)), + simulate, + "assertion failed" + ); + } + { + reverts_with_assert_eq_msg!( + MatchEnum::AssertEqPrimitive((32, 64)), + call, + "assertion failed" + ); + reverts_with_assert_eq_msg!( + MatchEnum::AssertEqPrimitive((32, 64)), + simulate, + "assertion failed" + ); } { let test_struct = TestStruct { @@ -1204,32 +1260,31 @@ async fn test_script_asserts_log() -> Result<()> { field_1: false, field_2: 32, }; - - let error = script_instance - .main(MatchEnum::AssertEqStruct(( - test_struct.clone(), - test_struct2.clone(), - ))) - .call() - .await - .expect_err("should return a revert error"); - - assert_assert_eq_containing_msg(test_struct, test_struct2, error); + reverts_with_assert_eq_msg!( + MatchEnum::AssertEqStruct((test_struct.clone(), test_struct2.clone(),)), + call, + "assertion failed" + ); + reverts_with_assert_eq_msg!( + MatchEnum::AssertEqStruct((test_struct.clone(), test_struct2.clone(),)), + simulate, + "assertion failed" + ); } { let test_enum = TestEnum::VariantOne; let test_enum2 = TestEnum::VariantTwo; - let error = script_instance - .main(MatchEnum::AssertEqEnum(( - test_enum.clone(), - test_enum2.clone(), - ))) - .call() - .await - .expect_err("should return a revert error"); - - assert_assert_eq_containing_msg(test_enum, test_enum2, error); + reverts_with_assert_eq_msg!( + MatchEnum::AssertEqEnum((test_enum.clone(), test_enum2.clone(),)), + call, + "assertion failed" + ); + reverts_with_assert_eq_msg!( + MatchEnum::AssertEqEnum((test_enum.clone(), test_enum2.clone(),)), + simulate, + "assertion failed" + ); } Ok(()) @@ -1256,6 +1311,13 @@ async fn contract_token_ops_error_messages() -> Result<()> { let asset_id = contract_id.asset_id(&Bits256::zeroed()); let address = wallet.address(); + let error = contract_methods + .transfer(1_000_000, asset_id, address.into()) + .simulate() + .await + .expect_err("should return a revert error"); + assert_revert_containing_msg("failed transfer to address", error); + let error = contract_methods .transfer(1_000_000, asset_id, address.into()) .call()