From a196261f49d4828cb74c70724217ce8d2ce96817 Mon Sep 17 00:00:00 2001 From: Rodrigo Quelhas <22591718+RomarQ@users.noreply.github.com> Date: Mon, 17 Feb 2025 11:35:32 +0000 Subject: [PATCH] Fix foreign assets migration (#3190) * fix: foreign assets migration * fix: warnings * remove used method * update hard-coded gas limits to a reasonable value * fix test * improve code --- pallets/moonbeam-foreign-assets/src/evm.rs | 132 ++++++++++++++---- pallets/moonbeam-foreign-assets/src/lib.rs | 1 - .../src/benchmarks.rs | 2 + .../src/foreign_asset.rs | 46 ++++-- pallets/moonbeam-lazy-migrations/src/lib.rs | 1 + .../test-mock-hrmp-transact-ethereum-12.ts | 18 +-- 6 files changed, 144 insertions(+), 56 deletions(-) diff --git a/pallets/moonbeam-foreign-assets/src/evm.rs b/pallets/moonbeam-foreign-assets/src/evm.rs index 391e6f7df6..a4d2fd1185 100644 --- a/pallets/moonbeam-foreign-assets/src/evm.rs +++ b/pallets/moonbeam-foreign-assets/src/evm.rs @@ -25,7 +25,7 @@ use precompile_utils::solidity::codec::{Address, BoundedString}; use precompile_utils::solidity::Codec; use precompile_utils_macro::keccak256; use sp_runtime::traits::ConstU32; -use sp_runtime::DispatchError; +use sp_runtime::{format, DispatchError, SaturatedConversion}; use sp_std::vec::Vec; use xcm::latest::Error as XcmError; @@ -33,21 +33,22 @@ const ERC20_CALL_MAX_CALLDATA_SIZE: usize = 4 + 32 + 32; // selector + address + const ERC20_CREATE_MAX_CALLDATA_SIZE: usize = 16 * 1024; // 16Ko // Hardcoded gas limits (from manual binary search) -const ERC20_CREATE_GAS_LIMIT: u64 = 3_410_000; // highest failure: 3_406_000 -pub(crate) const ERC20_BURN_FROM_GAS_LIMIT: u64 = 155_000; // highest failure: 154_000 -pub(crate) const ERC20_MINT_INTO_GAS_LIMIT: u64 = 155_000; // highest failure: 154_000 -const ERC20_PAUSE_GAS_LIMIT: u64 = 150_500; // highest failure: 150_500 -pub(crate) const ERC20_TRANSFER_GAS_LIMIT: u64 = 155_000; // highest failure: 154_000 -pub(crate) const ERC20_APPROVE_GAS_LIMIT: u64 = 154_000; // highest failure: 153_000 -const ERC20_UNPAUSE_GAS_LIMIT: u64 = 151_000; // highest failure: 149_500 - +const ERC20_CREATE_GAS_LIMIT: u64 = 3_600_000; // highest failure: 3_600_000 +pub(crate) const ERC20_BURN_FROM_GAS_LIMIT: u64 = 160_000; // highest failure: 154_000 +pub(crate) const ERC20_MINT_INTO_GAS_LIMIT: u64 = 160_000; // highest failure: 154_000 +const ERC20_PAUSE_GAS_LIMIT: u64 = 160_000; // highest failure: 150_500 +pub(crate) const ERC20_TRANSFER_GAS_LIMIT: u64 = 160_000; // highest failure: 154_000 +pub(crate) const ERC20_APPROVE_GAS_LIMIT: u64 = 160_000; // highest failure: 153_000 +const ERC20_UNPAUSE_GAS_LIMIT: u64 = 160_000; // highest failure: 149_500 + +#[derive(Debug)] pub enum EvmError { - BurnFromFail, + BurnFromFail(String), ContractReturnInvalidValue, DispatchError(DispatchError), - EvmCallFail, - MintIntoFail, - TransferFail, + EvmCallFail(String), + MintIntoFail(String), + TransferFail(String), } impl From for EvmError { @@ -59,7 +60,8 @@ impl From for EvmError { impl From for XcmError { fn from(error: EvmError) -> XcmError { match error { - EvmError::BurnFromFail => { + EvmError::BurnFromFail(err) => { + log::debug!("BurnFromFail error: {:?}", err); XcmError::FailedToTransactAsset("Erc20 contract call burnFrom fail") } EvmError::ContractReturnInvalidValue => { @@ -69,11 +71,16 @@ impl From for XcmError { log::debug!("dispatch error: {:?}", err); Self::FailedToTransactAsset("storage layer error") } - EvmError::EvmCallFail => XcmError::FailedToTransactAsset("Fail to call erc20 contract"), - EvmError::MintIntoFail => { - XcmError::FailedToTransactAsset("Erc20 contract call mintInto fail") + EvmError::EvmCallFail(err) => { + log::debug!("EvmCallFail error: {:?}", err); + XcmError::FailedToTransactAsset("Fail to call erc20 contract") + } + EvmError::MintIntoFail(err) => { + log::debug!("MintIntoFail error: {:?}", err); + XcmError::FailedToTransactAsset("Erc20 contract call mintInto fail+") } - EvmError::TransferFail => { + EvmError::TransferFail(err) => { + log::debug!("TransferFail error: {:?}", err); XcmError::FailedToTransactAsset("Erc20 contract call transfer fail") } } @@ -134,7 +141,10 @@ impl EvmCaller { &::config(), contract_adress, ) - .map_err(|_| Error::Erc20ContractCreationFail)?; + .map_err(|err| { + log::debug!("erc20_create (error): {:?}", err.error.into()); + Error::::Erc20ContractCreationFail + })?; ensure!( matches!( @@ -179,14 +189,17 @@ impl EvmCaller { Some(0), &::config(), ) - .map_err(|_| EvmError::EvmCallFail)?; + .map_err(|err| EvmError::MintIntoFail(format!("{:?}", err.error.into())))?; ensure!( matches!( exec_info.exit_reason, ExitReason::Succeed(ExitSucceed::Returned | ExitSucceed::Stopped) ), - EvmError::MintIntoFail + { + let err = error_on_execution_failure(&exec_info.exit_reason, &exec_info.value); + EvmError::MintIntoFail(err) + } ); Ok(()) @@ -225,14 +238,17 @@ impl EvmCaller { Some(0), &::config(), ) - .map_err(|_| EvmError::EvmCallFail)?; + .map_err(|err| EvmError::TransferFail(format!("{:?}", err.error.into())))?; ensure!( matches!( exec_info.exit_reason, ExitReason::Succeed(ExitSucceed::Returned | ExitSucceed::Stopped) ), - EvmError::TransferFail + { + let err = error_on_execution_failure(&exec_info.exit_reason, &exec_info.value); + EvmError::TransferFail(err) + } ); // return value is true. @@ -280,14 +296,17 @@ impl EvmCaller { Some(0), &::config(), ) - .map_err(|_| EvmError::EvmCallFail)?; + .map_err(|err| EvmError::EvmCallFail(format!("{:?}", err.error.into())))?; ensure!( matches!( exec_info.exit_reason, ExitReason::Succeed(ExitSucceed::Returned | ExitSucceed::Stopped) ), - EvmError::EvmCallFail + { + let err = error_on_execution_failure(&exec_info.exit_reason, &exec_info.value); + EvmError::EvmCallFail(err) + } ); Ok(()) @@ -325,14 +344,17 @@ impl EvmCaller { Some(0), &::config(), ) - .map_err(|_| EvmError::EvmCallFail)?; + .map_err(|err| EvmError::EvmCallFail(format!("{:?}", err.error.into())))?; ensure!( matches!( exec_info.exit_reason, ExitReason::Succeed(ExitSucceed::Returned | ExitSucceed::Stopped) ), - EvmError::BurnFromFail + { + let err = error_on_execution_failure(&exec_info.exit_reason, &exec_info.value); + EvmError::BurnFromFail(err) + } ); Ok(()) @@ -362,14 +384,21 @@ impl EvmCaller { Some(0), &::config(), ) - .map_err(|_| Error::::EvmInternalError)?; + .map_err(|err| { + log::debug!("erc20_pause (error): {:?}", err.error.into()); + Error::::EvmInternalError + })?; ensure!( matches!( exec_info.exit_reason, ExitReason::Succeed(ExitSucceed::Returned | ExitSucceed::Stopped) ), - Error::::EvmCallPauseFail + { + let err = error_on_execution_failure(&exec_info.exit_reason, &exec_info.value); + log::debug!("erc20_pause (error): {:?}", err); + Error::::EvmCallPauseFail + } ); Ok(()) @@ -400,16 +429,57 @@ impl EvmCaller { Some(0), &::config(), ) - .map_err(|_| Error::::EvmInternalError)?; + .map_err(|err| { + log::debug!("erc20_unpause (error): {:?}", err.error.into()); + Error::::EvmInternalError + })?; ensure!( matches!( exec_info.exit_reason, ExitReason::Succeed(ExitSucceed::Returned | ExitSucceed::Stopped) ), - Error::::EvmCallUnpauseFail + { + let err = error_on_execution_failure(&exec_info.exit_reason, &exec_info.value); + log::debug!("erc20_unpause (error): {:?}", err); + Error::::EvmCallUnpauseFail + } ); Ok(()) } } + +fn error_on_execution_failure(reason: &ExitReason, data: &[u8]) -> String { + match reason { + ExitReason::Succeed(_) => String::new(), + ExitReason::Error(err) => format!("evm error: {err:?}"), + ExitReason::Fatal(err) => format!("evm fatal: {err:?}"), + ExitReason::Revert(_) => extract_revert_message(data), + } +} + +/// The data should contain a UTF-8 encoded revert reason with a minimum size consisting of: +/// error function selector (4 bytes) + offset (32 bytes) + reason string length (32 bytes) +fn extract_revert_message(data: &[u8]) -> String { + const LEN_START: usize = 36; + const MESSAGE_START: usize = 68; + const BASE_MESSAGE: &str = "VM Exception while processing transaction: revert"; + // Return base message if data is too short + if data.len() <= MESSAGE_START { + return BASE_MESSAGE.into(); + } + // Extract message length and calculate end position + let message_len = U256::from(&data[LEN_START..MESSAGE_START]).saturated_into::(); + let message_end = MESSAGE_START.saturating_add(message_len); + // Return base message if data is shorter than expected message end + if data.len() < message_end { + return BASE_MESSAGE.into(); + } + // Extract and decode the message + let body = &data[MESSAGE_START..message_end]; + match core::str::from_utf8(body) { + Ok(reason) => format!("{BASE_MESSAGE} {reason}"), + Err(_) => BASE_MESSAGE.into(), + } +} diff --git a/pallets/moonbeam-foreign-assets/src/lib.rs b/pallets/moonbeam-foreign-assets/src/lib.rs index e3c9398f4d..d1bc939018 100644 --- a/pallets/moonbeam-foreign-assets/src/lib.rs +++ b/pallets/moonbeam-foreign-assets/src/lib.rs @@ -172,7 +172,6 @@ pub mod pallet { AssetIdFiltered, AssetNotFrozen, CorruptedStorageOrphanLocation, - //Erc20ContractCallFail, Erc20ContractCreationFail, EvmCallPauseFail, EvmCallUnpauseFail, diff --git a/pallets/moonbeam-lazy-migrations/src/benchmarks.rs b/pallets/moonbeam-lazy-migrations/src/benchmarks.rs index 7ef1d0bd4d..d19993abf4 100644 --- a/pallets/moonbeam-lazy-migrations/src/benchmarks.rs +++ b/pallets/moonbeam-lazy-migrations/src/benchmarks.rs @@ -21,6 +21,7 @@ use frame_support::traits::Currency; use frame_support::BoundedVec; use frame_system::RawOrigin; use pallet_asset_manager::AssetRegistrar; +use sp_core::H160; use sp_core::{Get, U256}; use sp_runtime::traits::StaticLookup; use sp_runtime::Saturating; @@ -112,6 +113,7 @@ benchmarks! { where ::Balance: Into, T::ForeignAssetType: Into>, + ::AccountId: Into + From, } approve_assets_to_migrate { let n in 1 .. 100u32; diff --git a/pallets/moonbeam-lazy-migrations/src/foreign_asset.rs b/pallets/moonbeam-lazy-migrations/src/foreign_asset.rs index 3c25994f84..c7c8e8402f 100644 --- a/pallets/moonbeam-lazy-migrations/src/foreign_asset.rs +++ b/pallets/moonbeam-lazy-migrations/src/foreign_asset.rs @@ -20,7 +20,7 @@ use super::*; use frame_support::sp_runtime::Saturating; use frame_support::traits::{fungibles::metadata::Inspect, ReservableCurrency}; use parity_scale_codec::{Decode, Encode, MaxEncodedLen}; -use sp_core::U256; +use sp_core::{H160, U256}; #[derive(Debug, Encode, Decode, scale_info::TypeInfo, PartialEq, MaxEncodedLen)] pub enum ForeignAssetMigrationStatus { @@ -47,6 +47,7 @@ impl Pallet where ::Balance: Into, ::ForeignAssetType: Into>, + ::AccountId: Into + From, { /// Start a foreign asset migration by freezing the asset and creating the SC with the moonbeam /// foreign assets pallet. @@ -139,14 +140,20 @@ where _ => {} }; - MIGRATING_FOREIGN_ASSETS::using_once(&mut true, || { - pallet_moonbeam_foreign_assets::Pallet::::mint_into( - info.asset_id, - who.clone(), - asset.balance.into(), - ) - }) - .map_err(|_| Error::::MintFailed)?; + let zero_address = T::AccountId::from(H160::zero()); + if who.clone() != zero_address { + MIGRATING_FOREIGN_ASSETS::using_once(&mut true, || { + pallet_moonbeam_foreign_assets::Pallet::::mint_into( + info.asset_id, + who.clone(), + asset.balance.into(), + ) + }) + .map_err(|err| { + log::debug!("Error: {err:?}"); + Error::::MintFailed + })?; + } info.remaining_balances = info.remaining_balances.saturating_sub(1); Ok::<(), Error>(()) @@ -171,14 +178,29 @@ where ::Currency::unreserve(&owner, approval.deposit); MIGRATING_FOREIGN_ASSETS::using_once(&mut true, || { - pallet_moonbeam_foreign_assets::Pallet::::approve( + let address: H160 = owner.clone().into(); + + // Temporarily remove metadata + let meta = pallet_evm::AccountCodesMetadata::::take(address.clone()); + + let result = pallet_moonbeam_foreign_assets::Pallet::::approve( info.asset_id, owner.clone(), beneficiary, approval.amount.into(), - ) + ); + + // Re-add metadata + if let Some(metadata) = meta { + pallet_evm::AccountCodesMetadata::::insert(address, metadata); + } + + result }) - .map_err(|_| Error::::ApprovalFailed)?; + .map_err(|err| { + log::debug!("Error: {err:?}"); + Error::::ApprovalFailed + })?; info.remaining_approvals = info.remaining_approvals.saturating_sub(1); Ok::<(), Error>(()) diff --git a/pallets/moonbeam-lazy-migrations/src/lib.rs b/pallets/moonbeam-lazy-migrations/src/lib.rs index 603ac215ce..2e1877f9aa 100644 --- a/pallets/moonbeam-lazy-migrations/src/lib.rs +++ b/pallets/moonbeam-lazy-migrations/src/lib.rs @@ -130,6 +130,7 @@ pub mod pallet { where ::Balance: Into, ::ForeignAssetType: Into>, + ::AccountId: Into + From, { #[pallet::call_index(2)] #[pallet::weight(Pallet::::create_contract_metadata_weight(MAX_CONTRACT_CODE_SIZE))] diff --git a/test/suites/dev/moonbase/test-xcm-v3/test-mock-hrmp-transact-ethereum-12.ts b/test/suites/dev/moonbase/test-xcm-v3/test-mock-hrmp-transact-ethereum-12.ts index e63ff0e398..ebdfbeb48a 100644 --- a/test/suites/dev/moonbase/test-xcm-v3/test-mock-hrmp-transact-ethereum-12.ts +++ b/test/suites/dev/moonbase/test-xcm-v3/test-mock-hrmp-transact-ethereum-12.ts @@ -56,7 +56,7 @@ describeSuite({ const amountToTransfer = transferredBalance / 10n; - const GAS_LIMIT = 500_000; + const GAS_LIMIT = 500_000n; // We will put a very high gas limit. However, the weight accounted // for the block should only @@ -82,13 +82,7 @@ describeSuite({ let expectedTransferredAmount = 0n; let expectedTransferredAmountPlusFees = 0n; - // Just to make sure lazy state trie migration is done - // probably not needed after migration is done - for (let i = 0; i < 10; i++) { - await context.createBlock(); - } - - const targetXcmWeight = 500_000n * 25000n + STORAGE_READ_COST + 4_250_000_000n; + const targetXcmWeight = GAS_LIMIT * 25000n + STORAGE_READ_COST + 4_250_000_000n; const targetXcmFee = targetXcmWeight * 50_000n; for (const xcmTransaction of xcmTransactions) { @@ -114,7 +108,7 @@ describeSuite({ ], weight_limit: { refTime: targetXcmWeight, - proofSize: (GAS_LIMIT / GAS_LIMIT_POV_RATIO) * 2, + proofSize: (Number(GAS_LIMIT) / GAS_LIMIT_POV_RATIO) * 2, } as any, descend_origin: sendingAddress, }) @@ -126,8 +120,8 @@ describeSuite({ originKind: "SovereignAccount", // 500_000 gas limit + db read (41_742_000) requireWeightAtMost: { - refTime: 12_525_000_000n + STORAGE_READ_COST, - proofSize: GAS_LIMIT / GAS_LIMIT_POV_RATIO, + refTime: 12_500_000_000n + STORAGE_READ_COST, + proofSize: Number(GAS_LIMIT) / GAS_LIMIT_POV_RATIO, }, call: { encoded: transferCallEncoded, @@ -149,7 +143,7 @@ describeSuite({ expect(testAccountBalance).to.eq(expectedTransferredAmount); // Make sure ALITH has been deducted fees once (in xcm-executor) and balance has been - // transfered through evm. + // transferred through evm. const alithAccountBalance = await context.viem().getBalance({ address: descendAddress }); expect(BigInt(alithAccountBalance)).to.eq( transferredBalance - expectedTransferredAmountPlusFees