Multisig approve call should return error data back to the user #1422
Conversation
| if let Some(r) = e.take_data() { | ||
| out = RawBytes::new(r.data); | ||
| } | ||
|
|
There was a problem hiding this comment.
@arajasek One question:
Why does this function "succeed" i.e. we return an Ok value from this function even though the transaction fails ?
There was a problem hiding this comment.
Because we successfully fail. If we returned an error, we'd revert the multisig's state leaving the pending message in the state. The user would then have to send a second message to "abort" it.
This is one of the many reasons we want to let users deploy their own smart contracts/actors. Because there's no "one size fits all" answer here.
There was a problem hiding this comment.
Expanding just a bit, I think "philosophically" it makes sense to succeed here. When this method is called (or a top-level "approve and execute" method is called), you're asked to execute the transaction, if possible. If for some reason the transaction isn't executable, that's a failure.
But if the transaction executes-and-fails, that's not really a failure for the top-level viewer. We were asked to execute the tx, we were able to do so, and the result of that execution happened to be a failure.
But @Stebalien is right, there are definitely users who won't want this behaviour, so getting this out of built-in actors would be great.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1422 +/- ##
==========================================
+ Coverage 91.03% 91.10% +0.06%
==========================================
Files 145 145
Lines 27848 27850 +2
==========================================
+ Hits 25352 25373 +21
+ Misses 2496 2477 -19
|
| if let Some(r) = e.take_data() { | ||
| out = RawBytes::new(r.data); | ||
| } | ||
|
|
There was a problem hiding this comment.
Because we successfully fail. If we returned an error, we'd revert the multisig's state leaving the pending message in the state. The user would then have to send a second message to "abort" it.
This is one of the many reasons we want to let users deploy their own smart contracts/actors. Because there's no "one size fits all" answer here.
|
@arajasek thoughts on merging/punting? |
| if let Some(r) = e.take_data() { | ||
| out = RawBytes::new(r.data); | ||
| } | ||
|
|
There was a problem hiding this comment.
Expanding just a bit, I think "philosophically" it makes sense to succeed here. When this method is called (or a top-level "approve and execute" method is called), you're asked to execute the transaction, if possible. If for some reason the transaction isn't executable, that's a failure.
But if the transaction executes-and-fails, that's not really a failure for the top-level viewer. We were asked to execute the tx, we were able to do so, and the result of that execution happened to be a failure.
But @Stebalien is right, there are definitely users who won't want this behaviour, so getting this out of built-in actors would be great.
Closes #1251 .