From 912fb00dde9c794ccc3018e4f0872d01c01426e9 Mon Sep 17 00:00:00 2001 From: hunjixin <1084400399@qq.com> Date: Tue, 12 Jul 2022 14:10:37 +0800 Subject: [PATCH 1/6] feat:implement beneficiary fip0029 --- actors/miner/src/beneficiary.rs | 87 +++++++++++++ actors/miner/src/lib.rs | 218 ++++++++++++++++++++++++++++---- actors/miner/src/state.rs | 10 ++ actors/miner/src/types.rs | 30 +++++ 4 files changed, 323 insertions(+), 22 deletions(-) create mode 100644 actors/miner/src/beneficiary.rs diff --git a/actors/miner/src/beneficiary.rs b/actors/miner/src/beneficiary.rs new file mode 100644 index 000000000..6a26a1d44 --- /dev/null +++ b/actors/miner/src/beneficiary.rs @@ -0,0 +1,87 @@ +use fvm_ipld_encoding::tuple::*; +use fvm_ipld_encoding::Cbor; +use fvm_shared::address::Address; +use fvm_shared::bigint::bigint_ser; +use fvm_shared::clock::ChainEpoch; +use fvm_shared::econ::TokenAmount; +use std::cmp::max; + +#[derive(Debug, PartialEq, Serialize_tuple, Deserialize_tuple)] +pub struct BeneficiaryTerm { + // Quota: The total amount the current beneficiary can withdraw. Monotonic, but reset when beneficiary changes. + #[serde(with = "bigint_ser")] + pub quota: TokenAmount, + // UsedQuota: The amount of quota the current beneficiary has already withdrawn + #[serde(with = "bigint_ser")] + pub used_quota: TokenAmount, + // Expiration: The epoch at which the beneficiary's rights expire and revert to the owner + pub expiration: ChainEpoch, +} + +impl Cbor for BeneficiaryTerm {} + +impl BeneficiaryTerm { + pub fn default() -> BeneficiaryTerm { + BeneficiaryTerm { + quota: TokenAmount::default(), + expiration: 0, + used_quota: TokenAmount::default(), + } + } + + pub fn new( + quota: TokenAmount, + used_quota: TokenAmount, + expiration: ChainEpoch, + ) -> BeneficiaryTerm { + BeneficiaryTerm { quota, expiration, used_quota } + } + + // IsUsedUp check whether beneficiary has use up all quota + pub fn is_used_up(&self) -> bool { + self.used_quota >= self.quota + } + + // IsExpire check if the beneficiary is within the validity period + pub fn is_expire(&self, cur: ChainEpoch) -> bool { + self.expiration <= cur + } + + // Available get the amount that the beneficiary has not yet withdrawn + pub fn available(&self, cur: ChainEpoch) -> TokenAmount { + // Return 0 when the usedQuota > Quota for safe + if self.is_expire(cur) { + TokenAmount::default() + } else { + max(self.quota.clone() - self.used_quota.clone(), TokenAmount::default()) + } + } +} + +#[derive(Debug, PartialEq, Serialize_tuple, Deserialize_tuple)] +pub struct PendingBeneficiaryChange { + pub new_beneficiary: Address, + #[serde(with = "bigint_ser")] + pub new_quota: TokenAmount, + pub new_expiration: ChainEpoch, + pub approved_by_beneficiary: bool, + pub approved_by_nominee: bool, +} + +impl Cbor for PendingBeneficiaryChange {} + +impl PendingBeneficiaryChange { + pub fn new( + new_beneficiary: Address, + new_quota: TokenAmount, + new_expiration: ChainEpoch, + ) -> Self { + PendingBeneficiaryChange { + new_beneficiary, + new_quota, + new_expiration, + approved_by_beneficiary: false, + approved_by_nominee: false, + } + } +} diff --git a/actors/miner/src/lib.rs b/actors/miner/src/lib.rs index 98e963600..294563863 100644 --- a/actors/miner/src/lib.rs +++ b/actors/miner/src/lib.rs @@ -53,6 +53,7 @@ pub use state::*; pub use termination::*; pub use types::*; pub use vesting_state::*; +pub use beneficiary::*; use crate::commd::{is_unsealed_sector, CompactCommD}; use crate::Code::Blake2b256; @@ -60,6 +61,7 @@ use crate::Code::Blake2b256; #[cfg(feature = "fil-actor")] fil_actors_runtime::wasm_trampoline!(Actor); +mod beneficiary; mod bitfield_queue; mod commd; mod deadline_assignment; @@ -118,6 +120,8 @@ pub enum Method { ProveReplicaUpdates = 27, PreCommitSectorBatch2 = 28, ProveReplicaUpdates2 = 29, + ChangeBeneficiary =30, + GetBeneficiary =31, } pub const ERR_BALANCE_INVARIANTS_BROKEN: ExitCode = ExitCode::new(1000); @@ -314,6 +318,15 @@ impl Actor { new_address )); } + + // Change beneficiary address to new owner if current beneficiary address equal to old owner address + if info.beneficiary == info.owner { + info.beneficiary = pending_address; + } + // Cancel pending beneficiary term change when the owner changes + info.pending_beneficiary_term = None; + + // Set the new owner address info.owner = pending_address; } @@ -3235,9 +3248,9 @@ impl Actor { )); } - let (info, newly_vested, fee_to_burn, available_balance, state) = + let (info, amount_withdrawn, newly_vested, fee_to_burn, state) = rt.transaction(|state: &mut State, rt| { - let info = get_miner_info(rt.store(), state)?; + let mut info = get_miner_info(rt.store(), state)?; // Only the owner is allowed to withdraw the balance as it belongs to/is controlled by the owner // and not the worker. @@ -3273,29 +3286,40 @@ impl Actor { // Verify unlocked funds cover both InitialPledgeRequirement and FeeDebt // and repay fee debt now. let fee_to_burn = repay_debts_or_abort(rt, state)?; - - Ok((info, newly_vested, fee_to_burn, available_balance, state.clone())) + let mut amount_withdrawn = std::cmp::min(&available_balance, ¶ms.amount_requested); + if amount_withdrawn.is_negative() { + return Err(actor_error!( + illegal_state, + "negative amount to withdraw: {}", + amount_withdrawn + )); + } + if info.beneficiary != info.owner { + let remaining_quota = info.beneficiary_term.available(rt.curr_epoch()); + if !remaining_quota.is_positive() { + return Err(actor_error!( + forbidden, + "beneficiary{} quota {} used quota {} expiration epoch {} current epoch {}", + info.beneficiary, + info.beneficiary_term.quota, + info.beneficiary_term.used_quota, + info.beneficiary_term.expiration, + rt.curr_epoch() + )); + } + amount_withdrawn = std::cmp::min(amount_withdrawn, &remaining_quota); + info.beneficiary_term.used_quota = info.beneficiary_term.used_quota + amount_withdrawn; + state.save_info(rt.store(), &info).map_err(|e| { + e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "failed to save miner info") + })?; + Ok((info, amount_withdrawn.clone(), newly_vested, fee_to_burn, state.clone())) + }else{ + Ok((info, amount_withdrawn.clone(), newly_vested, fee_to_burn, state.clone())) + } })?; - let amount_withdrawn = std::cmp::min(&available_balance, ¶ms.amount_requested); - if amount_withdrawn.is_negative() { - return Err(actor_error!( - illegal_state, - "negative amount to withdraw: {}", - amount_withdrawn - )); - } - if amount_withdrawn > &available_balance { - return Err(actor_error!( - illegal_state, - "amount to withdraw {} < available {}", - amount_withdrawn, - available_balance - )); - } - if amount_withdrawn.is_positive() { - rt.send(info.owner, METHOD_SEND, RawBytes::default(), amount_withdrawn.clone())?; + rt.send(info.beneficiary, METHOD_SEND, RawBytes::default(), amount_withdrawn.clone())?; } burn_funds(rt, fee_to_burn)?; @@ -3305,6 +3329,148 @@ impl Actor { Ok(WithdrawBalanceReturn { amount_withdrawn: amount_withdrawn.clone() }) } + /// Proposes or confirms a change of owner address. + /// If invoked by the current owner, proposes a new owner address for confirmation. If the proposed address is the + /// current owner address, revokes any existing proposal. + /// If invoked by the previously proposed address, with the same proposal, changes the current owner address to be + /// that proposed address. + fn change_beneficiary( + rt: &mut RT, + params: ChangeBeneficiaryParams, + ) -> Result<(), ActorError> + where + BS: Blockstore, + RT: Runtime, + { + let new_beneficiary = rt.resolve_address(¶ms.new_beneficiary).ok_or_else(|| { + actor_error!(illegal_argument, "unable to resolve address: {}", params.new_beneficiary) + })?; + + rt.transaction(|state: &mut State, rt| { + let mut info = get_miner_info(rt.store(), state)?; + if rt.message().caller() == info.owner { + // This is a ChangeBeneficiary proposal when the caller is Owner + if new_beneficiary != info.owner { + // When beneficiary is not owner, just check quota in params, + // Expiration maybe an expiration value, but wouldn't cause problem, just the new beneficiary never get any benefit + if !params.new_quota.is_positive() { + return Err(actor_error!( + illegal_argument, + "beneficial quota {} must bigger than zero", + params.new_quota + )); + } + } else { + // Expiration/quota must set to 0 while change beneficiary to owner + if !params.new_quota.is_zero() { + return Err(actor_error!( + illegal_argument, + "owner beneficial quota {} must be zero", + params.new_quota + )); + } + + if params.new_expiration != 0 { + return Err(actor_error!( + illegal_argument, + "owner beneficial expiration {} must be zero", + params.new_expiration + )); + } + } + + let mut pending_beneficiary_term = PendingBeneficiaryChange::new( + new_beneficiary, + params.new_quota, + params.new_expiration, + ); + if info.beneficiary_term.available(rt.curr_epoch()).is_zero() { + // Set current beneficiary to approved when current beneficiary is not effective + pending_beneficiary_term.approved_by_beneficiary = true; + } + info.pending_beneficiary_term = Some(pending_beneficiary_term); + } else { + if info.pending_beneficiary_term.is_none() { + return Err(actor_error!(forbidden, "No changeBeneficiary proposal exists")); + } + if let Some(pending_term) = &info.pending_beneficiary_term { + if pending_term.new_beneficiary != new_beneficiary { + return Err(actor_error!( + forbidden, + "new beneficiary address must be equal expect {}, but got {}", + pending_term.new_beneficiary, + params.new_beneficiary + )); + } + if pending_term.new_quota != params.new_quota { + return Err(actor_error!( + forbidden, + "new beneficiary quota must be equal expect {}, but got {}", + pending_term.new_quota, + params.new_quota + )); + } + if pending_term.new_expiration != params.new_expiration { + return Err(actor_error!( + forbidden, + "new beneficiary expire date must be equal expect {}, but got {}", + pending_term.new_expiration, + params.new_expiration + )); + } + } + } + + if let Some(pending_term) = info.pending_beneficiary_term.as_mut() { + if rt.message().caller() == info.beneficiary { + pending_term.approved_by_beneficiary = true + } + + if rt.message().caller() == new_beneficiary { + pending_term.approved_by_nominee = true + } + + if pending_term.approved_by_beneficiary && pending_term.approved_by_nominee { + //approved by both beneficiary and nominee + info.beneficiary = new_beneficiary; + info.beneficiary_term.quota = pending_term.new_quota.clone(); + info.beneficiary_term.expiration = pending_term.new_expiration; + if new_beneficiary != info.beneficiary { + info.beneficiary_term.used_quota = TokenAmount::default(); + } + // Clear the pending proposal + info.pending_beneficiary_term = None; + } + } + + state.save_info(rt.store(), &info).map_err(|e| { + e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "failed to save miner info") + })?; + Ok(()) + }) + } + + // GetBeneficiary retrieves the currently active and proposed beneficiary information. + // This method is for use by other actors (such as those acting as beneficiaries), + // and to abstract the state representation for clients. + fn get_beneficiary(rt: &mut RT) -> Result + where + BS: Blockstore, + RT: Runtime, + { + rt.validate_immediate_caller_accept_any()?; + let info = + rt.transaction(|state: &mut State, rt| Ok(get_miner_info(rt.store(), &state)?))?; + + Ok(GetBeneficiaryReturn { + active: ActiveBeneficiary { + beneficiary: info.beneficiary, + term: info.beneficiary_term, + }, + proposed: info.pending_beneficiary_term, + }) + } + fn repay_debt(rt: &mut RT) -> Result<(), ActorError> where BS: Blockstore, @@ -4689,6 +4855,14 @@ impl ActorCode for Actor { let res = Self::prove_replica_updates2(rt, cbor::deserialize_params(params)?)?; Ok(RawBytes::serialize(res)?) } + Some(Method::ChangeBeneficiary) => { + Self::change_beneficiary(rt, cbor::deserialize_params(params)?)?; + Ok(RawBytes::default()) + } + Some(Method::GetBeneficiary) => { + let res = Self::get_beneficiary(rt)?; + Ok(RawBytes::serialize(res)?) + } None => Err(actor_error!(unhandled_message, "Invalid method")), } } diff --git a/actors/miner/src/state.rs b/actors/miner/src/state.rs index ce91e4110..3c542ddfd 100644 --- a/actors/miner/src/state.rs +++ b/actors/miner/src/state.rs @@ -30,6 +30,7 @@ use num_traits::{Signed, Zero}; use super::deadlines::new_deadline_info; use super::policy::*; use super::types::*; +use super::beneficiary::*; use super::{ assign_deadlines, deadline_is_mutable, new_deadline_info_from_offset_and_epoch, quant_spec_for_deadline, BitFieldQueue, Deadline, DeadlineInfo, DeadlineSectorMap, Deadlines, @@ -1230,6 +1231,12 @@ pub struct MinerInfo { /// Optional worker key to update at an epoch pub pending_worker_key: Option, + // Beneficiary account for receive miner benefits, withdraw on miner must send to this address, + // beneficiary set owner address by default when create miner + pub beneficiary: Address, + pub beneficiary_term: BeneficiaryTerm, + pub pending_beneficiary_term: Option, + /// Libp2p identity that should be used when connecting to this miner #[serde(with = "serde_bytes")] pub peer_id: Vec, @@ -1278,6 +1285,9 @@ impl MinerInfo { worker, control_addresses, pending_worker_key: None, + beneficiary: owner, + beneficiary_term: BeneficiaryTerm::default(), + pending_beneficiary_term: None, peer_id, multi_address, window_post_proof_type, diff --git a/actors/miner/src/types.rs b/actors/miner/src/types.rs index f0b5a2c98..e36f3efd8 100644 --- a/actors/miner/src/types.rs +++ b/actors/miner/src/types.rs @@ -2,6 +2,8 @@ // SPDX-License-Identifier: Apache-2.0, MIT use crate::commd::CompactCommD; +use crate::beneficiary_term::BeneficiaryTerm; +use crate::PendingBeneficiaryChange; use cid::Cid; use fil_actors_runtime::DealWeight; use fvm_ipld_bitfield::UnvalidatedBitField; @@ -18,6 +20,7 @@ use fvm_shared::sector::{ StoragePower, }; use fvm_shared::smooth::FilterEstimate; +use super::beneficiary::*; pub type CronEvent = i64; @@ -407,3 +410,30 @@ pub struct ProveReplicaUpdatesParams2 { } impl Cbor for ProveReplicaUpdatesParams2 {} + + +#[derive(Debug, Serialize_tuple, Deserialize_tuple)] +pub struct ChangeBeneficiaryParams { + pub new_beneficiary: Address, + #[serde(with = "bigint_ser")] + pub new_quota: TokenAmount, + pub new_expiration: ChainEpoch, +} + +impl Cbor for ChangeBeneficiaryParams {} + +#[derive(Debug, Serialize_tuple, Deserialize_tuple)] +pub struct ActiveBeneficiary { + pub beneficiary: Address, + pub term: BeneficiaryTerm, +} + +impl Cbor for ActiveBeneficiary {} + +#[derive(Debug, Serialize_tuple, Deserialize_tuple)] +pub struct GetBeneficiaryReturn { + pub active: ActiveBeneficiary, + pub proposed: Option, +} + +impl Cbor for GetBeneficiaryReturn {} From 27d4ebd170a034ef5889a5b079f78b3a3f9c7f9e Mon Sep 17 00:00:00 2001 From: hunjixin <1084400399@qq.com> Date: Mon, 25 Jul 2022 16:27:36 +0800 Subject: [PATCH 2/6] test: add test for beneficiary --- actors/miner/src/beneficiary.rs | 35 +- actors/miner/src/lib.rs | 24 +- actors/miner/src/state.rs | 19 +- actors/miner/src/types.rs | 4 +- actors/miner/tests/change_beneficiary_test.rs | 411 ++++++++++++++++++ .../miner/tests/change_owner_address_test.rs | 1 + actors/miner/tests/util.rs | 102 ++++- actors/miner/tests/withdraw_balance.rs | 101 ++++- 8 files changed, 642 insertions(+), 55 deletions(-) create mode 100644 actors/miner/tests/change_beneficiary_test.rs diff --git a/actors/miner/src/beneficiary.rs b/actors/miner/src/beneficiary.rs index 6a26a1d44..0bfd6d2b9 100644 --- a/actors/miner/src/beneficiary.rs +++ b/actors/miner/src/beneficiary.rs @@ -4,17 +4,18 @@ use fvm_shared::address::Address; use fvm_shared::bigint::bigint_ser; use fvm_shared::clock::ChainEpoch; use fvm_shared::econ::TokenAmount; -use std::cmp::max; +use num_traits::Zero; +use std::ops::Sub; -#[derive(Debug, PartialEq, Serialize_tuple, Deserialize_tuple)] +#[derive(Debug, PartialEq, Clone, Serialize_tuple, Deserialize_tuple)] pub struct BeneficiaryTerm { - // Quota: The total amount the current beneficiary can withdraw. Monotonic, but reset when beneficiary changes. + /// The total amount the current beneficiary can withdraw. Monotonic, but reset when beneficiary changes. #[serde(with = "bigint_ser")] pub quota: TokenAmount, - // UsedQuota: The amount of quota the current beneficiary has already withdrawn + /// The amount of quota the current beneficiary has already withdrawn #[serde(with = "bigint_ser")] pub used_quota: TokenAmount, - // Expiration: The epoch at which the beneficiary's rights expire and revert to the owner + /// The epoch at which the beneficiary's rights expire and revert to the owner pub expiration: ChainEpoch, } @@ -23,9 +24,9 @@ impl Cbor for BeneficiaryTerm {} impl BeneficiaryTerm { pub fn default() -> BeneficiaryTerm { BeneficiaryTerm { - quota: TokenAmount::default(), + quota: TokenAmount::zero(), expiration: 0, - used_quota: TokenAmount::default(), + used_quota: TokenAmount::zero(), } } @@ -37,23 +38,13 @@ impl BeneficiaryTerm { BeneficiaryTerm { quota, expiration, used_quota } } - // IsUsedUp check whether beneficiary has use up all quota - pub fn is_used_up(&self) -> bool { - self.used_quota >= self.quota - } - - // IsExpire check if the beneficiary is within the validity period - pub fn is_expire(&self, cur: ChainEpoch) -> bool { - self.expiration <= cur - } - - // Available get the amount that the beneficiary has not yet withdrawn + /// get the amount that the beneficiary has not yet withdrawn pub fn available(&self, cur: ChainEpoch) -> TokenAmount { // Return 0 when the usedQuota > Quota for safe - if self.is_expire(cur) { - TokenAmount::default() - } else { - max(self.quota.clone() - self.used_quota.clone(), TokenAmount::default()) + if self.expiration > cur { + (&self.quota).sub(&self.used_quota).max(TokenAmount::zero()) + }else{ + TokenAmount::zero() } } } diff --git a/actors/miner/src/lib.rs b/actors/miner/src/lib.rs index 294563863..f0699098b 100644 --- a/actors/miner/src/lib.rs +++ b/actors/miner/src/lib.rs @@ -3254,7 +3254,7 @@ impl Actor { // Only the owner is allowed to withdraw the balance as it belongs to/is controlled by the owner // and not the worker. - rt.validate_immediate_caller_is(&[info.owner])?; + rt.validate_immediate_caller_is(&[info.owner, info.beneficiary])?; // Ensure we don't have any pending terminations. if !state.early_terminations.is_empty() { @@ -3299,7 +3299,7 @@ impl Actor { if !remaining_quota.is_positive() { return Err(actor_error!( forbidden, - "beneficiary{} quota {} used quota {} expiration epoch {} current epoch {}", + "quota not enough beneficiary {} quota {} used quota {} expiration epoch {} current epoch {}", info.beneficiary, info.beneficiary_term.quota, info.beneficiary_term.used_quota, @@ -3390,13 +3390,10 @@ impl Actor { } info.pending_beneficiary_term = Some(pending_beneficiary_term); } else { - if info.pending_beneficiary_term.is_none() { - return Err(actor_error!(forbidden, "No changeBeneficiary proposal exists")); - } if let Some(pending_term) = &info.pending_beneficiary_term { if pending_term.new_beneficiary != new_beneficiary { return Err(actor_error!( - forbidden, + illegal_argument, "new beneficiary address must be equal expect {}, but got {}", pending_term.new_beneficiary, params.new_beneficiary @@ -3404,7 +3401,7 @@ impl Actor { } if pending_term.new_quota != params.new_quota { return Err(actor_error!( - forbidden, + illegal_argument, "new beneficiary quota must be equal expect {}, but got {}", pending_term.new_quota, params.new_quota @@ -3412,12 +3409,14 @@ impl Actor { } if pending_term.new_expiration != params.new_expiration { return Err(actor_error!( - forbidden, + illegal_argument, "new beneficiary expire date must be equal expect {}, but got {}", pending_term.new_expiration, params.new_expiration )); } + } else { + return Err(actor_error!(forbidden, "No changeBeneficiary proposal exists")); } } @@ -3432,13 +3431,14 @@ impl Actor { if pending_term.approved_by_beneficiary && pending_term.approved_by_nominee { //approved by both beneficiary and nominee + if new_beneficiary != info.beneficiary { + //if beneficiary changes, reset used_quota to zero + info.beneficiary_term.used_quota = TokenAmount::zero(); + } info.beneficiary = new_beneficiary; info.beneficiary_term.quota = pending_term.new_quota.clone(); info.beneficiary_term.expiration = pending_term.new_expiration; - if new_beneficiary != info.beneficiary { - info.beneficiary_term.used_quota = TokenAmount::default(); - } - // Clear the pending proposal + // clear the pending proposal info.pending_beneficiary_term = None; } } diff --git a/actors/miner/src/state.rs b/actors/miner/src/state.rs index 3c542ddfd..2cec75857 100644 --- a/actors/miner/src/state.rs +++ b/actors/miner/src/state.rs @@ -27,10 +27,10 @@ use fvm_shared::sector::{RegisteredPoStProof, SectorNumber, SectorSize, MAX_SECT use fvm_shared::HAMT_BIT_WIDTH; use num_traits::{Signed, Zero}; +use super::beneficiary::*; use super::deadlines::new_deadline_info; use super::policy::*; use super::types::*; -use super::beneficiary::*; use super::{ assign_deadlines, deadline_is_mutable, new_deadline_info_from_offset_and_epoch, quant_spec_for_deadline, BitFieldQueue, Deadline, DeadlineInfo, DeadlineSectorMap, Deadlines, @@ -1231,12 +1231,6 @@ pub struct MinerInfo { /// Optional worker key to update at an epoch pub pending_worker_key: Option, - // Beneficiary account for receive miner benefits, withdraw on miner must send to this address, - // beneficiary set owner address by default when create miner - pub beneficiary: Address, - pub beneficiary_term: BeneficiaryTerm, - pub pending_beneficiary_term: Option, - /// Libp2p identity that should be used when connecting to this miner #[serde(with = "serde_bytes")] pub peer_id: Vec, @@ -1261,6 +1255,17 @@ pub struct MinerInfo { /// A proposed new owner account for this miner. /// Must be confirmed by a message from the pending address itself. pub pending_owner_address: Option
, + + /// Account for receive miner benefits, withdraw on miner must send to this address, + /// set owner address by default when create miner + pub beneficiary: Address, + + /// beneficiary's total quota, how much quota has been withdraw, + /// and when this beneficiary expired + pub beneficiary_term: BeneficiaryTerm, + + /// A proposal new beneficiary message for this miner + pub pending_beneficiary_term: Option, } impl MinerInfo { diff --git a/actors/miner/src/types.rs b/actors/miner/src/types.rs index e36f3efd8..2847231fa 100644 --- a/actors/miner/src/types.rs +++ b/actors/miner/src/types.rs @@ -2,8 +2,7 @@ // SPDX-License-Identifier: Apache-2.0, MIT use crate::commd::CompactCommD; -use crate::beneficiary_term::BeneficiaryTerm; -use crate::PendingBeneficiaryChange; +use super::beneficiary::*; use cid::Cid; use fil_actors_runtime::DealWeight; use fvm_ipld_bitfield::UnvalidatedBitField; @@ -20,7 +19,6 @@ use fvm_shared::sector::{ StoragePower, }; use fvm_shared::smooth::FilterEstimate; -use super::beneficiary::*; pub type CronEvent = i64; diff --git a/actors/miner/tests/change_beneficiary_test.rs b/actors/miner/tests/change_beneficiary_test.rs new file mode 100644 index 000000000..f4f769595 --- /dev/null +++ b/actors/miner/tests/change_beneficiary_test.rs @@ -0,0 +1,411 @@ +use fil_actor_miner::BeneficiaryTerm; +use fil_actors_runtime::test_utils::{expect_abort, MockRuntime}; +use fvm_shared::clock::ChainEpoch; +use fvm_shared::{address::Address, econ::TokenAmount, error::ExitCode}; +use num_traits::Zero; + +mod util; +use util::*; + +fn setup() -> (ActorHarness, MockRuntime) { + let big_balance = 20u128.pow(23); + let period_offset = 100; + + let h = ActorHarness::new(period_offset); + let mut rt = h.new_runtime(); + h.construct_and_verify(&mut rt); + rt.balance.replace(TokenAmount::from(big_balance)); + + (h, rt) +} + +#[test] +fn successfully_change_owner_to_another_address_two_message() { + let (mut h, mut rt) = setup(); + let first_beneficiary_id = Address::new_id(999); + + let beneficiary_change = + BeneficiaryChange::new(first_beneficiary_id, TokenAmount::from(100), ChainEpoch::from(200)); + // proposal beneficiary change + h.change_beneficiary(&mut rt, h.owner, beneficiary_change.clone(), None).unwrap(); + // assert change has been made in state + let mut info = h.get_info(&mut rt); + let pending_beneficiary_term = info.pending_beneficiary_term.unwrap(); + assert_eq!(beneficiary_change.expiration, pending_beneficiary_term.new_expiration); + assert_eq!(beneficiary_change.quota.clone(), pending_beneficiary_term.new_quota); + assert_eq!(first_beneficiary_id, pending_beneficiary_term.new_beneficiary); + + //confirm proposal + h.change_beneficiary( + &mut rt, + first_beneficiary_id, + beneficiary_change.clone(), + Some(first_beneficiary_id), + ) + .unwrap(); + + info = h.get_info(&mut rt); + assert_eq!(None, info.pending_beneficiary_term); + assert_eq!(first_beneficiary_id, info.beneficiary); + assert_eq!(beneficiary_change.quota.clone(), info.beneficiary_term.quota); + assert_eq!(beneficiary_change.expiration, info.beneficiary_term.expiration); + + h.check_state(&rt); +} + +#[test] +fn successfully_change_from_not_owner_beneficiary_to_another_address_three_message() { + let (mut h, mut rt) = setup(); + let first_beneficiary_id = Address::new_id(999); + let second_beneficiary_id = Address::new_id(1001); + + let first_beneficiary_term = + BeneficiaryTerm::new(TokenAmount::from(100), TokenAmount::zero(), ChainEpoch::from(200)); + h.propose_approve_initial_beneficiary(&mut rt, first_beneficiary_id, first_beneficiary_term) + .unwrap(); + + let second_beneficiary_change = BeneficiaryChange::new( + second_beneficiary_id, + TokenAmount::from(101), + ChainEpoch::from(201), + ); + h.change_beneficiary(&mut rt, h.owner, second_beneficiary_change.clone(), None).unwrap(); + let mut info = h.get_info(&mut rt); + assert_eq!(first_beneficiary_id, info.beneficiary); + + let mut pending_beneficiary_term = info.pending_beneficiary_term.unwrap(); + assert_eq!(second_beneficiary_change.expiration, pending_beneficiary_term.new_expiration); + assert_eq!(second_beneficiary_change.quota.clone(), pending_beneficiary_term.new_quota); + assert_eq!(second_beneficiary_id, pending_beneficiary_term.new_beneficiary); + assert!(!pending_beneficiary_term.approved_by_beneficiary); + assert!(!pending_beneficiary_term.approved_by_nominee); + + h.change_beneficiary(&mut rt, first_beneficiary_id, second_beneficiary_change.clone(), None) + .unwrap(); + info = h.get_info(&mut rt); + assert_eq!(first_beneficiary_id, info.beneficiary); + + pending_beneficiary_term = info.pending_beneficiary_term.unwrap(); + assert_eq!(second_beneficiary_change.expiration, pending_beneficiary_term.new_expiration); + assert_eq!(second_beneficiary_change.quota.clone(), pending_beneficiary_term.new_quota); + assert_eq!(second_beneficiary_id, pending_beneficiary_term.new_beneficiary); + assert!(pending_beneficiary_term.approved_by_beneficiary); + assert!(!pending_beneficiary_term.approved_by_nominee); + + h.change_beneficiary(&mut rt, second_beneficiary_id, second_beneficiary_change.clone(), None) + .unwrap(); + info = h.get_info(&mut rt); + assert_eq!(second_beneficiary_id, info.beneficiary); + + assert_eq!(None, info.pending_beneficiary_term); + assert_eq!(second_beneficiary_change.expiration, info.beneficiary_term.expiration); + assert_eq!(second_beneficiary_change.quota, info.beneficiary_term.quota); + assert_eq!(TokenAmount::zero(), info.beneficiary_term.used_quota); +} + +#[test] +fn successfully_change_from_not_owner_beneficiary_to_another_address_when_beneficiary_inefficient_two_message( +) { + let (mut h, mut rt) = setup(); + let first_beneficiary_id = Address::new_id(999); + let second_beneficiary_id = Address::new_id(1000); + + let quota = TokenAmount::from(100); + let expiration = ChainEpoch::from(200); + h.propose_approve_initial_beneficiary( + &mut rt, + first_beneficiary_id, + BeneficiaryTerm::new(quota.clone(), TokenAmount::zero(), expiration), + ) + .unwrap(); + + rt.set_epoch(201); + let another_quota = TokenAmount::from(1001); + let another_expiration = ChainEpoch::from(3); + let another_beneficiary_change = + BeneficiaryChange::new(second_beneficiary_id, another_quota.clone(), another_expiration); + h.change_beneficiary(&mut rt, h.owner, another_beneficiary_change.clone(), None).unwrap(); + let mut info = h.get_info(&mut rt); + + let pending_beneficiary_term = info.pending_beneficiary_term.unwrap(); + assert_eq!(second_beneficiary_id, pending_beneficiary_term.new_beneficiary); + assert_eq!(another_quota, pending_beneficiary_term.new_quota); + assert_eq!(another_expiration, pending_beneficiary_term.new_expiration); + + h.change_beneficiary(&mut rt, second_beneficiary_id, another_beneficiary_change, None).unwrap(); + info = h.get_info(&mut rt); + assert_eq!(None, info.pending_beneficiary_term); + assert_eq!(second_beneficiary_id, info.beneficiary); + assert_eq!(another_quota, info.beneficiary_term.quota); + assert_eq!(another_expiration, info.beneficiary_term.expiration); + assert_eq!(TokenAmount::zero(), info.beneficiary_term.used_quota); + h.check_state(&rt); +} + +#[test] +fn successfully_owner_immediate_revoking_unapproved_proposal() { + let (mut h, mut rt) = setup(); + let first_beneficiary_id = Address::new_id(999); + + let beneficiary_change = + BeneficiaryChange::new(first_beneficiary_id, TokenAmount::from(100), ChainEpoch::from(200)); + // proposal beneficiary change + h.change_beneficiary(&mut rt, h.owner, beneficiary_change.clone(), None).unwrap(); + // assert change has been made in state + let mut info = h.get_info(&mut rt); + let pending_beneficiary_term = info.pending_beneficiary_term.unwrap(); + assert_eq!(beneficiary_change.expiration, pending_beneficiary_term.new_expiration); + assert_eq!(beneficiary_change.quota, pending_beneficiary_term.new_quota); + assert_eq!(first_beneficiary_id, pending_beneficiary_term.new_beneficiary); + + //revoking unapprovel proposal + h.change_beneficiary( + &mut rt, + h.owner, + BeneficiaryChange::new(h.owner, TokenAmount::zero(), ChainEpoch::from(0)), + Some(h.owner), + ) + .unwrap(); + + info = h.get_info(&mut rt); + assert_eq!(None, info.pending_beneficiary_term); + assert_eq!(h.owner, info.beneficiary); + + h.check_state(&rt); +} + +#[test] +fn successfully_immediately_change_back_to_owner_address_while_used_up_quota() { + let (mut h, mut rt) = setup(); + let first_beneficiary_id = Address::new_id(999); + + let quota = TokenAmount::from(100); + let expiration = ChainEpoch::from(200); + h.propose_approve_initial_beneficiary( + &mut rt, + first_beneficiary_id, + BeneficiaryTerm::new(quota.clone(), TokenAmount::zero(), expiration), + ) + .unwrap(); + + h.withdraw_funds(&mut rt, h.beneficiary, "a, "a, &TokenAmount::zero()).unwrap(); + h.change_beneficiary( + &mut rt, + h.owner, + BeneficiaryChange::new(h.owner, TokenAmount::zero(), ChainEpoch::from(0)), + Some(h.owner), + ) + .unwrap(); + + let info = h.get_info(&mut rt); + assert_eq!(None, info.pending_beneficiary_term); + assert_eq!(h.owner, info.beneficiary); + assert_eq!(TokenAmount::zero(), info.beneficiary_term.quota); + assert_eq!(ChainEpoch::from(0), info.beneficiary_term.expiration); + assert_eq!(TokenAmount::zero(), info.beneficiary_term.used_quota); + h.check_state(&rt); +} + +#[test] +fn successfully_immediately_change_back_to_owner_while_expired() { + let (mut h, mut rt) = setup(); + let first_beneficiary_id = Address::new_id(999); + + let quota = TokenAmount::from(100); + let expiration = ChainEpoch::from(200); + h.propose_approve_initial_beneficiary( + &mut rt, + first_beneficiary_id, + BeneficiaryTerm::new(quota.clone(), TokenAmount::zero(), expiration), + ) + .unwrap(); + + rt.set_epoch(201); + h.change_beneficiary( + &mut rt, + h.owner, + BeneficiaryChange::new(h.owner, TokenAmount::zero(), ChainEpoch::from(0)), + Some(h.owner), + ) + .unwrap(); + + let info = h.get_info(&mut rt); + assert_eq!(None, info.pending_beneficiary_term); + assert_eq!(h.owner, info.beneficiary); + assert_eq!(TokenAmount::zero(), info.beneficiary_term.quota); + assert_eq!(ChainEpoch::from(0), info.beneficiary_term.expiration); + assert_eq!(TokenAmount::zero(), info.beneficiary_term.used_quota); + h.check_state(&rt); +} + +#[test] +fn successfully_increase_quota() { + let (mut h, mut rt) = setup(); + let first_beneficiary_id = Address::new_id(999); + let beneficiary_term = + BeneficiaryTerm::new(TokenAmount::from(100), TokenAmount::zero(), ChainEpoch::from(200)); + h.propose_approve_initial_beneficiary(&mut rt, first_beneficiary_id, beneficiary_term.clone()) + .unwrap(); + + //increase quota + let increase_quota = TokenAmount::from(100) + beneficiary_term.quota.clone(); + let increase_beneficiary_change = BeneficiaryChange::new( + first_beneficiary_id, + increase_quota.clone(), + beneficiary_term.expiration, + ); + h.change_beneficiary(&mut rt, h.owner, increase_beneficiary_change.clone(), None).unwrap(); + let mut info = h.get_info(&mut rt); + let pending_beneficiary_term = info.pending_beneficiary_term.unwrap(); + assert_eq!(first_beneficiary_id, info.beneficiary); + assert_eq!(increase_quota, pending_beneficiary_term.new_quota); + assert_eq!(beneficiary_term.expiration, pending_beneficiary_term.new_expiration); + + //confirm increase quota + h.change_beneficiary( + &mut rt, + first_beneficiary_id, + increase_beneficiary_change, + Some(first_beneficiary_id), + ) + .unwrap(); + info = h.get_info(&mut rt); + assert_eq!(None, info.pending_beneficiary_term); + assert_eq!(first_beneficiary_id, info.beneficiary); + assert_eq!(increase_quota, info.beneficiary_term.quota); + assert_eq!(beneficiary_term.expiration, info.beneficiary_term.expiration); + h.check_state(&rt); +} + +#[test] +fn fails_approval_message_with_invalidate_params() { + let (mut h, mut rt) = setup(); + let first_beneficiary_id = Address::new_id(999); + + // proposal beneficiary + h.change_beneficiary( + &mut rt, + h.owner, + BeneficiaryChange::new(first_beneficiary_id, TokenAmount::from(100), 200), + None, + ) + .unwrap(); + assert!(h.get_info(&mut rt).pending_beneficiary_term.is_some()); + + //expiration in approval message must equal with proposal + expect_abort( + ExitCode::USR_ILLEGAL_ARGUMENT, + h.change_beneficiary( + &mut rt, + first_beneficiary_id, + BeneficiaryChange::new(first_beneficiary_id, TokenAmount::from(100), 201), + None, + ), + ); + + //quota in approval message must equal with proposal + expect_abort( + ExitCode::USR_ILLEGAL_ARGUMENT, + h.change_beneficiary( + &mut rt, + first_beneficiary_id, + BeneficiaryChange::new(first_beneficiary_id, TokenAmount::from(101), 200), + None, + ), + ); + + //beneficiary in approval message must equal with proposal + let second_beneficiary_id = Address::new_id(1010); + expect_abort( + ExitCode::USR_ILLEGAL_ARGUMENT, + h.change_beneficiary( + &mut rt, + first_beneficiary_id, + BeneficiaryChange::new(second_beneficiary_id, TokenAmount::from(100), 200), + None, + ), + ); +} + +#[test] +fn fails_proposal_beneficiary_with_invalidate_params() { + let (mut h, mut rt) = setup(); + let first_beneficiary_id = Address::new_id(999); + + //not-call unable to proposal beneficiary + expect_abort( + ExitCode::USR_FORBIDDEN, + h.change_beneficiary( + &mut rt, + first_beneficiary_id, + BeneficiaryChange::new(first_beneficiary_id, TokenAmount::from(100), 200), + None, + ), + ); + + //quota must bigger than zero while change beneficiary to address(not owner) + expect_abort( + ExitCode::USR_ILLEGAL_ARGUMENT, + h.change_beneficiary( + &mut rt, + h.owner, + BeneficiaryChange::new(first_beneficiary_id, TokenAmount::from(0), 200), + None, + ), + ); + + //quota must be zero while change beneficiary to owner address + expect_abort( + ExitCode::USR_ILLEGAL_ARGUMENT, + h.change_beneficiary( + &mut rt, + h.owner, + BeneficiaryChange::new(h.owner, TokenAmount::from(20), 0), + None, + ), + ); + + //expiration must be zero while change beneficiary to owner address + expect_abort( + ExitCode::USR_ILLEGAL_ARGUMENT, + h.change_beneficiary( + &mut rt, + h.owner, + BeneficiaryChange::new(h.owner, TokenAmount::from(0), 1), + None, + ), + ); +} + +#[test] +fn successfully_get_beneficiary() { + let (mut h, mut rt) = setup(); + let mut info = h.get_info(&mut rt); + assert_eq!(h.owner, info.beneficiary); + assert_eq!(ChainEpoch::from(0), info.beneficiary_term.expiration); + assert_eq!(TokenAmount::zero(), info.beneficiary_term.quota); + assert_eq!(TokenAmount::zero(), info.beneficiary_term.used_quota); + + let first_beneficiary_id = Address::new_id(999); + let beneficiary_term = + BeneficiaryTerm::new(TokenAmount::from(100), TokenAmount::zero(), ChainEpoch::from(200)); + h.propose_approve_initial_beneficiary(&mut rt, first_beneficiary_id, beneficiary_term.clone()) + .unwrap(); + + info = h.get_info(&mut rt); + assert_eq!(first_beneficiary_id, info.beneficiary); + assert_eq!(beneficiary_term.expiration, info.beneficiary_term.expiration); + assert_eq!(beneficiary_term.quota, info.beneficiary_term.quota); + assert_eq!(TokenAmount::zero(), info.beneficiary_term.used_quota); + + let withdraw_fund = TokenAmount::from(40); + let left_quota = TokenAmount::from(60); + h.withdraw_funds(&mut rt, h.beneficiary, &withdraw_fund, &withdraw_fund, &TokenAmount::zero()) + .unwrap(); + + info = h.get_info(&mut rt); + assert_eq!(first_beneficiary_id, info.beneficiary); + assert_eq!(beneficiary_term.expiration, info.beneficiary_term.expiration); + assert_eq!(left_quota, info.beneficiary_term.available(rt.epoch)); + assert_eq!(withdraw_fund, info.beneficiary_term.used_quota); +} diff --git a/actors/miner/tests/change_owner_address_test.rs b/actors/miner/tests/change_owner_address_test.rs index a6b6603f0..56fd905dc 100644 --- a/actors/miner/tests/change_owner_address_test.rs +++ b/actors/miner/tests/change_owner_address_test.rs @@ -37,6 +37,7 @@ fn successful_change() { let info = h.get_info(&rt); assert_eq!(NEW_ADDRESS, info.owner); + assert_eq!(NEW_ADDRESS, info.beneficiary); assert!(info.pending_owner_address.is_none()); h.check_state(&rt); diff --git a/actors/miner/tests/util.rs b/actors/miner/tests/util.rs index b2db036bd..f74061367 100644 --- a/actors/miner/tests/util.rs +++ b/actors/miner/tests/util.rs @@ -13,12 +13,13 @@ use fil_actor_miner::{ initial_pledge_for_power, locked_reward_from_reward, max_prove_commit_duration, new_deadline_info_from_offset_and_epoch, pledge_penalty_for_continued_fault, power_for_sectors, qa_power_for_sector, qa_power_for_weight, reward_for_consensus_slash_report, Actor, - ApplyRewardParams, BitFieldQueue, ChangeMultiaddrsParams, ChangePeerIDParams, - ChangeWorkerAddressParams, CheckSectorProvenParams, CompactPartitionsParams, - CompactSectorNumbersParams, ConfirmSectorProofsParams, CronEventPayload, Deadline, - DeadlineInfo, Deadlines, DeclareFaultsParams, DeclareFaultsRecoveredParams, - DeferredCronEventParams, DisputeWindowedPoStParams, ExpirationQueue, ExpirationSet, - ExtendSectorExpirationParams, FaultDeclaration, GetControlAddressesReturn, Method, + ApplyRewardParams, BeneficiaryTerm, BitFieldQueue, ChangeBeneficiaryParams, + ChangeMultiaddrsParams, ChangePeerIDParams, ChangeWorkerAddressParams, CheckSectorProvenParams, + CompactPartitionsParams, CompactSectorNumbersParams, ConfirmSectorProofsParams, + CronEventPayload, Deadline, DeadlineInfo, Deadlines, DeclareFaultsParams, + DeclareFaultsRecoveredParams, DeferredCronEventParams, DisputeWindowedPoStParams, + ExpirationQueue, ExpirationSet, ExtendSectorExpirationParams, FaultDeclaration, + GetBeneficiaryReturn, GetControlAddressesReturn, Method, MinerConstructorParams as ConstructorParams, MinerInfo, Partition, PoStPartition, PowerPair, PreCommitSectorBatchParams, PreCommitSectorParams, ProveCommitSectorParams, RecoveryDeclaration, ReportConsensusFaultParams, SectorOnChainInfo, SectorPreCommitOnChainInfo, @@ -103,6 +104,7 @@ pub struct ActorHarness { pub owner: Address, pub worker: Address, pub worker_key: Address, + pub beneficiary: Address, pub control_addrs: Vec
, @@ -141,6 +143,7 @@ impl ActorHarness { worker_key, control_addrs, + beneficiary: owner, seal_proof_type: proof_type, window_post_proof_type: proof_type.registered_window_post_proof().unwrap(), sector_size: proof_type.sector_size().unwrap(), @@ -1936,15 +1939,16 @@ impl ActorHarness { pub fn withdraw_funds( &self, rt: &mut MockRuntime, + from_address: Address, amount_requested: &TokenAmount, expected_withdrawn: &TokenAmount, expected_debt_repaid: &TokenAmount, ) -> Result<(), ActorError> { - rt.set_caller(*ACCOUNT_ACTOR_CODE_ID, self.owner); - rt.expect_validate_caller_addr(vec![self.owner]); + rt.set_caller(*ACCOUNT_ACTOR_CODE_ID, from_address); + rt.expect_validate_caller_addr(vec![self.owner, self.beneficiary]); rt.expect_send( - self.owner, + self.beneficiary, METHOD_SEND, RawBytes::default(), expected_withdrawn.clone(), @@ -2051,6 +2055,72 @@ impl ActorHarness { Ok(()) } + pub fn propose_approve_initial_beneficiary( + &mut self, + rt: &mut MockRuntime, + beneficiary_id_addr: Address, + beneficiary_term: BeneficiaryTerm, + ) -> Result<(), ActorError> { + rt.set_caller(*ACCOUNT_ACTOR_CODE_ID, self.owner); + + let param = ChangeBeneficiaryParams { + new_beneficiary: beneficiary_id_addr, + new_quota: beneficiary_term.quota, + new_expiration: beneficiary_term.expiration, + }; + let raw_bytes = &RawBytes::serialize(param).unwrap(); + rt.call::(Method::ChangeBeneficiary as u64, raw_bytes)?; + rt.verify(); + + rt.set_caller(*ACCOUNT_ACTOR_CODE_ID, beneficiary_id_addr); + rt.call::(Method::ChangeBeneficiary as u64, raw_bytes)?; + rt.verify(); + + self.beneficiary = beneficiary_id_addr; + Ok(()) + } + + pub fn change_beneficiary( + &mut self, + rt: &mut MockRuntime, + expect_caller: Address, + beneficiary_change: BeneficiaryChange, + expect_beneficiary_addr: Option
, + ) -> Result { + rt.set_address_actor_type( + beneficiary_change.beneficiary_addr.clone(), + *ACCOUNT_ACTOR_CODE_ID, + ); + let caller_id = rt.get_id_address(&expect_caller).unwrap(); + let param = ChangeBeneficiaryParams { + new_beneficiary: beneficiary_change.beneficiary_addr, + new_quota: beneficiary_change.quota, + new_expiration: beneficiary_change.expiration, + }; + rt.set_caller(*ACCOUNT_ACTOR_CODE_ID, caller_id); + let ret = rt.call::( + Method::ChangeBeneficiary as u64, + &RawBytes::serialize(param).unwrap(), + )?; + rt.verify(); + + if let Some(beneficiary) = expect_beneficiary_addr { + self.beneficiary = beneficiary.clone(); + } + + Ok(ret) + } + + pub fn get_beneficiary( + &mut self, + rt: &mut MockRuntime, + ) -> Result { + rt.expect_validate_caller_any(); + let ret = rt.call::(Method::ChangeBeneficiary as u64, &RawBytes::default())?; + rt.verify(); + Ok(ret.deserialize::().unwrap()) + } + pub fn extend_sectors( &self, rt: &mut MockRuntime, @@ -2273,6 +2343,20 @@ pub struct PoStDisputeResult { pub expected_reward: Option, } +#[derive(Clone)] +pub struct BeneficiaryChange { + pub beneficiary_addr: Address, + pub quota: TokenAmount, + pub expiration: ChainEpoch, +} + +impl BeneficiaryChange { + #[allow(dead_code)] + pub fn new(beneficiary_addr: Address, quota: TokenAmount, expiration: ChainEpoch) -> Self { + BeneficiaryChange { beneficiary_addr, quota, expiration } + } +} + #[allow(dead_code)] pub fn assert_bitfield_equals(bf: &BitField, bits: &[u64]) { let mut rbf = BitField::new(); diff --git a/actors/miner/tests/withdraw_balance.rs b/actors/miner/tests/withdraw_balance.rs index 224e2b108..993d5aeeb 100644 --- a/actors/miner/tests/withdraw_balance.rs +++ b/actors/miner/tests/withdraw_balance.rs @@ -1,4 +1,6 @@ -use fil_actors_runtime::test_utils::expect_abort_contains_message; +use fil_actor_miner::BeneficiaryTerm; +use fil_actors_runtime::test_utils::{expect_abort, expect_abort_contains_message}; +use fvm_shared::address::Address; use fvm_shared::bigint::Zero; use fvm_shared::clock::ChainEpoch; use fvm_shared::econ::TokenAmount; @@ -21,6 +23,7 @@ fn happy_path_withdraws_funds() { h.withdraw_funds( &mut rt, + h.owner, &TokenAmount::from(ONE_PERCENT_BALANCE), &TokenAmount::from(ONE_PERCENT_BALANCE), &TokenAmount::zero(), @@ -44,6 +47,7 @@ fn fails_if_miner_cant_repay_fee_debt() { "unlocked balance can not repay fee debt", h.withdraw_funds( &mut rt, + h.owner, &TokenAmount::from(ONE_PERCENT_BALANCE), &TokenAmount::from(ONE_PERCENT_BALANCE), &TokenAmount::zero(), @@ -66,6 +70,99 @@ fn withdraw_only_what_we_can_after_fee_debt() { let requested = rt.balance.borrow().to_owned(); let expected_withdraw = &requested - &fee_debt; - h.withdraw_funds(&mut rt, &requested, &expected_withdraw, &fee_debt).unwrap(); + h.withdraw_funds(&mut rt, h.owner, &requested, &expected_withdraw, &fee_debt).unwrap(); + h.check_state(&rt); +} + +#[test] +fn successfully_withdraw() { + let mut h = ActorHarness::new(PERIOD_OFFSET); + let mut rt = h.new_runtime(); + rt.set_balance(TokenAmount::from(BIG_BALANCE)); + h.construct_and_verify(&mut rt); + + let one = TokenAmount::from(1); + h.withdraw_funds(&mut rt, h.owner, &one, &one, &TokenAmount::zero()).unwrap(); + + let first_beneficiary_id = Address::new_id(999); + let quota = TokenAmount::from(ONE_PERCENT_BALANCE); + h.propose_approve_initial_beneficiary( + &mut rt, + first_beneficiary_id, + BeneficiaryTerm::new(quota.clone(), TokenAmount::zero(), PERIOD_OFFSET + 100), + ) + .unwrap(); + h.withdraw_funds(&mut rt, h.owner, &one, &one, &TokenAmount::zero()).unwrap(); + h.withdraw_funds(&mut rt, h.beneficiary, &one, &one, &TokenAmount::zero()).unwrap(); + h.check_state(&rt); +} + +#[test] +fn successfully_withdraw_from_non_main_beneficiary_and_failure_when_used_all_quota() { + let mut h = ActorHarness::new(PERIOD_OFFSET); + let mut rt = h.new_runtime(); + rt.set_balance(TokenAmount::from(BIG_BALANCE)); + h.construct_and_verify(&mut rt); + + let first_beneficiary_id = Address::new_id(999); + let quota = TokenAmount::from(ONE_PERCENT_BALANCE); + h.propose_approve_initial_beneficiary( + &mut rt, + first_beneficiary_id, + BeneficiaryTerm::new(quota.clone(), TokenAmount::zero(), PERIOD_OFFSET + 100), + ) + .unwrap(); + h.withdraw_funds(&mut rt, h.beneficiary, "a, "a, &TokenAmount::zero()).unwrap(); + expect_abort( + ExitCode::USR_FORBIDDEN, + h.withdraw_funds(&mut rt, h.beneficiary, "a, "a, &TokenAmount::zero()), + ); + h.check_state(&rt); +} + +#[test] +fn successfully_withdraw_more_than_quota() { + let mut h = ActorHarness::new(PERIOD_OFFSET); + let mut rt = h.new_runtime(); + rt.set_balance(TokenAmount::from(BIG_BALANCE)); + h.construct_and_verify(&mut rt); + + let first_beneficiary_id = Address::new_id(999); + let quota = TokenAmount::from(ONE_PERCENT_BALANCE); + h.propose_approve_initial_beneficiary( + &mut rt, + first_beneficiary_id, + BeneficiaryTerm::new(quota.clone(), TokenAmount::zero(), PERIOD_OFFSET + 100), + ) + .unwrap(); + + let withdraw_amount = TokenAmount::from(ONE_PERCENT_BALANCE * 2); + h.withdraw_funds(&mut rt, h.beneficiary, &withdraw_amount, "a, &TokenAmount::zero()) + .unwrap(); + h.check_state(&rt); +} + +#[test] +fn fails_withdraw_when_beneficiary_expired() { + let mut h = ActorHarness::new(PERIOD_OFFSET); + let mut rt = h.new_runtime(); + rt.set_balance(TokenAmount::from(BIG_BALANCE)); + h.construct_and_verify(&mut rt); + + let first_beneficiary_id = Address::new_id(999); + let quota = TokenAmount::from(ONE_PERCENT_BALANCE); + h.propose_approve_initial_beneficiary( + &mut rt, + first_beneficiary_id, + BeneficiaryTerm::new(quota.clone(), TokenAmount::zero(), PERIOD_OFFSET - 10), + ) + .unwrap(); + let info = h.get_info(&mut rt); + assert_eq!(PERIOD_OFFSET - 10, info.beneficiary_term.expiration); + rt.set_epoch(100); + expect_abort( + ExitCode::USR_FORBIDDEN, + h.withdraw_funds(&mut rt, h.beneficiary, "a, "a, &TokenAmount::zero()), + ); h.check_state(&rt); } From 8b648e2dc312e5d0b9c873aa8cecb670f6ba6232 Mon Sep 17 00:00:00 2001 From: hunjixin <1084400399@qq.com> Date: Wed, 27 Jul 2022 09:43:53 +0800 Subject: [PATCH 3/6] fix: allow quota zero pass in withdraw function --- actors/miner/src/beneficiary.rs | 8 +- actors/miner/src/lib.rs | 77 +++++++++---------- actors/miner/src/types.rs | 3 +- actors/miner/tests/change_beneficiary_test.rs | 46 +++++------ actors/miner/tests/util.rs | 20 +++-- actors/miner/tests/withdraw_balance.rs | 66 +++++++++++++--- 6 files changed, 133 insertions(+), 87 deletions(-) diff --git a/actors/miner/src/beneficiary.rs b/actors/miner/src/beneficiary.rs index 0bfd6d2b9..2c5243f7d 100644 --- a/actors/miner/src/beneficiary.rs +++ b/actors/miner/src/beneficiary.rs @@ -38,12 +38,14 @@ impl BeneficiaryTerm { BeneficiaryTerm { quota, expiration, used_quota } } - /// get the amount that the beneficiary has not yet withdrawn + /// Get the amount that the beneficiary has not yet withdrawn + /// return 0 when expired + /// return 0 when the usedQuota >= Quota for safe + /// otherwise Return quota-used_quota pub fn available(&self, cur: ChainEpoch) -> TokenAmount { - // Return 0 when the usedQuota > Quota for safe if self.expiration > cur { (&self.quota).sub(&self.used_quota).max(TokenAmount::zero()) - }else{ + } else { TokenAmount::zero() } } diff --git a/actors/miner/src/lib.rs b/actors/miner/src/lib.rs index f0699098b..657980d0b 100644 --- a/actors/miner/src/lib.rs +++ b/actors/miner/src/lib.rs @@ -33,6 +33,7 @@ use fvm_shared::econ::TokenAmount; // The following errors are particular cases of illegal state. // They're not expected to ever happen, but if they do, distinguished codes can help us // diagnose the problem. +pub use beneficiary::*; use fil_actors_runtime::cbor::{deserialize, serialize, serialize_vec}; use fil_actors_runtime::runtime::builtins::Type; use fvm_shared::error::*; @@ -53,7 +54,6 @@ pub use state::*; pub use termination::*; pub use types::*; pub use vesting_state::*; -pub use beneficiary::*; use crate::commd::{is_unsealed_sector, CompactCommD}; use crate::Code::Blake2b256; @@ -120,8 +120,8 @@ pub enum Method { ProveReplicaUpdates = 27, PreCommitSectorBatch2 = 28, ProveReplicaUpdates2 = 29, - ChangeBeneficiary =30, - GetBeneficiary =31, + ChangeBeneficiary = 30, + GetBeneficiary = 31, } pub const ERR_BALANCE_INVARIANTS_BROKEN: ExitCode = ExitCode::new(1000); @@ -3296,7 +3296,7 @@ impl Actor { } if info.beneficiary != info.owner { let remaining_quota = info.beneficiary_term.available(rt.curr_epoch()); - if !remaining_quota.is_positive() { + if remaining_quota.is_negative() { return Err(actor_error!( forbidden, "quota not enough beneficiary {} quota {} used quota {} expiration epoch {} current epoch {}", @@ -3308,10 +3308,12 @@ impl Actor { )); } amount_withdrawn = std::cmp::min(amount_withdrawn, &remaining_quota); - info.beneficiary_term.used_quota = info.beneficiary_term.used_quota + amount_withdrawn; - state.save_info(rt.store(), &info).map_err(|e| { - e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "failed to save miner info") - })?; + if amount_withdrawn.is_positive() { + info.beneficiary_term.used_quota += amount_withdrawn; + state.save_info(rt.store(), &info).map_err(|e| { + e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "failed to save miner info") + })?; + } Ok((info, amount_withdrawn.clone(), newly_vested, fee_to_burn, state.clone())) }else{ Ok((info, amount_withdrawn.clone(), newly_vested, fee_to_burn, state.clone())) @@ -3326,7 +3328,7 @@ impl Actor { notify_pledge_changed(rt, &newly_vested.neg())?; state.check_balance_invariants(&rt.current_balance()).map_err(balance_invariants_broken)?; - Ok(WithdrawBalanceReturn { amount_withdrawn: amount_withdrawn.clone() }) + Ok(WithdrawBalanceReturn { amount_withdrawn }) } /// Proposes or confirms a change of owner address. @@ -3389,35 +3391,33 @@ impl Actor { pending_beneficiary_term.approved_by_beneficiary = true; } info.pending_beneficiary_term = Some(pending_beneficiary_term); - } else { - if let Some(pending_term) = &info.pending_beneficiary_term { - if pending_term.new_beneficiary != new_beneficiary { - return Err(actor_error!( - illegal_argument, - "new beneficiary address must be equal expect {}, but got {}", - pending_term.new_beneficiary, - params.new_beneficiary - )); - } - if pending_term.new_quota != params.new_quota { - return Err(actor_error!( - illegal_argument, - "new beneficiary quota must be equal expect {}, but got {}", - pending_term.new_quota, - params.new_quota - )); - } - if pending_term.new_expiration != params.new_expiration { - return Err(actor_error!( - illegal_argument, - "new beneficiary expire date must be equal expect {}, but got {}", - pending_term.new_expiration, - params.new_expiration - )); - } - } else { - return Err(actor_error!(forbidden, "No changeBeneficiary proposal exists")); + } else if let Some(pending_term) = &info.pending_beneficiary_term { + if pending_term.new_beneficiary != new_beneficiary { + return Err(actor_error!( + illegal_argument, + "new beneficiary address must be equal expect {}, but got {}", + pending_term.new_beneficiary, + params.new_beneficiary + )); + } + if pending_term.new_quota != params.new_quota { + return Err(actor_error!( + illegal_argument, + "new beneficiary quota must be equal expect {}, but got {}", + pending_term.new_quota, + params.new_quota + )); + } + if pending_term.new_expiration != params.new_expiration { + return Err(actor_error!( + illegal_argument, + "new beneficiary expire date must be equal expect {}, but got {}", + pending_term.new_expiration, + params.new_expiration + )); } + } else { + return Err(actor_error!(forbidden, "No changeBeneficiary proposal exists")); } if let Some(pending_term) = info.pending_beneficiary_term.as_mut() { @@ -3459,8 +3459,7 @@ impl Actor { RT: Runtime, { rt.validate_immediate_caller_accept_any()?; - let info = - rt.transaction(|state: &mut State, rt| Ok(get_miner_info(rt.store(), &state)?))?; + let info = rt.transaction(|state: &mut State, rt| get_miner_info(rt.store(), state))?; Ok(GetBeneficiaryReturn { active: ActiveBeneficiary { diff --git a/actors/miner/src/types.rs b/actors/miner/src/types.rs index 2847231fa..24ec1cef5 100644 --- a/actors/miner/src/types.rs +++ b/actors/miner/src/types.rs @@ -1,8 +1,8 @@ // Copyright 2019-2022 ChainSafe Systems // SPDX-License-Identifier: Apache-2.0, MIT -use crate::commd::CompactCommD; use super::beneficiary::*; +use crate::commd::CompactCommD; use cid::Cid; use fil_actors_runtime::DealWeight; use fvm_ipld_bitfield::UnvalidatedBitField; @@ -409,7 +409,6 @@ pub struct ProveReplicaUpdatesParams2 { impl Cbor for ProveReplicaUpdatesParams2 {} - #[derive(Debug, Serialize_tuple, Deserialize_tuple)] pub struct ChangeBeneficiaryParams { pub new_beneficiary: Address, diff --git a/actors/miner/tests/change_beneficiary_test.rs b/actors/miner/tests/change_beneficiary_test.rs index f4f769595..f9013933c 100644 --- a/actors/miner/tests/change_beneficiary_test.rs +++ b/actors/miner/tests/change_beneficiary_test.rs @@ -29,10 +29,10 @@ fn successfully_change_owner_to_another_address_two_message() { // proposal beneficiary change h.change_beneficiary(&mut rt, h.owner, beneficiary_change.clone(), None).unwrap(); // assert change has been made in state - let mut info = h.get_info(&mut rt); + let mut info = h.get_info(&rt); let pending_beneficiary_term = info.pending_beneficiary_term.unwrap(); assert_eq!(beneficiary_change.expiration, pending_beneficiary_term.new_expiration); - assert_eq!(beneficiary_change.quota.clone(), pending_beneficiary_term.new_quota); + assert_eq!(beneficiary_change.quota, pending_beneficiary_term.new_quota); assert_eq!(first_beneficiary_id, pending_beneficiary_term.new_beneficiary); //confirm proposal @@ -44,10 +44,10 @@ fn successfully_change_owner_to_another_address_two_message() { ) .unwrap(); - info = h.get_info(&mut rt); + info = h.get_info(&rt); assert_eq!(None, info.pending_beneficiary_term); assert_eq!(first_beneficiary_id, info.beneficiary); - assert_eq!(beneficiary_change.quota.clone(), info.beneficiary_term.quota); + assert_eq!(beneficiary_change.quota, info.beneficiary_term.quota); assert_eq!(beneficiary_change.expiration, info.beneficiary_term.expiration); h.check_state(&rt); @@ -70,31 +70,31 @@ fn successfully_change_from_not_owner_beneficiary_to_another_address_three_messa ChainEpoch::from(201), ); h.change_beneficiary(&mut rt, h.owner, second_beneficiary_change.clone(), None).unwrap(); - let mut info = h.get_info(&mut rt); + let mut info = h.get_info(&rt); assert_eq!(first_beneficiary_id, info.beneficiary); let mut pending_beneficiary_term = info.pending_beneficiary_term.unwrap(); assert_eq!(second_beneficiary_change.expiration, pending_beneficiary_term.new_expiration); - assert_eq!(second_beneficiary_change.quota.clone(), pending_beneficiary_term.new_quota); + assert_eq!(second_beneficiary_change.quota, pending_beneficiary_term.new_quota); assert_eq!(second_beneficiary_id, pending_beneficiary_term.new_beneficiary); assert!(!pending_beneficiary_term.approved_by_beneficiary); assert!(!pending_beneficiary_term.approved_by_nominee); h.change_beneficiary(&mut rt, first_beneficiary_id, second_beneficiary_change.clone(), None) .unwrap(); - info = h.get_info(&mut rt); + info = h.get_info(&rt); assert_eq!(first_beneficiary_id, info.beneficiary); pending_beneficiary_term = info.pending_beneficiary_term.unwrap(); assert_eq!(second_beneficiary_change.expiration, pending_beneficiary_term.new_expiration); - assert_eq!(second_beneficiary_change.quota.clone(), pending_beneficiary_term.new_quota); + assert_eq!(second_beneficiary_change.quota, pending_beneficiary_term.new_quota); assert_eq!(second_beneficiary_id, pending_beneficiary_term.new_beneficiary); assert!(pending_beneficiary_term.approved_by_beneficiary); assert!(!pending_beneficiary_term.approved_by_nominee); h.change_beneficiary(&mut rt, second_beneficiary_id, second_beneficiary_change.clone(), None) .unwrap(); - info = h.get_info(&mut rt); + info = h.get_info(&rt); assert_eq!(second_beneficiary_id, info.beneficiary); assert_eq!(None, info.pending_beneficiary_term); @@ -115,7 +115,7 @@ fn successfully_change_from_not_owner_beneficiary_to_another_address_when_benefi h.propose_approve_initial_beneficiary( &mut rt, first_beneficiary_id, - BeneficiaryTerm::new(quota.clone(), TokenAmount::zero(), expiration), + BeneficiaryTerm::new(quota, TokenAmount::zero(), expiration), ) .unwrap(); @@ -125,7 +125,7 @@ fn successfully_change_from_not_owner_beneficiary_to_another_address_when_benefi let another_beneficiary_change = BeneficiaryChange::new(second_beneficiary_id, another_quota.clone(), another_expiration); h.change_beneficiary(&mut rt, h.owner, another_beneficiary_change.clone(), None).unwrap(); - let mut info = h.get_info(&mut rt); + let mut info = h.get_info(&rt); let pending_beneficiary_term = info.pending_beneficiary_term.unwrap(); assert_eq!(second_beneficiary_id, pending_beneficiary_term.new_beneficiary); @@ -133,7 +133,7 @@ fn successfully_change_from_not_owner_beneficiary_to_another_address_when_benefi assert_eq!(another_expiration, pending_beneficiary_term.new_expiration); h.change_beneficiary(&mut rt, second_beneficiary_id, another_beneficiary_change, None).unwrap(); - info = h.get_info(&mut rt); + info = h.get_info(&rt); assert_eq!(None, info.pending_beneficiary_term); assert_eq!(second_beneficiary_id, info.beneficiary); assert_eq!(another_quota, info.beneficiary_term.quota); @@ -152,7 +152,7 @@ fn successfully_owner_immediate_revoking_unapproved_proposal() { // proposal beneficiary change h.change_beneficiary(&mut rt, h.owner, beneficiary_change.clone(), None).unwrap(); // assert change has been made in state - let mut info = h.get_info(&mut rt); + let mut info = h.get_info(&rt); let pending_beneficiary_term = info.pending_beneficiary_term.unwrap(); assert_eq!(beneficiary_change.expiration, pending_beneficiary_term.new_expiration); assert_eq!(beneficiary_change.quota, pending_beneficiary_term.new_quota); @@ -167,7 +167,7 @@ fn successfully_owner_immediate_revoking_unapproved_proposal() { ) .unwrap(); - info = h.get_info(&mut rt); + info = h.get_info(&rt); assert_eq!(None, info.pending_beneficiary_term); assert_eq!(h.owner, info.beneficiary); @@ -197,7 +197,7 @@ fn successfully_immediately_change_back_to_owner_address_while_used_up_quota() { ) .unwrap(); - let info = h.get_info(&mut rt); + let info = h.get_info(&rt); assert_eq!(None, info.pending_beneficiary_term); assert_eq!(h.owner, info.beneficiary); assert_eq!(TokenAmount::zero(), info.beneficiary_term.quota); @@ -216,7 +216,7 @@ fn successfully_immediately_change_back_to_owner_while_expired() { h.propose_approve_initial_beneficiary( &mut rt, first_beneficiary_id, - BeneficiaryTerm::new(quota.clone(), TokenAmount::zero(), expiration), + BeneficiaryTerm::new(quota, TokenAmount::zero(), expiration), ) .unwrap(); @@ -229,7 +229,7 @@ fn successfully_immediately_change_back_to_owner_while_expired() { ) .unwrap(); - let info = h.get_info(&mut rt); + let info = h.get_info(&rt); assert_eq!(None, info.pending_beneficiary_term); assert_eq!(h.owner, info.beneficiary); assert_eq!(TokenAmount::zero(), info.beneficiary_term.quota); @@ -255,7 +255,7 @@ fn successfully_increase_quota() { beneficiary_term.expiration, ); h.change_beneficiary(&mut rt, h.owner, increase_beneficiary_change.clone(), None).unwrap(); - let mut info = h.get_info(&mut rt); + let mut info = h.get_info(&rt); let pending_beneficiary_term = info.pending_beneficiary_term.unwrap(); assert_eq!(first_beneficiary_id, info.beneficiary); assert_eq!(increase_quota, pending_beneficiary_term.new_quota); @@ -269,7 +269,7 @@ fn successfully_increase_quota() { Some(first_beneficiary_id), ) .unwrap(); - info = h.get_info(&mut rt); + info = h.get_info(&rt); assert_eq!(None, info.pending_beneficiary_term); assert_eq!(first_beneficiary_id, info.beneficiary); assert_eq!(increase_quota, info.beneficiary_term.quota); @@ -290,7 +290,7 @@ fn fails_approval_message_with_invalidate_params() { None, ) .unwrap(); - assert!(h.get_info(&mut rt).pending_beneficiary_term.is_some()); + assert!(h.get_info(&rt).pending_beneficiary_term.is_some()); //expiration in approval message must equal with proposal expect_abort( @@ -380,7 +380,7 @@ fn fails_proposal_beneficiary_with_invalidate_params() { #[test] fn successfully_get_beneficiary() { let (mut h, mut rt) = setup(); - let mut info = h.get_info(&mut rt); + let mut info = h.get_info(&rt); assert_eq!(h.owner, info.beneficiary); assert_eq!(ChainEpoch::from(0), info.beneficiary_term.expiration); assert_eq!(TokenAmount::zero(), info.beneficiary_term.quota); @@ -392,7 +392,7 @@ fn successfully_get_beneficiary() { h.propose_approve_initial_beneficiary(&mut rt, first_beneficiary_id, beneficiary_term.clone()) .unwrap(); - info = h.get_info(&mut rt); + info = h.get_info(&rt); assert_eq!(first_beneficiary_id, info.beneficiary); assert_eq!(beneficiary_term.expiration, info.beneficiary_term.expiration); assert_eq!(beneficiary_term.quota, info.beneficiary_term.quota); @@ -403,7 +403,7 @@ fn successfully_get_beneficiary() { h.withdraw_funds(&mut rt, h.beneficiary, &withdraw_fund, &withdraw_fund, &TokenAmount::zero()) .unwrap(); - info = h.get_info(&mut rt); + info = h.get_info(&rt); assert_eq!(first_beneficiary_id, info.beneficiary); assert_eq!(beneficiary_term.expiration, info.beneficiary_term.expiration); assert_eq!(left_quota, info.beneficiary_term.available(rt.epoch)); diff --git a/actors/miner/tests/util.rs b/actors/miner/tests/util.rs index f74061367..b8157bf10 100644 --- a/actors/miner/tests/util.rs +++ b/actors/miner/tests/util.rs @@ -1947,14 +1947,18 @@ impl ActorHarness { rt.set_caller(*ACCOUNT_ACTOR_CODE_ID, from_address); rt.expect_validate_caller_addr(vec![self.owner, self.beneficiary]); - rt.expect_send( - self.beneficiary, - METHOD_SEND, - RawBytes::default(), - expected_withdrawn.clone(), - RawBytes::default(), - ExitCode::OK, - ); + if expected_withdrawn.is_positive() { + //no send when real withdraw amount is zero + rt.expect_send( + self.beneficiary, + METHOD_SEND, + RawBytes::default(), + expected_withdrawn.clone(), + RawBytes::default(), + ExitCode::OK, + ); + } + if expected_debt_repaid.is_positive() { rt.expect_send( *BURNT_FUNDS_ACTOR_ADDR, diff --git a/actors/miner/tests/withdraw_balance.rs b/actors/miner/tests/withdraw_balance.rs index 993d5aeeb..70e3fa320 100644 --- a/actors/miner/tests/withdraw_balance.rs +++ b/actors/miner/tests/withdraw_balance.rs @@ -89,7 +89,7 @@ fn successfully_withdraw() { h.propose_approve_initial_beneficiary( &mut rt, first_beneficiary_id, - BeneficiaryTerm::new(quota.clone(), TokenAmount::zero(), PERIOD_OFFSET + 100), + BeneficiaryTerm::new(quota, TokenAmount::zero(), PERIOD_OFFSET + 100), ) .unwrap(); h.withdraw_funds(&mut rt, h.owner, &one, &one, &TokenAmount::zero()).unwrap(); @@ -98,25 +98,27 @@ fn successfully_withdraw() { } #[test] -fn successfully_withdraw_from_non_main_beneficiary_and_failure_when_used_all_quota() { +fn successfully_withdraw_allow_zero() { let mut h = ActorHarness::new(PERIOD_OFFSET); let mut rt = h.new_runtime(); rt.set_balance(TokenAmount::from(BIG_BALANCE)); h.construct_and_verify(&mut rt); let first_beneficiary_id = Address::new_id(999); - let quota = TokenAmount::from(ONE_PERCENT_BALANCE); h.propose_approve_initial_beneficiary( &mut rt, first_beneficiary_id, - BeneficiaryTerm::new(quota.clone(), TokenAmount::zero(), PERIOD_OFFSET + 100), + BeneficiaryTerm::new(TokenAmount::from(1), TokenAmount::zero(), PERIOD_OFFSET + 100), + ) + .unwrap(); + h.withdraw_funds( + &mut rt, + first_beneficiary_id, + &TokenAmount::zero(), + &TokenAmount::zero(), + &TokenAmount::zero(), ) .unwrap(); - h.withdraw_funds(&mut rt, h.beneficiary, "a, "a, &TokenAmount::zero()).unwrap(); - expect_abort( - ExitCode::USR_FORBIDDEN, - h.withdraw_funds(&mut rt, h.beneficiary, "a, "a, &TokenAmount::zero()), - ); h.check_state(&rt); } @@ -143,7 +145,7 @@ fn successfully_withdraw_more_than_quota() { } #[test] -fn fails_withdraw_when_beneficiary_expired() { +fn allow_withdraw_but_no_send_when_beneficiary_not_efficient() { let mut h = ActorHarness::new(PERIOD_OFFSET); let mut rt = h.new_runtime(); rt.set_balance(TokenAmount::from(BIG_BALANCE)); @@ -157,12 +159,52 @@ fn fails_withdraw_when_beneficiary_expired() { BeneficiaryTerm::new(quota.clone(), TokenAmount::zero(), PERIOD_OFFSET - 10), ) .unwrap(); - let info = h.get_info(&mut rt); + let info = h.get_info(&rt); assert_eq!(PERIOD_OFFSET - 10, info.beneficiary_term.expiration); rt.set_epoch(100); + h.withdraw_funds(&mut rt, h.beneficiary, "a, &TokenAmount::zero(), &TokenAmount::zero()) + .unwrap(); + h.check_state(&rt); +} + +#[test] +fn fail_withdraw_from_non_beneficiary() { + let mut h = ActorHarness::new(PERIOD_OFFSET); + let mut rt = h.new_runtime(); + rt.set_balance(TokenAmount::from(BIG_BALANCE)); + h.construct_and_verify(&mut rt); + + let first_beneficiary_id = Address::new_id(999); + let another_actor = Address::new_id(1000); + let quota = TokenAmount::from(ONE_PERCENT_BALANCE); + let one = TokenAmount::from(1); + + expect_abort( + ExitCode::USR_FORBIDDEN, + h.withdraw_funds( + &mut rt, + first_beneficiary_id, + &one, + &TokenAmount::zero(), + &TokenAmount::zero(), + ), + ); + + h.propose_approve_initial_beneficiary( + &mut rt, + first_beneficiary_id, + BeneficiaryTerm::new(quota, TokenAmount::zero(), PERIOD_OFFSET - 10), + ) + .unwrap(); + expect_abort( ExitCode::USR_FORBIDDEN, - h.withdraw_funds(&mut rt, h.beneficiary, "a, "a, &TokenAmount::zero()), + h.withdraw_funds(&mut rt, another_actor, &one, &TokenAmount::zero(), &TokenAmount::zero()), ); + + //allow owner withdraw + h.withdraw_funds(&mut rt, h.owner, &one, &one, &TokenAmount::zero()).unwrap(); + //allow beneficiary withdraw + h.withdraw_funds(&mut rt, first_beneficiary_id, &one, &one, &TokenAmount::zero()).unwrap(); h.check_state(&rt); } From 2455ca6097ac732b10a02fddef20a3258cf54cf8 Mon Sep 17 00:00:00 2001 From: hunjixin <1084400399@qq.com> Date: Mon, 15 Aug 2022 16:09:58 +0800 Subject: [PATCH 4/6] test:slightly refrator and add more beneficiary test --- actors/miner/src/beneficiary.rs | 4 +- actors/miner/src/lib.rs | 35 +- actors/miner/tests/change_beneficiary_test.rs | 319 +++++++++--------- actors/miner/tests/util.rs | 44 ++- actors/miner/tests/withdraw_balance.rs | 2 +- 5 files changed, 218 insertions(+), 186 deletions(-) diff --git a/actors/miner/src/beneficiary.rs b/actors/miner/src/beneficiary.rs index 2c5243f7d..911ae9d4b 100644 --- a/actors/miner/src/beneficiary.rs +++ b/actors/miner/src/beneficiary.rs @@ -7,7 +7,7 @@ use fvm_shared::econ::TokenAmount; use num_traits::Zero; use std::ops::Sub; -#[derive(Debug, PartialEq, Clone, Serialize_tuple, Deserialize_tuple)] +#[derive(Debug, PartialEq, Eq, Clone, Serialize_tuple, Deserialize_tuple)] pub struct BeneficiaryTerm { /// The total amount the current beneficiary can withdraw. Monotonic, but reset when beneficiary changes. #[serde(with = "bigint_ser")] @@ -51,7 +51,7 @@ impl BeneficiaryTerm { } } -#[derive(Debug, PartialEq, Serialize_tuple, Deserialize_tuple)] +#[derive(Debug, PartialEq, Eq, Serialize_tuple, Deserialize_tuple)] pub struct PendingBeneficiaryChange { pub new_beneficiary: Address, #[serde(with = "bigint_ser")] diff --git a/actors/miner/src/lib.rs b/actors/miner/src/lib.rs index 657980d0b..1956fd74f 100644 --- a/actors/miner/src/lib.rs +++ b/actors/miner/src/lib.rs @@ -33,6 +33,7 @@ use fvm_shared::econ::TokenAmount; // The following errors are particular cases of illegal state. // They're not expected to ever happen, but if they do, distinguished codes can help us // diagnose the problem. + pub use beneficiary::*; use fil_actors_runtime::cbor::{deserialize, serialize, serialize_vec}; use fil_actors_runtime::runtime::builtins::Type; @@ -3286,36 +3287,30 @@ impl Actor { // Verify unlocked funds cover both InitialPledgeRequirement and FeeDebt // and repay fee debt now. let fee_to_burn = repay_debts_or_abort(rt, state)?; - let mut amount_withdrawn = std::cmp::min(&available_balance, ¶ms.amount_requested); + let mut amount_withdrawn = + std::cmp::min(&available_balance, ¶ms.amount_requested); if amount_withdrawn.is_negative() { - return Err(actor_error!( - illegal_state, - "negative amount to withdraw: {}", - amount_withdrawn - )); + return Err(actor_error!( + illegal_state, + "negative amount to withdraw: {}", + amount_withdrawn + )); } if info.beneficiary != info.owner { + // remaining_quota always zero and positive let remaining_quota = info.beneficiary_term.available(rt.curr_epoch()); - if remaining_quota.is_negative() { - return Err(actor_error!( - forbidden, - "quota not enough beneficiary {} quota {} used quota {} expiration epoch {} current epoch {}", - info.beneficiary, - info.beneficiary_term.quota, - info.beneficiary_term.used_quota, - info.beneficiary_term.expiration, - rt.curr_epoch() - )); - } amount_withdrawn = std::cmp::min(amount_withdrawn, &remaining_quota); if amount_withdrawn.is_positive() { info.beneficiary_term.used_quota += amount_withdrawn; state.save_info(rt.store(), &info).map_err(|e| { - e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "failed to save miner info") + e.downcast_default( + ExitCode::USR_ILLEGAL_STATE, + "failed to save miner info", + ) })?; } Ok((info, amount_withdrawn.clone(), newly_vested, fee_to_burn, state.clone())) - }else{ + } else { Ok((info, amount_withdrawn.clone(), newly_vested, fee_to_burn, state.clone())) } })?; @@ -3444,7 +3439,7 @@ impl Actor { } state.save_info(rt.store(), &info).map_err(|e| { - e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "failed to save miner info") + e.downcast_default(ExitCode::USR_FORBIDDEN, "failed to save miner info") })?; Ok(()) }) diff --git a/actors/miner/tests/change_beneficiary_test.rs b/actors/miner/tests/change_beneficiary_test.rs index f9013933c..19bbe29c8 100644 --- a/actors/miner/tests/change_beneficiary_test.rs +++ b/actors/miner/tests/change_beneficiary_test.rs @@ -27,28 +27,24 @@ fn successfully_change_owner_to_another_address_two_message() { let beneficiary_change = BeneficiaryChange::new(first_beneficiary_id, TokenAmount::from(100), ChainEpoch::from(200)); // proposal beneficiary change - h.change_beneficiary(&mut rt, h.owner, beneficiary_change.clone(), None).unwrap(); + h.change_beneficiary(&mut rt, h.owner, &beneficiary_change, None).unwrap(); // assert change has been made in state - let mut info = h.get_info(&rt); - let pending_beneficiary_term = info.pending_beneficiary_term.unwrap(); - assert_eq!(beneficiary_change.expiration, pending_beneficiary_term.new_expiration); - assert_eq!(beneficiary_change.quota, pending_beneficiary_term.new_quota); - assert_eq!(first_beneficiary_id, pending_beneficiary_term.new_beneficiary); + let mut beneficiary_return = h.get_beneficiary(&mut rt).unwrap(); + let pending_beneficiary_term = beneficiary_return.proposed.unwrap(); + assert_eq!(beneficiary_change, BeneficiaryChange::from_pending(&pending_beneficiary_term)); //confirm proposal h.change_beneficiary( &mut rt, first_beneficiary_id, - beneficiary_change.clone(), + &beneficiary_change, Some(first_beneficiary_id), ) .unwrap(); - info = h.get_info(&rt); - assert_eq!(None, info.pending_beneficiary_term); - assert_eq!(first_beneficiary_id, info.beneficiary); - assert_eq!(beneficiary_change.quota, info.beneficiary_term.quota); - assert_eq!(beneficiary_change.expiration, info.beneficiary_term.expiration); + beneficiary_return = h.get_beneficiary(&mut rt).unwrap(); + assert_eq!(None, beneficiary_return.proposed); + assert_eq!(beneficiary_change, BeneficiaryChange::from_active(&beneficiary_return.active)); h.check_state(&rt); } @@ -69,38 +65,38 @@ fn successfully_change_from_not_owner_beneficiary_to_another_address_three_messa TokenAmount::from(101), ChainEpoch::from(201), ); - h.change_beneficiary(&mut rt, h.owner, second_beneficiary_change.clone(), None).unwrap(); - let mut info = h.get_info(&rt); - assert_eq!(first_beneficiary_id, info.beneficiary); - - let mut pending_beneficiary_term = info.pending_beneficiary_term.unwrap(); - assert_eq!(second_beneficiary_change.expiration, pending_beneficiary_term.new_expiration); - assert_eq!(second_beneficiary_change.quota, pending_beneficiary_term.new_quota); - assert_eq!(second_beneficiary_id, pending_beneficiary_term.new_beneficiary); + h.change_beneficiary(&mut rt, h.owner, &second_beneficiary_change, None).unwrap(); + let mut beneficiary_return = h.get_beneficiary(&mut rt).unwrap(); + assert_eq!(first_beneficiary_id, beneficiary_return.active.beneficiary); + + let mut pending_beneficiary_term = beneficiary_return.proposed.unwrap(); + assert_eq!( + second_beneficiary_change, + BeneficiaryChange::from_pending(&pending_beneficiary_term) + ); assert!(!pending_beneficiary_term.approved_by_beneficiary); assert!(!pending_beneficiary_term.approved_by_nominee); - h.change_beneficiary(&mut rt, first_beneficiary_id, second_beneficiary_change.clone(), None) - .unwrap(); - info = h.get_info(&rt); - assert_eq!(first_beneficiary_id, info.beneficiary); - - pending_beneficiary_term = info.pending_beneficiary_term.unwrap(); - assert_eq!(second_beneficiary_change.expiration, pending_beneficiary_term.new_expiration); - assert_eq!(second_beneficiary_change.quota, pending_beneficiary_term.new_quota); - assert_eq!(second_beneficiary_id, pending_beneficiary_term.new_beneficiary); - assert!(pending_beneficiary_term.approved_by_beneficiary); - assert!(!pending_beneficiary_term.approved_by_nominee); - - h.change_beneficiary(&mut rt, second_beneficiary_id, second_beneficiary_change.clone(), None) - .unwrap(); - info = h.get_info(&rt); - assert_eq!(second_beneficiary_id, info.beneficiary); + h.change_beneficiary(&mut rt, second_beneficiary_id, &second_beneficiary_change, None).unwrap(); + beneficiary_return = h.get_beneficiary(&mut rt).unwrap(); + assert_eq!(first_beneficiary_id, beneficiary_return.active.beneficiary); - assert_eq!(None, info.pending_beneficiary_term); - assert_eq!(second_beneficiary_change.expiration, info.beneficiary_term.expiration); - assert_eq!(second_beneficiary_change.quota, info.beneficiary_term.quota); - assert_eq!(TokenAmount::zero(), info.beneficiary_term.used_quota); + pending_beneficiary_term = beneficiary_return.proposed.unwrap(); + assert_eq!( + second_beneficiary_change, + BeneficiaryChange::from_pending(&pending_beneficiary_term) + ); + assert!(!pending_beneficiary_term.approved_by_beneficiary); + assert!(pending_beneficiary_term.approved_by_nominee); + + h.change_beneficiary(&mut rt, first_beneficiary_id, &second_beneficiary_change, None).unwrap(); + beneficiary_return = h.get_beneficiary(&mut rt).unwrap(); + assert_eq!(None, beneficiary_return.proposed); + assert_eq!( + second_beneficiary_change, + BeneficiaryChange::from_active(&beneficiary_return.active) + ); + assert!(beneficiary_return.active.term.used_quota.is_zero()); } #[test] @@ -123,22 +119,25 @@ fn successfully_change_from_not_owner_beneficiary_to_another_address_when_benefi let another_quota = TokenAmount::from(1001); let another_expiration = ChainEpoch::from(3); let another_beneficiary_change = - BeneficiaryChange::new(second_beneficiary_id, another_quota.clone(), another_expiration); - h.change_beneficiary(&mut rt, h.owner, another_beneficiary_change.clone(), None).unwrap(); - let mut info = h.get_info(&rt); - - let pending_beneficiary_term = info.pending_beneficiary_term.unwrap(); - assert_eq!(second_beneficiary_id, pending_beneficiary_term.new_beneficiary); - assert_eq!(another_quota, pending_beneficiary_term.new_quota); - assert_eq!(another_expiration, pending_beneficiary_term.new_expiration); + BeneficiaryChange::new(second_beneficiary_id, another_quota, another_expiration); + h.change_beneficiary(&mut rt, h.owner, &another_beneficiary_change, None).unwrap(); + + let mut beneficiary_return = h.get_beneficiary(&mut rt).unwrap(); + let pending_beneficiary_term = beneficiary_return.proposed.unwrap(); + assert_eq!( + another_beneficiary_change, + BeneficiaryChange::from_pending(&pending_beneficiary_term) + ); - h.change_beneficiary(&mut rt, second_beneficiary_id, another_beneficiary_change, None).unwrap(); - info = h.get_info(&rt); - assert_eq!(None, info.pending_beneficiary_term); - assert_eq!(second_beneficiary_id, info.beneficiary); - assert_eq!(another_quota, info.beneficiary_term.quota); - assert_eq!(another_expiration, info.beneficiary_term.expiration); - assert_eq!(TokenAmount::zero(), info.beneficiary_term.used_quota); + h.change_beneficiary(&mut rt, second_beneficiary_id, &another_beneficiary_change, None) + .unwrap(); + beneficiary_return = h.get_beneficiary(&mut rt).unwrap(); + assert_eq!(None, beneficiary_return.proposed); + assert_eq!( + another_beneficiary_change, + BeneficiaryChange::from_active(&beneficiary_return.active) + ); + assert!(beneficiary_return.active.term.used_quota.is_zero()); h.check_state(&rt); } @@ -150,26 +149,24 @@ fn successfully_owner_immediate_revoking_unapproved_proposal() { let beneficiary_change = BeneficiaryChange::new(first_beneficiary_id, TokenAmount::from(100), ChainEpoch::from(200)); // proposal beneficiary change - h.change_beneficiary(&mut rt, h.owner, beneficiary_change.clone(), None).unwrap(); + h.change_beneficiary(&mut rt, h.owner, &beneficiary_change, None).unwrap(); // assert change has been made in state - let mut info = h.get_info(&rt); - let pending_beneficiary_term = info.pending_beneficiary_term.unwrap(); - assert_eq!(beneficiary_change.expiration, pending_beneficiary_term.new_expiration); - assert_eq!(beneficiary_change.quota, pending_beneficiary_term.new_quota); - assert_eq!(first_beneficiary_id, pending_beneficiary_term.new_beneficiary); + let mut beneficiary_return = h.get_beneficiary(&mut rt).unwrap(); + let pending_beneficiary_term = beneficiary_return.proposed.unwrap(); + assert_eq!(beneficiary_change, BeneficiaryChange::from_pending(&pending_beneficiary_term)); //revoking unapprovel proposal - h.change_beneficiary( - &mut rt, - h.owner, - BeneficiaryChange::new(h.owner, TokenAmount::zero(), ChainEpoch::from(0)), - Some(h.owner), - ) - .unwrap(); - - info = h.get_info(&rt); - assert_eq!(None, info.pending_beneficiary_term); - assert_eq!(h.owner, info.beneficiary); + let back_owner_beneficiary_change = + BeneficiaryChange::new(h.owner, TokenAmount::zero(), ChainEpoch::from(0)); + h.change_beneficiary(&mut rt, h.owner, &back_owner_beneficiary_change, Some(h.owner)).unwrap(); + + beneficiary_return = h.get_beneficiary(&mut rt).unwrap(); + assert_eq!(None, beneficiary_return.proposed); + assert_eq!( + back_owner_beneficiary_change, + BeneficiaryChange::from_active(&beneficiary_return.active) + ); + assert!(beneficiary_return.active.term.quota.is_zero()); h.check_state(&rt); } @@ -189,20 +186,17 @@ fn successfully_immediately_change_back_to_owner_address_while_used_up_quota() { .unwrap(); h.withdraw_funds(&mut rt, h.beneficiary, "a, "a, &TokenAmount::zero()).unwrap(); - h.change_beneficiary( - &mut rt, - h.owner, - BeneficiaryChange::new(h.owner, TokenAmount::zero(), ChainEpoch::from(0)), - Some(h.owner), - ) - .unwrap(); - - let info = h.get_info(&rt); - assert_eq!(None, info.pending_beneficiary_term); - assert_eq!(h.owner, info.beneficiary); - assert_eq!(TokenAmount::zero(), info.beneficiary_term.quota); - assert_eq!(ChainEpoch::from(0), info.beneficiary_term.expiration); - assert_eq!(TokenAmount::zero(), info.beneficiary_term.used_quota); + let back_owner_beneficiary_change = + BeneficiaryChange::new(h.owner, TokenAmount::zero(), ChainEpoch::from(0)); + h.change_beneficiary(&mut rt, h.owner, &back_owner_beneficiary_change, Some(h.owner)).unwrap(); + + let beneficiary_return = h.get_beneficiary(&mut rt).unwrap(); + assert_eq!(None, beneficiary_return.proposed); + assert_eq!( + back_owner_beneficiary_change, + BeneficiaryChange::from_active(&beneficiary_return.active) + ); + assert!(beneficiary_return.active.term.quota.is_zero()); h.check_state(&rt); } @@ -221,25 +215,22 @@ fn successfully_immediately_change_back_to_owner_while_expired() { .unwrap(); rt.set_epoch(201); - h.change_beneficiary( - &mut rt, - h.owner, - BeneficiaryChange::new(h.owner, TokenAmount::zero(), ChainEpoch::from(0)), - Some(h.owner), - ) - .unwrap(); - - let info = h.get_info(&rt); - assert_eq!(None, info.pending_beneficiary_term); - assert_eq!(h.owner, info.beneficiary); - assert_eq!(TokenAmount::zero(), info.beneficiary_term.quota); - assert_eq!(ChainEpoch::from(0), info.beneficiary_term.expiration); - assert_eq!(TokenAmount::zero(), info.beneficiary_term.used_quota); + let back_owner_beneficiary_change = + BeneficiaryChange::new(h.owner, TokenAmount::zero(), ChainEpoch::from(0)); + h.change_beneficiary(&mut rt, h.owner, &back_owner_beneficiary_change, Some(h.owner)).unwrap(); + + let beneficiary_return = h.get_beneficiary(&mut rt).unwrap(); + assert_eq!(None, beneficiary_return.proposed); + assert_eq!( + back_owner_beneficiary_change, + BeneficiaryChange::from_active(&beneficiary_return.active) + ); + assert!(beneficiary_return.active.term.quota.is_zero()); h.check_state(&rt); } #[test] -fn successfully_increase_quota() { +fn successfully_change_quota_and_test_withdraw() { let (mut h, mut rt) = setup(); let first_beneficiary_id = Address::new_id(999); let beneficiary_term = @@ -247,33 +238,63 @@ fn successfully_increase_quota() { h.propose_approve_initial_beneficiary(&mut rt, first_beneficiary_id, beneficiary_term.clone()) .unwrap(); - //increase quota - let increase_quota = TokenAmount::from(100) + beneficiary_term.quota.clone(); - let increase_beneficiary_change = BeneficiaryChange::new( + let withdraw_fund = TokenAmount::from(80); + h.withdraw_funds(&mut rt, h.beneficiary, &withdraw_fund, &withdraw_fund, &TokenAmount::zero()) + .unwrap(); + //decrease quota + let decrease_quota = TokenAmount::from(50); + let decrease_beneficiary_change = + BeneficiaryChange::new(first_beneficiary_id, decrease_quota, beneficiary_term.expiration); + h.change_beneficiary(&mut rt, h.owner, &decrease_beneficiary_change, None).unwrap(); + h.change_beneficiary( + &mut rt, first_beneficiary_id, - increase_quota.clone(), - beneficiary_term.expiration, + &decrease_beneficiary_change, + Some(first_beneficiary_id), + ) + .unwrap(); + let mut beneficiary_return = h.get_beneficiary(&mut rt).unwrap(); + assert_eq!( + decrease_beneficiary_change, + BeneficiaryChange::from_active(&beneficiary_return.active) ); - h.change_beneficiary(&mut rt, h.owner, increase_beneficiary_change.clone(), None).unwrap(); - let mut info = h.get_info(&rt); - let pending_beneficiary_term = info.pending_beneficiary_term.unwrap(); - assert_eq!(first_beneficiary_id, info.beneficiary); - assert_eq!(increase_quota, pending_beneficiary_term.new_quota); - assert_eq!(beneficiary_term.expiration, pending_beneficiary_term.new_expiration); + assert_eq!(withdraw_fund, beneficiary_return.active.term.used_quota); + + //withdraw 0 zero + let withdraw_left = TokenAmount::from(20); + h.withdraw_funds( + &mut rt, + h.beneficiary, + &withdraw_left, + &TokenAmount::zero(), + &TokenAmount::zero(), + ) + .unwrap(); + + let increase_quota = TokenAmount::from(120); + let increase_beneficiary_change = + BeneficiaryChange::new(first_beneficiary_id, increase_quota, beneficiary_term.expiration); - //confirm increase quota + h.change_beneficiary(&mut rt, h.owner, &increase_beneficiary_change, None).unwrap(); h.change_beneficiary( &mut rt, first_beneficiary_id, - increase_beneficiary_change, + &increase_beneficiary_change, Some(first_beneficiary_id), ) .unwrap(); - info = h.get_info(&rt); - assert_eq!(None, info.pending_beneficiary_term); - assert_eq!(first_beneficiary_id, info.beneficiary); - assert_eq!(increase_quota, info.beneficiary_term.quota); - assert_eq!(beneficiary_term.expiration, info.beneficiary_term.expiration); + + beneficiary_return = h.get_beneficiary(&mut rt).unwrap(); + assert_eq!( + increase_beneficiary_change, + BeneficiaryChange::from_active(&beneficiary_return.active) + ); + assert_eq!(withdraw_fund, beneficiary_return.active.term.used_quota); + + //success withdraw 40 atto fil + let withdraw_left = TokenAmount::from(40); + h.withdraw_funds(&mut rt, h.beneficiary, &withdraw_left, &withdraw_left, &TokenAmount::zero()) + .unwrap(); h.check_state(&rt); } @@ -283,14 +304,14 @@ fn fails_approval_message_with_invalidate_params() { let first_beneficiary_id = Address::new_id(999); // proposal beneficiary - h.change_beneficiary( - &mut rt, - h.owner, - BeneficiaryChange::new(first_beneficiary_id, TokenAmount::from(100), 200), - None, - ) - .unwrap(); - assert!(h.get_info(&rt).pending_beneficiary_term.is_some()); + let beneficiary_change = + &BeneficiaryChange::new(first_beneficiary_id, TokenAmount::from(100), 200); + h.change_beneficiary(&mut rt, h.owner, beneficiary_change, None).unwrap(); + let beneficiary_return = h.get_beneficiary(&mut rt).unwrap(); + assert_eq!( + beneficiary_change, + &BeneficiaryChange::from_pending(&beneficiary_return.proposed.unwrap()) + ); //expiration in approval message must equal with proposal expect_abort( @@ -298,7 +319,7 @@ fn fails_approval_message_with_invalidate_params() { h.change_beneficiary( &mut rt, first_beneficiary_id, - BeneficiaryChange::new(first_beneficiary_id, TokenAmount::from(100), 201), + &BeneficiaryChange::new(first_beneficiary_id, TokenAmount::from(100), 201), None, ), ); @@ -309,7 +330,7 @@ fn fails_approval_message_with_invalidate_params() { h.change_beneficiary( &mut rt, first_beneficiary_id, - BeneficiaryChange::new(first_beneficiary_id, TokenAmount::from(101), 200), + &BeneficiaryChange::new(first_beneficiary_id, TokenAmount::from(101), 200), None, ), ); @@ -321,7 +342,7 @@ fn fails_approval_message_with_invalidate_params() { h.change_beneficiary( &mut rt, first_beneficiary_id, - BeneficiaryChange::new(second_beneficiary_id, TokenAmount::from(100), 200), + &BeneficiaryChange::new(second_beneficiary_id, TokenAmount::from(100), 200), None, ), ); @@ -338,7 +359,7 @@ fn fails_proposal_beneficiary_with_invalidate_params() { h.change_beneficiary( &mut rt, first_beneficiary_id, - BeneficiaryChange::new(first_beneficiary_id, TokenAmount::from(100), 200), + &BeneficiaryChange::new(first_beneficiary_id, TokenAmount::from(100), 200), None, ), ); @@ -349,7 +370,7 @@ fn fails_proposal_beneficiary_with_invalidate_params() { h.change_beneficiary( &mut rt, h.owner, - BeneficiaryChange::new(first_beneficiary_id, TokenAmount::from(0), 200), + &BeneficiaryChange::new(first_beneficiary_id, TokenAmount::from(0), 200), None, ), ); @@ -360,7 +381,7 @@ fn fails_proposal_beneficiary_with_invalidate_params() { h.change_beneficiary( &mut rt, h.owner, - BeneficiaryChange::new(h.owner, TokenAmount::from(20), 0), + &BeneficiaryChange::new(h.owner, TokenAmount::from(20), 0), None, ), ); @@ -371,7 +392,7 @@ fn fails_proposal_beneficiary_with_invalidate_params() { h.change_beneficiary( &mut rt, h.owner, - BeneficiaryChange::new(h.owner, TokenAmount::from(0), 1), + &BeneficiaryChange::new(h.owner, TokenAmount::from(0), 1), None, ), ); @@ -380,32 +401,28 @@ fn fails_proposal_beneficiary_with_invalidate_params() { #[test] fn successfully_get_beneficiary() { let (mut h, mut rt) = setup(); - let mut info = h.get_info(&rt); - assert_eq!(h.owner, info.beneficiary); - assert_eq!(ChainEpoch::from(0), info.beneficiary_term.expiration); - assert_eq!(TokenAmount::zero(), info.beneficiary_term.quota); - assert_eq!(TokenAmount::zero(), info.beneficiary_term.used_quota); + let mut beneficiary_return = h.get_beneficiary(&mut rt).unwrap(); + assert_eq!(h.owner, beneficiary_return.active.beneficiary); + assert_eq!(BeneficiaryTerm::default(), beneficiary_return.active.term); let first_beneficiary_id = Address::new_id(999); let beneficiary_term = BeneficiaryTerm::new(TokenAmount::from(100), TokenAmount::zero(), ChainEpoch::from(200)); - h.propose_approve_initial_beneficiary(&mut rt, first_beneficiary_id, beneficiary_term.clone()) - .unwrap(); + h.propose_approve_initial_beneficiary(&mut rt, first_beneficiary_id, beneficiary_term).unwrap(); - info = h.get_info(&rt); - assert_eq!(first_beneficiary_id, info.beneficiary); - assert_eq!(beneficiary_term.expiration, info.beneficiary_term.expiration); - assert_eq!(beneficiary_term.quota, info.beneficiary_term.quota); - assert_eq!(TokenAmount::zero(), info.beneficiary_term.used_quota); + let beneficiary = h.get_beneficiary(&mut rt).unwrap(); + let mut info = h.get_info(&rt); + assert_eq!(beneficiary.active.beneficiary, info.beneficiary); + assert_eq!(beneficiary.active.term.expiration, info.beneficiary_term.expiration); + assert_eq!(beneficiary.active.term.quota, info.beneficiary_term.quota); + assert_eq!(beneficiary.active.term.used_quota, info.beneficiary_term.used_quota); let withdraw_fund = TokenAmount::from(40); - let left_quota = TokenAmount::from(60); h.withdraw_funds(&mut rt, h.beneficiary, &withdraw_fund, &withdraw_fund, &TokenAmount::zero()) .unwrap(); + beneficiary_return = h.get_beneficiary(&mut rt).unwrap(); info = h.get_info(&rt); - assert_eq!(first_beneficiary_id, info.beneficiary); - assert_eq!(beneficiary_term.expiration, info.beneficiary_term.expiration); - assert_eq!(left_quota, info.beneficiary_term.available(rt.epoch)); - assert_eq!(withdraw_fund, info.beneficiary_term.used_quota); + assert_eq!(beneficiary_return.active.beneficiary, info.beneficiary); + assert_eq!(beneficiary_return.active.term, info.beneficiary_term); } diff --git a/actors/miner/tests/util.rs b/actors/miner/tests/util.rs index b8157bf10..27ea19755 100644 --- a/actors/miner/tests/util.rs +++ b/actors/miner/tests/util.rs @@ -12,20 +12,20 @@ use fil_actor_miner::{ aggregate_pre_commit_network_fee, aggregate_prove_commit_network_fee, consensus_fault_penalty, initial_pledge_for_power, locked_reward_from_reward, max_prove_commit_duration, new_deadline_info_from_offset_and_epoch, pledge_penalty_for_continued_fault, power_for_sectors, - qa_power_for_sector, qa_power_for_weight, reward_for_consensus_slash_report, Actor, - ApplyRewardParams, BeneficiaryTerm, BitFieldQueue, ChangeBeneficiaryParams, + qa_power_for_sector, qa_power_for_weight, reward_for_consensus_slash_report, ActiveBeneficiary, + Actor, ApplyRewardParams, BeneficiaryTerm, BitFieldQueue, ChangeBeneficiaryParams, ChangeMultiaddrsParams, ChangePeerIDParams, ChangeWorkerAddressParams, CheckSectorProvenParams, CompactPartitionsParams, CompactSectorNumbersParams, ConfirmSectorProofsParams, CronEventPayload, Deadline, DeadlineInfo, Deadlines, DeclareFaultsParams, DeclareFaultsRecoveredParams, DeferredCronEventParams, DisputeWindowedPoStParams, ExpirationQueue, ExpirationSet, ExtendSectorExpirationParams, FaultDeclaration, GetBeneficiaryReturn, GetControlAddressesReturn, Method, - MinerConstructorParams as ConstructorParams, MinerInfo, Partition, PoStPartition, PowerPair, - PreCommitSectorBatchParams, PreCommitSectorParams, ProveCommitSectorParams, - RecoveryDeclaration, ReportConsensusFaultParams, SectorOnChainInfo, SectorPreCommitOnChainInfo, - Sectors, State, SubmitWindowedPoStParams, TerminateSectorsParams, TerminationDeclaration, - VestingFunds, WindowedPoSt, WithdrawBalanceParams, WithdrawBalanceReturn, - CRON_EVENT_PROVING_DEADLINE, SECTORS_AMT_BITWIDTH, + MinerConstructorParams as ConstructorParams, MinerInfo, Partition, PendingBeneficiaryChange, + PoStPartition, PowerPair, PreCommitSectorBatchParams, PreCommitSectorParams, + ProveCommitSectorParams, RecoveryDeclaration, ReportConsensusFaultParams, SectorOnChainInfo, + SectorPreCommitOnChainInfo, Sectors, State, SubmitWindowedPoStParams, TerminateSectorsParams, + TerminationDeclaration, VestingFunds, WindowedPoSt, WithdrawBalanceParams, + WithdrawBalanceReturn, CRON_EVENT_PROVING_DEADLINE, SECTORS_AMT_BITWIDTH, }; use fil_actor_miner::{Method as MinerMethod, ProveCommitAggregateParams}; use fil_actor_power::{ @@ -2088,7 +2088,7 @@ impl ActorHarness { &mut self, rt: &mut MockRuntime, expect_caller: Address, - beneficiary_change: BeneficiaryChange, + beneficiary_change: &BeneficiaryChange, expect_beneficiary_addr: Option
, ) -> Result { rt.set_address_actor_type( @@ -2098,7 +2098,7 @@ impl ActorHarness { let caller_id = rt.get_id_address(&expect_caller).unwrap(); let param = ChangeBeneficiaryParams { new_beneficiary: beneficiary_change.beneficiary_addr, - new_quota: beneficiary_change.quota, + new_quota: beneficiary_change.quota.clone(), new_expiration: beneficiary_change.expiration, }; rt.set_caller(*ACCOUNT_ACTOR_CODE_ID, caller_id); @@ -2109,6 +2109,8 @@ impl ActorHarness { rt.verify(); if let Some(beneficiary) = expect_beneficiary_addr { + let beneficiary_return = self.get_beneficiary(rt)?; + assert_eq!(beneficiary, beneficiary_return.active.beneficiary); self.beneficiary = beneficiary.clone(); } @@ -2120,7 +2122,7 @@ impl ActorHarness { rt: &mut MockRuntime, ) -> Result { rt.expect_validate_caller_any(); - let ret = rt.call::(Method::ChangeBeneficiary as u64, &RawBytes::default())?; + let ret = rt.call::(Method::GetBeneficiary as u64, &RawBytes::default())?; rt.verify(); Ok(ret.deserialize::().unwrap()) } @@ -2347,7 +2349,7 @@ pub struct PoStDisputeResult { pub expected_reward: Option, } -#[derive(Clone)] +#[derive(Clone, Debug, PartialEq, Eq)] pub struct BeneficiaryChange { pub beneficiary_addr: Address, pub quota: TokenAmount, @@ -2359,6 +2361,24 @@ impl BeneficiaryChange { pub fn new(beneficiary_addr: Address, quota: TokenAmount, expiration: ChainEpoch) -> Self { BeneficiaryChange { beneficiary_addr, quota, expiration } } + + #[allow(dead_code)] + pub fn from_pending(pending_beneficiary: &PendingBeneficiaryChange) -> Self { + BeneficiaryChange { + beneficiary_addr: pending_beneficiary.new_beneficiary, + quota: pending_beneficiary.new_quota.clone(), + expiration: pending_beneficiary.new_expiration, + } + } + + #[allow(dead_code)] + pub fn from_active(info: &ActiveBeneficiary) -> Self { + BeneficiaryChange { + beneficiary_addr: info.beneficiary, + quota: info.term.quota.clone(), + expiration: info.term.expiration, + } + } } #[allow(dead_code)] diff --git a/actors/miner/tests/withdraw_balance.rs b/actors/miner/tests/withdraw_balance.rs index 70e3fa320..01133796d 100644 --- a/actors/miner/tests/withdraw_balance.rs +++ b/actors/miner/tests/withdraw_balance.rs @@ -123,7 +123,7 @@ fn successfully_withdraw_allow_zero() { } #[test] -fn successfully_withdraw_more_than_quota() { +fn successfully_withdraw_limited_to_quota() { let mut h = ActorHarness::new(PERIOD_OFFSET); let mut rt = h.new_runtime(); rt.set_balance(TokenAmount::from(BIG_BALANCE)); From 10920adeb6d1c86e23a27dedda8ec205f60b42d2 Mon Sep 17 00:00:00 2001 From: hunjixin <1084400399@qq.com> Date: Mon, 22 Aug 2022 13:55:37 +0800 Subject: [PATCH 5/6] fix: check caller when confirm beneficiary proposal --- actors/miner/src/lib.rs | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/actors/miner/src/lib.rs b/actors/miner/src/lib.rs index 1956fd74f..34a0c6021 100644 --- a/actors/miner/src/lib.rs +++ b/actors/miner/src/lib.rs @@ -3326,11 +3326,10 @@ impl Actor { Ok(WithdrawBalanceReturn { amount_withdrawn }) } - /// Proposes or confirms a change of owner address. - /// If invoked by the current owner, proposes a new owner address for confirmation. If the proposed address is the - /// current owner address, revokes any existing proposal. - /// If invoked by the previously proposed address, with the same proposal, changes the current owner address to be - /// that proposed address. + /// Proposes or confirms a change of beneficiary address. + /// A proposal must be submitted by the owner, and takes effect after approval of both the proposed beneficiary and current beneficiary, + /// if applicable, any current beneficiary that has time and quota remaining. + //// See FIP-0029, https://github.com/filecoin-project/FIPs/blob/master/FIPS/fip-0029.md fn change_beneficiary( rt: &mut RT, params: ChangeBeneficiaryParams, @@ -3339,13 +3338,14 @@ impl Actor { BS: Blockstore, RT: Runtime, { + let caller = rt.message().caller(); let new_beneficiary = rt.resolve_address(¶ms.new_beneficiary).ok_or_else(|| { actor_error!(illegal_argument, "unable to resolve address: {}", params.new_beneficiary) })?; rt.transaction(|state: &mut State, rt| { let mut info = get_miner_info(rt.store(), state)?; - if rt.message().caller() == info.owner { + if caller == info.owner { // This is a ChangeBeneficiary proposal when the caller is Owner if new_beneficiary != info.owner { // When beneficiary is not owner, just check quota in params, @@ -3387,6 +3387,16 @@ impl Actor { } info.pending_beneficiary_term = Some(pending_beneficiary_term); } else if let Some(pending_term) = &info.pending_beneficiary_term { + if caller != info.beneficiary && caller != pending_term.new_beneficiary { + return Err(actor_error!( + forbidden, + "message caller {} is neither proposal beneficiary{} nor current beneficiary{}", + caller, + params.new_beneficiary, + info.beneficiary + )); + } + if pending_term.new_beneficiary != new_beneficiary { return Err(actor_error!( illegal_argument, @@ -3416,11 +3426,11 @@ impl Actor { } if let Some(pending_term) = info.pending_beneficiary_term.as_mut() { - if rt.message().caller() == info.beneficiary { + if caller == info.beneficiary { pending_term.approved_by_beneficiary = true } - if rt.message().caller() == new_beneficiary { + if caller == new_beneficiary { pending_term.approved_by_nominee = true } @@ -3439,7 +3449,7 @@ impl Actor { } state.save_info(rt.store(), &info).map_err(|e| { - e.downcast_default(ExitCode::USR_FORBIDDEN, "failed to save miner info") + e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "failed to save miner info") })?; Ok(()) }) From a38bfbdcc862aff0b40cb4d0f89d70397d883687 Mon Sep 17 00:00:00 2001 From: hunjixin <1084400399@qq.com> Date: Mon, 29 Aug 2022 13:29:44 +0800 Subject: [PATCH 6/6] fix: change beneficiary ActorId to Address --- actors/miner/src/lib.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/actors/miner/src/lib.rs b/actors/miner/src/lib.rs index 34a0c6021..f3960ba48 100644 --- a/actors/miner/src/lib.rs +++ b/actors/miner/src/lib.rs @@ -3339,9 +3339,14 @@ impl Actor { RT: Runtime, { let caller = rt.message().caller(); - let new_beneficiary = rt.resolve_address(¶ms.new_beneficiary).ok_or_else(|| { - actor_error!(illegal_argument, "unable to resolve address: {}", params.new_beneficiary) - })?; + let new_beneficiary = + Address::new_id(rt.resolve_address(¶ms.new_beneficiary).ok_or_else(|| { + actor_error!( + illegal_argument, + "unable to resolve address: {}", + params.new_beneficiary + ) + })?); rt.transaction(|state: &mut State, rt| { let mut info = get_miner_info(rt.store(), state)?;