diff --git a/crates/storage/src/lazy/lazy_cell.rs b/crates/storage/src/lazy/lazy_cell.rs index a09c4afea70..ea6ef5909c0 100644 --- a/crates/storage/src/lazy/lazy_cell.rs +++ b/crates/storage/src/lazy/lazy_cell.rs @@ -523,7 +523,7 @@ mod tests { // We prevent the intermediate instance from clearing the storage preemtively by wrapping // it inside `ManuallyDrop`. The third step will clean up the same storage region afterwards. // - // We explicitely do not touch or assert the value of `pulled_pair.0` in order to trigger + // We explicitly do not touch or assert the value of `pulled_pair.0` in order to trigger // the bug. let pulled_pair: (LazyCell, i32) = SpreadLayout::pull_spread(&mut KeyPtr::from(root_key)); @@ -545,4 +545,57 @@ mod tests { Ok(()) }) } + + #[test] + fn regression_test_for_issue_570() -> ink_env::Result<()> { + run_test::(|_| { + let root_key = Key::from([0x00; 32]); + { + // Step 1: Push two valid values one after the other to contract storage. + // The first value needs to be an `Option::None` value, since the bug was + // then messing up following pointers. + let v1: Option = None; + let v2: u32 = 13; + let mut ptr = KeyPtr::from(root_key); + + SpreadLayout::push_spread(&v1, &mut ptr); + SpreadLayout::push_spread(&v2, &mut ptr); + } + { + // Step 2: Pull the values from the step before. + // + // 1. Change the first values `None` to `Some(...)`. + // 2. Push the first value again to contract storage. + // + // We prevent the intermediate instance from clearing the storage preemptively + // by wrapping it inside `ManuallyDrop`. The third step will clean up the same + // storage region afterwards. + let mut ptr = KeyPtr::from(root_key); + let pulled_v1: Option = SpreadLayout::pull_spread(&mut ptr); + let mut pulled_v1 = core::mem::ManuallyDrop::new(pulled_v1); + + let pulled_v2: u32 = SpreadLayout::pull_spread(&mut ptr); + let pulled_v2 = core::mem::ManuallyDrop::new(pulled_v2); + + assert_eq!(*pulled_v1, None); + assert_eq!(*pulled_v2, 13); + + *pulled_v1 = Some(99u32); + SpreadLayout::push_spread(&*pulled_v1, &mut KeyPtr::from(root_key)); + } + { + // Step 3: Pull the values again from the storage. + // + // If the bug with `Option` has been fixed in PR #520 we must be able to inspect + // the correct values for both entries. + let mut ptr = KeyPtr::from(root_key); + let pulled_v1: Option = SpreadLayout::pull_spread(&mut ptr); + let pulled_v2: u32 = SpreadLayout::pull_spread(&mut ptr); + + assert_eq!(pulled_v1, Some(99)); + assert_eq!(pulled_v2, 13); + } + Ok(()) + }) + } } diff --git a/crates/storage/src/traits/impls/prims.rs b/crates/storage/src/traits/impls/prims.rs index b1dfa3a11ad..387afcc1e3b 100644 --- a/crates/storage/src/traits/impls/prims.rs +++ b/crates/storage/src/traits/impls/prims.rs @@ -66,6 +66,8 @@ where ::push_spread(&(self.is_some() as u8), ptr); if let Some(value) = self { ::push_spread(value, ptr); + } else { + ptr.advance_by(::FOOTPRINT); } } @@ -76,12 +78,17 @@ where ::clear_spread(&0, ptr); if let Some(value) = self { ::clear_spread(value, ptr) + } else { + ptr.advance_by(::FOOTPRINT); } } fn pull_spread(ptr: &mut KeyPtr) -> Self { match ::pull_spread(ptr) { - 0u8 => None, + 0u8 => { + ptr.advance_by(::FOOTPRINT); + None + } 1u8 => Some(::pull_spread(ptr)), _ => unreachable!("invalid Option discriminant"), } diff --git a/examples/multisig_plain/lib.rs b/examples/multisig_plain/lib.rs index f80a5243b53..3389fa368af 100755 --- a/examples/multisig_plain/lib.rs +++ b/examples/multisig_plain/lib.rs @@ -155,6 +155,14 @@ mod multisig_plain { pub gas_limit: u64, } + /// Errors that can occur upon calling this contract. + #[derive(Copy, Clone, Debug, PartialEq, Eq, scale::Encode, scale::Decode)] + #[cfg_attr(feature = "std", derive(::scale_info::TypeInfo))] + pub enum Error { + /// Returned if the call failed. + TransactionFailed, + } + #[ink(storage)] pub struct MultisigPlain { /// Every entry in this map represents the confirmation of an owner for a @@ -227,7 +235,7 @@ mod multisig_plain { /// the output in bytes. The Option is `None` when the transaction was executed through /// `invoke_transaction` rather than `evaluate_transaction`. #[ink(topic)] - result: Result>, ()>, + result: Result>, Error>, } /// Emitted when an owner is added to the wallet. @@ -479,7 +487,10 @@ mod multisig_plain { /// Its return value indicates whether the called transaction was successful. /// This can be called by anyone. #[ink(message)] - pub fn invoke_transaction(&mut self, trans_id: TransactionId) -> Result<(), ()> { + pub fn invoke_transaction( + &mut self, + trans_id: TransactionId, + ) -> Result<(), Error> { self.ensure_confirmed(trans_id); let t = self.take_transaction(trans_id).expect(WRONG_TRANSACTION_ID); let result = build_call::<::Env>() @@ -491,7 +502,7 @@ mod multisig_plain { ) .returns::<()>() .fire() - .map_err(|_| ()); + .map_err(|_| Error::TransactionFailed); self.env().emit_event(Execution { transaction: trans_id, result: result.map(|_| None), @@ -502,13 +513,13 @@ mod multisig_plain { /// Evaluate a confirmed execution and return its output as bytes. /// /// Its return value indicates whether the called transaction was successful and contains - /// its output when sucesful. + /// its output when successful. /// This can be called by anyone. #[ink(message)] pub fn eval_transaction( &mut self, trans_id: TransactionId, - ) -> Result, ()> { + ) -> Result, Error> { self.ensure_confirmed(trans_id); let t = self.take_transaction(trans_id).expect(WRONG_TRANSACTION_ID); let result = build_call::<::Env>() @@ -520,7 +531,7 @@ mod multisig_plain { ) .returns::>>() .fire() - .map_err(|_| ()); + .map_err(|_| Error::TransactionFailed); self.env().emit_event(Execution { transaction: trans_id, result: result.clone().map(Some),