From 376989e8b04cbcc6c604398242aea5c70fec645e Mon Sep 17 00:00:00 2001 From: Mitchell Turner Date: Fri, 4 Aug 2023 15:20:20 -0700 Subject: [PATCH] Add more test cases for `transfer` and `transfer_output` (#535) * Do small refactor, rename variables, fix bad test, add naive new test case * Beef up TRO test and add internal case for TR test * Update CHANGELOG * Improve docs/param names * Move CHANGELOG entry * Remove debugging stuff, add sad cases * Unify memory lookup helpers * Simplify const construction * Add tests that switch memory offsets * Simplify helper functions with existing `read_bytes` function * REmove helpers * Add overflow tests * Consolidate tests --- CHANGELOG.md | 1 + fuel-vm/src/interpreter/contract.rs | 73 +++---- fuel-vm/src/interpreter/contract/tests.rs | 232 +++++++++++++++++----- 3 files changed, 211 insertions(+), 95 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d5a47cc02..75fd18323d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - [#525](https://github.com/FuelLabs/fuel-vm/pull/525): The `$hp` register is no longer restored to it's previous value when returning from a call, making it possible to return heap-allocated types from `CALL`. - [#531](https://github.com/FuelLabs/fuel-vm/pull/531): UtxoId::from_str and TxPointer::from_str no longer crash on invalid input with multibyte characters. Also adds clippy lints to prevent future issues. +- [#535](https://github.com/FuelLabs/fuel-vm/pull/535): Add better test coverage for TR and TRO #### Breaking diff --git a/fuel-vm/src/interpreter/contract.rs b/fuel-vm/src/interpreter/contract.rs index 38a46bed82..92faf9fd71 100644 --- a/fuel-vm/src/interpreter/contract.rs +++ b/fuel-vm/src/interpreter/contract.rs @@ -51,6 +51,7 @@ use fuel_types::{ ContractId, }; +use crate::interpreter::memory::read_bytes; use std::borrow::Cow; #[cfg(test)] @@ -201,37 +202,27 @@ struct TransferCtx<'vm, S, Tx> { } impl<'vm, S, Tx> TransferCtx<'vm, S, Tx> { + /// In Fuel specs: + /// Transfer $rB coins with asset ID at $rC to contract with ID at $rA. + /// $rA -> recipient_contract_id_offset + /// $rB -> transfer_amount + /// $rC -> asset_id_offset pub(crate) fn transfer( self, panic_context: &mut PanicContext, - a: Word, - b: Word, - c: Word, + recipient_contract_id_offset: Word, + transfer_amount: Word, + asset_id_offset: Word, ) -> Result<(), RuntimeError> where Tx: ExecutableTransaction, S: ContractsAssetsStorage, >::Error: Into, { - let ax = a - .checked_add(ContractId::LEN as Word) - .ok_or(PanicReason::ArithmeticOverflow)?; - - let cx = c - .checked_add(AssetId::LEN as Word) - .ok_or(PanicReason::ArithmeticOverflow)?; - - // if above usize::MAX then it cannot be safely cast to usize, - // check the tighter bound between VM_MAX_RAM and usize::MAX - if ax > MIN_VM_MAX_RAM_USIZE_MAX || cx > MIN_VM_MAX_RAM_USIZE_MAX { - return Err(PanicReason::MemoryOverflow.into()) - } - - let amount = b; - let destination = ContractId::try_from(&self.memory[a as usize..ax as usize]) - .expect("Unreachable! Checked memory range"); - let asset_id = AssetId::try_from(&self.memory[c as usize..cx as usize]) - .expect("Unreachable! Checked memory range"); + let amount = transfer_amount; + let destination = + ContractId::from(read_bytes(self.memory, recipient_contract_id_offset)?); + let asset_id = AssetId::from(read_bytes(self.memory, asset_id_offset)?); InputContracts::new(self.tx.input_contracts(), panic_context) .check(&destination)?; @@ -282,38 +273,28 @@ impl<'vm, S, Tx> TransferCtx<'vm, S, Tx> { inc_pc(self.pc) } + /// In Fuel specs: + /// Transfer $rC coins with asset ID at $rD to address at $rA, with output $rB. + /// $rA -> recipient_offset + /// $rB -> output_index + /// $rC -> transfer_amount + /// $rD -> asset_id_offset pub(crate) fn transfer_output( self, - a: Word, - b: Word, - c: Word, - d: Word, + recipient_offset: Word, + output_index: Word, + transfer_amount: Word, + asset_id_offset: Word, ) -> Result<(), RuntimeError> where Tx: ExecutableTransaction, S: ContractsAssetsStorage, >::Error: Into, { - let ax = a - .checked_add(ContractId::LEN as Word) - .ok_or(PanicReason::ArithmeticOverflow)?; - - let dx = d - .checked_add(AssetId::LEN as Word) - .ok_or(PanicReason::ArithmeticOverflow)?; - - // if above usize::MAX then it cannot be safely cast to usize, - // check the tighter bound between VM_MAX_RAM and usize::MAX - if ax > MIN_VM_MAX_RAM_USIZE_MAX || dx > MIN_VM_MAX_RAM_USIZE_MAX { - return Err(PanicReason::MemoryOverflow.into()) - } - - let out_idx = b as usize; - let to = Address::try_from(&self.memory[a as usize..ax as usize]) - .expect("Unreachable! Checked memory range"); - let asset_id = AssetId::try_from(&self.memory[d as usize..dx as usize]) - .expect("Unreachable! Checked memory range"); - let amount = c; + let out_idx = output_index as usize; + let to = Address::from(read_bytes(self.memory, recipient_offset)?); + let asset_id = AssetId::from(read_bytes(self.memory, asset_id_offset)?); + let amount = transfer_amount; if amount == 0 { return Err(PanicReason::TransferZeroCoins.into()) diff --git a/fuel-vm/src/interpreter/contract/tests.rs b/fuel-vm/src/interpreter/contract/tests.rs index 5b59e52480..bb6a5a9cc9 100644 --- a/fuel-vm/src/interpreter/contract/tests.rs +++ b/fuel-vm/src/interpreter/contract/tests.rs @@ -4,6 +4,7 @@ use crate::{ }; use super::*; +use crate::interpreter::internal::absolute_output_mem_range; use fuel_tx::{ field::{ Inputs, @@ -12,6 +13,7 @@ use fuel_tx::{ Input, Script, }; +use fuel_types::bytes::Deserializable; use test_case::test_case; #[test_case(0, 32 => Ok(()); "Can read contract balance")] @@ -52,20 +54,56 @@ fn test_contract_balance(b: Word, c: Word) -> Result<(), RuntimeError> { Ok(()) } -#[test_case(true, 0, 50, 32 => Ok(()); "Can transfer from external balance")] -fn test_transfer(external: bool, a: Word, b: Word, c: Word) -> Result<(), RuntimeError> { +#[test_case(true, 0, 0, 50, 32, 32 => Ok(()); "Can transfer from external balance")] +#[test_case(false, 0, 0, 50, 32, 32 => Ok(()); "Can transfer from internal balance")] +#[test_case(true, 32, 32, 50, 0, 0 => Ok(()); "Can transfer from external balance with flipped offsets")] +#[test_case(false, 32, 32, 50, 0, 0 => Ok(()); "Can transfer from internal balance with flipped offsets")] +#[test_case(true, 0, 0, 70, 32, 32 => Err(RuntimeError::Recoverable(PanicReason::NotEnoughBalance)); "Cannot transfer from external balance insufficient funds")] +#[test_case(false, 0, 0, 70, 32, 32 => Err(RuntimeError::Recoverable(PanicReason::NotEnoughBalance)); "Cannot transfer from internal balance insufficient funds")] +#[test_case(true, MEM_SIZE as u64 - 1, 0, 50, 32, 32 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "External transfer errors if contract_id lookup overflows")] +#[test_case(false, MEM_SIZE as u64 - 1, 0, 50, 32, 32 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "Internal transfer errors if contract_id lookup overflows")] +#[test_case(true, 0, 0, 50, MEM_SIZE as u64 - 1, 32 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "External transfer errors if asset_id lookup overflows")] +#[test_case(false, 0, 0, 50, MEM_SIZE as u64 - 1, 32 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "Internal transfer errors if asset_id lookup overflows")] +fn test_transfer( + external: bool, + requested_contract_id_offset: Word, + real_contract_id_offset: Word, + transfer_amount: Word, + requested_asset_id_offset: Word, + real_asset_id_offset: Word, +) -> Result<(), RuntimeError> { + // Given + + const ASSET_ID: AssetId = AssetId::new([2u8; AssetId::LEN]); + const RECIPIENT_CONTRACT_ID: ContractId = ContractId::new([3u8; ContractId::LEN]); + const SOURCE_CONTRACT_ID: ContractId = ContractId::new([5u8; ContractId::LEN]); + + let mut pc = 4; + + // Arbitrary value + let fp = 2048; + let is = 0; + let mut memory: Memory = vec![1u8; MEM_SIZE].try_into().unwrap(); - memory[a as usize..(a as usize + ContractId::LEN)] - .copy_from_slice(&[3u8; ContractId::LEN][..]); - memory[c as usize..(c as usize + AssetId::LEN)] - .copy_from_slice(&[2u8; AssetId::LEN][..]); - let contract_id = ContractId::from([3u8; 32]); - let asset_id = AssetId::from([2u8; 32]); + memory[real_contract_id_offset as usize + ..(real_contract_id_offset as usize + ContractId::LEN)] + .copy_from_slice(RECIPIENT_CONTRACT_ID.as_ref()); + memory[real_asset_id_offset as usize..(real_asset_id_offset as usize + AssetId::LEN)] + .copy_from_slice(ASSET_ID.as_ref()); + memory[fp as usize..(fp as usize + ContractId::LEN)] + .copy_from_slice(SOURCE_CONTRACT_ID.as_ref()); + let mut storage = MemoryStorage::new(Default::default(), Default::default()); + + let initial_recipient_contract_balance = 0; + let initial_source_contract_balance = 60; storage - .merkle_contract_asset_id_balance_insert(&contract_id, &asset_id, 60) + .merkle_contract_asset_id_balance_insert( + &SOURCE_CONTRACT_ID, + &ASSET_ID, + initial_source_contract_balance, + ) .unwrap(); - let mut pc = 4; let context = if external { Context::Script { @@ -77,8 +115,9 @@ fn test_transfer(external: bool, a: Word, b: Word, c: Word) -> Result<(), Runtim } }; - let mut balances = - RuntimeBalances::try_from_iter([(AssetId::from([2u8; 32]), 50)]).unwrap(); + let mut balances = RuntimeBalances::try_from_iter([(ASSET_ID, 50)]).unwrap(); + let start_balance = balances.balance(&ASSET_ID).unwrap(); + let mut receipts = Default::default(); let mut panic_context = PanicContext::None; let mut tx = Script::default(); @@ -87,13 +126,10 @@ fn test_transfer(external: bool, a: Word, b: Word, c: Word) -> Result<(), Runtim Default::default(), Default::default(), Default::default(), - contract_id, + RECIPIENT_CONTRACT_ID, )]; - let fp = 0; - let is = 0; - - let input = TransferCtx { + let transfer_ctx = TransferCtx { storage: &mut storage, memory: &mut memory, pc: RegMut::new(&mut pc), @@ -106,39 +142,104 @@ fn test_transfer(external: bool, a: Word, b: Word, c: Word) -> Result<(), Runtim is: Reg::new(&is), }; - input.transfer(&mut panic_context, a, b, c)?; + // When - assert_eq!(pc, 8); - let amount = storage - .merkle_contract_asset_id_balance(&contract_id, &asset_id) + transfer_ctx.transfer( + &mut panic_context, + requested_contract_id_offset, + transfer_amount, + requested_asset_id_offset, + )?; + + // Then + + let final_recipient_contract_balance = storage + .merkle_contract_asset_id_balance(&RECIPIENT_CONTRACT_ID, &ASSET_ID) + .unwrap() + .unwrap(); + + let final_source_contract_balance = storage + .merkle_contract_asset_id_balance(&SOURCE_CONTRACT_ID, &ASSET_ID) .unwrap() .unwrap(); - assert_eq!(balances.balance(&asset_id).unwrap(), 0); - assert_eq!(amount, 60 + b); + + assert_eq!(pc, 8); + assert_eq!( + final_recipient_contract_balance, + initial_recipient_contract_balance + transfer_amount + ); + if external { + assert_eq!( + balances.balance(&ASSET_ID).unwrap(), + start_balance - transfer_amount + ); + assert_eq!( + final_source_contract_balance, + initial_source_contract_balance + ); + } else { + assert_eq!(balances.balance(&ASSET_ID).unwrap(), start_balance); + assert_eq!( + final_source_contract_balance, + initial_source_contract_balance - transfer_amount + ); + } Ok(()) } -#[test_case(true, 0, 0, 50, 32 => Ok(()); "Can transfer from external balance")] +#[test_case(true, 0, 0, 0, 50, 32, 32 => Ok(()); "Can transfer from external balance")] +#[test_case(false, 0, 0, 0, 50, 32, 32 => Ok(()); "Can transfer from internal balance")] +#[test_case(true, 32, 32, 0, 50, 0, 0 => Ok(()); "Can transfer from external balance with flipped offsets")] +#[test_case(false,32, 32, 0, 50, 0, 0 => Ok(()); "Can transfer from internal balance with flipped offsets")] +#[test_case(false, 0, 0, 0, 70, 32, 32 => Err(RuntimeError::Recoverable(PanicReason::NotEnoughBalance)); "Cannot transfer from external balance insufficient funds")] +#[test_case(false, 0, 0, 0, 70, 32, 32 => Err(RuntimeError::Recoverable(PanicReason::NotEnoughBalance)); "Cannot transfer from internal balance insufficient funds")] +#[test_case(true, MEM_SIZE as u64 - 1, 0, 0, 50, 32, 32 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "External transfer errors if contract_id lookup overflows")] +#[test_case(false, MEM_SIZE as u64 - 1, 0, 0, 50, 32, 32 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "Internal transfer errors if contract_id lookup overflows")] +#[test_case(true, 0, 0, 0, 50, MEM_SIZE as u64 - 1, 32 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "External transfer errors if asset_id lookup overflows")] +#[test_case(false, 0, 0, 0, 50, MEM_SIZE as u64 - 1, 32 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "Internal transfer errors if asset_id lookup overflows")] fn test_transfer_output( external: bool, - a: Word, - b: Word, - c: Word, - d: Word, + requested_recipient_offset: Word, + real_recipient_offset: Word, + output_index: Word, + transfer_amount: Word, + requested_asset_id_offset: Word, + real_asset_id_offset: Word, ) -> Result<(), RuntimeError> { + // Given + + const ASSET_ID: AssetId = AssetId::new([2u8; AssetId::LEN]); + const SOURCE_CONTRACT_ID: ContractId = ContractId::new([3u8; ContractId::LEN]); + const RECIPIENT_ADDRESS: Address = Address::new([4u8; Address::LEN]); + + let mut pc = 4; + + // Arbitrary value + let fp = 2048; + let is = 0; + let mut memory: Memory = vec![1u8; MEM_SIZE].try_into().unwrap(); - memory[a as usize..(a as usize + Address::LEN)] - .copy_from_slice(&[3u8; Address::LEN][..]); - memory[d as usize..(d as usize + AssetId::LEN)] - .copy_from_slice(&[2u8; AssetId::LEN][..]); - let contract_id = ContractId::from([3u8; 32]); - let asset_id = AssetId::from([2u8; 32]); + + memory + [real_recipient_offset as usize..(real_recipient_offset as usize + Address::LEN)] + .copy_from_slice(RECIPIENT_ADDRESS.as_ref()); + memory[real_asset_id_offset as usize..(real_asset_id_offset as usize + AssetId::LEN)] + .copy_from_slice(ASSET_ID.as_ref()); + memory[fp as usize..(fp as usize + ContractId::LEN)] + .copy_from_slice(SOURCE_CONTRACT_ID.as_ref()); + let mut storage = MemoryStorage::new(Default::default(), Default::default()); + + let initial_contract_balance = 60; + storage - .merkle_contract_asset_id_balance_insert(&contract_id, &asset_id, 60) + .merkle_contract_asset_id_balance_insert( + &SOURCE_CONTRACT_ID, + &ASSET_ID, + initial_contract_balance, + ) .unwrap(); - let mut pc = 4; let context = if external { Context::Script { @@ -150,8 +251,10 @@ fn test_transfer_output( } }; + let balance_of_start = transfer_amount; + let mut balances = - RuntimeBalances::try_from_iter([(AssetId::from([2u8; 32]), 50)]).unwrap(); + RuntimeBalances::try_from_iter([(ASSET_ID, balance_of_start)]).unwrap(); let mut receipts = Default::default(); let mut tx = Script::default(); *tx.inputs_mut() = vec![Input::contract( @@ -159,18 +262,21 @@ fn test_transfer_output( Default::default(), Default::default(), Default::default(), - contract_id, + SOURCE_CONTRACT_ID, )]; + *tx.outputs_mut() = vec![Output::variable( - Default::default(), + RECIPIENT_ADDRESS, Default::default(), Default::default(), )]; - let fp = 0; - let is = 0; + let tx_offset = 512; - let input = TransferCtx { + let output_range = + absolute_output_mem_range(&tx, tx_offset, output_index as usize)?.unwrap(); + + let transfer_ctx = TransferCtx { storage: &mut storage, memory: &mut memory, pc: RegMut::new(&mut pc), @@ -178,20 +284,48 @@ fn test_transfer_output( balances: &mut balances, receipts: &mut receipts, tx: &mut tx, - tx_offset: 0, + tx_offset, fp: Reg::new(&fp), is: Reg::new(&is), }; - input.transfer_output(a, b, c, d)?; + // When - assert_eq!(pc, 8); - let amount = storage - .merkle_contract_asset_id_balance(&contract_id, &asset_id) + transfer_ctx.transfer_output( + requested_recipient_offset, + output_index, + transfer_amount, + requested_asset_id_offset, + )?; + + // Then + + let final_contract_balance = storage + .merkle_contract_asset_id_balance(&SOURCE_CONTRACT_ID, &ASSET_ID) .unwrap() .unwrap(); - assert_eq!(balances.balance(&asset_id).unwrap(), 0); - assert_eq!(amount, 60 + b); + + assert_eq!(pc, 8); + + let output_bytes: &[u8] = &memory[output_range.start..output_range.end]; + let output = Output::from_bytes(output_bytes).unwrap(); + let output_amount = output.amount().unwrap(); + assert_eq!(output_amount, transfer_amount); + + if external { + // In an external context, decrease MEM[balanceOfStart(MEM[$rD, 32]), 8] by $rC. + assert_eq!( + balances.balance(&ASSET_ID).unwrap(), + balance_of_start - transfer_amount + ); + assert_eq!(final_contract_balance, initial_contract_balance); + } else { + assert_eq!(balances.balance(&ASSET_ID).unwrap(), balance_of_start); + assert_eq!( + final_contract_balance, + initial_contract_balance - transfer_amount + ); + } Ok(()) }