[Neo Core Exception] Create WalletException and use it to replace all exceptions in wallet #3434

Jim8y · 2024-07-20T17:01:24Z

Description

We use general exceptions in the neo core that is hard to tell the exact exception reason. Thus in this pr i define a new exception type WalletException, it contains multiple error types.

For instance, in CalculateNetworkFee method, the exception reason can be very different, but we will not know the cause of the exception if we catched the exception, as they are all ArgumentExceptions, thus we are not able to have an exact rpcserver response code.

Fixes #

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

How Has This Been Tested?

Existing tests that are related to the walletexception are updated accordingly.

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-07-20T17:29:47Z

Problem of this pr is that maybe its depencencies need to update there exception handling logic as the exception has being changed.

tests/Neo.UnitTests/Wallets/UT_AssetDescriptor.cs

src/Neo/Wallets/NEP6/NEP6Wallet.cs

Jim8y · 2024-07-23T16:35:18Z

@shargon

Jim8y · 2024-07-25T17:39:30Z

@shargon any further suggestion?

src/Neo/Wallets/Wallet.cs

src/Neo/Wallets/Helper.cs

vncoelho

I am reviewing the PR, @Jim8y. But there are many changes still to review and test here from my side.

Jim8y · 2024-07-31T14:32:40Z

blocked for vitor to review

vncoelho

I have some problems with UT_Transactions.cs, It is not appearing in my test environment.
The same for Neo-Plugins.RpcServer.Tests.

cschuchardt88 · 2024-07-31T16:54:52Z

@vncoelho

You have to build 1st.

vncoelho · 2024-07-31T17:04:03Z

@vncoelho

You have to build 1st.

And I also noticed it is a Partial Class, so they show differently later.
But as default the devcontainer runs dotnet build.
For some reason sometime it starts with problems.

Before I had dotnet restokre && dotnet build --no-restore
Perhaps it works better.

shargon · 2024-08-01T08:57:46Z

src/Neo/Wallets/Helper.cs

@@ -90,78 +104,90 @@ internal static byte[] XOR(byte[] x, byte[] y)
        /// <returns>The network fee of the transaction.</returns>
        public static long CalculateNetworkFee(this Transaction tx, DataCache snapshot, ProtocolSettings settings, Func<UInt160, byte[]> accountScript, long maxExecutionCost = ApplicationEngine.TestModeGas)


This will make conflicts into my never ending pull request #3385 :'(

We may merge #3385 as it is now and move non-standard verification scripts support to another issue. I think it's acceptable way since #3385 contains everything that's required except non-standard verification scripts handling.

Moved to #3539.

shargon · 2024-08-01T08:59:08Z

src/Neo/Wallets/Wallet.cs

