From acbc483b59ae19c520015ec37a2106c902082a23 Mon Sep 17 00:00:00 2001 From: John Jannotti Date: Fri, 14 May 2021 12:10:44 -0400 Subject: [PATCH] code review --- data/transactions/logic/assembler.go | 59 ++++++++++--------- data/transactions/logic/assembler_test.go | 30 +++++----- .../transactions/logic/backwardCompat_test.go | 8 +-- data/transactions/logic/eval.go | 14 ++--- data/transactions/logic/eval_test.go | 52 ++++++++++++++++ test/scripts/e2e_subs/v26/teal-v3-only.sh | 2 - 6 files changed, 108 insertions(+), 57 deletions(-) diff --git a/data/transactions/logic/assembler.go b/data/transactions/logic/assembler.go index 6d5ae9ac89..5c4d864d01 100644 --- a/data/transactions/logic/assembler.go +++ b/data/transactions/logic/assembler.go @@ -99,7 +99,7 @@ func (ops *OpStream) createLabel(label string) { ops.labels = make(map[string]int) } if _, ok := ops.labels[label]; ok { - ops.errorf("duplicate label %s", label) + ops.errorf("duplicate label %#v", label) } ops.labels[label] = ops.pending.Len() } @@ -590,14 +590,14 @@ func assembleTxn(ops *OpStream, spec *OpSpec, args []string) error { } fs, ok := txnFieldSpecByName[args[0]] if !ok { - return ops.errorf("txn unknown field: %v", args[0]) + return ops.errorf("txn unknown field: %#v", args[0]) } _, ok = txnaFieldSpecByField[fs.field] if ok { - return ops.errorf("found array field %v in txn op", args[0]) + return ops.errorf("found array field %#v in txn op", args[0]) } if fs.version > ops.Version { - return ops.errorf("field %s available in version %d. Missed #pragma version?", args[0], fs.version) + return ops.errorf("field %#v available in version %d. Missed #pragma version?", args[0], fs.version) } ops.pending.WriteByte(spec.Opcode) ops.pending.WriteByte(uint8(fs.field)) @@ -623,14 +623,14 @@ func assembleTxna(ops *OpStream, spec *OpSpec, args []string) error { } fs, ok := txnFieldSpecByName[args[0]] if !ok { - return ops.errorf("txna unknown field: %v", args[0]) + return ops.errorf("txna unknown field: %#v", args[0]) } _, ok = txnaFieldSpecByField[fs.field] if !ok { - return ops.errorf("txna unknown field: %v", args[0]) + return ops.errorf("txna unknown field: %#v", args[0]) } if fs.version > ops.Version { - return ops.errorf("txna %s available in version %d. Missed #pragma version?", args[0], fs.version) + return ops.errorf("txna %#v available in version %d. Missed #pragma version?", args[0], fs.version) } arrayFieldIdx, err := strconv.ParseUint(args[1], 0, 64) if err != nil { @@ -661,14 +661,14 @@ func assembleGtxn(ops *OpStream, spec *OpSpec, args []string) error { fs, ok := txnFieldSpecByName[args[1]] if !ok { - return ops.errorf("gtxn unknown field: %v", args[1]) + return ops.errorf("gtxn unknown field: %#v", args[1]) } _, ok = txnaFieldSpecByField[fs.field] if ok { - return ops.errorf("found array field %v in gtxn op", args[1]) + return ops.errorf("found array field %#v in gtxn op", args[1]) } if fs.version > ops.Version { - return ops.errorf("field %s available in version %d. Missed #pragma version?", args[1], fs.version) + return ops.errorf("field %#v available in version %d. Missed #pragma version?", args[1], fs.version) } ops.pending.WriteByte(spec.Opcode) @@ -703,14 +703,14 @@ func assembleGtxna(ops *OpStream, spec *OpSpec, args []string) error { fs, ok := txnFieldSpecByName[args[1]] if !ok { - return ops.errorf("gtxna unknown field: %v", args[1]) + return ops.errorf("gtxna unknown field: %#v", args[1]) } _, ok = txnaFieldSpecByField[fs.field] if !ok { - return ops.errorf("gtxna unknown field: %v", args[1]) + return ops.errorf("gtxna unknown field: %#v", args[1]) } if fs.version > ops.Version { - return ops.errorf("gtxna %s available in version %d. Missed #pragma version?", args[1], fs.version) + return ops.errorf("gtxna %#v available in version %d. Missed #pragma version?", args[1], fs.version) } arrayFieldIdx, err := strconv.ParseUint(args[2], 0, 64) if err != nil { @@ -738,14 +738,14 @@ func assembleGtxns(ops *OpStream, spec *OpSpec, args []string) error { } fs, ok := txnFieldSpecByName[args[0]] if !ok { - return ops.errorf("gtxns unknown field: %v", args[0]) + return ops.errorf("gtxns unknown field: %#v", args[0]) } _, ok = txnaFieldSpecByField[fs.field] if ok { - return ops.errorf("found array field %v in gtxns op", args[0]) + return ops.errorf("found array field %#v in gtxns op", args[0]) } if fs.version > ops.Version { - return ops.errorf("field %s available in version %d. Missed #pragma version?", args[0], fs.version) + return ops.errorf("field %#v available in version %d. Missed #pragma version?", args[0], fs.version) } ops.pending.WriteByte(spec.Opcode) @@ -760,14 +760,14 @@ func assembleGtxnsa(ops *OpStream, spec *OpSpec, args []string) error { } fs, ok := txnFieldSpecByName[args[0]] if !ok { - return ops.errorf("gtxnsa unknown field: %v", args[0]) + return ops.errorf("gtxnsa unknown field: %#v", args[0]) } _, ok = txnaFieldSpecByField[fs.field] if !ok { - return ops.errorf("gtxnsa unknown field: %v", args[0]) + return ops.errorf("gtxnsa unknown field: %#v", args[0]) } if fs.version > ops.Version { - return ops.errorf("gtxnsa %s available in version %d. Missed #pragma version?", args[0], fs.version) + return ops.errorf("gtxnsa %#v available in version %d. Missed #pragma version?", args[0], fs.version) } arrayFieldIdx, err := strconv.ParseUint(args[1], 0, 64) if err != nil { @@ -789,7 +789,7 @@ func assembleGlobal(ops *OpStream, spec *OpSpec, args []string) error { } fs, ok := globalFieldSpecByName[args[0]] if !ok { - return ops.errorf("global unknown field: %v", args[0]) + return ops.errorf("global unknown field: %#v", args[0]) } if fs.version > ops.Version { // no return here. we may as well continue to maintain typestack @@ -810,7 +810,7 @@ func assembleAssetHolding(ops *OpStream, spec *OpSpec, args []string) error { } val, ok := assetHoldingFields[args[0]] if !ok { - return ops.errorf("asset_holding_get unknown arg: %v", args[0]) + return ops.errorf("asset_holding_get unknown arg: %#v", args[0]) } ops.pending.WriteByte(spec.Opcode) ops.pending.WriteByte(uint8(val)) @@ -824,7 +824,7 @@ func assembleAssetParams(ops *OpStream, spec *OpSpec, args []string) error { } val, ok := assetParamsFields[args[0]] if !ok { - return ops.errorf("asset_params_get unknown arg: %v", args[0]) + return ops.errorf("asset_params_get unknown arg: %#v", args[0]) } ops.pending.WriteByte(spec.Opcode) ops.pending.WriteByte(uint8(val)) @@ -1077,7 +1077,7 @@ func (ops *OpStream) assemble(fin io.Reader) error { if ops.Version <= 1 { for label, dest := range ops.labels { if dest == ops.pending.Len() { - ops.errorf("label %v is too far away", label) + ops.errorf("label %#v is too far away", label) } } } @@ -1148,7 +1148,7 @@ func (ops *OpStream) resolveLabels() { dest, ok := ops.labels[lr.label] if !ok { if !reported[lr.label] { - ops.errorf("reference to undefined label %v", lr.label) + ops.errorf("reference to undefined label %#v", lr.label) } reported[lr.label] = true continue @@ -1156,12 +1156,12 @@ func (ops *OpStream) resolveLabels() { // all branch instructions (currently) are opcode byte and 2 offset bytes, and the destination is relative to the next pc as if the branch was a no-op naturalPc := lr.position + 3 if ops.Version < backBranchEnabledVersion && dest < naturalPc { - ops.errorf("label %v is a back reference, back jump support was introduced in TEAL v4", lr.label) + ops.errorf("label %#v is a back reference, back jump support was introduced in TEAL v4", lr.label) continue } jump := dest - naturalPc if jump > 0x7fff { - ops.errorf("label %v is too far away", lr.label) + ops.errorf("label %#v is too far away", lr.label) continue } raw[lr.position+1] = uint8(jump >> 8) @@ -1563,7 +1563,7 @@ func guessByteFormat(bytes []byte) string { return fmt.Sprintf("addr %s", short.String()) } if allPrintableASCII(bytes) { - return fmt.Sprintf("\"%s\"", string(bytes)) + return fmt.Sprintf("%#v", string(bytes)) } return "0x" + hex.EncodeToString(bytes) } @@ -1720,9 +1720,12 @@ func disBranch(dis *disassembleState, spec *OpSpec) (string, error) { dis.nextpc = dis.pc + 3 offset := (uint(dis.program[dis.pc+1]) << 8) | uint(dis.program[dis.pc+2]) target := int(offset) + dis.pc + 3 + if target > 0xffff { + target -= 0x10000 + } var label string if dis.numericTargets { - label = fmt.Sprintf("+%d", offset+3) // +3 so it's easy to calculate destination from current + label = fmt.Sprintf("%d", target) } else { if known, ok := dis.pendingLabels[target]; ok { label = known diff --git a/data/transactions/logic/assembler_test.go b/data/transactions/logic/assembler_test.go index 9680031b1b..1fca9c8494 100644 --- a/data/transactions/logic/assembler_test.go +++ b/data/transactions/logic/assembler_test.go @@ -414,11 +414,11 @@ func testLine(t *testing.T, line string, ver uint64, expected string) { func TestAssembleTxna(t *testing.T) { testLine(t, "txna Accounts 256", AssemblerMaxVersion, "txna array index beyond 255: 256") testLine(t, "txna ApplicationArgs 256", AssemblerMaxVersion, "txna array index beyond 255: 256") - testLine(t, "txna Sender 256", AssemblerMaxVersion, "txna unknown field: Sender") + testLine(t, "txna Sender 256", AssemblerMaxVersion, "txna unknown field: \"Sender\"") testLine(t, "gtxna 0 Accounts 256", AssemblerMaxVersion, "gtxna array index beyond 255: 256") testLine(t, "gtxna 0 ApplicationArgs 256", AssemblerMaxVersion, "gtxna array index beyond 255: 256") testLine(t, "gtxna 256 Accounts 0", AssemblerMaxVersion, "gtxna group index beyond 255: 256") - testLine(t, "gtxna 0 Sender 256", AssemblerMaxVersion, "gtxna unknown field: Sender") + testLine(t, "gtxna 0 Sender 256", AssemblerMaxVersion, "gtxna unknown field: \"Sender\"") testLine(t, "txn Accounts 0", 1, "txn expects one argument") testLine(t, "txn Accounts 0 1", 2, "txn expects one or two arguments") testLine(t, "txna Accounts 0 1", AssemblerMaxVersion, "txna expects two arguments") @@ -428,20 +428,20 @@ func TestAssembleTxna(t *testing.T) { testLine(t, "gtxna 0 Accounts 1 2", AssemblerMaxVersion, "gtxna expects three arguments") testLine(t, "gtxna a Accounts 0", AssemblerMaxVersion, "strconv.ParseUint...") testLine(t, "gtxna 0 Accounts a", AssemblerMaxVersion, "strconv.ParseUint...") - testLine(t, "txn ABC", 2, "txn unknown field: ABC") - testLine(t, "gtxn 0 ABC", 2, "gtxn unknown field: ABC") + testLine(t, "txn ABC", 2, "txn unknown field: \"ABC\"") + testLine(t, "gtxn 0 ABC", 2, "gtxn unknown field: \"ABC\"") testLine(t, "gtxn a ABC", 2, "strconv.ParseUint...") - testLine(t, "txn Accounts", AssemblerMaxVersion, "found array field Accounts in txn op") - testLine(t, "txn Accounts", 1, "found array field Accounts in txn op") + testLine(t, "txn Accounts", AssemblerMaxVersion, "found array field \"Accounts\" in txn op") + testLine(t, "txn Accounts", 1, "found array field \"Accounts\" in txn op") testLine(t, "txn Accounts 0", AssemblerMaxVersion, "") - testLine(t, "gtxn 0 Accounts", AssemblerMaxVersion, "found array field Accounts in gtxn op") - testLine(t, "gtxn 0 Accounts", 1, "found array field Accounts in gtxn op") + testLine(t, "gtxn 0 Accounts", AssemblerMaxVersion, "found array field \"Accounts\" in gtxn op") + testLine(t, "gtxn 0 Accounts", 1, "found array field \"Accounts\" in gtxn op") testLine(t, "gtxn 0 Accounts 1", AssemblerMaxVersion, "") } func TestAssembleGlobal(t *testing.T) { testLine(t, "global", AssemblerMaxVersion, "global expects one argument") - testLine(t, "global a", AssemblerMaxVersion, "global unknown field: a") + testLine(t, "global a", AssemblerMaxVersion, "global unknown field: \"a\"") } func TestAssembleDefault(t *testing.T) { @@ -779,7 +779,7 @@ bnz wat int 2` for v := uint64(1); v < backBranchEnabledVersion; v++ { t.Run(fmt.Sprintf("v=%d", v), func(t *testing.T) { - testProg(t, source, v, expect{3, "label wat is a back reference..."}) + testProg(t, source, v, expect{3, "label \"wat\" is a back reference..."}) }) } for v := uint64(backBranchEnabledVersion); v <= AssemblerMaxVersion; v++ { @@ -820,7 +820,7 @@ bnz nowhere int 2` for v := uint64(1); v <= AssemblerMaxVersion; v++ { t.Run(fmt.Sprintf("v=%d", v), func(t *testing.T) { - testProg(t, source, v, expect{2, "reference to undefined label nowhere"}) + testProg(t, source, v, expect{2, "reference to undefined label \"nowhere\""}) }) } } @@ -850,8 +850,8 @@ int 2` for v := uint64(1); v <= AssemblerMaxVersion; v++ { t.Run(fmt.Sprintf("v=%d", v), func(t *testing.T) { testProg(t, source, v, - expect{2, "reference to undefined label nowhere"}, - expect{4, "txn unknown field: XYZ"}) + expect{2, "reference to undefined label \"nowhere\""}, + expect{4, "txn unknown field: \"XYZ\""}) }) } } @@ -1171,13 +1171,13 @@ func TestAssembleAsset(t *testing.T) { testProg(t, "int 1; int 1; asset_holding_get ABC 1", v, expect{3, "asset_holding_get expects one argument"}) testProg(t, "int 1; int 1; asset_holding_get ABC", v, - expect{3, "asset_holding_get unknown arg: ABC"}) + expect{3, "asset_holding_get unknown arg: \"ABC\""}) testProg(t, "byte 0x1234; asset_params_get ABC 1", v, expect{2, "asset_params_get arg 0 wanted type uint64..."}) testLine(t, "asset_params_get ABC 1", v, "asset_params_get expects one argument") - testLine(t, "asset_params_get ABC", v, "asset_params_get unknown arg: ABC") + testLine(t, "asset_params_get ABC", v, "asset_params_get unknown arg: \"ABC\"") } } diff --git a/data/transactions/logic/backwardCompat_test.go b/data/transactions/logic/backwardCompat_test.go index d5d51fe2e1..5c6ada5633 100644 --- a/data/transactions/logic/backwardCompat_test.go +++ b/data/transactions/logic/backwardCompat_test.go @@ -458,7 +458,7 @@ func TestBackwardCompatTxnFields(t *testing.T) { if _, ok := txnaFieldSpecByField[fs.field]; ok { parts := strings.Split(text, " ") op := parts[0] - asmError = fmt.Sprintf("found array field %s in %s op", field, op) + asmError = fmt.Sprintf("found array field %#v in %s op", field, op) } // check assembler fails if version before introduction testLine(t, text, assemblerNoVersion, asmError) @@ -521,15 +521,15 @@ bnz done done:` t.Run("v=default", func(t *testing.T) { - testProg(t, source, assemblerNoVersion, expect{4, "label done is too far away"}) + testProg(t, source, assemblerNoVersion, expect{4, "label \"done\" is too far away"}) }) t.Run("v=default", func(t *testing.T) { - testProg(t, source, 0, expect{4, "label done is too far away"}) + testProg(t, source, 0, expect{4, "label \"done\" is too far away"}) }) t.Run("v=default", func(t *testing.T) { - testProg(t, source, 1, expect{4, "label done is too far away"}) + testProg(t, source, 1, expect{4, "label \"done\" is too far away"}) }) for v := uint64(2); v <= AssemblerMaxVersion; v++ { diff --git a/data/transactions/logic/eval.go b/data/transactions/logic/eval.go index 32ee496a68..91557d3bc0 100644 --- a/data/transactions/logic/eval.go +++ b/data/transactions/logic/eval.go @@ -432,20 +432,18 @@ func eval(program []byte, cx *evalContext) (pass bool, err error) { } // CheckStateful should be faster than EvalStateful. It can perform -// static checks and reject programs that are invalid. Returns a cost -// estimate of relative execution time. This cost is not relevent when -// the program is v4 or higher, and so 1 is returned so that callers -// may continue to check for a high cost being invalid. +// static checks and reject programs that are invalid. Prior to v4, +// these static checks include a cost estimate that must be low enough +// (controlled by params.Proto). func CheckStateful(program []byte, params EvalParams) error { params.runModeFlags = runModeApplication return check(program, params) } // Check should be faster than Eval. It can perform static checks and -// reject programs that are invalid. Returns a cost estimate of -// relative execution time. This cost is not relevent when -// the program is v4 or higher, and so 1 is returned so that callers -// may continue to check for a high cost being invalid. +// reject programs that are invalid. Prior to v4, these static checks +// include a cost estimate that must be low enough (controlled by +// params.Proto). func Check(program []byte, params EvalParams) error { params.runModeFlags = runModeSignature return check(program, params) diff --git a/data/transactions/logic/eval_test.go b/data/transactions/logic/eval_test.go index 05207c2eff..74dcc251fc 100644 --- a/data/transactions/logic/eval_test.go +++ b/data/transactions/logic/eval_test.go @@ -4089,6 +4089,58 @@ main: int 120 == `, 4) + + // Mutually recursive odd/even. Each is intentionally done in a slightly different way. + testAccepts(t, ` +b main +odd: // If 0, return false, else return !even + dup + bz retfalse + callsub even + ! + retsub + +retfalse: + pop + int 0 + retsub + + +even: // If 0, return true, else decrement and return even + dup + bz rettrue + int 1 + - + callsub odd + retsub + +rettrue: + pop + int 1 + retsub + + +main: + int 1 + callsub odd + assert + + int 0 + callsub even + assert + + int 10 + callsub even + assert + + int 10 + callsub odd + ! + assert + + int 1 +`, 4) + testPanics(t, "int 1; retsub", 4) testPanics(t, "int 1; recur: callsub recur; int 1", 4) diff --git a/test/scripts/e2e_subs/v26/teal-v3-only.sh b/test/scripts/e2e_subs/v26/teal-v3-only.sh index ef39350bb4..ee3fe7d865 100755 --- a/test/scripts/e2e_subs/v26/teal-v3-only.sh +++ b/test/scripts/e2e_subs/v26/teal-v3-only.sh @@ -20,7 +20,6 @@ ACCOUNTB=$(${gcmd} account new|awk '{ print $6 }') cat >${TEMPDIR}/true.teal<${TEMPDIR}/true4.teal<