[Neo Rpc Methods] fix contract rpc methods parameters #3485

Jim8y · 2024-09-11T15:52:53Z

Description

This pr updates the rpc plugin contract related methods.

Fixes # (issue)

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?

TestInvokeFunction
TestInvokeScript
TestTraverseIterator
TestGetUnclaimedGas

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

src/Plugins/RpcServer/RpcServer.SmartContract.cs

src/Plugins/RpcServer/ParameterConverter.cs

AnnaShaleva · 2024-09-12T13:51:15Z

src/Plugins/RpcServer/RpcServer.SmartContract.cs

+        /// <param name="useDiagnostic">Optional. Flag to enable diagnostic information.</param>
+        /// <returns>A JToken containing the result of the invocation.</returns>
+        [RpcMethodWithParams]
+        protected internal virtual JToken InvokeFunction(string scriptHash, string operation, ContractParameter[] args = null, Signer[] signers = null, bool useDiagnostic = false)


This behaviour diverges from an old one. This method should accept the list of signers with witnesses as the fourth parameter, not only the list of signers.

It never worked. And not bing of any use at anywhere.

And not bing of any use at anywhere.

You don't know that. It's an external API and it's a very widely used one. Your changes can easily break real applications. Refactorings must not break compatibility.

src/Plugins/RpcServer/RpcServer.SmartContract.cs

cschuchardt88 · 2024-09-13T04:09:26Z

src/Plugins/RpcServer/Model/SignerOrWitness.cs

+
+namespace Neo.Plugins.RpcServer.Model;
+
+public class SignerOrWitness


Why not make a base class for common stuff? Than two different classes one, being Signer and other being Witness. This way we can get Signer attributes like Rules or other properties that separate the two classes.

make it work first, optimize it later. I think you are better than me to make the code more elegent.

AnnaShaleva

You may take a look at https://github.com/nspcc-dev/neo-go/blob/e1d5ac8557d7e087158e6a766d059913a56df79f/pkg/neorpc/types.go#L75 and https://github.com/nspcc-dev/neo-go/blob/e1d5ac8557d7e087158e6a766d059913a56df79f/pkg/neorpc/types.go#L83 as a reference, both Signer and Witness should be accepted at the same time.

src/Plugins/RpcServer/Model/SignerOrWitness.cs

src/Plugins/RpcServer/ParameterConverter.cs

Jim8y · 2024-09-14T16:03:01Z

@AnnaShaleva please check latest update to make it SignerWithWitness, @chenzhitong please update the document accordingly, @superboyiii may you please test this pr? expecially the InvokeFunction and InvokeScript that may contain signer and/or witness.

nan01ab · 2024-09-17T14:07:40Z

src/Plugins/RpcServer/ParameterConverter.cs

+                throw new RpcException(CreateInvalidParamError<Signer>(token));
+            }
+        }
+        throw new RpcException(CreateInvalidParamError<Signer>(token));


Would adding an else statement here make it clearer?

src/Plugins/RpcServer/Model/SignerWithWitness.cs

AnnaShaleva · 2024-09-18T17:54:55Z

src/Plugins/RpcServer/Model/SignerWithWitness.cs

+            AllowedContracts = ((JArray)jObject["allowedcontracts"])?.Select(p => UInt160.Parse(p.AsString())).ToArray() ?? Array.Empty<UInt160>(),
+            AllowedGroups = ((JArray)jObject["allowedgroups"])?.Select(p => ECPoint.Parse(p.AsString(), ECCurve.Secp256r1)).ToArray() ?? Array.Empty<ECPoint>(),
+            Rules = ((JArray)jObject["rules"])?.Select(r => WitnessRule.FromJson((JObject)r)).ToArray() ?? Array.Empty<WitnessRule>()
+        };


We also need a check against the maximum number of parsed contracts, rules and groups, it's 16.

You may serialize and deserialize parsed items to ensure validity, exactly how the old code does it.

Still not resolved, we need validation.

this is already copied from the original code, if its still not working, i would say it has never worked, and should not be a concern of this pr.

src/Plugins/RpcServer/Model/SignerWithWitness.cs

src/Plugins/RpcServer/ParameterConverter.cs

AnnaShaleva · 2024-09-18T18:05:16Z

src/Plugins/RpcServer/ParameterConverter.cs

+                return new[] { signerOrWitness };
+            }
+        }
+        throw new RpcException(RpcError.InvalidParams.WithData($"Invalid SignerOrWitness format: {token}"));


src/Plugins/RpcServer/ParameterConverter.cs

src/Plugins/RpcServer/RpcServer.SmartContract.cs

Jim8y · 2024-09-21T06:00:16Z

@AnnaShaleva updated this pr to mock the complete http request process, now should be able to work without miss any detail.

Jim8y · 2024-09-27T12:35:14Z

@shargon @superboyiii

vncoelho

testes some methods and they are working as expected

vncoelho · 2024-09-27T14:33:45Z

However, maybe @superboyiii should do a more extensive tests, these changes affects various methods

Jim8y · 2024-09-28T09:46:02Z

However, maybe @superboyiii should do a more extensive tests, these changes affects various methods

