From 6c70b55cb61c3c4e65cda9a0d7ca8f9bde9d4985 Mon Sep 17 00:00:00 2001 From: Josh Klopfenstein Date: Tue, 5 Sep 2023 16:20:53 -0500 Subject: [PATCH] Simplify error handling in hintrunner See https://github.com/uber-go/guide/blob/master/style.md#errors --- pkg/hintrunner/error.go | 56 ------------------------- pkg/hintrunner/hint.go | 38 ++++++++++------- pkg/hintrunner/hintrunner.go | 6 ++- pkg/hintrunner/operand.go | 79 ++++++++++++++++++++---------------- 4 files changed, 70 insertions(+), 109 deletions(-) delete mode 100644 pkg/hintrunner/error.go diff --git a/pkg/hintrunner/error.go b/pkg/hintrunner/error.go deleted file mode 100644 index b8ae9c81b..000000000 --- a/pkg/hintrunner/error.go +++ /dev/null @@ -1,56 +0,0 @@ -package hintrunner - -import "fmt" - -// HintRunnerError represents error ocurring during the hint runner -type HintRunnerError struct { - err error -} - -func NewHintRunnerError(err error) *HintRunnerError { - return &HintRunnerError{err} -} - -func (e *HintRunnerError) Error() string { - return fmt.Sprintf("error in hint runner: %s", e.err.Error()) -} - -func (e *HintRunnerError) Unwrap() error { - return e.err -} - -// HintError stores information about the hint being executed as well as its cause -type HintError struct { - hintName string - err error -} - -func NewHintError(hintName string, err error) *HintError { - return &HintError{hintName, err} -} - -func (e *HintError) Error() string { - return fmt.Sprintf("error executing hint %s: %s", e.hintName, e.err.Error()) -} - -func (e *HintError) Unwrap() error { - return e.err -} - -// OperandError is returned when the error is detected during an operand get/resolve execution -type OperandError struct { - operandName string - err error -} - -func NewOperandError(operandName string, err error) *OperandError { - return &OperandError{operandName, err} -} - -func (e *OperandError) Error() string { - return fmt.Sprintf("failed to get/resolve operand %s: %s", e.operandName, e.err.Error()) -} - -func (e *OperandError) Unwrap() error { - return e.err -} diff --git a/pkg/hintrunner/hint.go b/pkg/hintrunner/hint.go index 14d06cd81..661d5b415 100644 --- a/pkg/hintrunner/hint.go +++ b/pkg/hintrunner/hint.go @@ -1,65 +1,73 @@ package hintrunner import ( + "fmt" + VM "github.com/NethermindEth/cairo-vm-go/pkg/vm" "github.com/NethermindEth/cairo-vm-go/pkg/vm/memory" f "github.com/consensys/gnark-crypto/ecc/stark-curve/fp" ) type Hinter interface { - Execute(vm *VM.VirtualMachine) *HintError -} + fmt.Stringer -const allocSegmentName = "AllocSegment" + Execute(vm *VM.VirtualMachine) error +} type AllocSegment struct { dst CellRefer } -func (hint AllocSegment) Execute(vm *VM.VirtualMachine) *HintError { +func (hint AllocSegment) String() string { + return "AllocSegment" +} + +func (hint AllocSegment) Execute(vm *VM.VirtualMachine) error { segmentIndex := vm.MemoryManager.Memory.AllocateEmptySegment() memAddress := memory.MemoryValueFromSegmentAndOffset(segmentIndex, 0) cell, err := hint.dst.Get(vm) if err != nil { - return NewHintError(allocSegmentName, err) + return fmt.Errorf("get destination cell: %v", err) } err = cell.Write(memAddress) if err != nil { - return NewHintError(allocSegmentName, err) + return fmt.Errorf("write cell: %v", err) } return nil } -const testLessThanName = "TestLessThan" - type TestLessThan struct { dst CellRefer lhs ResOperander rhs ResOperander } -func (hint TestLessThan) Execute(vm *VM.VirtualMachine) *HintError { +func (hint TestLessThan) String() string { + return "TestLessThan" +} + +func (hint TestLessThan) Execute(vm *VM.VirtualMachine) error { lhsVal, err := hint.lhs.Resolve(vm) if err != nil { - return NewHintError(testLessThanName, err) + return fmt.Errorf("resolve lhs: %v", err) } rhsVal, err := hint.rhs.Resolve(vm) if err != nil { - return NewHintError(testLessThanName, err) + return fmt.Errorf("resolve rhs: %v", err) } lhsFelt, err := lhsVal.ToFieldElement() if err != nil { - return NewHintError(testLessThanName, err) + return err } rhsFelt, err := rhsVal.ToFieldElement() if err != nil { - return NewHintError(testLessThanName, err) + return err } resFelt := f.Element{} @@ -69,12 +77,12 @@ func (hint TestLessThan) Execute(vm *VM.VirtualMachine) *HintError { dstCell, err := hint.dst.Get(vm) if err != nil { - return NewHintError(testLessThanName, err) + return fmt.Errorf("get destination cell: %v", err) } err = dstCell.Write(memory.MemoryValueFromFieldElement(&resFelt)) if err != nil { - return NewHintError(testLessThanName, err) + return fmt.Errorf("write cell: %v", err) } return nil diff --git a/pkg/hintrunner/hintrunner.go b/pkg/hintrunner/hintrunner.go index c91abd147..720a8c8a3 100644 --- a/pkg/hintrunner/hintrunner.go +++ b/pkg/hintrunner/hintrunner.go @@ -1,6 +1,8 @@ package hintrunner import ( + "fmt" + VM "github.com/NethermindEth/cairo-vm-go/pkg/vm" ) @@ -14,7 +16,7 @@ func CreateHintRunner(hints map[uint64]Hinter) HintRunner { return HintRunner{hints} } -func (hr HintRunner) RunHint(vm *VM.VirtualMachine) *HintRunnerError { +func (hr HintRunner) RunHint(vm *VM.VirtualMachine) error { hint := hr.hints[vm.Context.Pc] if hint == nil { return nil @@ -22,7 +24,7 @@ func (hr HintRunner) RunHint(vm *VM.VirtualMachine) *HintRunnerError { err := hint.Execute(vm) if err != nil { - return NewHintRunnerError(err) + return fmt.Errorf("execute hint %s: %v", hint, err) } return nil } diff --git a/pkg/hintrunner/operand.go b/pkg/hintrunner/operand.go index c4cf77efd..fea8988a7 100644 --- a/pkg/hintrunner/operand.go +++ b/pkg/hintrunner/operand.go @@ -10,44 +10,43 @@ import ( f "github.com/consensys/gnark-crypto/ecc/stark-curve/fp" ) -const ( - apCellRefName = "ApCellRef" - fpCellRefName = "FpCellRef" - derefName = "Deref" - doubleDerefName = "DoubleDeref" - immediateName = "Immediate" - binOpName = "BinaryOperator" -) - // // All CellRef definitions type CellRefer interface { + fmt.Stringer + Get(vm *VM.VirtualMachine) (*memory.Cell, error) } +func cellRefErr(c CellRefer, err error) error { + return fmt.Errorf("%s: %v", c, err) +} + type ApCellRef int16 +func (ap ApCellRef) String() string { + return "ApCellRef" +} + func (ap ApCellRef) Get(vm *VM.VirtualMachine) (*memory.Cell, error) { res, overflow := safemath.SafeOffset(vm.Context.Ap, int16(ap)) if overflow { - return nil, NewOperandError( - apCellRefName, - fmt.Errorf("%d + %d is outside of the [0, 2**64) range", vm.Context.Ap, ap), - ) + return nil, safemath.NewSafeOffsetError(vm.Context.Ap, int16(ap)) } return vm.MemoryManager.Memory.Peek(VM.ExecutionSegment, res) } type FpCellRef int16 +func (fp FpCellRef) String() string { + return "FpCellRef" +} + func (fp FpCellRef) Get(vm *VM.VirtualMachine) (*memory.Cell, error) { res, overflow := safemath.SafeOffset(vm.Context.Fp, int16(fp)) if overflow { - return nil, NewOperandError( - fpCellRefName, - fmt.Errorf("%d + %d is outside of the [0, 2**64) range", vm.Context.Ap, fp), - ) + return nil, safemath.NewSafeOffsetError(vm.Context.Ap, int16(fp)) } return vm.MemoryManager.Memory.Peek(VM.ExecutionSegment, res) } @@ -55,14 +54,24 @@ func (fp FpCellRef) Get(vm *VM.VirtualMachine) (*memory.Cell, error) { // // All ResOperand definitions +type ResOperander interface { + fmt.Stringer + + Resolve(vm *VM.VirtualMachine) (*memory.MemoryValue, error) +} + type Deref struct { deref CellRefer } +func (deref Deref) String() string { + return "Deref" +} + func (deref Deref) Resolve(vm *VM.VirtualMachine) (*memory.MemoryValue, error) { cell, err := deref.deref.Get(vm) if err != nil { - return nil, NewOperandError(derefName, err) + return nil, fmt.Errorf("get cell: %v", err) } return cell.Read(), nil } @@ -75,28 +84,25 @@ type DoubleDeref struct { func (dderef DoubleDeref) Resolve(vm *VM.VirtualMachine) (*memory.MemoryValue, error) { cell, err := dderef.deref.Get(vm) if err != nil { - return nil, NewOperandError(doubleDerefName, err) + return nil, fmt.Errorf("get cell: %v", err) } lhs := cell.Read() // Double deref implies the first value read must be an address address, err := lhs.ToMemoryAddress() if err != nil { - return nil, NewOperandError(doubleDerefName, err) + return nil, err } newOffset, overflow := safemath.SafeOffset(address.Offset, dderef.offset) if overflow { - return nil, NewOperandError( - doubleDerefName, - safemath.NewSafeOffsetError(address.Offset, dderef.offset), - ) + return nil, safemath.NewSafeOffsetError(address.Offset, dderef.offset) } resAddr := memory.NewMemoryAddress(address.SegmentIndex, newOffset) value, err := vm.MemoryManager.Memory.ReadFromAddress(resAddr) if err != nil { - return nil, NewOperandError(doubleDerefName, err) + return nil, fmt.Errorf("read cell: %v", err) } return value, nil @@ -104,6 +110,10 @@ func (dderef DoubleDeref) Resolve(vm *VM.VirtualMachine) (*memory.MemoryValue, e type Immediate big.Int +func (imm Immediate) String() string { + return "Immediate" +} + // todo(rodro): Specs from Starkware stablish this can be uint256 and not a felt. // Should we respect that, or go straight to felt? func (imm Immediate) Resolve(vm *VM.VirtualMachine) (*memory.MemoryValue, error) { @@ -123,26 +133,26 @@ const ( Mul ) -type ResOperander interface { - Resolve(vm *VM.VirtualMachine) (*memory.MemoryValue, error) -} - type BinaryOp struct { operator Operator lhs CellRefer rhs ResOperander // (except DoubleDeref and BinaryOp) } +func (bop BinaryOp) String() string { + return "BinaryOperator" +} + func (bop BinaryOp) Resolve(vm *VM.VirtualMachine) (*memory.MemoryValue, error) { cell, err := bop.lhs.Get(vm) if err != nil { - return nil, err + return nil, fmt.Errorf("get lhs op %d: %v", bop.lhs, err) } lhs := cell.Read() rhs, err := bop.rhs.Resolve(vm) if err != nil { - return nil, err + return nil, fmt.Errorf("resolve rhs op %s: %v", rhs, err) } switch bop.operator { @@ -150,10 +160,7 @@ func (bop BinaryOp) Resolve(vm *VM.VirtualMachine) (*memory.MemoryValue, error) return memory.EmptyMemoryValueAs(lhs.IsAddress()).Add(lhs, rhs) case Mul: return memory.EmptyMemoryValueAsFelt().Mul(lhs, rhs) + default: + return nil, fmt.Errorf("unknown binary operator: %d", bop.operator) } - - return nil, NewOperandError( - "BinaryOp", - fmt.Errorf("unknown binary operator id: %d", bop.operator), - ) }