-
Notifications
You must be signed in to change notification settings - Fork 27
Conversation
14fcc05
to
ae04e6e
Compare
gen/suites/actor_deletion/main.go
Outdated
ID: "fail-delete-w-balance-and-self-beneficiary", | ||
Version: "v1", | ||
Desc: "fails when actor with non-zero balance is deleted but beneficiary is the calling actor", | ||
Comment: "should abort if the beneficiary is the calling actor, see https://github.com/filecoin-project/specs-actors/blob/bcd83e8eb0a98b684851e574a2bd8d4130e21a51/actors/runtime/runtime.go#L117", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ZenGround0 @anorth 🙏 can you confirm what the exit code should be? My guess would be SysErrorIllegalArgument
based on https://github.com/filecoin-project/specs-actors/blob/master/actors/runtime/exitcode/reserved.go#L48?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SysErrorIllegalArgument
sounds perfect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gen/suites/actor_deletion/main.go
Outdated
// Desc: "fails when actor with non-zero balance is deleted but beneficiary address is unknown", | ||
// }, | ||
// Selector: Selector{"chaos_actor": "true"}, | ||
// Func: deleteActorWithBeneficiary(big.NewInt(50), MustNewSECP256K1Addr("!👹*_👹!"), exitcode.SysErrorIllegalActor), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ZenGround0 @anorth can you confirm that this shouldn't cause a fatal panic and instead should abort and return an SysErrorIllegalActor
exit code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you confirm that this shouldn't cause a fatal panic and instead should abort
Looks like it. FYI @magik6k
Seems like a fine exit code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Roll with those suggestions. We could also consider changing the defined behaviour to burn the funds if the beneficiary is either the actor itself, or an unresolvable ID- or Actor- address (i.e. still allow a pubkey beneficiary). Since we control all the callers of DeleteActor right now and are pretty sure they won't exercise this, it's not critical to decide now, and aborting is fine. |
ae04e6e
to
6e4ea63
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alanshaw This LGTM so far! A few comments. Re-request another review from me when you're done adjusting based on Wyatt's and Alex's feedback!
Co-authored-by: Raúl Kripalani <[email protected]>
Co-authored-by: Raúl Kripalani <[email protected]>
This PR adds tests for actor deletion.
It adds a method to the chaos actor allowing it to delete itself.
...and adds test vectors for:
resolves #14