Feature: call tracer #21
Conversation
|
Almost all of my comments are "go-style" type comments or improvement suggestions, nothing blocking. I also am not expert enough about this functionality to confirm that it's logically sound, but it looks good to me. |
| if *config.Tracer == "goCallTracer" { | ||
| tracer = NewCallTracer(statedb) |
There was a problem hiding this comment.
Presumably New has the potential to throw errors, since it outputs one. By comparison, your NewCallTracer does not throw errors because it simply builds a struct and returns it.
My question: What functionality did New provide which could generate errors? Should that functionality be included in your NewCallTracer function? (I don't think you need exact answers here, just thinking out loud)
| Calls []*call `json:"calls,omitempty"` | ||
| Error string `json:"error,omitempty"` | ||
| startTime time.Time | ||
| outOff uint64 |
| case TracerResult: | ||
| return tracer.GetResult() | ||
|
|
||
| case Tracer: | ||
| return tracer.GetResult() |
There was a problem hiding this comment.
These two cases are identical and I don't see anything that triggers one or the other. Can we just have the one?
| func (tracer *CallTracer) CaptureState(pc uint64, op vm.OpCode, gas, cost uint64, scope *vm.ScopeContext, rData []byte, depth int, err error) { | ||
| // for depth < len(tracer.callStack) { | ||
| // c := tracer.callStack[tracer.i()] | ||
| // c.GasUsed = c.Gas - gas | ||
| // tracer.callStack = tracer.callStack[:tracer.i()] | ||
| // } | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| tracer.callStack[tracer.i()].Error = "internal failure" | ||
| log.Warn("Panic during trace. Recovered.", "err", r) | ||
| } | ||
| }() | ||
| if op == vm.CREATE || op == vm.CREATE2 { | ||
| inOff := scope.Stack.Back(1).Uint64() | ||
| inLen := scope.Stack.Back(2).Uint64() | ||
| hvalue := hexutil.Big(*scope.Contract.Value()) | ||
| tracer.descend(&call{ |
There was a problem hiding this comment.
Looks like this method effectively takes a call structure, or something like it.
You could define a type CaptureStateInput struct to more carefully define the inputs to this function if you wanted. You don't win a ton from that besides a little cleaner function signature, and you'd also see keys when calling this function
t.CaptureState( CaptureStateInput{Arg1: 1, Arg2: 2})
func (tracer *CallTracer) CaptureState(i CaptureStateInput){}
| return tracer.callStack[0], nil | ||
| } | ||
|
|
||
| func (tracer *CallTracer) CaptureStart(evm *vm.EVM, from common.Address, to common.Address, create bool, input []byte, gas uint64, value *big.Int) { |
There was a problem hiding this comment.
I personally don't mind this AT ALL! But those weird gopher people will probably tell you something like "you should shorten tracer to just t".
| }) | ||
| return | ||
| } | ||
| if op == vm.CALL || op == vm.CALLCODE || op == vm.DELEGATECALL || op == vm.STATICCALL { |
There was a problem hiding this comment.
When dealing with many if statements, IMO it's worth it to break them across new lines.
| off := 1 | ||
| if op == vm.DELEGATECALL || op == vm.STATICCALL { | ||
| off = 0 | ||
| } |
There was a problem hiding this comment.
Me five minutes ago: "Oh you should use a ternary, let me look up an example"
Me now: "Oh this is exactly what golang wants you to do, they don't support ternary"
I want my ? I want my : I want my Ternary
| if depth == len(tracer.callStack)-1 { | ||
| c := tracer.callStack[tracer.i()] | ||
| // c.Time = fmt.Sprintf("%v", time.Since(c.startTime)) | ||
| tracer.callStack = tracer.callStack[:len(tracer.callStack)-1] | ||
| if vm.StringToOp(c.Type) == vm.CREATE || vm.StringToOp(c.Type) == vm.CREATE2 { | ||
| c.GasUsed = hexutil.Uint64(c.gasIn - c.gasCost - gas) | ||
| ret := scope.Stack.Back(0) | ||
| if ret.Uint64() != 0 { | ||
| c.To = common.BytesToAddress(ret.Bytes()) | ||
| c.Output = tracer.statedb.GetCode(c.To) | ||
| } else if c.Error == "" { | ||
| c.Error = "internal failure" | ||
| } | ||
| } else { | ||
| c.GasUsed = hexutil.Uint64(c.gasIn - c.gasCost + uint64(c.Gas) - gas) | ||
| ret := scope.Stack.Back(0) | ||
| if ret.Uint64() != 0 { | ||
| c.Output = hexutil.Bytes(scope.Memory.GetCopy(int64(c.outOff), int64(c.outLen))) | ||
| } else if c.Error == "" { | ||
| c.Error = "internal failure" | ||
| } |
There was a problem hiding this comment.
This appears to be gas calculation based on what the transaction type is. Could we write these as several internal functions, and delegate the calculations to them?
if tracer.isContractCreate { gasUsed := tracer.contractCreateGasUsed()}
What
Testing