From cc85dc334f58c558b7af4d15e315ffcb0bba6361 Mon Sep 17 00:00:00 2001 From: Michael Birch Date: Wed, 4 May 2022 17:25:06 -0400 Subject: [PATCH] Fix(connector): Return an error when storage cannot be read instead of panicking (#501) --- engine-standalone-storage/src/sync/mod.rs | 30 +++++++---- .../src/test_utils/standalone/mocks/mod.rs | 1 + engine/src/connector.rs | 40 +++++++++++---- engine/src/engine.rs | 14 ++++-- engine/src/lib.rs | 50 +++++++++++++++---- 5 files changed, 99 insertions(+), 36 deletions(-) diff --git a/engine-standalone-storage/src/sync/mod.rs b/engine-standalone-storage/src/sync/mod.rs index 73da3064f..513f394fe 100644 --- a/engine-standalone-storage/src/sync/mod.rs +++ b/engine-standalone-storage/src/sync/mod.rs @@ -201,7 +201,8 @@ fn non_submit_execute<'db>( engine::Engine::new(relayer_address, env.current_account_id(), io, &env)?; if env.predecessor_account_id == env.current_account_id { - connector::EthConnectorContract::init_instance(io).ft_on_transfer(&engine, args)?; + connector::EthConnectorContract::init_instance(io)? + .ft_on_transfer(&engine, args)?; } else { engine.receive_erc20_tokens( &env.predecessor_account_id, @@ -216,7 +217,7 @@ fn non_submit_execute<'db>( } TransactionKind::FtTransferCall(args) => { - let mut connector = connector::EthConnectorContract::init_instance(io); + let mut connector = connector::EthConnectorContract::init_instance(io)?; let promise_args = connector.ft_transfer_call( env.predecessor_account_id.clone(), env.current_account_id.clone(), @@ -228,21 +229,21 @@ fn non_submit_execute<'db>( } TransactionKind::ResolveTransfer(args, promise_result) => { - let mut connector = connector::EthConnectorContract::init_instance(io); + let mut connector = connector::EthConnectorContract::init_instance(io)?; connector.ft_resolve_transfer(args.clone(), promise_result.clone()); None } TransactionKind::FtTransfer(args) => { - let mut connector = connector::EthConnectorContract::init_instance(io); + let mut connector = connector::EthConnectorContract::init_instance(io)?; connector.ft_transfer(&env.predecessor_account_id, args.clone())?; None } TransactionKind::Withdraw(args) => { - let mut connector = connector::EthConnectorContract::init_instance(io); + let mut connector = connector::EthConnectorContract::init_instance(io)?; connector.withdraw_eth_from_near( &env.current_account_id, &env.predecessor_account_id, @@ -253,7 +254,7 @@ fn non_submit_execute<'db>( } TransactionKind::Deposit(raw_proof) => { - let connector_contract = connector::EthConnectorContract::init_instance(io); + let connector_contract = connector::EthConnectorContract::init_instance(io)?; let promise_args = connector_contract.deposit( raw_proof.clone(), env.current_account_id(), @@ -264,7 +265,7 @@ fn non_submit_execute<'db>( } TransactionKind::FinishDeposit(finish_args) => { - let mut connector = connector::EthConnectorContract::init_instance(io); + let mut connector = connector::EthConnectorContract::init_instance(io)?; let maybe_promise_args = connector.finish_deposit( env.predecessor_account_id(), env.current_account_id(), @@ -276,7 +277,7 @@ fn non_submit_execute<'db>( } TransactionKind::StorageDeposit(args) => { - let mut connector = connector::EthConnectorContract::init_instance(io); + let mut connector = connector::EthConnectorContract::init_instance(io)?; let _ = connector.storage_deposit( env.predecessor_account_id, Yocto::new(env.attached_deposit), @@ -287,21 +288,21 @@ fn non_submit_execute<'db>( } TransactionKind::StorageUnregister(force) => { - let mut connector = connector::EthConnectorContract::init_instance(io); + let mut connector = connector::EthConnectorContract::init_instance(io)?; let _ = connector.storage_unregister(env.predecessor_account_id, *force)?; None } TransactionKind::StorageWithdraw(args) => { - let mut connector = connector::EthConnectorContract::init_instance(io); + let mut connector = connector::EthConnectorContract::init_instance(io)?; connector.storage_withdraw(&env.predecessor_account_id, args.clone())?; None } TransactionKind::SetPausedFlags(args) => { - let mut connector = connector::EthConnectorContract::init_instance(io); + let mut connector = connector::EthConnectorContract::init_instance(io)?; connector.set_paused_flags(args.clone()); None @@ -398,6 +399,7 @@ pub mod error { FtStorageFunding(fungible_token::error::StorageFundingError), InvalidAddress(aurora_engine_types::types::address::error::AddressError), ConnectorInit(connector::error::InitContractError), + ConnectorStorage(connector::error::StorageReadError), } impl From for Error { @@ -465,4 +467,10 @@ pub mod error { Self::ConnectorInit(e) } } + + impl From for Error { + fn from(e: connector::error::StorageReadError) -> Self { + Self::ConnectorStorage(e) + } + } } diff --git a/engine-tests/src/test_utils/standalone/mocks/mod.rs b/engine-tests/src/test_utils/standalone/mocks/mod.rs index f71f2fc75..304a25551 100644 --- a/engine-tests/src/test_utils/standalone/mocks/mod.rs +++ b/engine-tests/src/test_utils/standalone/mocks/mod.rs @@ -115,6 +115,7 @@ pub fn mint_evm_account( io.remove_storage(&proof_key); aurora_engine::connector::EthConnectorContract::init_instance(io) + .unwrap() .finish_deposit( aurora_account_id.clone(), aurora_account_id.clone(), diff --git a/engine/src/connector.rs b/engine/src/connector.rs index aa471e3e1..c76b7ab36 100644 --- a/engine/src/connector.rs +++ b/engine/src/connector.rs @@ -62,14 +62,14 @@ impl EthConnectorContract { /// Init Eth-connector contract instance. /// Load contract data from storage and init I/O handler. /// Used as single point of contract access for various contract actions - pub fn init_instance(io: I) -> Self { - Self { - contract: get_contract_data(&io, &EthConnectorStorageId::Contract), - ft: get_contract_data::(&io, &EthConnectorStorageId::FungibleToken) + pub fn init_instance(io: I) -> Result { + Ok(Self { + contract: get_contract_data(&io, &EthConnectorStorageId::Contract)?, + ft: get_contract_data::(&io, &EthConnectorStorageId::FungibleToken)? .ops(io), - paused_mask: get_contract_data(&io, &EthConnectorStorageId::PausedMask), + paused_mask: get_contract_data(&io, &EthConnectorStorageId::PausedMask)?, io, - } + }) } /// Create contract data - init eth-connector contract specific data. @@ -687,11 +687,16 @@ fn construct_contract_key(suffix: &EthConnectorStorageId) -> Vec { crate::prelude::bytes_to_key(KeyPrefix::EthConnector, &[*suffix as u8]) } -fn get_contract_data(io: &I, suffix: &EthConnectorStorageId) -> T { +fn get_contract_data( + io: &I, + suffix: &EthConnectorStorageId, +) -> Result { io.read_storage(&construct_contract_key(suffix)) - .expect("Failed read storage") - .to_value() - .unwrap() + .ok_or(error::StorageReadError::KeyNotFound) + .and_then(|x| { + x.to_value() + .map_err(|_| error::StorageReadError::BorshDeserialize) + }) } /// Sets the contract data and returns it back @@ -735,6 +740,21 @@ pub mod error { const PROOF_EXIST: &[u8; 15] = b"ERR_PROOF_EXIST"; + #[cfg_attr(not(target_arch = "wasm32"), derive(Debug))] + pub enum StorageReadError { + KeyNotFound, + BorshDeserialize, + } + + impl AsRef<[u8]> for StorageReadError { + fn as_ref(&self) -> &[u8] { + match self { + Self::KeyNotFound => b"ERR_CONNECTOR_STORAGE_KEY_NOT_FOUND", + Self::BorshDeserialize => b"ERR_FAILED_DESERIALIZE_CONNECTOR_DATA", + } + } + } + #[cfg_attr(not(target_arch = "wasm32"), derive(Debug))] pub enum DepositError { Paused, diff --git a/engine/src/engine.rs b/engine/src/engine.rs index 7f5a3c2cf..1de7d6982 100644 --- a/engine/src/engine.rs +++ b/engine/src/engine.rs @@ -1277,12 +1277,16 @@ pub fn set_balance(io: &mut I, address: &Address, balance: &Wei) { pub fn remove_balance(io: &mut I, address: &Address) { let balance = get_balance(io, address); - // Apply changes for eth-connector. The `unwrap` is safe here because (a) if the connector - // is implemented correctly then the total supply wll never underflow and (b) we are passing - // in the balance directly so there will always be enough balance. + // Apply changes for eth-connector. We ignore the `StorageReadError` intentionally since + // if we cannot read the storage then there is nothing to remove. EthConnectorContract::init_instance(*io) - .internal_remove_eth(address, balance) - .unwrap(); + .map(|mut connector| { + // The `unwrap` is safe here because (a) if the connector + // is implemented correctly then the total supply will never underflow and (b) we are passing + // in the balance directly so there will always be enough balance. + connector.internal_remove_eth(address, balance).unwrap(); + }) + .ok(); io.remove_storage(&address_to_key(KeyPrefix::Balance, address)); } diff --git a/engine/src/lib.rs b/engine/src/lib.rs index 3a9e9f348..72e4b249c 100644 --- a/engine/src/lib.rs +++ b/engine/src/lib.rs @@ -340,6 +340,7 @@ mod contract { if predecessor_account_id == current_account_id { EthConnectorContract::init_instance(io) + .sdk_unwrap() .ft_on_transfer(&engine, &args) .sdk_unwrap(); } else { @@ -522,6 +523,7 @@ mod contract { let current_account_id = io.current_account_id(); let predecessor_account_id = io.predecessor_account_id(); let result = EthConnectorContract::init_instance(io) + .sdk_unwrap() .withdraw_eth_from_near(¤t_account_id, &predecessor_account_id, args) .sdk_unwrap(); let result_bytes = result.try_to_vec().sdk_expect("ERR_SERIALIZE"); @@ -535,6 +537,7 @@ mod contract { let current_account_id = io.current_account_id(); let predecessor_account_id = io.predecessor_account_id(); let promise_args = EthConnectorContract::init_instance(io) + .sdk_unwrap() .deposit(raw_proof, current_account_id, predecessor_account_id) .sdk_unwrap(); let promise_id = io.promise_create_with_callback(&promise_args); @@ -564,6 +567,7 @@ mod contract { let current_account_id = io.current_account_id(); let predecessor_account_id = io.predecessor_account_id(); let maybe_promise_args = EthConnectorContract::init_instance(io) + .sdk_unwrap() .finish_deposit( predecessor_account_id, current_account_id, @@ -583,7 +587,9 @@ mod contract { let mut io = Runtime; let args: IsUsedProofCallArgs = io.read_input_borsh().sdk_unwrap(); - let is_used_proof = EthConnectorContract::init_instance(io).is_used_proof(args.proof); + let is_used_proof = EthConnectorContract::init_instance(io) + .sdk_unwrap() + .is_used_proof(args.proof); let res = is_used_proof.try_to_vec().unwrap(); io.return_output(&res[..]); } @@ -591,19 +597,25 @@ mod contract { #[no_mangle] pub extern "C" fn ft_total_supply() { let io = Runtime; - EthConnectorContract::init_instance(io).ft_total_eth_supply_on_near(); + EthConnectorContract::init_instance(io) + .sdk_unwrap() + .ft_total_eth_supply_on_near(); } #[no_mangle] pub extern "C" fn ft_total_eth_supply_on_near() { let io = Runtime; - EthConnectorContract::init_instance(io).ft_total_eth_supply_on_near(); + EthConnectorContract::init_instance(io) + .sdk_unwrap() + .ft_total_eth_supply_on_near(); } #[no_mangle] pub extern "C" fn ft_total_eth_supply_on_aurora() { let io = Runtime; - EthConnectorContract::init_instance(io).ft_total_eth_supply_on_aurora(); + EthConnectorContract::init_instance(io) + .sdk_unwrap() + .ft_total_eth_supply_on_aurora(); } #[no_mangle] @@ -613,7 +625,9 @@ mod contract { parse_json(&io.read_input().to_vec()).sdk_unwrap(), ) .sdk_unwrap(); - EthConnectorContract::init_instance(io).ft_balance_of(args); + EthConnectorContract::init_instance(io) + .sdk_unwrap() + .ft_balance_of(args); } #[no_mangle] @@ -621,6 +635,7 @@ mod contract { let io = Runtime; let args: parameters::BalanceOfEthCallArgs = io.read_input().to_value().sdk_unwrap(); EthConnectorContract::init_instance(io) + .sdk_unwrap() .ft_balance_of_eth_on_aurora(args) .sdk_unwrap(); } @@ -635,6 +650,7 @@ mod contract { ) .sdk_unwrap(); EthConnectorContract::init_instance(io) + .sdk_unwrap() .ft_transfer(&predecessor_account_id, args) .sdk_unwrap(); } @@ -651,7 +667,9 @@ mod contract { let args: ResolveTransferCallArgs = io.read_input().to_value().sdk_unwrap(); let promise_result = io.promise_result(0).sdk_unwrap(); - EthConnectorContract::init_instance(io).ft_resolve_transfer(args, promise_result); + EthConnectorContract::init_instance(io) + .sdk_unwrap() + .ft_resolve_transfer(args, promise_result); } #[no_mangle] @@ -668,6 +686,7 @@ mod contract { let current_account_id = io.current_account_id(); let predecessor_account_id = io.predecessor_account_id(); let promise_args = EthConnectorContract::init_instance(io) + .sdk_unwrap() .ft_transfer_call( predecessor_account_id, current_account_id, @@ -686,6 +705,7 @@ mod contract { let predecessor_account_id = io.predecessor_account_id(); let amount = Yocto::new(io.attached_deposit()); let maybe_promise = EthConnectorContract::init_instance(io) + .sdk_unwrap() .storage_deposit(predecessor_account_id, amount, args) .sdk_unwrap(); if let Some(promise) = maybe_promise { @@ -700,6 +720,7 @@ mod contract { let predecessor_account_id = io.predecessor_account_id(); let force = parse_json(&io.read_input().to_vec()).and_then(|args| args.bool("force").ok()); let maybe_promise = EthConnectorContract::init_instance(io) + .sdk_unwrap() .storage_unregister(predecessor_account_id, force) .sdk_unwrap(); if let Some(promise) = maybe_promise { @@ -715,6 +736,7 @@ mod contract { StorageWithdrawCallArgs::from(parse_json(&io.read_input().to_vec()).sdk_unwrap()); let predecessor_account_id = io.predecessor_account_id(); EthConnectorContract::init_instance(io) + .sdk_unwrap() .storage_withdraw(&predecessor_account_id, args) .sdk_unwrap() } @@ -726,13 +748,17 @@ mod contract { parse_json(&io.read_input().to_vec()).sdk_unwrap(), ) .sdk_unwrap(); - EthConnectorContract::init_instance(io).storage_balance_of(args) + EthConnectorContract::init_instance(io) + .sdk_unwrap() + .storage_balance_of(args) } #[no_mangle] pub extern "C" fn get_paused_flags() { let mut io = Runtime; - let paused_flags = EthConnectorContract::init_instance(io).get_paused_flags(); + let paused_flags = EthConnectorContract::init_instance(io) + .sdk_unwrap() + .get_paused_flags(); let data = paused_flags.try_to_vec().expect(ERR_FAILED_PARSE); io.return_output(&data[..]); } @@ -743,13 +769,17 @@ mod contract { io.assert_private_call().sdk_unwrap(); let args: PauseEthConnectorCallArgs = io.read_input_borsh().sdk_unwrap(); - EthConnectorContract::init_instance(io).set_paused_flags(args); + EthConnectorContract::init_instance(io) + .sdk_unwrap() + .set_paused_flags(args); } #[no_mangle] pub extern "C" fn get_accounts_counter() { let io = Runtime; - EthConnectorContract::init_instance(io).get_accounts_counter(); + EthConnectorContract::init_instance(io) + .sdk_unwrap() + .get_accounts_counter(); } #[no_mangle]