[Neo Core Fix] Check params at Calling Contract #3438
Conversation
Jim8y
added
Waiting for Review
Blocked
|
Wait, wait, wait a minute! As much as I love this change you can't just roll it out with the nearest hardfork. Remember all the pain of #2810. We need to:
|
|
Will this affect calls between contracts? |
Exactly, this pr is marked as blocked, so will not be merged without careful review and discussion. |
Yes, it will. That is why this pr may only take effect to contracts that are depolyed or updated after the next hardfork,,,,,, will be discussed. |
| @@ -449,6 +449,7 @@ protected internal Signer[] GetCurrentSigners() | |||
| private static bool CheckItemType(StackItem item, ContractParameterType type) | |||
| { | |||
| StackItemType aType = item.Type; | |||
| if (aType == StackItemType.Any) return true; | |||
There was a problem hiding this comment.
I don't think it's correct for VM-compatible SC types, because for example, if you require stackitem to be Boolean, then Any just doesn't match this requirement.
There was a problem hiding this comment.
I see what you mean, will update.
| @@ -319,7 +319,11 @@ private ExecutionContext CallContractInternal(ContractState contract, ContractMe | |||
| state.CallingContext = currentContext; | |||
|
|
|||
| for (int i = args.Count - 1; i >= 0; i--) | |||
| { | |||
| if (!CheckItemType(args[i], method.Parameters[i].Type)) | |||
| throw new InvalidOperationException($"The type of the argument `{args[i]}` does not match the formal parameter."); | |||
There was a problem hiding this comment.
It may be too harsh for mainnet contracts (even if enabled with hardfrok), this change may break some of them and even prevent them from update. What do you think about using the same approach as it was done for SC Events parameters check (#2810 (comment)): in next release we firstly enable a warning log in case of parameters mismatch, and then in the subsequent release we convert the log to an exception.
There was a problem hiding this comment.
It may be too harsh for mainnet contracts (even if enabled with hardfrok), this change may break some of them and even prevent them from update. What do you think about using the same approach as it was done for SC Events parameters check (#2810 (comment)): in next release we firstly enable a warning log in case of parameters mismatch, and then in the subsequent release we convert the log to an exception.
We have discussed this in the last meeting, check will only happen for newly deployed or updated contracts, this pr will be updated.
Description
Neo core does not check the type of vales being passed to the contract, and this can cause problems.
for example: neo-project/neo-devpack-dotnet#863
Fixes # #1891 neo-project/neo-devpack-dotnet#863
Type of change
How Has This Been Tested?
Test Configuration:
Checklist: