Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: remove beneficiary from self-destruct #1362

Merged
merged 1 commit into from
Sep 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions actors/paych/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -293,8 +293,11 @@ impl Actor {
extract_send_result(rt.send_simple(&st.to, METHOD_SEND, None, st.to_send))
.map_err(|e| e.wrap("Failed to send funds to `to` address"))?;

// the remaining balance will be returned to "From" upon deletion.
rt.delete_actor(&st.from)?;
// return remaining balance back to the "from" address.
extract_send_result(rt.send_simple(&st.from, METHOD_SEND, None, rt.current_balance()))
.map_err(|e| e.wrap("Failed to send funds to `from` address"))?;

rt.delete_actor()?;

Ok(())
}
Expand Down
10 changes: 9 additions & 1 deletion actors/paych/tests/paych_actor_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -949,7 +949,15 @@ mod actor_collect {
// Collect.
rt.set_caller(*ACCOUNT_ACTOR_CODE_ID, st.to);
rt.expect_validate_caller_addr(vec![st.from, st.to]);
rt.expect_delete_actor(st.from);
rt.expect_send_simple(
st.from,
METHOD_SEND,
Default::default(),
&*rt.balance.borrow() - &st.to_send,
Default::default(),
ExitCode::OK,
);
rt.expect_delete_actor();
let res = call(&rt, Method::Collect as u64, None);
assert!(res.is_none());
check_state(&rt);
Expand Down
4 changes: 2 additions & 2 deletions runtime/src/runtime/fvm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -350,13 +350,13 @@ where
})
}

fn delete_actor(&self, beneficiary: &Address) -> Result<(), ActorError> {
fn delete_actor(&self) -> Result<(), ActorError> {
if *self.in_transaction.borrow() {
return Err(
actor_error!(assertion_failed; "delete_actor is not allowed during transaction"),
);
}
Ok(fvm::sself::self_destruct(beneficiary)?)
Ok(fvm::sself::self_destruct(false)?)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing false here will cause self_destruct to fail if there are any remaining funds. At the moment, that's the only behavior we want in the builtin actors.

}

fn total_fil_circ_supply(&self) -> TokenAmount {
Expand Down
7 changes: 4 additions & 3 deletions runtime/src/runtime/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -206,10 +206,11 @@ pub trait Runtime: Primitives + Verifier + RuntimePolicy {
predictable_address: Option<Address>,
) -> Result<(), ActorError>;

/// Deletes the executing actor from the state tree, transferring any balance to beneficiary.
/// Aborts if the beneficiary does not exist.
/// Deletes the executing actor from the state tree. Fails if there is any unspent balance in
/// the actor.
///
/// May only be called by the actor itself.
fn delete_actor(&self, beneficiary: &Address) -> Result<(), ActorError>;
fn delete_actor(&self) -> Result<(), ActorError>;

/// Returns whether the specified CodeCID belongs to a built-in actor.
fn resolve_builtin_actor_type(&self, code_id: &Cid) -> Option<Type>;
Expand Down
24 changes: 8 additions & 16 deletions runtime/src/test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ pub struct Expectations {
pub expect_validate_caller_type: Option<Vec<Type>>,
pub expect_sends: VecDeque<ExpectedMessage>,
pub expect_create_actor: Option<ExpectCreateActor>,
pub expect_delete_actor: Option<Address>,
pub expect_delete_actor: bool,
pub expect_verify_sigs: VecDeque<ExpectedVerifySig>,
pub expect_verify_post: Option<ExpectVerifyPoSt>,
pub expect_compute_unsealed_sector_cid: VecDeque<ExpectComputeUnsealedSectorCid>,
Expand Down Expand Up @@ -245,11 +245,7 @@ impl Expectations {
"expected actor to be created, uncreated actor: {:?}",
this.expect_create_actor
);
assert!(
this.expect_delete_actor.is_none(),
"expected actor to be deleted: {:?}",
this.expect_delete_actor
);
assert!(!this.expect_delete_actor, "expected actor to be deleted",);
assert!(
this.expect_verify_sigs.is_empty(),
"expect_verify_sigs: {:?}, not received",
Expand Down Expand Up @@ -624,8 +620,8 @@ impl<BS: Blockstore> MockRuntime<BS> {
}

#[allow(dead_code)]
pub fn expect_delete_actor(&self, beneficiary: Address) {
self.expectations.borrow_mut().expect_delete_actor = Some(beneficiary);
pub fn expect_delete_actor(&self) {
self.expectations.borrow_mut().expect_delete_actor = true;
}

#[allow(dead_code)]
Expand Down Expand Up @@ -1185,18 +1181,14 @@ impl<BS: Blockstore> Runtime for MockRuntime<BS> {
Ok(())
}

fn delete_actor(&self, addr: &Address) -> Result<(), ActorError> {
fn delete_actor(&self) -> Result<(), ActorError> {
self.require_in_call();
if *self.in_transaction.borrow() {
return Err(actor_error!(assertion_failed; "side-effect within transaction"));
}
let exp_act = self.expectations.borrow_mut().expect_delete_actor.take();
if exp_act.is_none() {
panic!("unexpected call to delete actor: {}", addr);
}
if exp_act.as_ref().unwrap() != addr {
panic!("attempt to delete wrong actor. Expected: {}, got: {}", exp_act.unwrap(), addr);
}
let mut exp = self.expectations.borrow_mut();
assert!(exp.expect_delete_actor, "unexpected call to delete actor");
exp.expect_delete_actor = false;
Ok(())
}

Expand Down
2 changes: 1 addition & 1 deletion test_vm/src/messaging.rs
Original file line number Diff line number Diff line change
Expand Up @@ -611,7 +611,7 @@ where
Ok(Address::new_actor(&b))
}

fn delete_actor(&self, _beneficiary: &Address) -> Result<(), ActorError> {
fn delete_actor(&self) -> Result<(), ActorError> {
panic!("TODO implement me")
}

Expand Down
Loading