Add diagnostic for ApplicationEngine #2613
Conversation
| @@ -250,6 +259,8 @@ internal ContractTask<T> CallFromNativeContract<T>(UInt160 callingScriptHash, UI | |||
| protected override void ContextUnloaded(ExecutionContext context) | |||
| { | |||
| base.ContextUnloaded(context); | |||
| if (Diagnostic is not null) | |||
| currentNodeOfInvocationTree = currentNodeOfInvocationTree.Parent; | |||
There was a problem hiding this comment.
What if the parent of currentNodeOfInvocationTree is null? should we also remove the root of Diagnostic?
|
|
||
| namespace Neo.SmartContract | ||
| { | ||
| public class Diagnostic |
There was a problem hiding this comment.
Diagnostic seems like a poor name for this class. It sounds to me as if it is more general purpose than simply tracking the tree of contract invocations. Could we call the class InvocationTree and call the property Contracts?
There was a problem hiding this comment.
We can add more functions to Diagnostic later.
| /// <returns>The engine instance created.</returns> | ||
| public static ApplicationEngine Create(TriggerType trigger, IVerifiable container, DataCache snapshot, Block persistingBlock = null, ProtocolSettings settings = null, long gas = TestModeGas) | ||
| public static ApplicationEngine Create(TriggerType trigger, IVerifiable container, DataCache snapshot, Block persistingBlock = null, ProtocolSettings settings = null, long gas = TestModeGas, Diagnostic diagnostic = null) |
There was a problem hiding this comment.
Diagnostic parameter should have null default value to avoid code breakage
There was a problem hiding this comment.
It does have a null default value.
| @@ -28,7 +28,8 @@ public interface IApplicationEngineProvider | |||
| /// <param name="persistingBlock">The block being persisted. It should be <see langword="null"/> if the <paramref name="trigger"/> is <see cref="TriggerType.Verification"/>.</param> | |||
| /// <param name="settings">The <see cref="ProtocolSettings"/> used by the engine.</param> | |||
| /// <param name="gas">The maximum gas used in this execution. The execution will fail when the gas is exhausted.</param> | |||
| /// <param name="diagnostic">The diagnostic to be used by the <see cref="ApplicationEngine"/>.</param> | |||
There was a problem hiding this comment.
Diagnostic parameter should have null default value to avoid code breakage
There was a problem hiding this comment.
This is an interface. I think it is meaningless to add the default parameter. Because all classes that implement this interface should be updated.
There was a problem hiding this comment.
Unless there's a null default value, we also have to update code that calls into classes that implement this interface.
There was a problem hiding this comment.
The classes are not to be called directly. We need to use ApplicationEngine.Create() instead.
| var state = context.GetState<ExecutionContextState>(); | ||
| state.ScriptHash ??= ((byte[])context.Script).ToScriptHash(); | ||
| invocationCounter.TryAdd(state.ScriptHash, 1); | ||
|
|
||
| if (Diagnostic is not null) |
There was a problem hiding this comment.
There is an interesting problem related to #2130 (comment), suppose we have a deployed contract with onNEP17Payment, we transfer some NEO to it, add a number of blocks and then transfer 1 NEO more. This triggers GAS distribution and onNEP17Payment call from GAS contract (even before onNEP17Payment call from NEO). The contract can ask GetCallingScriptHash() and get GAS hash in return, because this invocation is made from the GAS contract. But this will never be reflected in the invocation tree, there we'll have
- entry
- NEO
- contract
- contract
- NEO
instead of
- entry
- NEO
- GAS
- contract
- contract
- GAS
- NEO
No description provided.