Merge #1516

1516: Problem (Fix #1515): unbond tx don't subtract fee from bonded r=tomtau a=yihuang

Solution:
- Fix the bug and add unit test


Co-authored-by: yihuang <[email protected]>

chain-abci/src/app/mod.rs

@@ -366,8 +366,9 @@ fn generate_tx_staking_change_event(tx_action: TxAction) -> Option<abci::Event>
             TxPublicAction::Unbond {
                 unbond,
                 unbonded_from,
+                fee,
                 ..
-            } => Some(StakingEvent::Unbond(&unbond.0, unbond.1, unbonded_from).into()),
+            } => Some(StakingEvent::Unbond(&unbond.0, unbond.1, unbonded_from, fee).into()),
             TxPublicAction::NodeJoin(staking_address, council_node) => {
                 Some(StakingEvent::NodeJoin(&staking_address, council_node).into())
             }

chain-abci/src/app/staking_event.rs

@@ -7,10 +7,11 @@ use abci::*;
 use chain_core::common::{TendermintEventKey, TendermintEventType, Timespec};
 use chain_core::init::coin::Coin;
 use chain_core::state::account::{CouncilNode, PunishmentKind, StakedStateAddress};
+use chain_core::tx::fee::Fee;
 
 pub(crate) enum StakingEvent<'a> {
     Deposit(&'a StakedStateAddress, Coin),
-    Unbond(&'a StakedStateAddress, Coin, Timespec),
+    Unbond(&'a StakedStateAddress, Coin, Timespec, Fee),
     Withdraw(&'a StakedStateAddress, Coin),
     NodeJoin(&'a StakedStateAddress, CouncilNode),
     Reward(&'a StakedStateAddress, Coin),
@@ -27,8 +28,8 @@ impl<'a> From<StakingEvent<'a>> for Event {
             StakingEvent::Deposit(staking_address, deposit_amount) => {
                 builder.deposit(staking_address, deposit_amount)
             }
-            StakingEvent::Unbond(staking_address, unbond_amount, unbonded_from) => {
-                builder.unbond(staking_address, unbond_amount, unbonded_from)
+            StakingEvent::Unbond(staking_address, unbond_amount, unbonded_from, fee) => {
+                builder.unbond(staking_address, unbond_amount, unbonded_from, fee)
             }
             StakingEvent::Withdraw(staking_address, withdraw_amount) => {
                 builder.withdraw(staking_address, withdraw_amount)
@@ -85,14 +86,18 @@ impl StakingEventBuilder {
         staking_address: &StakedStateAddress,
         unbond_amount: Coin,
         unbonded_from: Timespec,
+        fee: Fee,
     ) {
         self.attributes
             .push(staking_address_attribute(staking_address));
         self.attributes.push(StakingEventOpType::Unbond.into());
 
         self.attributes.push(
             StakingDiffField(vec![
-                StakingDiff::Bonded(StakingCoinChange::Decrease, unbond_amount),
+                StakingDiff::Bonded(
+                    StakingCoinChange::Decrease,
+                    (unbond_amount + fee.to_coin()).unwrap(),
+                ),
                 StakingDiff::Unbonded(StakingCoinChange::Increase, unbond_amount),
                 StakingDiff::UnbondedFrom(unbonded_from),
             ])
@@ -354,6 +359,7 @@ mod tests {
     use super::*;
     use chain_core::state::account::ConfidentialInit;
     use chain_core::state::tendermint::TendermintValidatorPubKey;
+    use chain_core::tx::fee::Fee;
     use std::str::FromStr;
 
     mod staking_diff_field {
@@ -489,9 +495,13 @@ mod tests {
                 let any_amount = Coin::unit();
                 let any_unbonded_from: Timespec = 1587071014;
 
-                let event: Event =
-                    StakingEvent::Unbond(&any_staking_address, any_amount, any_unbonded_from)
-                        .into();
+                let event: Event = StakingEvent::Unbond(
+                    &any_staking_address,
+                    any_amount,
+                    any_unbonded_from,
+                    Fee::new(Coin::zero()),
+                )
+                .into();
 
                 assert_unbonded_event(event, &any_staking_address, any_amount, any_unbonded_from);
             }

chain-abci/src/staking/mod.rs

@@ -22,6 +22,7 @@ mod tests {
     };
     use chain_core::state::tendermint::{BlockHeight, TendermintValidatorPubKey};
     use chain_core::state::validator::NodeJoinRequestTx;
+    use chain_core::tx::fee::Fee;
     use chain_storage::buffer::{Get, GetStaking, MemStore, StoreStaking};
     use test_common::chain_env::get_init_network_params;
 
@@ -153,7 +154,14 @@ mod tests {
             attributes: Default::default(),
         };
         table
-            .unbond(&mut store, 10, 0, BlockHeight::genesis(), &unbond)
+            .unbond(
+                &mut store,
+                10,
+                0,
+                BlockHeight::genesis(),
+                &unbond,
+                Fee::new(Coin::zero()),
+            )
             .unwrap();
         assert_eq!(
             table.end_block(&store, 3),
@@ -233,7 +241,7 @@ mod tests {
             attributes: Default::default(),
         };
         assert!(matches!(
-            table.unbond(&mut store, 10, 2, 3.into(), &unbond),
+            table.unbond(&mut store, 10, 2, 3.into(), &unbond, Fee::new(Coin::zero())),
             Err(PublicTxError::Unbond(UnbondError::IsJailed))
         ));
         assert!(matches!(
@@ -301,7 +309,9 @@ mod tests {
             value: amount,
             attributes: Default::default(),
         };
-        table.unbond(store, 10, 0, 1.into(), &unbond).unwrap();
+        table
+            .unbond(store, 10, 0, 1.into(), &unbond, Fee::new(Coin::zero()))
+            .unwrap();
         assert_eq!(
             table.end_block(&*store, 3),
             vec![(val_pk.clone(), Coin::zero().into())]
@@ -557,7 +567,16 @@ mod tests {
             value: Coin::new(11_0000_0000).unwrap(),
             attributes: Default::default(),
         };
-        table.unbond(&mut store, 10, 10, 9.into(), &unbond).unwrap();
+        table
+            .unbond(
+                &mut store,
+                10,
+                10,
+                9.into(),
+                &unbond,
+                Fee::new(Coin::zero()),
+            )
+            .unwrap();
 
         assert_eq!(
             table.end_block(&mut store, 3),
@@ -642,7 +661,9 @@ mod tests {
             value: unbond_amount,
             attributes: Default::default(),
         };
-        table.unbond(&mut store, 10, 1, 1.into(), &unbond).unwrap();
+        table
+            .unbond(&mut store, 10, 1, 1.into(), &unbond, Fee::new(Coin::zero()))
+            .unwrap();
         assert_eq!(store.get(&addr1).unwrap().unbonded, unbond_amount);
         let bonded = (bonded - unbond_amount).unwrap();
 
@@ -690,7 +711,9 @@ mod tests {
             value: Coin::new(12_0000_0000).unwrap(),
             attributes: Default::default(),
         };
-        table.unbond(&mut store, 10, 2, 2.into(), &unbond).unwrap();
+        table
+            .unbond(&mut store, 10, 2, 2.into(), &unbond, Fee::new(Coin::zero()))
+            .unwrap();
         assert_eq!(
             table.end_block(&mut store, 3),
             vec![(val_pk2.clone(), Coin::zero().into())]

chain-abci/src/staking/tx.rs

@@ -3,6 +3,7 @@ use chain_core::init::coin::Coin;
 use chain_core::state::account::{StakedStateAddress, UnbondTx, UnjailTx, Validator};
 use chain_core::state::tendermint::{BlockHeight, TendermintValidatorAddress};
 use chain_core::state::validator::NodeJoinRequestTx;
+use chain_core::tx::fee::Fee;
 use chain_storage::buffer::StoreStaking;
 
 use super::table::{set_staking, StakingTable};
@@ -145,6 +146,7 @@ impl StakingTable {
         block_time: Timespec,
         block_height: BlockHeight,
         tx: &UnbondTx,
+        fee: Fee,
     ) -> Result<Timespec, PublicTxError> {
         let mut staking = self.get_or_default(heap, &tx.from_staked_account);
         if tx.nonce != staking.nonce {
@@ -153,12 +155,18 @@ impl StakingTable {
         if staking.is_jailed() {
             return Err(UnbondError::IsJailed.into());
         }
+        let fee_amount = fee.to_coin();
         if tx.value == Coin::zero() {
             return Err(UnbondError::ZeroValue.into());
         }
         let unbonded = (staking.unbonded + tx.value).map_err(UnbondError::CoinError)?;
-        self.sub_bonded(block_time, block_height, tx.value, &mut staking)
-            .map_err(UnbondError::CoinError)?;
+        self.sub_bonded(
+            block_time,
+            block_height,
+            (tx.value + fee_amount).map_err(UnbondError::CoinError)?,
+            &mut staking,
+        )
+        .map_err(UnbondError::CoinError)?;
         staking.unbonded = unbonded;
 
         let unbonded_from = block_time.saturating_add(unbonding_period);

chain-abci/src/storage/mod.rs

@@ -303,6 +303,7 @@ pub fn process_public_tx(
                 chain_info.block_time,
                 chain_info.block_height,
                 &maintx,
+                chain_info.min_fee_computed,
             )?;
 
             Ok(TxPublicAction::unbond(

chain-abci/tests/abci_app.rs

@@ -1029,16 +1029,22 @@ fn all_valid_tx_types_should_commit() {
     );
     let witness4 = StakedStateOpWitness::new(get_ecdsa_witness(&secp, &tx4.id(), &secret_key));
     let unbondtx = TxAux::PublicTx(TxPublicAux::UnbondStakeTx(tx4, witness4));
-    {
+    let bonded = {
         let account = get_account(&addr, &app).expect("account not exist");
         assert_eq!(account.unbonded, Coin::zero());
         assert_eq!(account.nonce, 2);
-    }
+        account.bonded
+    };
     block_commit(&mut app, unbondtx, 6);
     {
         let account = get_account(&addr, &app).expect("account not exist");
         assert_eq!(account.unbonded, Coin::unit());
         assert_eq!(account.nonce, 3);
+        // fee is non zero
+        assert!(
+            account.bonded < (bonded - Coin::unit()).unwrap(),
+            "bonded should subtract fee amount"
+        );
     }
 }
 

client-network/src/network_ops/default_network_ops_client.rs

@@ -230,13 +230,6 @@ where
             )
         })?;
 
-        if staked_state.bonded < value {
-            return Err(Error::new(
-                ErrorKind::InvalidInput,
-                "Staking account does not have enough coins to unbond (synchronizing your wallet may help)",
-            ));
-        }
-
         let nonce = staked_state.nonce;
 
         let transaction = UnbondTx::new(address, nonce, value, attributes);
@@ -257,10 +250,26 @@ where
 
         let signature = sign_key.sign(&tx).map(StakedStateOpWitness::new)?;
 
-        Ok(TxAux::PublicTx(TxPublicAux::UnbondStakeTx(
-            transaction,
-            signature,
-        )))
+        let txaux = TxAux::PublicTx(TxPublicAux::UnbondStakeTx(transaction, signature));
+
+        let fee = self
+            .fee_algorithm
+            .calculate_for_txaux(&txaux)
+            .chain(|| {
+                (
+                    ErrorKind::IllegalInput,
+                    "Calculated fee is more than the maximum allowed value",
+                )
+            })?
+            .to_coin();
+        if staked_state.bonded < (value + fee).unwrap() {
+            return Err(Error::new(
+                ErrorKind::InvalidInput,
+                "Staking account does not have enough coins to unbond (synchronizing your wallet may help)",
+            ));
+        }
+
+        Ok(txaux)
     }
 
     fn create_withdraw_unbonded_stake_transaction(