I have updated to make it use existing unit test, without updating the unit test logic makes sure that changes in this pr follows the old processing logic.

shargon · 2024-09-30T16:52:33Z

src/Plugins/RpcServer/Model/SignerWithWitness.cs

+            {
+                signer = SignerFromJson(jObject, settings);
+            }
+            if (jObject.ContainsProperty("invocation") || jObject.ContainsProperty("verification"))


Suggested change

if (jObject.ContainsProperty("invocation") || jObject.ContainsProperty("verification"))

else if (jObject.ContainsProperty("invocation") || jObject.ContainsProperty("verification"))

The suggestion is not correct because both account and witness are allowed at the same time.

vncoelho

This is probably a breaking change to several dapps.
InvokeFunction is a methods that probably many services use.
Perhaps its parameters will now face this modification from array.

AnnaShaleva · 2024-09-30T19:53:03Z

src/Plugins/RpcServer/Model/SignerWithWitness.cs

+            {
+                signer = SignerFromJson(jObject, settings);
+            }
+            if (jObject.ContainsProperty("invocation") || jObject.ContainsProperty("verification"))


The suggestion is not correct because both account and witness are allowed at the same time.

src/Plugins/RpcServer/RpcServer.SmartContract.cs

AnnaShaleva · 2024-09-30T19:59:10Z

src/Plugins/RpcServer/RpcServer.SmartContract.cs

+            var signers = signerWithWitnesses?.Where(u => u.Signer != null).Select(u => u.Signer).ToArray() ?? [];
+            var witnesses = signerWithWitnesses?.Where(u => u.Witness != null).Select(u => u.Witness).ToArray() ?? [];


Checks .Where(u => u.Signer != null) and .Where(u => u.Witness != null) are not quite correct because it may lead to unexpected result if e.g. some witness is missing for a signer. These checks may lead to the situation when signers and witnesses have different length.

i dont think we have properly defined the behavior of when signers and witnesses have different length. we dont have any logic to process it in previous code.

AnnaShaleva · 2024-09-30T20:00:13Z

src/Plugins/RpcServer/Model/SignerWithWitness.cs

+            AllowedContracts = ((JArray)jObject["allowedcontracts"])?.Select(p => UInt160.Parse(p.AsString())).ToArray() ?? Array.Empty<UInt160>(),
+            AllowedGroups = ((JArray)jObject["allowedgroups"])?.Select(p => ECPoint.Parse(p.AsString(), ECCurve.Secp256r1)).ToArray() ?? Array.Empty<ECPoint>(),
+            Rules = ((JArray)jObject["rules"])?.Select(r => WitnessRule.FromJson((JObject)r)).ToArray() ?? Array.Empty<WitnessRule>()
+        };


Still not resolved, we need validation.

AnnaShaleva · 2024-09-30T20:01:39Z

src/Plugins/RpcServer/RpcServer.Wallet.cs

-            Signer[] signers = _params.Count >= 3 ? SignersFromJson((JArray)_params[2], system.Settings) : null;
-            Witness[] witnesses = _params.Count >= 3 ? WitnessesFromJson((JArray)_params[2]) : null;
+            Signer[] signers = _params.Count >= 3 ? SignerWithWitness.ParseArray((JArray)_params[2], system.Settings).Where(u => u.Signer != null).Select(u => u.Signer).ToArray() : null;
+            Witness[] witnesses = _params.Count >= 3 ? SignerWithWitness.ParseArray((JArray)_params[2], system.Settings).Where(u => u.Witness != null).Select(u => u.Witness).ToArray() : null;


roman-khimov · 2024-09-30T20:11:59Z

This is probably a breaking change to several dapps.

If it's a breaking change, it MUST NOT be merged. We have exactly zero reasons to break these interfaces. Any refactorings MUST keep compatibility.

nan01ab · 2024-10-01T01:37:11Z

src/Plugins/RpcServer/Model/SignerWithWitness.cs

+
+namespace Neo.Plugins.RpcServer.Model;
+
+public class SignerWithWitness(Signer? signer, Witness? witness)


SignerWithWitness ?
May be WitnessSigner is better?

nan01ab · 2024-10-01T01:38:56Z

src/Plugins/RpcServer/Model/SignerWithWitness.cs

+            {
+                signer = SignerFromJson(jObject, settings);
+            }
+            if (jObject.ContainsProperty("invocation") || jObject.ContainsProperty("verification"))


"invocation" and "verification" defined as constant values is better?

Jim8y added 3 commits September 11, 2024 23:49

fix contact rpc methods parameters

380c1ac

update hash string

292c3cc

update session and iterator

9fabbf5

shargon previously approved these changes Sep 11, 2024

View reviewed changes

Jim8y added the waiting for review label Sep 11, 2024

cschuchardt88 previously approved these changes Sep 11, 2024

View reviewed changes

cschuchardt88 added ready to merge and removed waiting for review labels Sep 11, 2024

AnnaShaleva reviewed Sep 12, 2024

View reviewed changes

AnnaShaleva added need update and removed ready to merge labels Sep 12, 2024

update invoke script

656bee9