@@ -141,6 +141,7 @@ public WalletAccount CreateAccount()
        {
            var privateKey = new byte[32];
            using var rng = RandomNumberGenerator.Create();
+            var maxTry = 100;


how much time it spend in 100 ? maybe is few number?

this is not likly t happen,,,,,but will do 10

Before it was infinite...eahuehauea
Any number is good to me here, but 10 looks small but unlikely to happen.

vncoelho · 2024-08-01T15:59:12Z

Tests completed from my side here. I will just check the code here on github again. But no blocker.

…ndardize-exception * 'standardize-exception' of github.com:Jim8y/neo: Clean using [Neo Core Add] add char support (neo-project#3441) [rpc] Extend `getversion` RPC response with additional protocol settings (neo-project#3443) update benchmark system (neo-project#3442) [`Optimization`] Parsing Smart Contract Script Analysis (neo-project#3420) `[Move]` Part-3 Classes into Different Library - `Neo.Extensions` (neo-project#3400)

shargon · 2024-08-02T08:01:09Z

I still reviewing this one

cschuchardt88 · 2024-08-10T09:12:47Z

@Jim8y can you add this Issue to this PR since your changing alot

#3167

AnnaShaleva · 2024-08-21T18:31:34Z

src/Neo/Wallets/Helper.cs

@@ -90,78 +104,90 @@ internal static byte[] XOR(byte[] x, byte[] y)
        /// <returns>The network fee of the transaction.</returns>
        public static long CalculateNetworkFee(this Transaction tx, DataCache snapshot, ProtocolSettings settings, Func<UInt160, byte[]> accountScript, long maxExecutionCost = ApplicationEngine.TestModeGas)


We may merge #3385 as it is now and move non-standard verification scripts support to another issue. I think it's acceptable way since #3385 contains everything that's required except non-standard verification scripts handling.

AnnaShaleva · 2024-08-21T18:34:42Z

src/Neo/Wallets/AssetDescriptor.cs

+            catch (Exception ex) when (ex is not WalletException)
            {
-                sb.EmitDynamicCall(asset_id, "decimals", CallFlags.ReadOnly);
-                sb.EmitDynamicCall(asset_id, "symbol", CallFlags.ReadOnly);
-                script = sb.ToArray();
+                throw WalletException.FromException(ex);
            }


WalletException constructor is able to create WalletException from WalletException, so we can safely remove when (ex is not WalletException).

AnnaShaleva · 2024-08-21T18:41:00Z

src/Neo/Wallets/Wallet.cs

+            }
+            catch (Exception e) when (e is not WalletException)
+            {
+                throw new WalletException(WalletErrorType.InvalidPrivateKey, "Invalid WIF format.", e);


"Invalid WIF format."

It may be something else, because invalid WIF format is handled earlier.

Jim8y added 2 commits July 21, 2024 00:55

standardize exception

d12d5e6

upate mac

c4cfbc1

Jim8y added the Waiting for Review label Jul 20, 2024

wrap methods in WalletException.

c5719a3

cschuchardt88 previously approved these changes Jul 21, 2024

View reviewed changes

Jim8y requested a review from shargon July 22, 2024 06:33

Merge branch 'master' into standardize-exception

3285c9a

shargon dismissed cschuchardt88’s stale review via 3285c9a July 23, 2024 07:26

shargon reviewed Jul 23, 2024

View reviewed changes

tests/Neo.UnitTests/Wallets/UT_AssetDescriptor.cs Outdated Show resolved Hide resolved

Update tests/Neo.UnitTests/Wallets/UT_AssetDescriptor.cs

58a3d23

shargon reviewed Jul 23, 2024

View reviewed changes

src/Neo/Wallets/NEP6/NEP6Wallet.cs Show resolved Hide resolved

shargon and others added 2 commits July 23, 2024 09:33

One file per class

f366801

check and fix all exception messages.

bfd0219

cschuchardt88 previously approved these changes Jul 23, 2024

View reviewed changes

Jim8y requested a review from shargon July 23, 2024 11:19

Merge branch 'master' into standardize-exception

bc1667c

Merge branch 'master' into standardize-exception

2dc9fb7

cschuchardt88 and others added 2 commits July 27, 2024 23:55

Merge branch 'master' into standardize-exception

25afa01

Merge branch 'master' into standardize-exception

3625988

Jim8y dismissed cschuchardt88’s stale review via 3625988 July 29, 2024 11:59

Jim8y commented Jul 29, 2024

View reviewed changes

src/Neo/Wallets/Wallet.cs Outdated Show resolved Hide resolved

Jim8y and others added 2 commits July 29, 2024 20:03

Update src/Neo/Wallets/Wallet.cs

21c0d99

fix

b43327b

Jim8y requested a review from cschuchardt88 July 29, 2024 14:14

shargon reviewed Jul 29, 2024

View reviewed changes

src/Neo/Wallets/Wallet.cs Outdated Show resolved Hide resolved

Jim8y requested a review from shargon July 30, 2024 04:01

Merge branch 'master' into standardize-exception

50c376a

cschuchardt88 previously approved these changes Jul 30, 2024

View reviewed changes

Merge branch 'master' into standardize-exception

18b3ac9

vncoelho reviewed Jul 31, 2024

View reviewed changes

src/Neo/Wallets/Helper.cs Outdated Show resolved Hide resolved

vncoelho reviewed Jul 31, 2024

View reviewed changes

Jim8y added the Blocked This issue can't be worked at the moment label Jul 31, 2024

vncoelho reviewed Jul 31, 2024

View reviewed changes

Clean using

7bded56

shargon dismissed cschuchardt88’s stale review via 7bded56 August 1, 2024 08:55

shargon reviewed Aug 1, 2024

View reviewed changes

Merge branch 'master' into standardize-exception

bca2ff6

vncoelho removed the Blocked This issue can't be worked at the moment label Aug 1, 2024

Jim8y added 2 commits August 2, 2024 09:24

less try

84b4555

do 30

472122c

Jim8y requested a review from shargon August 2, 2024 01:25

Jim8y added the Blocked This issue can't be worked at the moment label Aug 2, 2024

Merge branch 'master' into standardize-exception

3baffac

Jim8y removed the Blocked This issue can't be worked at the moment label Aug 10, 2024

AnnaShaleva reviewed Aug 21, 2024

View reviewed changes

Merge branch 'master' into standardize-exception

1ebd9f3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Neo Core Exception] Create WalletException and use it to replace all exceptions in wallet #3434

[Neo Core Exception] Create WalletException and use it to replace all exceptions in wallet #3434

Jim8y commented Jul 20, 2024 •

edited

Loading

Jim8y commented Jul 20, 2024

Jim8y commented Jul 23, 2024

Jim8y commented Jul 25, 2024

vncoelho left a comment

Jim8y commented Jul 31, 2024

vncoelho left a comment

cschuchardt88 commented Jul 31, 2024

vncoelho commented Jul 31, 2024

shargon Aug 1, 2024

AnnaShaleva Aug 21, 2024

roman-khimov Oct 17, 2024

shargon Aug 1, 2024

Jim8y Aug 1, 2024

vncoelho Aug 1, 2024

vncoelho commented Aug 1, 2024

shargon commented Aug 2, 2024

cschuchardt88 commented Aug 10, 2024

AnnaShaleva Aug 21, 2024

AnnaShaleva Aug 21, 2024

AnnaShaleva Aug 21, 2024

		@@ -90,78 +104,90 @@ internal static byte[] XOR(byte[] x, byte[] y)
		/// <returns>The network fee of the transaction.</returns>
		public static long CalculateNetworkFee(this Transaction tx, DataCache snapshot, ProtocolSettings settings, Func<UInt160, byte[]> accountScript, long maxExecutionCost = ApplicationEngine.TestModeGas)

[Neo Core Exception] Create WalletException and use it to replace all exceptions in wallet #3434

Are you sure you want to change the base?

[Neo Core Exception] Create WalletException and use it to replace all exceptions in wallet #3434

Conversation

Jim8y commented Jul 20, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

Jim8y commented Jul 20, 2024

Jim8y commented Jul 23, 2024

Jim8y commented Jul 25, 2024

vncoelho left a comment

Choose a reason for hiding this comment

Jim8y commented Jul 31, 2024

vncoelho left a comment

Choose a reason for hiding this comment

cschuchardt88 commented Jul 31, 2024

vncoelho commented Jul 31, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vncoelho commented Aug 1, 2024

shargon commented Aug 2, 2024

cschuchardt88 commented Aug 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jim8y commented Jul 20, 2024 •

edited

Loading