fix verify api #564

Ashuaidehao · 2021-03-16T08:39:52Z

Close #552

shargon · 2021-03-16T08:57:08Z

src/RpcServer/RpcServer.Wallet.cs


 Transaction tx = signers == null ? null : new Transaction
 {
 Signers = signers.GetSigners(),
- Attributes = Array.Empty<TransactionAttribute>()
+ Attributes = Array.Empty<TransactionAttribute>(),
+ Witnesses = signers.Witnesses,


Witnesses it wil lbe null, isn't it?

Yes, just like

neo-modules/src/RpcServer/RpcServer.SmartContract.cs

Line 69 in 93fb620

Witnesses = signers.Witnesses,

Removed it since no use here.

Can we pass witnesses via RPC request parameters, just like signers are passed? And then fill Witnesses here.

AnnaShaleva · 2021-03-16T09:45:17Z

src/RpcServer/RpcServer.Wallet.cs

@@ -348,20 +347,20 @@ private JObject GetVerificationResult(UInt160 scriptHash, ContractParameter[] ar
 {
 throw new RpcException(-100, "Unknown contract");
 }
- var methodName = "verify";
+ var md = contract.Manifest.Abi.GetMethod("verify", 0);


Can we set pcount to -1 here in order to be able to call verify with arguments? Just like in https://github.com/neo-project/neo/blob/master/src/neo/SmartContract/Helper.cs#L222.

Updated it.

AnnaShaleva · 2021-03-16T09:48:11Z

src/RpcServer/RpcServer.Wallet.cs

 using ApplicationEngine engine = ApplicationEngine.Create(TriggerType.Verification, tx, snapshot.CreateSnapshot(), settings: system.Settings);
- engine.LoadScript(new ScriptBuilder().EmitDynamicCall(scriptHash, methodName, args).ToArray(), rvcount: 1);
-
+ engine.LoadContract(contract, md, CallFlags.ReadOnly);


And create an invocation script from the given args (it will push all arguments on the stack). And load the script right after the contract script, just like in https://github.com/neo-project/neo/blob/master/src/neo/SmartContract/Helper.cs#L246.

Updated it. Please test it like:

{ "jsonrpc": "2.0", "method": "invokecontractverify", "params": [ "0x31f714e04766139c8f3705f5ce94518e1411ec07", [ { "type": "ByteArray", "value": "DED2HMp00JZ2zcJYjKgTp4Tl0GODm5r14PKN6ip8i2+8ox3JYqN2xNHxQzCgzENqSD52GhVF4eGx2i405zxM1KTH" } ], [ { "account": "NMA2FKN8up2cEwaJgtmAiDrZWB69ApnDfp", "scopes": "CalledByEntry" } ] ], "id": 1 }

It's not very convenient to construct an invocation script on the client. I would prefer to pass simple standard parameters to the server as we do in the InvokeFunction RPC method. Like the following:

{ "jsonrpc": "2.0", "method": "invokecontractverify", "params": [ "0x31f714e04766139c8f3705f5ce94518e1411ec07", [ { "type": "Boolean", "value": "true" }, { "type": "Integer", "value": 123 } ], [ { "account": "NMA2FKN8up2cEwaJgtmAiDrZWB69ApnDfp", "scopes": "CalledByEntry" } ] ], "id": 1 }

And then we can construct an invocation script on the server by performing the following:

using (ScriptBuilder sb = new ScriptBuilder()) { for (int i = args.Count - 1; i >= 0; i--) sb.EmitPush(args[i]); }

Agree with @AnnaShaleva

AnnaShaleva · 2021-03-16T10:02:11Z

src/RpcServer/RpcServer.Wallet.cs

+ if (md is null)
+ throw new RpcException(-101, $"The smart contract {contract.Hash} haven't got verify method without arguments");
+ if (md.ReturnType != ContractParameterType.Boolean)
+ throw new RpcException(-102, "The verify method doesn't return boolean value.");

 Transaction tx = signers == null ? null : new Transaction


Can we also fill the transaction when signers == null? We can do it because we know exactly at least one signer (it's the contract to be called). And we also know the witness for this signer: invocation script pushes args on the stack; verification script is empty because it's a contract witness.

…dules into Fix-verifyfee

AnnaShaleva · 2021-03-16T10:41:05Z

src/RpcServer/RpcServer.Wallet.cs

- var methodName = "verify";
+ var md = contract.Manifest.Abi.GetMethod("verify", -1);
+ if (md is null)
+ throw new RpcException(-101, $"The smart contract {contract.Hash} haven't got verify method without arguments");


Exception message should also be updated:

throw new RpcException(-101, $"The smart contract {contract.Hash} haven't got verify method");

AnnaShaleva · 2021-03-16T10:49:32Z

src/RpcServer/RpcServer.Wallet.cs


 Transaction tx = signers == null ? null : new Transaction
 {
 Signers = signers.GetSigners(),
- Attributes = Array.Empty<TransactionAttribute>()
+ Attributes = Array.Empty<TransactionAttribute>(),


Can we also fill the transaction script? I think that []byte{OpCode.RET} is enough.

This is needed for those verify methods that use System.Runtime.GetScriptContainer interop, because transaction can't be serialized without a script.

superboyiii · 2021-03-17T06:47:07Z

@AnnaShaleva Need your review again.

AnnaShaleva · 2021-03-17T07:21:38Z

src/RpcServer/RpcServer.Wallet.cs

 {
- Signers = signers.GetSigners(),
- Attributes = Array.Empty<TransactionAttribute>()
+ defaultSigner.AddRange(signers.GetSigners());


Can we make here defaultSigner = signers.GetSigners();?
It shouldn't be mandatory for the contract signer to be a sender. And if the user wants to provide signers by himself, then he should include the contract signer with the desired scope in the parameters list.

The other option is to check that signers does not contain the contract signer and if the contract signer is missing, we should add it to the signers list.

Updated it.

AnnaShaleva · 2021-03-17T07:56:38Z

src/RpcServer/RpcServer.Wallet.cs

- Attributes = Array.Empty<TransactionAttribute>()
+ Signers = signers == null ? new Signer[] { new() { Account = scriptHash } } : signers.GetSigners(),
+ Attributes = Array.Empty<TransactionAttribute>(),
+ Script = new[] { (byte)OpCode.RET }


And the last issue: at the current moment, the transaction is completely missing witnesses. I would suggest to pass witnesses along with signers via RPC request parameters using the following structure:

{ "jsonrpc": "2.0", "method": "invokecontractverify", "params": [ "0x31f714e04766139c8f3705f5ce94518e1411ec07", [ ], [ { "account": "NiRqSd5MtRZT5yUhgWd7oG11brkDG76Jim", "scopes": "CalledByEntry", "invocation": "DEDr2gA/8T/wxQvgOZVfCdkbj6uGrprkDgJvpOJCcbl+tvlKZkZytCZEWm6NoZhJyIlEI3VQSLtU3AHuJfShAT5L", "verification": "DCEDAS1H52IQrsc745qz0YbgpA/o2Gv6PU+r/aV7oTuI+WoLQZVEDXg=", }, { "account": "NMA2FKN8up2cEwaJgtmAiDrZWB69ApnDfp", "scopes": "CalledByEntry", } ] ], "id": 1 }

And then parse the provided witnesses in GetSignersFromJson method (https://github.com/neo-project/neo-modules/blob/master/src/RpcServer/RpcServer.SmartContract.cs#L94).

And then fill the transaction like the following:

Transaction tx = new Transaction { Witnesses = signers.Witnesses, ... }

It will help us to get rid of the private key dependency (because if we parse witnesses, then we don't need private keys in the RPC node wallet anymore). And that's the answer to the #552 (comment) question.

AnnaShaleva

Almost there.

AnnaShaleva · 2021-03-17T08:39:05Z

src/RpcServer/RpcServer.Wallet.cs

- Attributes = Array.Empty<TransactionAttribute>()
+ Signers = signers == null ? new Signer[] { new() { Account = scriptHash } } : signers.GetSigners(),
+ Attributes = Array.Empty<TransactionAttribute>(),
+ Witnesses = signers?.Witnesses,


Suggested change

Witnesses = signers?.Witnesses,

Witnesses = signers == null ? new Witness() { InvocationScript = sb.ToArray() } : signers.Witnesses,

where sb is from

if (args.Length > 0) { using ScriptBuilder sb = new ScriptBuilder(); for (int i = args.Length - 1; i >= 0; i--) sb.EmitPush(args[i]); }

sb will be loaded into engine directly, is this InvocationScript necessary?

We know exactly the invocation script, why not to fill it in? By doing this we're getting closer to the real transaction witnesses.

AnnaShaleva · 2021-03-17T08:41:25Z

src/RpcServer/RpcServer.SmartContract.cs

+ .Where(x => x.Invocation != null && x.Verification != null)
+ .Select(x => new Witness()
+ {
+ InvocationScript = Convert.FromBase64String(x.Invocation),
+ VerificationScript = Convert.FromBase64String(x.Verification)
+ }).ToArray()


Suggested change

.Where(x => x.Invocation != null && x.Verification != null)

.Select(x => new Witness()

{

InvocationScript = Convert.FromBase64String(x.Invocation),

VerificationScript = Convert.FromBase64String(x.Verification)

}).ToArray()

.Where(x => x.Invocation != null || x.Verification != null)

.Select(x => new Witness()

{

InvocationScript = x.Invocation != null ?? Convert.FromBase64String(x.Invocation) : null,

VerificationScript = x.Verification != null ?? Convert.FromBase64String(x.Verification) : null

}).ToArray()

AnnaShaleva · 2021-03-17T09:15:40Z

src/RpcServer/RpcServer.Wallet.cs


+ engine.LoadScript(new Script(sb.ToArray()), configureState: p => p.CallFlags = CallFlags.None);
+ }
 JObject json = new JObject();
 json["script"] = Convert.ToBase64String(contract.Script);


And let the resulting script be the InvocationScript (which is sb.ToArray()). Because 1) the user is aware of the contract being invoked and 2) the user should have the ability to check that all the parameters were converted as expected.

See nspcc-dev/neo-go#1825 (comment).

What script mean if sb is empty?

That no parameters were pushed on the stack before verify execution.

AnnaShaleva

Nice changes, it solves the problem.

* fix verify api * refac * default signer & parameter * refac * add Witness * format * update * update * update Co-authored-by: Owen Zhang <38493437+superboyiii@users.noreply.github.com>

fix verify api

2d467f1

shargon reviewed Mar 16, 2021

View reviewed changes

Merge branch 'master' into Fix-verifyfee

ca8d87f

AnnaShaleva requested changes Mar 16, 2021

View reviewed changes

Ashuaidehao added 2 commits March 16, 2021 18:18

refac

331e24a

Merge branch 'Fix-verifyfee' of https://github.com/Ashuaidehao/neo-mo…

ec221df

…dules into Fix-verifyfee

AnnaShaleva requested changes Mar 16, 2021

View reviewed changes

default signer & parameter

647f56b

superboyiii mentioned this pull request Mar 17, 2021

Generate manifest with static calls neo-project/neo-devpack-dotnet#478

Merged

AnnaShaleva requested changes Mar 17, 2021

View reviewed changes

refac

108b15e

AnnaShaleva requested changes Mar 17, 2021

View reviewed changes

Ashuaidehao added 2 commits March 17, 2021 16:30

add Witness

bf5034c

format

b79c6de

AnnaShaleva requested changes Mar 17, 2021

View reviewed changes

update

9f89979

AnnaShaleva requested changes Mar 17, 2021

View reviewed changes

Ashuaidehao added 2 commits March 17, 2021 17:39

update

1d4b121

update

1043c8f

AnnaShaleva approved these changes Mar 17, 2021

View reviewed changes

Merge branch 'master' into Fix-verifyfee

2069218

superboyiii approved these changes Mar 17, 2021

View reviewed changes

superboyiii merged commit 1c24a3f into neo-project:master Mar 17, 2021

Ashuaidehao deleted the Fix-verifyfee branch April 7, 2021 08:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix verify api #564

fix verify api #564

Ashuaidehao commented Mar 16, 2021

shargon Mar 16, 2021

Ashuaidehao Mar 16, 2021

Ashuaidehao Mar 16, 2021

AnnaShaleva Mar 16, 2021

AnnaShaleva Mar 16, 2021

Ashuaidehao Mar 16, 2021

AnnaShaleva Mar 16, 2021

Ashuaidehao Mar 16, 2021

AnnaShaleva Mar 16, 2021

shargon Mar 16, 2021

Ashuaidehao Mar 17, 2021

AnnaShaleva Mar 16, 2021

Ashuaidehao Mar 17, 2021

AnnaShaleva Mar 16, 2021

AnnaShaleva Mar 16, 2021

Ashuaidehao Mar 17, 2021

superboyiii commented Mar 17, 2021

AnnaShaleva Mar 17, 2021

Ashuaidehao Mar 17, 2021

AnnaShaleva Mar 17, 2021 •

edited

Loading

AnnaShaleva Mar 17, 2021 •

edited

Loading

Ashuaidehao Mar 17, 2021

AnnaShaleva left a comment

AnnaShaleva Mar 17, 2021

Ashuaidehao Mar 17, 2021

AnnaShaleva Mar 17, 2021

AnnaShaleva Mar 17, 2021

AnnaShaleva Mar 17, 2021

AnnaShaleva Mar 17, 2021

Ashuaidehao Mar 17, 2021

AnnaShaleva Mar 17, 2021

AnnaShaleva left a comment

	Witnesses = signers?.Witnesses,
	Witnesses = signers == null ? new Witness() { InvocationScript = sb.ToArray() } : signers.Witnesses,

fix verify api #564

fix verify api #564

Conversation

Ashuaidehao commented Mar 16, 2021

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

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

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

superboyiii commented Mar 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnnaShaleva Mar 17, 2021 • edited Loading

Choose a reason for hiding this comment

AnnaShaleva Mar 17, 2021 • edited Loading

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

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

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

AnnaShaleva Mar 17, 2021 •

edited

Loading

AnnaShaleva Mar 17, 2021 •

edited

Loading