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

Enhance Exception Handling for Improved Clarity and Debugging #3536

Open
Jim8y opened this issue Oct 17, 2024 · 6 comments
Open

Enhance Exception Handling for Improved Clarity and Debugging #3536

Jim8y opened this issue Oct 17, 2024 · 6 comments
Labels
Discussion Initial issue state - proposed but not yet accepted

Comments

@Jim8y
Copy link
Contributor

Jim8y commented Oct 17, 2024

Summary or problem description
The current exception handling in Neo VM and Contract lacks clarity and detailed information. While there are various exception types, they often fail to provide useful details about why a transaction fails. Additionally, it's not immediately clear to developers which exceptions can be caught by the VM and which cannot.

Do you have any solution you want to propose?
We propose to enhance the exception system with the following changes:

  1. Maintain the current exception types but improve their implementation:
    • Ensure all exceptions include detailed, informative error messages.
    • Clearly indicate in the exception name or documentation whether it's catchable by the VM.
  2. Implement a consistent naming convention:
    • Prefix catchable exceptions with "VMCatchable" (e.g., VMCatchableInvalidOperationException)
    • Prefix uncatchable exceptions with "VMUncatchable" (e.g., VMUncatchableOutOfGasException)
  3. Enhance exception documentation:
    • Provide clear guidelines on when each exception type should be used.
    • Include examples of proper exception throwing with detailed messages.

For example, instead of throwing an InvalidOperationException without a message, we would throw a VMUncatchableInvalidOperationException with a detailed description like: "Invalid operation: Attempt to divide by zero at instruction DIV with value 10".

This change will:

  • Improve clarity for users when transactions fail
  • Make it easier for developers to understand which exceptions can be caught by the VM
  • Provide more informative error messages for debugging and troubleshooting
  • Maintain the granularity of different exception types while clearly indicating their catchability

AND, we can also just maintain two exceptions: vmcachable and vmuncatchable exceptions.

Where in the software does this update applies to?

  • VM
  • Compiler (for generating appropriate exception handling code)
  • SDK (for handling these exceptions in client applications)
@Jim8y Jim8y added the Discussion Initial issue state - proposed but not yet accepted label Oct 17, 2024
@roman-khimov
Copy link
Contributor

exception handling in Neo VM and Contract

Please never mix these. In general VM exception is VM failure (I know we have CatchableException, but I would never add it and I certainly wouldn't like to extend its use). While contract exceptions are mostly fine on their own (except I can THROW anything, can be a strings, can be a struct, whatever).

I think what you're mostly referring to here is VM-level things. And I can't see any problem with it now. It just fails, except in case of CatchableException. And no one should be looking into VM for exceptions, it's an implementation detail (Go doesn't have exceptions, but VM behavior is the same, it fails when it has to), the end result is what matters and this result is a VM failure. I'm all for improving messages when VM fails (again, NeoGo usually provides more details in this case and you've seen that), but other changes don't make any sense to me.

@Jim8y
Copy link
Contributor Author

Jim8y commented Oct 17, 2024

nothing will be changed, exception will still be exception, just trying to.make the name more meaningful and enforce a detailed error message. its an issue purely for c# implementation, not protocol level.

@Hecate2
Copy link
Contributor

Hecate2 commented Oct 18, 2024

If possible, could the VM print some info of each context in the InvocationStack?
And it would be great if external tools using the VM are able to get the name of each executing contract in each invocation context.

@Jim8y
Copy link
Contributor Author

Jim8y commented Oct 18, 2024

If possible, could the VM print some info of each context in the InvocationStack? And it would be great if external tools using the VM are able to get the name of each executing contract in each invocation context.

I think we can make the error message standard, do you mind to provide a template? @Hecate2

@Hecate2
Copy link
Contributor

Hecate2 commented Oct 18, 2024

If possible, could the VM print some info of each context in the InvocationStack? And it would be great if external tools using the VM are able to get the name of each executing contract in each invocation context.

I think we can make the error message standard, do you mind to provide a template? @Hecate2

https://github.com/Hecate2/neo-fairy-test/blob/master/Fairy.Tester.cs#L153-L153

I wrote codes for this, but do we really need a standard for errors? Different languages may prefer different implememtations.

@Jim8y
Copy link
Contributor Author

Jim8y commented Oct 18, 2024

different

This is for C# itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Initial issue state - proposed but not yet accepted
Projects
None yet
Development

No branches or pull requests

3 participants