[VM] rename vm catchable and uncatchable exceptions #3538

Jim8y · 2024-10-17T15:55:50Z

Description

This pr tries to improve the VM exception type and inforamtion, making it easier to understand the VM exception.

Fixes # #3536

Type of change

Optimization (the change is only an optimization)
Style (the change is only a code style for better maintenance or standard purpose)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Test Configuration:

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

Jim8y · 2024-10-17T15:56:48Z

@roman-khimov @shargon please take a look, this is what i want to do with the exception currently. It just make it clear what the exception are and enfoce an exception message. If you think it is good to go, i will finish this pr. I think nothing will be influenced.

Jim8y · 2024-10-17T16:23:54Z

Well, need to udpate UT still~~~ UTs are affected since exception type are different

src/Neo.VM/ExecutionEngine.cs

shargon · 2024-10-17T16:49:07Z

src/Neo.VM/VMCatchableException.cs

+            if (string.IsNullOrEmpty(message))
+                throw new ArgumentException("Message cannot be null or empty.", nameof(message));


I prefer to avoid this, this error will be throw during runtime, is not a warning for us

yes remove this, and add a default message when null.

shargon · 2024-10-17T16:49:14Z

src/Neo.VM/VMUncatchableException.cs

+        if (string.IsNullOrEmpty(message))
+            throw new ArgumentException("Message cannot be null or empty.", nameof(message));


Add default message on null.

vncoelho

LGTM, @Jim8y

nan01ab · 2024-10-23T15:27:18Z

src/Neo.VM/VMCatchableException.cs

        {
+            if (string.IsNullOrEmpty(message))
+                throw new ArgumentException("Message cannot be null or empty.", nameof(message));


Throw another Exception when constructing an Exception is not good practice.

cschuchardt88 · 2024-11-06T13:25:42Z

src/Neo.VM/VMCatchableException.cs

+            if (string.IsNullOrEmpty(message))
+                throw new ArgumentException("Message cannot be null or empty.", nameof(message));


yes remove this, and add a default message when null.

cschuchardt88 · 2024-11-06T13:25:56Z

src/Neo.VM/VMUncatchableException.cs

+        if (string.IsNullOrEmpty(message))
+            throw new ArgumentException("Message cannot be null or empty.", nameof(message));


Add default message on null.

shargon · 2024-11-07T09:17:19Z

@Jim8y please take a look that there are no any more left, and the most important, no changes in catchable exceptions

click wrong button

cschuchardt88 · 2024-11-29T00:46:47Z

src/Neo.VM/Exceptions/IVMException.cs

+
+namespace Neo.VM.Exceptions
+{
+    public interface IVMException


Making a requirement of for this is irrelevant, just use Exception.

Doing this will make the message required for example:

public class BadScriptException(string message) : Exception(message) { }

cschuchardt88 · 2024-11-29T00:47:45Z

src/Neo.VM/VMExceptions/VMCatchableException.cs

-        {
-        }
-    }
+    public class VMCatchableException(string message) : Exception($"{message}"), IVMException;


Remove the interface, its unneeded. also the $"" is unneeded too.

cschuchardt88 · 2024-11-29T00:48:01Z

src/Neo.VM/VMExceptions/VMUncatchableException.cs

+    /// is not catchable by the VM, and will directly cause the VM execution to fault.
+    /// </summary>
+    /// <param name="message">The error message that explains the reason for the exception.</param>
+    public class VmUncatchableException(string message) : Exception(message), IVMException


remove the interface

Jim8y · 2024-12-17T07:28:47Z

Its hard to implement what i planed without change the exception name, maybe just keep it as it is.

rename vm catchable and uncatchable exceptions

10c2d71

Jim8y added 2 commits October 17, 2024 23:58

format

b59cc87

format

7e4e662

Jim8y added 2 commits October 18, 2024 00:33

update exception

3d8c357

update exception

7fdc631

shargon reviewed Oct 17, 2024

View reviewed changes

vncoelho reviewed Oct 17, 2024

View reviewed changes

cschuchardt88 approved these changes Oct 17, 2024

View reviewed changes

Merge branch 'master' into vm-type

94cb434

nan01ab reviewed Oct 23, 2024

View reviewed changes

Jim8y added 2 commits October 24, 2024 17:57

update exception

dada9ef

Merge branch 'master' into vm-type

d61cd6e

cschuchardt88 requested changes Nov 6, 2024

View reviewed changes

cschuchardt88 and others added 5 commits November 6, 2024 08:26

Merge branch 'master' into vm-type

30c1776

Revert rename

8c83211

Move to a folder and create interface

9266889

Add more

f0310bd

Add more

8a58be0

Merge branch 'master' into vm-type

32ba899

cschuchardt88 self-requested a review November 12, 2024 23:06

cschuchardt88 previously approved these changes Nov 12, 2024

View reviewed changes

cschuchardt88 added the Need Update label Nov 12, 2024

shargon and others added 3 commits November 14, 2024 10:52

Merge branch 'master' into vm-type

3db7457

Merge branch 'vm-type' of github.com:Jim8y/neo into vm-type

7143c4f

resolve conflict

9d7c71f

cschuchardt88 reviewed Nov 29, 2024

View reviewed changes

resolve conflicts

85b953f

Jim8y closed this Dec 17, 2024

Jim8y deleted the vm-type branch December 17, 2024 07:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VM] rename vm catchable and uncatchable exceptions #3538

[VM] rename vm catchable and uncatchable exceptions #3538

Jim8y commented Oct 17, 2024 •

edited

Loading

Jim8y commented Oct 17, 2024 •

edited

Loading

Jim8y commented Oct 17, 2024

shargon Oct 17, 2024

cschuchardt88 Nov 6, 2024

shargon Oct 17, 2024

cschuchardt88 Nov 6, 2024

vncoelho left a comment

nan01ab Oct 23, 2024 •

edited

Loading

cschuchardt88 Nov 6, 2024

cschuchardt88 Nov 6, 2024

shargon commented Nov 7, 2024

cschuchardt88 Nov 29, 2024

cschuchardt88 Nov 29, 2024 •

edited

Loading

cschuchardt88 Nov 29, 2024

Jim8y commented Dec 17, 2024

		if (string.IsNullOrEmpty(message))
		throw new ArgumentException("Message cannot be null or empty.", nameof(message));

[VM] rename vm catchable and uncatchable exceptions #3538

[VM] rename vm catchable and uncatchable exceptions #3538

Conversation

Jim8y commented Oct 17, 2024 • edited Loading

Description

Type of change

Checklist:

Jim8y commented Oct 17, 2024 • edited Loading

Jim8y commented Oct 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vncoelho left a comment

Choose a reason for hiding this comment

nan01ab Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shargon commented Nov 7, 2024

Choose a reason for hiding this comment

cschuchardt88 Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jim8y commented Dec 17, 2024

Jim8y commented Oct 17, 2024 •

edited

Loading

Jim8y commented Oct 17, 2024 •

edited

Loading

nan01ab Oct 23, 2024 •

edited

Loading

cschuchardt88 Nov 29, 2024 •

edited

Loading