improve parse method in neo-cli#3204
Conversation
src/Neo.CLI/CLI/MainService.Tools.cs
Outdated
| /// <returns>Returns null when the string does not represent a private key; otherwise | ||
| /// returns the string that represents the converted public key. | ||
| /// </returns> | ||
| public static string? PrivateKeyToPublicKey(string wif) |
There was a problem hiding this comment.
wif to pubkey?
Yeah. What's the problem?
There was a problem hiding this comment.
nothing,just thinking maybe you can make it both support wif and privatkey @shargon
There was a problem hiding this comment.
It worries me that people use a private key so openly, why not create a wallet?
src/Neo.CLI/CLI/MainService.Tools.cs
Outdated
| //Initialize all OpCodes | ||
| var OperandSizePrefixTable = new int[256]; | ||
| var OperandSizeTable = new int[256]; | ||
| foreach (FieldInfo field in typeof(OpCode).GetFields(BindingFlags.Public | BindingFlags.Static)) |
There was a problem hiding this comment.
Why not use Script and GetInstruction?
There was a problem hiding this comment.
Yes use Script class or something like this
byte[] data = [];
foreach (var d in data)
{
Console.WriteLine(Enum.GetName((OpCode)d));
}| private string? AddressToScripthashLE(string address) | ||
| { | ||
| try | ||
| { | ||
| var bigEndScript = address.ToScriptHash(NeoSystem.Settings.AddressVersion); | ||
|
|
||
| return bigEndScript.ToArray().ToHexString(); | ||
| } | ||
| catch | ||
| { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Could you please add the following compatibility test for this API: convert address NRQJbjKWgcLX214Kbx6PDH2CHJdqbsSi4Q to LE script hash. I'd like to ensure that NeoGo has the same LE/BE conversion result:
anna@kiwi:~/Documents/GitProjects/nspcc-dev/neo-go$ ./bin/neo-go util convert NRQJbjKWgcLX214Kbx6PDH2CHJdqbsSi4Q
Address to BE ScriptHash 3c347b368199840085f7d92704a14becd578fa3d
Address to LE ScriptHash 3dfa78d5ec4ba10427d9f78500849981367b343c
Address to Base64 (BE) PDR7NoGZhACF99knBKFL7NV4+j0=
Address to Base64 (LE) Pfp41exLoQQn2feFAISZgTZ7NDw=
String to Hex 4e52514a626a4b5767634c583231344b6278365044483243484a6471627353693451
String to Base64 TlJRSmJqS1dnY0xYMjE0S2J4NlBESDJDSEpkcWJzU2k0UQ==
There was a problem hiding this comment.
public static UInt160? TryParseUInt160(string? value)
{
if (string.IsNullOrEmpty(value)) return default;
if (UInt160.TryParse(value, out var result))
return result;
return UInt160.Zero;
}
src/Neo.CLI/CLI/MainService.Tools.cs
Outdated
| public string? BigEndianToLittleEndian(string hex) | ||
| { | ||
| hex = hex.ToLower(); | ||
| if (!new Regex("^0x([0-9a-f]{2})+$").IsMatch(hex)) return null; |
There was a problem hiding this comment.
Can we make 0x prefix optional?
There was a problem hiding this comment.
Already modified
src/Neo.CLI/CLI/MainService.Tools.cs
Outdated
| if (op.ToString().StartsWith("PUSHINT")) | ||
| { | ||
| result.Add($"{op} {new BigInteger(operand)}"); | ||
| } | ||
| else if (op == OpCode.SYSCALL) | ||
| { | ||
| result.Add($"{op} {dic[BitConverter.ToUInt32(operand)]}"); | ||
| } | ||
| else | ||
| { | ||
| result.Add($"{op} {operand.ToHexString()}"); | ||
| } |
There was a problem hiding this comment.
We may extend the list of available formatters, take a look at the https://github.com/nspcc-dev/neo-go/blob/198af7f3aef039995b8f8cacefefb3a807b41ff3/pkg/vm/vm.go#L184-L230
There was a problem hiding this comment.
- Dont use
RegExis so expensive and cost way to much CPU and can hang forever. - Your naming convention. Change all methods to the proper names. To many for me to change for you; in a review. If you want I can change for you in
Visual Studio; just would need access to your repo.
src/Neo.CLI/CLI/MainService.Tools.cs
Outdated
| var parseFunctions = new Dictionary<string, Func<string, string?>>() | ||
| { | ||
| { "Address to ScriptHash", AddressToScripthash }, | ||
| { "Address to ScriptHash (big-endian)", AddressToScripthash }, |
There was a problem hiding this comment.
Can we use Attribute class for the function description?
There was a problem hiding this comment.
Good idea, I'll do it.
There was a problem hiding this comment.
Already modified
src/Neo.CLI/CLI/MainService.Tools.cs
Outdated
| /// </returns> | ||
| private string? NumberToHexToString(string input) | ||
| { | ||
| if (!new Regex("^\\d+$").IsMatch(input) || input.StartsWith('0')) |
There was a problem hiding this comment.
TryCatch is faster and cheaper for checking HexString encoding. Just TryCatch Convert.ToHexString and look for FormatException
There was a problem hiding this comment.
Already modified
src/Neo.CLI/CLI/MainService.Tools.cs
Outdated
| /// <returns>Correct Base64 string</returns> | ||
| public static string Base64Fixed(string str) | ||
| { | ||
| MatchCollection mc = Regex.Matches(str, @"\\u([\w]{2})([\w]{2})", RegexOptions.Compiled | RegexOptions.IgnoreCase); |
There was a problem hiding this comment.
TryCatch is faster and cheaper for checking Base64 encoding. Just TryCatch Convert.FromBase64String and look for FormatException
src/Neo.CLI/CLI/MainService.Tools.cs
Outdated
| result.Add($"{op}"); | ||
| } | ||
| } | ||
| return "\r\n" + string.Join("\r\n", result.ToArray()); |
There was a problem hiding this comment.
Use Environment.NewLine we do support different Operating Systems.
There was a problem hiding this comment.
Already modified
I invited you. Thanks. |
src/Neo.CLI/CLI/MainService.Tools.cs
Outdated
| private string? BigEndianToLittleEndian(string hex) | ||
| { | ||
| bool hasHexPrefix = hex.StartsWith("0x", StringComparison.InvariantCultureIgnoreCase); | ||
| hex = hex[2..]; |
There was a problem hiding this comment.
this will throw an exception if the input is "a"
src/Neo.CLI/CLI/MainService.Tools.cs
Outdated
|
|
||
| namespace Neo.CLI | ||
| { | ||
| public class ParseFunctionAttribute : Attribute |
There was a problem hiding this comment.
Unless it has Interface with it.
src/Neo.CLI/CLI/MainService.Tools.cs
Outdated
| [ParseFunction("Public Key to Address")] | ||
| private string? PublicKeyToAddress(string pubKey) | ||
| { | ||
| bool IsValidPubKey(string pubKey) |
src/Neo.CLI/CLI/MainService.Tools.cs
Outdated
| if (pubKey.Length != 66) return false; | ||
| if (pubKey[0] != '0') return false; | ||
| if (pubKey[1] != '2' && pubKey[1] != '3') return false; | ||
|
|
||
| for (var i = 2; i < pubKey.Length; i++) | ||
| { | ||
| var c = pubKey[i]; | ||
| if (!((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F'))) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; |
There was a problem hiding this comment.
| if (pubKey.Length != 66) return false; | |
| if (pubKey[0] != '0') return false; | |
| if (pubKey[1] != '2' && pubKey[1] != '3') return false; | |
| for (var i = 2; i < pubKey.Length; i++) | |
| { | |
| var c = pubKey[i]; | |
| if (!((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F'))) | |
| { | |
| return false; | |
| } | |
| } | |
| return true; | |
| return ECPoint.TryParse(pubkey, ECCurve.Secp256r1, out _); |
There was a problem hiding this comment.
public keys can be many lengths and prefixes. Look at Neo.Cryptography.ECC.ECPoint class in method TryParse, Parse, DecodePoint or FromBytes
There was a problem hiding this comment.
Already removed IsValidPubKey method
cschuchardt88
left a comment
There was a problem hiding this comment.
Please remove all the hex methods in the file. No need for it. DotNet has Convert.ToHexString or Convert.FromHexString built-in.
src/Neo.CLI/CLI/MainService.Tools.cs
Outdated
| [ParseFunction("Hex String to String")] | ||
| private string? HexToString(string hexString) | ||
| { | ||
| try |
There was a problem hiding this comment.
This can be replaced with Convert.ToHexString
There was a problem hiding this comment.
Already removed HexToString StringToHex NumberToHexToString HexToNumber Base64ToHexString HexScriptsToOpCode method
src/Neo.CLI/CLI/MainService.Tools.cs
Outdated
| if (!IsHex(hex)) return null; | ||
| return "0x" + hex.HexToBytes().Reverse().ToArray().ToHexString(); ; |
There was a problem hiding this comment.
| if (!IsHex(hex)) return null; | |
| return "0x" + hex.HexToBytes().Reverse().ToArray().ToHexString(); ; | |
| Convert.FromHexString(hex); // check for is Hex | |
| var bytes = System.Text.Encoding.UTF8.GetBytes(hex); | |
| return "0x" + Convert.ToHexString([.. bytes.Reverse()]).ToLower(); |
There was a problem hiding this comment.
This will output an uppercase string
There was a problem hiding this comment.
There I changed to LowerCase.
src/Neo.CLI/CLI/MainService.Tools.cs
Outdated
| bool hasHexPrefix = hex.StartsWith("0x", StringComparison.InvariantCultureIgnoreCase); | ||
| hex = hasHexPrefix ? hex[2..] : hex; | ||
| if (!hasHexPrefix || !IsHex(hex)) return null; | ||
| return hex.HexToBytes().Reverse().ToArray().ToHexString(); |
There was a problem hiding this comment.
| bool hasHexPrefix = hex.StartsWith("0x", StringComparison.InvariantCultureIgnoreCase); | |
| hex = hasHexPrefix ? hex[2..] : hex; | |
| if (!hasHexPrefix || !IsHex(hex)) return null; | |
| return hex.HexToBytes().Reverse().ToArray().ToHexString(); | |
| Convert.FromHexString(hex); // Check for hex | |
| var bytes = System.Text.Encoding.UTF8.GetBytes(hex); | |
| return Convert.ToHexString([.. bytes.Reverse()]); |
There was a problem hiding this comment.
It will crash when parse "0x3ff68d232a60f23a5805b8c40f7e61747f6f61ce"
src/Neo.CLI/CLI/MainService.Tools.cs
Outdated
| /// </returns> | ||
| /// <param name="str">Base64 strings containing unicode</param> | ||
| /// <returns>Correct Base64 string</returns> | ||
| public string Base64Fixed(string str) |
There was a problem hiding this comment.
I believe Console.OutputEncoding = Utility.StrictUTF8; will fix this problem.
There was a problem hiding this comment.
The purpose of this one is to handle the output of other wallets or SDKs that have Base64 with Unicode escaped characters in them
| var bigEndScript = address.ToScriptHash(NeoSystem.Settings.AddressVersion); | ||
|
|
||
| return bigEndScript.ToArray().ToHexString(); |
There was a problem hiding this comment.
| var bigEndScript = address.ToScriptHash(NeoSystem.Settings.AddressVersion); | |
| return bigEndScript.ToArray().ToHexString(); | |
| return address.ToScriptHash(NeoSystem.Settings.AddressVersion); |
There was a problem hiding this comment.
This will output a result of type UInt160, not a string.
src/Neo.CLI/CLI/MainService.Tools.cs
Outdated
| var bytes = Convert.FromBase64String(base64.Trim()); | ||
| return bytes.ToHexString(); |
There was a problem hiding this comment.
| var bytes = Convert.FromBase64String(base64.Trim()); | |
| return bytes.ToHexString(); | |
| var bytes = Convert.FromBase64String(base64.Trim()); | |
| return $"0x{Convert.ToHexString(bytes)}"; |
There was a problem hiding this comment.
Already modified
src/Neo.CLI/CLI/MainService.Tools.cs
Outdated
| if (!IsValidPubKey(pubKey)) return null; | ||
| return Contract.CreateSignatureContract(ECPoint.Parse(pubKey, ECCurve.Secp256r1)).ScriptHash.ToAddress(NeoSystem.Settings.AddressVersion); |
There was a problem hiding this comment.
| if (!IsValidPubKey(pubKey)) return null; | |
| return Contract.CreateSignatureContract(ECPoint.Parse(pubKey, ECCurve.Secp256r1)).ScriptHash.ToAddress(NeoSystem.Settings.AddressVersion); | |
| if (ECPoint.TryParse(pubKey, ECCurve.Secp256r1, out var publicKey) == false) | |
| return null; | |
| return Contract.CreateSignatureContract(publicKey) | |
| .ScriptHash | |
| .ToAddress(NeoSystem.Settings.AddressVersion); |
There was a problem hiding this comment.
Already modified
| { | ||
| var privateKey = Wallet.GetPrivateKeyFromWIF(wif); | ||
| var account = new KeyPair(privateKey); | ||
| return account.PublicKey.ToArray().ToHexString(); |
There was a problem hiding this comment.
| return account.PublicKey.ToArray().ToHexString(); | |
| return Convert.ToHexString(account.PublicKey.EncodePoint(true)).ToLower(); |
There was a problem hiding this comment.
This will output an uppercase string
src/Neo.CLI/CLI/MainService.Tools.cs
Outdated
| var pubKey = WIFToPublicKey(wif); | ||
| return Contract.CreateSignatureContract(ECPoint.Parse(pubKey, ECCurve.Secp256r1)).ScriptHash.ToAddress(0x35); |
There was a problem hiding this comment.
| var pubKey = WIFToPublicKey(wif); | |
| return Contract.CreateSignatureContract(ECPoint.Parse(pubKey, ECCurve.Secp256r1)).ScriptHash.ToAddress(0x35); | |
| var privateKey = Wallet.GetPrivateKeyFromWIF(wif); | |
| var account = new KeyPair(privateKey); | |
| return account.PublicKeyHash.ToAddress(NeoSystem.Settings.AddressVersion); |
There was a problem hiding this comment.
These two results are not the same
There was a problem hiding this comment.
oh sorry, but what's the 0x35 for?
|
Can this be merged? |
cschuchardt88
left a comment
There was a problem hiding this comment.
Also please run dotnet format we have made changes the .editorconfig to re-run .github action tests.
src/Neo.CLI/CLI/MainService.Tools.cs
Outdated
| private bool IsHex(string str) => str.Length % 2 == 0 && str.All(c => (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F')); | ||
|
|
There was a problem hiding this comment.
This will always be false because of the prefix 0x having the x in it.
There was a problem hiding this comment.
It is by design, only for hexstring without '0x'
There was a problem hiding this comment.
dotnet format passed with no changes
Already modified, and |
|
Can this be merged? @shargon @cschuchardt88 @Jim8y |
|
Need a minimum of two people to approve on a PR before you can merge it. |
|
Let us wait until the 3.7 mainnet is online please. |
|
I think that we should wait a little as @Jim8y emphasized. |
…gins * 'latest-plugins' of github.com:Jim8y/neo: (21 commits) fix: custom plugins won't shown by command `plugins` (neo-project#3269) COVERALL: Improve maintenance and readbility of some variables (neo-project#3248) Update nuget (neo-project#3262) [**Part-2**] Neo module/master fixes (neo-project#3244) Fix `dotnet pack` error (neo-project#3266) Fix and Update devcontainer.json to use Dockerfile (neo-project#3259) Add optimization to template (neo-project#3247) Optimize plugin's models (neo-project#3246) fix CancelTransaction !signers.Any() (neo-project#3263) COVERALL: fix broken by changing report from lcov to cobertura (neo-project#3252) fix TraverseIterator count (neo-project#3261) Native: include DeprecatedIn hardfork into usedHardforks (neo-project#3245) [**Part-1**] `neo-module/master` (neo-project#3232) Make `ApplicationEngine.LoadContext` protection level `public` (neo-project#3243) improve parse method in neo-cli (neo-project#3204) Fix neo-project#3239 (neo-project#3242) Neo.CLI: enable hardforks for NeoFS mainnet (neo-project#3240) v3.7.4 (neo-project#3237) fix hardfork issues (neo-project#3234) Update src/Neo.CLI/CLI/MainService.Plugins.cs ... # Conflicts: # src/Neo.CLI/CLI/MainService.Plugins.cs
close #3201
I didn't add the Mnemonic conversion because it would require bringing in other NuGet packages. For Mnemonic conversion you can use the web version of the conversion tool https://neo.org/converter/index