[NEP4] Feature-dynamic-invoke verification #120
Conversation
| byte[] script_hash = EvaluationStack.Peek().GetByteArray(); | ||
| CachedScriptTable script_table = (CachedScriptTable)this.table; | ||
| ContractState contract_state = script_table.GetContractState(script_hash); | ||
| return contract_state.HasDynamicInvoke; |
There was a problem hiding this comment.
Here you obtain HasDynamicInvoke from the first item of the stack, but in fee calculation at the third.
If both are good, im prefer the third for save CachedScriptTable logics
There was a problem hiding this comment.
That is a good point. Better to make both of them use CachedScriptTable
There was a problem hiding this comment.
The fee calculation one would be fastest ( EvaluationStack.Peek(3) )
There was a problem hiding this comment.
@shargon I changed them to both use EvaluationStack.Peek(3) as it is faster. This also allows me to not need to use CachedScriptTable, and keep the IScriptTable table property on ExecutionEngine.cs in the VM private instead of protected.
neo/neo.csproj
Outdated
| </PackageReference> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup> |
There was a problem hiding this comment.
Before neo-vm was in Nuget, is this change an oversight?
There was a problem hiding this comment.
Yes, I should not have committed that :)
| @@ -228,6 +228,16 @@ private bool CheckStackSize(OpCode nextInstruction) | |||
| return true; | |||
| } | |||
|
|
|||
| private bool CheckDynamicInvoke(OpCode nextInstruction) | |||
| { | |||
| if(nextInstruction == OpCode.APPCALL) | |||
There was a problem hiding this comment.
Need to check both APPCALL and TAILCALL.
neo/SmartContract/StateMachine.cs
Outdated
| @@ -201,6 +201,7 @@ private bool Contract_Create(ExecutionEngine engine) | |||
| ContractParameterType[] parameter_list = engine.EvaluationStack.Pop().GetByteArray().Select(p => (ContractParameterType)p).ToArray(); | |||
| if (parameter_list.Length > 252) return false; | |||
| ContractParameterType return_type = (ContractParameterType)(byte)engine.EvaluationStack.Pop().GetBigInteger(); | |||
| ContractPropertyState contract_properties = (ContractPropertyState)(byte)engine.EvaluationStack.Pop().GetBigInteger(); | |||
| bool need_storage = engine.EvaluationStack.Pop().GetBoolean(); | |||
neo/Core/ContractState.cs
Outdated
| @@ -98,6 +103,7 @@ public override JObject ToJson() | |||
| json["parameters"] = new JArray(ParameterList.Select(p => (JObject)p)); | |||
| json["returntype"] = ReturnType; | |||
| json["storage"] = HasStorage; | |||
| json["dynamic_invoke"] = HasDynamicInvoke; | |||
There was a problem hiding this comment.
json["properties"] = new JObject();
json["properties"]["storage"] = HasStorage;
json["properties"]["dynamic_invoke"] = HasDynamicInvoke;Is this better?
| { | ||
| if(nextInstruction == OpCode.APPCALL || nextInstruction == OpCode.TAILCALL) | ||
| { | ||
| for (int i = CurrentContext.InstructionPointer + 1; i < CurrentContext.InstructionPointer + 20; i++) |
There was a problem hiding this comment.
if (CurrentContext.InstructionPointer + 20 > CurrentContext.Script.Length) ?
There was a problem hiding this comment.
Not sure if we need this check. If we go beyond the end of the script, the error will be caught in the Execute method
|
One more thing, if we have an invocation chain: A -> B -> C |
|
It should behave that way. I will test it out to make sure. |
…to feature-dynamic-invoke
|
Is it ready to merge? |
|
Still want to test an invocation chain |
|
all set |
In regards to NEP4 neo-project/proposals#19
Works in tandem with these PR
Adds the following changes
neo.Core.ContractPropertyStateHasStorageas attribute ofneo.Core.ContractStateand replace withContractPropertiesneo.SmartContract.ApplicationEnginethat a Contract has theHasDynamicInvokeproperty set before using theDYNAMICCALLopcodeNeo.Contract.CreateandNeo.Contract.Migratewith differing amounts based on theContractPropertiesof the contract to be created.