From 6411ae9f8facd7321b8b4a73ff800aad72f3fbff Mon Sep 17 00:00:00 2001 From: xgreenx Date: Mon, 4 Jul 2022 12:30:12 +0100 Subject: [PATCH 1/4] Fixed the bug with transfer value. --- frame/contracts/common/src/lib.rs | 2 + frame/contracts/src/exec.rs | 82 ++++++++++++++++++++++++++++++- 2 files changed, 82 insertions(+), 2 deletions(-) diff --git a/frame/contracts/common/src/lib.rs b/frame/contracts/common/src/lib.rs index f810725afcd36..e33b2735bd159 100644 --- a/frame/contracts/common/src/lib.rs +++ b/frame/contracts/common/src/lib.rs @@ -116,6 +116,8 @@ bitflags! { #[cfg_attr(feature = "std", derive(Serialize, Deserialize))] #[cfg_attr(feature = "std", serde(rename_all = "camelCase", transparent))] pub struct ReturnFlags: u32 { + /// If all bits are zero it is successful case. + const SUCCESS = 0x0; /// If this bit is set all changes made by the contract execution are rolled back. const REVERT = 0x0000_0001; } diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index ec7e1e9335182..230600bbdc7d2 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -797,8 +797,10 @@ where )?; } - // Every call or instantiate also optionally transferres balance. - self.initial_transfer()?; + // Every non delegate call or instantiate also optionally transfers the balance. + if self.top_frame().delegate_caller.is_none() { + self.initial_transfer()?; + } // Call into the wasm blob. let output = executable @@ -1560,6 +1562,82 @@ mod tests { }); } + #[test] + fn correct_transfer_on_call() { + let origin = ALICE; + let dest = BOB; + let value = 55; + + let success_ch = MockLoader::insert(Call, move |ctx, _| { + assert_eq!(ctx.ext.value_transferred(), value); + Ok(ExecReturnValue { flags: ReturnFlags::SUCCESS, data: Bytes(Vec::new()) }) + }); + + ExtBuilder::default().build().execute_with(|| { + let schedule = ::Schedule::get(); + place_contract(&dest, success_ch); + set_balance(&origin, 100); + let balance = get_balance(&dest); + let mut storage_meter = storage::meter::Meter::new(&origin, Some(0), 55).unwrap(); + + let _ = MockStack::run_call( + origin.clone(), + dest.clone(), + &mut GasMeter::::new(GAS_LIMIT), + &mut storage_meter, + &schedule, + value, + vec![], + None, + ) + .unwrap(); + + assert_eq!(get_balance(&origin), 100 - value); + assert_eq!(get_balance(&dest), balance + value); + }); + } + + #[test] + fn correct_transfer_on_delegate_call() { + let origin = ALICE; + let dest = BOB; + let value = 35; + + let success_ch = MockLoader::insert(Call, move |ctx, _| { + assert_eq!(ctx.ext.value_transferred(), value); + Ok(ExecReturnValue { flags: ReturnFlags::SUCCESS, data: Bytes(Vec::new()) }) + }); + + let delegate_ch = MockLoader::insert(Call, move |ctx, _| { + assert_eq!(ctx.ext.value_transferred(), value); + let _ = ctx.ext.delegate_call(success_ch, Vec::new())?; + Ok(ExecReturnValue { flags: ReturnFlags::SUCCESS, data: Bytes(Vec::new()) }) + }); + + ExtBuilder::default().build().execute_with(|| { + let schedule = ::Schedule::get(); + place_contract(&dest, delegate_ch); + set_balance(&origin, 100); + let balance = get_balance(&dest); + let mut storage_meter = storage::meter::Meter::new(&origin, Some(0), 55).unwrap(); + + let _ = MockStack::run_call( + origin.clone(), + dest.clone(), + &mut GasMeter::::new(GAS_LIMIT), + &mut storage_meter, + &schedule, + value, + vec![], + None, + ) + .unwrap(); + + assert_eq!(get_balance(&origin), 100 - value); + assert_eq!(get_balance(&dest), balance + value); + }); + } + #[test] fn changes_are_reverted_on_failing_call() { // This test verifies that changes are reverted on a call which fails (or equally, returns From 9e406a256a84b156eb9c4d307ef3f6260ac93219 Mon Sep 17 00:00:00 2001 From: GreenBaneling | Supercolony Date: Mon, 4 Jul 2022 18:23:20 +0100 Subject: [PATCH 2/4] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Alexander Theißen --- frame/contracts/common/src/lib.rs | 2 -- frame/contracts/src/exec.rs | 6 +++--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/frame/contracts/common/src/lib.rs b/frame/contracts/common/src/lib.rs index e33b2735bd159..f810725afcd36 100644 --- a/frame/contracts/common/src/lib.rs +++ b/frame/contracts/common/src/lib.rs @@ -116,8 +116,6 @@ bitflags! { #[cfg_attr(feature = "std", derive(Serialize, Deserialize))] #[cfg_attr(feature = "std", serde(rename_all = "camelCase", transparent))] pub struct ReturnFlags: u32 { - /// If all bits are zero it is successful case. - const SUCCESS = 0x0; /// If this bit is set all changes made by the contract execution are rolled back. const REVERT = 0x0000_0001; } diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index 230600bbdc7d2..53988d9f7490b 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -1570,7 +1570,7 @@ mod tests { let success_ch = MockLoader::insert(Call, move |ctx, _| { assert_eq!(ctx.ext.value_transferred(), value); - Ok(ExecReturnValue { flags: ReturnFlags::SUCCESS, data: Bytes(Vec::new()) }) + Ok(ExecReturnValue { flags: ReturnFlags::empty(), data: Bytes(Vec::new()) }) }); ExtBuilder::default().build().execute_with(|| { @@ -1605,13 +1605,13 @@ mod tests { let success_ch = MockLoader::insert(Call, move |ctx, _| { assert_eq!(ctx.ext.value_transferred(), value); - Ok(ExecReturnValue { flags: ReturnFlags::SUCCESS, data: Bytes(Vec::new()) }) + Ok(ExecReturnValue { flags: ReturnFlags::empty(), data: Bytes(Vec::new()) }) }); let delegate_ch = MockLoader::insert(Call, move |ctx, _| { assert_eq!(ctx.ext.value_transferred(), value); let _ = ctx.ext.delegate_call(success_ch, Vec::new())?; - Ok(ExecReturnValue { flags: ReturnFlags::SUCCESS, data: Bytes(Vec::new()) }) + Ok(ExecReturnValue { flags: ReturnFlags::empty(), data: Bytes(Vec::new()) }) }); ExtBuilder::default().build().execute_with(|| { From 31de0a86a093bb63c202d20add240d2e67c56fa5 Mon Sep 17 00:00:00 2001 From: xgreenx Date: Mon, 4 Jul 2022 18:29:03 +0100 Subject: [PATCH 3/4] Moved check into `initial_transfer` --- frame/contracts/src/exec.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index 53988d9f7490b..36dce212592fc 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -798,9 +798,7 @@ where } // Every non delegate call or instantiate also optionally transfers the balance. - if self.top_frame().delegate_caller.is_none() { - self.initial_transfer()?; - } + self.initial_transfer()?; // Call into the wasm blob. let output = executable @@ -961,8 +959,14 @@ where // The transfer as performed by a call or instantiate. fn initial_transfer(&self) -> DispatchResult { let frame = self.top_frame(); - let value = frame.value_transferred; + // If it is a delegate call, then we've already transferred tokens in the + // last non-delegate frame. + if frame.delegate_caller.is_some() { + return Ok(()) + } + + let value = frame.value_transferred; Self::transfer(ExistenceRequirement::KeepAlive, self.caller(), &frame.account_id, value) } From 0843408def16ae223521b21b05938b73f15ab9f8 Mon Sep 17 00:00:00 2001 From: xgreenx Date: Mon, 4 Jul 2022 19:31:38 +0100 Subject: [PATCH 4/4] Fmt --- frame/contracts/src/exec.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index 36dce212592fc..d93c646872c6f 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -1594,7 +1594,7 @@ mod tests { vec![], None, ) - .unwrap(); + .unwrap(); assert_eq!(get_balance(&origin), 100 - value); assert_eq!(get_balance(&dest), balance + value); @@ -1635,7 +1635,7 @@ mod tests { vec![], None, ) - .unwrap(); + .unwrap(); assert_eq!(get_balance(&origin), 100 - value); assert_eq!(get_balance(&dest), balance + value);