Update `goal app method` handling of args and return values (#3352)

* Fix method call arg overflow handling

* Only check last log for return value

* Address feedback

* Add comment explaining ABI return prefix

4 files changed, 176 insertions, 28 deletions

diff --git a/cmd/goal/application.go b/cmd/goal/application.go
index c3439a245..8ecb5289a 100644
--- a/cmd/goal/application.go
+++ b/cmd/goal/application.go
@@ -22,6 +22,7 @@ import (
 	"encoding/base32"
 	"encoding/base64"
 	"encoding/binary"
+	"encoding/hex"
 	"fmt"
 	"os"
 	"strconv"
@@ -1162,8 +1163,8 @@ func populateMethodCallReferenceArgs(sender string, currentApp uint64, types []s
 
 var methodAppCmd = &cobra.Command{
 	Use:   "method",
-	Short: "Invoke a method",
-	Long:  `Invoke a method in an App (stateful contract) with an application call transaction`,
+	Short: "Invoke an ABI method",
+	Long:  `Invoke an ARC-4 ABI method on an App (stateful contract) with an application call transaction`,
 	Args:  validateNoPosArgsFn,
 	Run: func(cmd *cobra.Command, args []string) {
 		dataDir, client := getDataDirAndClient()
@@ -1369,36 +1370,29 @@ var methodAppCmd = &cobra.Command{
 				return
 			}
 
-			// specify the return hash prefix
-			hashRet := sha512.Sum512_256([]byte("return"))
-			hashRetPrefix := hashRet[:4]
+			// the 4-byte prefix for logged return values, from https://github.com/algorandfoundation/ARCs/blob/main/ARCs/arc-0004.md#standard-format
+			var abiReturnHash = []byte{0x15, 0x1f, 0x7c, 0x75}
 
-			var abiEncodedRet []byte
-			foundRet := false
-			if resp.Logs != nil {
-				for i := len(*resp.Logs) - 1; i >= 0; i-- {
-					retLog := (*resp.Logs)[i]
-					if bytes.HasPrefix(retLog, hashRetPrefix) {
-						abiEncodedRet = retLog[4:]
-						foundRet = true
-						break
-					}
-				}
+			if resp.Logs == nil || len(*resp.Logs) == 0 {
+				reportErrorf("method %s succeed but did not log a return value", method)
 			}
 
-			if !foundRet {
-				reportErrorf("cannot find return log for abi type %s", retTypeStr)
+			lastLog := (*resp.Logs)[len(*resp.Logs)-1]
+			if !bytes.HasPrefix(lastLog, abiReturnHash) {
+				reportErrorf("method %s succeed but did not log a return value", method)
 			}
 
-			decoded, err := retType.Decode(abiEncodedRet)
+			rawReturnValue := lastLog[len(abiReturnHash):]
+			decoded, err := retType.Decode(rawReturnValue)
 			if err != nil {
-				reportErrorf("cannot decode return value %v: %v", abiEncodedRet, err)
+				reportErrorf("method %s succeed but its return value could not be decoded.\nThe raw return value in hex is:%s\nThe error is: %s", method, hex.EncodeToString(rawReturnValue), err)
 			}
 
 			decodedJSON, err := retType.MarshalToJSON(decoded)
 			if err != nil {
-				reportErrorf("cannot marshal returned bytes %v to JSON: %v", decoded, err)
+				reportErrorf("method %s succeed but its return value could not be converted to JSON.\nThe raw return value in hex is:%s\nThe error is: %s", method, hex.EncodeToString(rawReturnValue), err)
 			}
+
 			fmt.Printf("method %s succeeded with output: %s\n", method, string(decodedJSON))
 		}
 	},
diff --git a/data/abi/abi_encode.go b/data/abi/abi_encode.go
index c2b7a2480..2a0ba100e 100644
--- a/data/abi/abi_encode.go
+++ b/data/abi/abi_encode.go
@@ -18,6 +18,7 @@ package abi
 
 import (
 	"encoding/binary"
+	"encoding/json"
 	"fmt"
 	"math/big"
 	"reflect"
@@ -478,6 +479,16 @@ func decodeTuple(encoded []byte, childT []Type) ([]interface{}, error) {
 	return values, nil
 }
 
+// maxAppArgs is the maximum number of arguments for an application call transaction, in compliance
+// with ARC-4. Currently this is the same as the MaxAppArgs consensus parameter, but the
+// difference is that the consensus parameter is liable to change in a future consensus upgrade.
+// However, the ARC-4 ABI argument encoding **MUST** always remain the same.
+const maxAppArgs = 16
+
+// The tuple threshold is maxAppArgs, minus 1 for the method selector in the first app arg,
+// minus 1 for the final app argument becoming a tuple of the remaining method args
+const methodArgsTupleThreshold = maxAppArgs - 2
+
 // ParseArgJSONtoByteSlice convert input method arguments to ABI encoded bytes
 // it converts funcArgTypes into a tuple type and apply changes over input argument string (in JSON format)
 // if there are greater or equal to 15 inputs, then we compact the tailing inputs into one tuple
@@ -495,16 +506,32 @@ func ParseArgJSONtoByteSlice(argTypes []string, jsonArgs []string, applicationAr
 		return fmt.Errorf("input argument number %d != method argument number %d", len(jsonArgs), len(abiTypes))
 	}
 
-	// change the input args to be 1 - 14 + 15 (compacting everything together)
-	if len(jsonArgs) > 14 {
-		compactedType, err := MakeTupleType(abiTypes[14:])
+	// Up to 16 app arguments can be passed to app call. First is reserved for method selector,
+	// and the rest are for method call arguments. But if more than 15 method call arguments
+	// are present, then the method arguments after the 14th are placed in a tuple in the last
+	// app argument slot
+	if len(abiTypes) > maxAppArgs-1 {
+		typesForTuple := make([]Type, len(abiTypes)-methodArgsTupleThreshold)
+		copy(typesForTuple, abiTypes[methodArgsTupleThreshold:])
+
+		compactedType, err := MakeTupleType(typesForTuple)
+		if err != nil {
+			return err
+		}
+
+		abiTypes = append(abiTypes[:methodArgsTupleThreshold], compactedType)
+
+		tupleValues := make([]json.RawMessage, len(jsonArgs)-methodArgsTupleThreshold)
+		for i, jsonArg := range jsonArgs[methodArgsTupleThreshold:] {
+			tupleValues[i] = []byte(jsonArg)
+		}
+
+		remainingJSON, err := json.Marshal(tupleValues)
 		if err != nil {
 			return err
 		}
-		abiTypes = append(abiTypes[:14], compactedType)
 
-		remainingJSON := "[" + strings.Join(jsonArgs[14:], ",") + "]"
-		jsonArgs = append(jsonArgs[:14], remainingJSON)
+		jsonArgs = append(jsonArgs[:methodArgsTupleThreshold], string(remainingJSON))
 	}
 
 	// parse JSON value to ABI encoded bytes
@@ -523,7 +550,7 @@ func ParseArgJSONtoByteSlice(argTypes []string, jsonArgs []string, applicationAr
 }
 
 // ParseMethodSignature parses a method of format `method(argType1,argType2,...)retType`
-// into `method` {`argType1`,`argType2`,..} and `retType`
+// into `method` {`argType1`,`argType2`,...} and `retType`
 func ParseMethodSignature(methodSig string) (name string, argTypes []string, returnType string, err error) {
 	argsStart := strings.Index(methodSig, "(")
 	if argsStart == -1 {
diff --git a/data/abi/abi_encode_test.go b/data/abi/abi_encode_test.go
index b3da406d2..3b6d0e4bf 100644
--- a/data/abi/abi_encode_test.go
+++ b/data/abi/abi_encode_test.go
@@ -19,6 +19,7 @@ package abi
 import (
 	"crypto/rand"
 	"encoding/binary"
+	"fmt"
 	"math/big"
 	"testing"
 
@@ -1056,6 +1057,124 @@ func TestRandomABIEncodeDecodeRoundTrip(t *testing.T) {
 	categorySelfRoundTripTest(t, testValuePool[Tuple])
 }
 
+func TestParseArgJSONtoByteSlice(t *testing.T) {
+	partitiontest.PartitionTest(t)
+
+	makeRepeatSlice := func(size int, value string) []string {
+		slice := make([]string, size)
+		for i := range slice {
+			slice[i] = value
+		}
+		return slice
+	}
+
+	tests := []struct {
+		argTypes        []string
+		jsonArgs        []string
+		expectedAppArgs [][]byte
+	}{
+		{
+			argTypes:        []string{},
+			jsonArgs:        []string{},
+			expectedAppArgs: [][]byte{},
+		},
+		{
+			argTypes:        []string{"uint8"},
+			jsonArgs:        []string{"100"},
+			expectedAppArgs: [][]byte{{100}},
+		},
+		{
+			argTypes:        []string{"uint8", "uint16"},
+			jsonArgs:        []string{"100", "65535"},
+			expectedAppArgs: [][]byte{{100}, {255, 255}},
+		},
+		{
+			argTypes: makeRepeatSlice(15, "string"),
+			jsonArgs: []string{
+				`"a"`,
+				`"b"`,
+				`"c"`,
+				`"d"`,
+				`"e"`,
+				`"f"`,
+				`"g"`,
+				`"h"`,
+				`"i"`,
+				`"j"`,
+				`"k"`,
+				`"l"`,
+				`"m"`,
+				`"n"`,
+				`"o"`,
+			},
+			expectedAppArgs: [][]byte{
+				{00, 01, 97},
+				{00, 01, 98},
+				{00, 01, 99},
+				{00, 01, 100},
+				{00, 01, 101},
+				{00, 01, 102},
+				{00, 01, 103},
+				{00, 01, 104},
+				{00, 01, 105},
+				{00, 01, 106},
+				{00, 01, 107},
+				{00, 01, 108},
+				{00, 01, 109},
+				{00, 01, 110},
+				{00, 01, 111},
+			},
+		},
+		{
+			argTypes: makeRepeatSlice(16, "string"),
+			jsonArgs: []string{
+				`"a"`,
+				`"b"`,
+				`"c"`,
+				`"d"`,
+				`"e"`,
+				`"f"`,
+				`"g"`,
+				`"h"`,
+				`"i"`,
+				`"j"`,
+				`"k"`,
+				`"l"`,
+				`"m"`,
+				`"n"`,
+				`"o"`,
+				`"p"`,
+			},
+			expectedAppArgs: [][]byte{
+				{00, 01, 97},
+				{00, 01, 98},
+				{00, 01, 99},
+				{00, 01, 100},
+				{00, 01, 101},
+				{00, 01, 102},
+				{00, 01, 103},
+				{00, 01, 104},
+				{00, 01, 105},
+				{00, 01, 106},
+				{00, 01, 107},
+				{00, 01, 108},
+				{00, 01, 109},
+				{00, 01, 110},
+				{00, 04, 00, 07, 00, 01, 111, 00, 01, 112},
+			},
+		},
+	}
+
+	for i, test := range tests {
+		t.Run(fmt.Sprintf("index=%d", i), func(t *testing.T) {
+			applicationArgs := [][]byte{}
+			err := ParseArgJSONtoByteSlice(test.argTypes, test.jsonArgs, &applicationArgs)
+			require.NoError(t, err)
+			require.Equal(t, test.expectedAppArgs, applicationArgs)
+		})
+	}
+}
+
 func TestParseMethodSignature(t *testing.T) {
 	partitiontest.PartitionTest(t)
 
diff --git a/test/scripts/e2e_subs/e2e-app-abi-method.sh b/test/scripts/e2e_subs/e2e-app-abi-method.sh
index 72bf746dd..210b54318 100755
--- a/test/scripts/e2e_subs/e2e-app-abi-method.sh
+++ b/test/scripts/e2e_subs/e2e-app-abi-method.sh
@@ -28,6 +28,14 @@ if [[ $RES != *"${EXPECTED}"* ]]; then
     false
 fi
 
+# No arguments or return value
+RES=$(${gcmd} app method --method "empty()void" --app-id $APPID --from $ACCOUNT 2>&1 || true)
+EXPECTED="method empty()void succeeded"
+if [[ $RES != *"${EXPECTED}" ]]; then
+    date '+app-abi-method-test FAIL the method call to empty()void should not fail %Y%m%d_%H%M%S'
+    false
+fi
+
 # 1 + 2 = 3
 RES=$(${gcmd} app method --method "add(uint64,uint64)uint64" --arg 1 --arg 2 --app-id $APPID --from $ACCOUNT 2>&1 || true)
 EXPECTED="method add(uint64,uint64)uint64 succeeded with output: 3"