Fix Safe Assert #2784
Fix Safe Assert #2784
Conversation
|
@erikzhang @roman-khimov now Log (Safe Assert) will FAULT, but at least the exception will be saved. What do you think? |
|
|
||
| if (!CurrentContext.GetState<ExecutionContextState>().CallFlags.HasFlag(CallFlags.AllowNotify)) | ||
| { | ||
| OnFault(new Exception(message)); |
There was a problem hiding this comment.
Why are we faulting on log if AllowNotify is enabled? Why would we assume that a call to Log must be a fault?
There was a problem hiding this comment.
Because Log require Notify rights so if we don't have it, it will fault in the previous version, now the same but at least we have the message captured. That's my favorite version for solve the issue
There was a problem hiding this comment.
Why are we faulting on log if AllowNotify is enabled? Why would we assume that a call to Log must be a fault?
If NOT
There was a problem hiding this comment.
OK, I see the logic. Since this PR changes requiredCallFlags to None, RuntimeLog will only go down the fault path in cases where we would have thrown in ValidateCallFlags previously.
Why are we calling OnFault instead of throwing like ValidateCallFlags does?
There was a problem hiding this comment.
Because with OnFault we can set the Log error message, and we don't lose the assert message
|
@roman-khimov @erikzhang Could you review this? I think throw error msg when fault is OK, at least we can make some logs. |
|
I'd rather go with neo-project/neo-vm#491, as noted in #2782. |
|
Closed by neo-project/neo-devpack-dotnet#758 |
Alternative to #2783 neo-project/neo-devpack-dotnet#758
Close #2782