Jim8y dismissed stale reviews from cschuchardt88 and shargon via 656bee9 September 13, 2024 04:04

Merge branch 'master' into rpc-contract

387561e

Jim8y added waiting for review and removed need update labels Sep 13, 2024

cschuchardt88 reviewed Sep 13, 2024

View reviewed changes

Jim8y added 3 commits September 13, 2024 12:18

update function as well

a8578c7

update function as well

12fb1b6

Merge branch 'rpc-contract' of github.com:Jim8y/neo into rpc-contract

c00d1e7

AnnaShaleva reviewed Sep 14, 2024

View reviewed changes

src/Plugins/RpcServer/Model/SignerOrWitness.cs Outdated Show resolved Hide resolved

src/Plugins/RpcServer/ParameterConverter.cs Show resolved Hide resolved

src/Plugins/RpcServer/ParameterConverter.cs Show resolved Hide resolved

Jim8y added 3 commits September 14, 2024 12:48

Merge branch 'master' into rpc-contract

55331a1

Merge branch 'master' into rpc-contract

86be323

update to signerwithwitness

c0930c1

nan01ab reviewed Sep 17, 2024

View reviewed changes

AnnaShaleva reviewed Sep 18, 2024

View reviewed changes

mock the complete http request process.

bd615eb

Jim8y requested review from shargon, cschuchardt88 and nan01ab September 21, 2024 06:13

Merge branch 'master' into rpc-contract

a8fe721

shargon previously approved these changes Sep 27, 2024

View reviewed changes

Jim8y added ready to merge and removed waiting for review labels Sep 27, 2024

vncoelho changed the title ~~[Neo Rpc Methods] fix contact rpc methods parameters~~ [Neo Rpc Methods] fix contract rpc methods parameters Sep 27, 2024

vncoelho previously approved these changes Sep 27, 2024

View reviewed changes

Merge branch 'master' into rpc-contract

2f50215

Jim8y dismissed stale reviews from vncoelho and shargon via 2f50215 September 28, 2024 09:50

Jim8y requested review from shargon and vncoelho September 28, 2024 09:53

Update SignerWithWitness.cs

d49a44a

shargon reviewed Sep 30, 2024

View reviewed changes

vncoelho reviewed Sep 30, 2024

View reviewed changes

AnnaShaleva reviewed Sep 30, 2024

View reviewed changes

nan01ab reviewed Oct 1, 2024

View reviewed changes

Merge branch 'master' into rpc-contract

fc1235c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Neo Rpc Methods] fix contract rpc methods parameters #3485

[Neo Rpc Methods] fix contract rpc methods parameters #3485

Jim8y commented Sep 11, 2024

AnnaShaleva Sep 12, 2024

Jim8y Sep 12, 2024

roman-khimov Sep 12, 2024

cschuchardt88 Sep 13, 2024

Jim8y Sep 13, 2024

AnnaShaleva left a comment

Jim8y commented Sep 14, 2024

nan01ab Sep 17, 2024

AnnaShaleva Sep 18, 2024

AnnaShaleva Sep 18, 2024

AnnaShaleva Sep 30, 2024

Jim8y Oct 1, 2024

AnnaShaleva Sep 18, 2024

Jim8y commented Sep 21, 2024

Jim8y commented Sep 27, 2024

vncoelho left a comment

vncoelho commented Sep 27, 2024

Jim8y commented Sep 28, 2024

shargon Sep 30, 2024

AnnaShaleva Sep 30, 2024

vncoelho left a comment

AnnaShaleva Sep 30, 2024

AnnaShaleva Sep 30, 2024

Jim8y Oct 1, 2024

AnnaShaleva Sep 30, 2024

AnnaShaleva Sep 30, 2024

roman-khimov commented Sep 30, 2024

nan01ab Oct 1, 2024

nan01ab Oct 1, 2024


		namespace Neo.Plugins.RpcServer.Model;

		public class SignerOrWitness

	if (jObject.ContainsProperty("invocation") \|\| jObject.ContainsProperty("verification"))
	else if (jObject.ContainsProperty("invocation") \|\| jObject.ContainsProperty("verification"))

		var signers = signerWithWitnesses?.Where(u => u.Signer != null).Select(u => u.Signer).ToArray() ?? [];
		var witnesses = signerWithWitnesses?.Where(u => u.Witness != null).Select(u => u.Witness).ToArray() ?? [];


		namespace Neo.Plugins.RpcServer.Model;

		public class SignerWithWitness(Signer? signer, Witness? witness)

[Neo Rpc Methods] fix contract rpc methods parameters #3485

Are you sure you want to change the base?

[Neo Rpc Methods] fix contract rpc methods parameters #3485

Conversation

Jim8y commented Sep 11, 2024

Description

Type of change

How Has This Been Tested?

Checklist:

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

AnnaShaleva left a comment

Choose a reason for hiding this comment

Jim8y commented Sep 14, 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

Jim8y commented Sep 21, 2024

Jim8y commented Sep 27, 2024

vncoelho left a comment

Choose a reason for hiding this comment

vncoelho commented Sep 27, 2024

Jim8y commented Sep 28, 2024

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

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

roman-khimov commented Sep 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment