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

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

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open
106 changes: 106 additions & 0 deletions src/Plugins/RpcServer/Model/SignerWithWitness.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
// Copyright (C) 2015-2024 The Neo Project.
//
// SignerWithWitness.cs file belongs to the neo project and is free
// software distributed under the MIT software license, see the
// accompanying file LICENSE in the main directory of the
// repository or http://www.opensource.org/licenses/mit-license.php
// for more details.
//
// Redistribution and use in source and binary forms with or without
// modifications are permitted.

#nullable enable

using Neo.Cryptography.ECC;
using Neo.Json;
using Neo.Network.P2P.Payloads;
using Neo.Wallets;
using System;
using System.Diagnostics.CodeAnalysis;
using System.Linq;

namespace Neo.Plugins.RpcServer.Model;

public class SignerWithWitness(Signer? signer, Witness? witness)
AnnaShaleva marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SignerWithWitness ?
May be WitnessSigner is better?

{
public Signer? Signer { get; } = signer;
public Witness? Witness { get; } = witness;

public static bool TryParse(JToken value, ProtocolSettings settings, [NotNullWhen(true)] out SignerWithWitness? signerWithWitness)
{
signerWithWitness = null;

if (value == null)
return false;

if (value is JObject jObject)
{
Signer? signer = null;
Witness? witness = null;

if (jObject.ContainsProperty("account"))
{
signer = SignerFromJson(jObject, settings);
}
if (jObject.ContainsProperty("invocation") || jObject.ContainsProperty("verification"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
witness = WitnessFromJson(jObject);
}

if (signer != null || witness != null)
{
signerWithWitness = new SignerWithWitness(signer, witness);
return true;
}
}

return false;
}

private static Signer SignerFromJson(JObject jObject, ProtocolSettings settings)
{
return new Signer
{
Account = AddressToScriptHash(jObject["account"].AsString(), settings.AddressVersion),

Check warning on line 64 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test-Everything

Dereference of a possibly null reference.

Check warning on line 64 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (ubuntu-latest)

Dereference of a possibly null reference.

Check warning on line 64 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (ubuntu-latest)

Dereference of a possibly null reference.

Check warning on line 64 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (windows-latest)

Dereference of a possibly null reference.

Check warning on line 64 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (macos-latest)

Dereference of a possibly null reference.
Scopes = (WitnessScope)Enum.Parse(typeof(WitnessScope), jObject["scopes"]?.AsString()),

Check warning on line 65 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test-Everything

Possible null reference argument for parameter 'value' in 'object Enum.Parse(Type enumType, string value)'.

Check warning on line 65 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (ubuntu-latest)

Possible null reference argument for parameter 'value' in 'object Enum.Parse(Type enumType, string value)'.

Check warning on line 65 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (windows-latest)

Possible null reference argument for parameter 'value' in 'object Enum.Parse(Type enumType, string value)'.

Check warning on line 65 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (macos-latest)

Possible null reference argument for parameter 'value' in 'object Enum.Parse(Type enumType, string value)'.
AllowedContracts = ((JArray)jObject["allowedcontracts"])?.Select(p => UInt160.Parse(p.AsString())).ToArray() ?? Array.Empty<UInt160>(),

Check warning on line 66 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test-Everything

Converting null literal or possible null value to non-nullable type.

Check warning on line 66 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test-Everything

Dereference of a possibly null reference.

Check warning on line 66 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (ubuntu-latest)

Converting null literal or possible null value to non-nullable type.

Check warning on line 66 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (ubuntu-latest)

Dereference of a possibly null reference.

Check warning on line 66 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (windows-latest)

Converting null literal or possible null value to non-nullable type.

Check warning on line 66 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (windows-latest)

Dereference of a possibly null reference.

Check warning on line 66 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (macos-latest)

Converting null literal or possible null value to non-nullable type.

Check warning on line 66 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (macos-latest)

Dereference of a possibly null reference.
AllowedGroups = ((JArray)jObject["allowedgroups"])?.Select(p => ECPoint.Parse(p.AsString(), ECCurve.Secp256r1)).ToArray() ?? Array.Empty<ECPoint>(),

Check warning on line 67 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test-Everything

Converting null literal or possible null value to non-nullable type.

Check warning on line 67 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test-Everything

Dereference of a possibly null reference.

Check warning on line 67 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (ubuntu-latest)

Converting null literal or possible null value to non-nullable type.

Check warning on line 67 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (ubuntu-latest)

Dereference of a possibly null reference.

Check warning on line 67 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (windows-latest)

Converting null literal or possible null value to non-nullable type.

Check warning on line 67 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (windows-latest)

Dereference of a possibly null reference.

Check warning on line 67 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (macos-latest)

Converting null literal or possible null value to non-nullable type.

Check warning on line 67 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (macos-latest)

Dereference of a possibly null reference.
Rules = ((JArray)jObject["rules"])?.Select(r => WitnessRule.FromJson((JObject)r)).ToArray() ?? Array.Empty<WitnessRule>(),

Check warning on line 68 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test-Everything

Converting null literal or possible null value to non-nullable type.

Check warning on line 68 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test-Everything

Converting null literal or possible null value to non-nullable type.

Check warning on line 68 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (ubuntu-latest)

Converting null literal or possible null value to non-nullable type.

Check warning on line 68 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (ubuntu-latest)

Converting null literal or possible null value to non-nullable type.

Check warning on line 68 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (windows-latest)

Converting null literal or possible null value to non-nullable type.

Check warning on line 68 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (windows-latest)

Converting null literal or possible null value to non-nullable type.

Check warning on line 68 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (macos-latest)

Converting null literal or possible null value to non-nullable type.

Check warning on line 68 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (macos-latest)

Converting null literal or possible null value to non-nullable type.
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not resolved, we need validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

private static Witness WitnessFromJson(JObject jObject)
{
return new Witness
{
InvocationScript = Convert.FromBase64String(jObject["invocation"]?.AsString() ?? string.Empty),
VerificationScript = Convert.FromBase64String(jObject["verification"]?.AsString() ?? string.Empty)
};
}

public static SignerWithWitness[] ParseArray(JArray array, ProtocolSettings settings)
{
if (array == null)
throw new ArgumentNullException(nameof(array));

if (array.Count > Transaction.MaxTransactionAttributes)
throw new RpcException(RpcError.InvalidParams.WithData("Max allowed signers or witnesses exceeded."));

return array.Select(item =>
{
if (TryParse(item, settings, out var signerWithWitness))

Check warning on line 91 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test-Everything

Possible null reference argument for parameter 'value' in 'bool SignerWithWitness.TryParse(JToken value, ProtocolSettings settings, out SignerWithWitness? signerWithWitness)'.

Check warning on line 91 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (ubuntu-latest)

Possible null reference argument for parameter 'value' in 'bool SignerWithWitness.TryParse(JToken value, ProtocolSettings settings, out SignerWithWitness? signerWithWitness)'.

Check warning on line 91 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (windows-latest)

Possible null reference argument for parameter 'value' in 'bool SignerWithWitness.TryParse(JToken value, ProtocolSettings settings, out SignerWithWitness? signerWithWitness)'.

Check warning on line 91 in src/Plugins/RpcServer/Model/SignerWithWitness.cs

View workflow job for this annotation

GitHub Actions / Test (macos-latest)

Possible null reference argument for parameter 'value' in 'bool SignerWithWitness.TryParse(JToken value, ProtocolSettings settings, out SignerWithWitness? signerWithWitness)'.
return signerWithWitness;
throw new ArgumentException($"Invalid signer or witness format: {item}");
}).ToArray();
}

private static UInt160 AddressToScriptHash(string address, byte version)
Jim8y marked this conversation as resolved.
Show resolved Hide resolved
{
if (UInt160.TryParse(address, out var scriptHash))
{
return scriptHash;
}

return address.ToScriptHash(version);
}
}
100 changes: 99 additions & 1 deletion src/Plugins/RpcServer/ParameterConverter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,13 @@
// modifications are permitted.

using Neo.Json;
using Neo.Network.P2P.Payloads;
using Neo.Plugins.RpcServer.Model;
using Neo.SmartContract;
using Neo.Wallets;
using System;
using System.Collections.Generic;
using System.Linq;
using JToken = Neo.Json.JToken;

namespace Neo.Plugins.RpcServer;
Expand All @@ -39,7 +42,12 @@ static ParameterConverter()
{ typeof(bool), token => Result.Ok_Or(token.AsBoolean, CreateInvalidParamError<bool>(token)) },
{ typeof(UInt256), ConvertUInt256 },
{ typeof(ContractNameOrHashOrId), ConvertContractNameOrHashOrId },
{ typeof(BlockHashOrIndex), ConvertBlockHashOrIndex }
{ typeof(BlockHashOrIndex), ConvertBlockHashOrIndex },
{ typeof(Signer), ConvertSigner },
{ typeof(ContractParameter), ConvertContractParameter },
{ typeof(Signer[]), ConvertSignerArray },
{ typeof(ContractParameter[]), ConvertContractParameterArray },
{ typeof(Guid), ConvertGuid }
Jim8y marked this conversation as resolved.
Show resolved Hide resolved
};
}

Expand Down Expand Up @@ -107,6 +115,23 @@ internal static object ConvertUInt160(JToken token, byte addressVersion)
RpcError.InvalidParams.WithData($"Invalid UInt160 Format: {token}"));
}

internal static object ConvertSignerWithWitnessArray(JToken token, ProtocolSettings settings)
{
if (token is JArray jArray)
{
return SignerWithWitness.ParseArray(jArray, settings);
}

if (token is JObject jObject)
{
if (SignerWithWitness.TryParse(jObject, settings, out var signerWithWitness))
{
return new[] { signerWithWitness };
}
}
throw new RpcException(RpcError.InvalidParams.WithData($"Invalid SignerWithWitness format: {token}"));
}

private static object ConvertUInt256(JToken token)
{
if (UInt256.TryParse(token.AsString(), out var hash))
Expand Down Expand Up @@ -134,6 +159,79 @@ private static object ConvertBlockHashOrIndex(JToken token)
throw new RpcException(RpcError.InvalidParams.WithData($"Invalid block hash or index Format: {token}"));
}

private static object ConvertSigner(JToken token)
{
if (token is JObject jObject)
{
try
{
return Signer.FromJson(jObject);
}
catch (FormatException)
{
throw new RpcException(CreateInvalidParamError<Signer>(token));
}
}
throw new RpcException(CreateInvalidParamError<Signer>(token));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would adding an else statement here make it clearer?

}

private static object ConvertContractParameter(JToken token)
{
if (token is JObject jObject)
{
try
{
return ContractParameter.FromJson(jObject);
}
catch (FormatException)
{
throw new RpcException(CreateInvalidParamError<ContractParameter>(token));
}
}
throw new RpcException(CreateInvalidParamError<ContractParameter>(token));
}

private static object ConvertSignerArray(JToken token)
{
if (token is JArray jArray)
{
try
{
return jArray.Select(t => Signer.FromJson(t as JObject)).ToArray();
}
catch (FormatException)
{
throw new RpcException(CreateInvalidParamError<Signer[]>(token));
}
}
throw new RpcException(CreateInvalidParamError<Signer[]>(token));
}
Jim8y marked this conversation as resolved.
Show resolved Hide resolved

private static object ConvertContractParameterArray(JToken token)
{
if (token is JArray jArray)
{
try
{
return jArray.Select(t => ContractParameter.FromJson(t as JObject)).ToArray();
}
catch (FormatException)
{
throw new RpcException(CreateInvalidParamError<ContractParameter[]>(token));
}
}
throw new RpcException(CreateInvalidParamError<ContractParameter[]>(token));
}

private static object ConvertGuid(JToken token)
{
if (Guid.TryParse(token.AsString(), out var guid))
{
return guid;
}
throw new RpcException(CreateInvalidParamError<Guid>(token));
}

private static RpcError CreateInvalidParamError<T>(JToken token)
{
return RpcError.InvalidParams.WithData($"Invalid {typeof(T)} value: {token}");
Expand Down
Loading
Loading