Merge pull request #601 from gz-c/no-tx-panic

Return error from TransactionDeserialize, dont panic

src/api/cli/broadcast_rawtx.go

@@ -21,7 +21,7 @@ func broadcastTxCmd() gcli.Command {
 			}
 
 			rpcClient := RpcClientFromContext(c)
-			txid, err := rpcClient.InjectTransaction(rawtx)
+			txid, err := rpcClient.InjectTransactionString(rawtx)
 			if err != nil {
 				return err
 			}

src/api/cli/create_rawtx.go

@@ -82,21 +82,23 @@ func createRawTxCmd(cfg Config) gcli.Command {
 		},
 		OnUsageError: onCommandUsageError(name),
 		Action: func(c *gcli.Context) error {
-			rawtx, err := createRawTx(c)
+			tx, err := createRawTx(c)
 			if err != nil {
 				errorWithHelp(c, err)
 				return nil
 			}
 
+			rawTx := hex.EncodeToString(tx.Serialize())
+
 			if c.Bool("json") {
 				return printJson(struct {
 					RawTx string `json:"rawtx"`
 				}{
-					RawTx: rawtx,
+					RawTx: rawTx,
 				})
 			}
 
-			fmt.Println(rawtx)
+			fmt.Println(rawTx)
 			return nil
 		},
 	}
@@ -217,37 +219,29 @@ func getAmount(c *gcli.Context) (uint64, error) {
 	return amt, nil
 }
 
-func createRawTx(c *gcli.Context) (string, error) {
+func createRawTx(c *gcli.Context) (*coin.Transaction, error) {
 	rpcClient := RpcClientFromContext(c)
 
 	wltAddr, err := fromWalletOrAddress(c)
 	if err != nil {
-		return "", err
+		return nil, err
 	}
 
 	chgAddr, err := getChangeAddress(wltAddr, c.String("c"))
 	if err != nil {
-		return "", err
+		return nil, err
 	}
 
 	toAddrs, err := getToAddresses(c)
 	if err != nil {
-		return "", err
+		return nil, err
 	}
 
-	var tx *coin.Transaction
 	if wltAddr.Address == "" {
-		tx, err = CreateRawTxFromWallet(rpcClient, wltAddr.Wallet, chgAddr, toAddrs)
-	} else {
-		tx, err = CreateRawTxFromAddress(rpcClient, wltAddr.Address, wltAddr.Wallet, chgAddr, toAddrs)
-	}
-
-	if err != nil {
-		return "", err
+		return CreateRawTxFromWallet(rpcClient, wltAddr.Wallet, chgAddr, toAddrs)
 	}
 
-	d := tx.Serialize()
-	return hex.EncodeToString(d), nil
+	return CreateRawTxFromAddress(rpcClient, wltAddr.Address, wltAddr.Wallet, chgAddr, toAddrs)
 }
 
 // PUBLIC

src/api/cli/send.go

@@ -1,7 +1,6 @@
 package cli
 
 import (
-	"encoding/hex"
 	"fmt"
 
 	"github.com/skycoin/skycoin/src/api/webrpc"
@@ -91,8 +90,7 @@ func SendFromWallet(c *webrpc.Client, walletFile, chgAddr string, toAddrs []Send
 		return "", err
 	}
 
-	tx := hex.EncodeToString(rawTx.Serialize())
-	return c.InjectTransaction(tx)
+	return c.InjectTransaction(rawTx)
 }
 
 // SendFromAddress sends from a specific address in a wallet. Returns txid.
@@ -102,6 +100,5 @@ func SendFromAddress(c *webrpc.Client, addr, walletFile, chgAddr string, toAddrs
 		return "", err
 	}
 
-	tx := hex.EncodeToString(rawTx.Serialize())
-	return c.InjectTransaction(tx)
+	return c.InjectTransaction(rawTx)
 }

src/api/cli/transaction.go

@@ -59,15 +59,20 @@ func decodeRawTxCmd() gcli.Command {
 
 			b, err := hex.DecodeString(rawTxStr)
 			if err != nil {
-				fmt.Printf("invalid raw transaction:%v\n", err)
-				return nil
+				fmt.Printf("invalid raw transaction: %v\n", err)
+				return err
+			}
+
+			tx, err := coin.TransactionDeserialize(b)
+			if err != nil {
+				fmt.Printf("Unable to deserialize transaction bytes: %v\n", err)
+				return err
 			}
 
-			tx := coin.TransactionDeserialize(b)
 			txStr, err := visor.TransactionToJSON(tx)
 			if err != nil {
 				fmt.Println(err)
-				return nil
+				return err
 			}
 
 			fmt.Println(txStr)

src/api/webrpc/client.go

@@ -2,25 +2,30 @@ package webrpc
 
 import (
 	"bytes"
+	"encoding/hex"
 	"encoding/json"
 	"errors"
 	"fmt"
 	"net/http"
 	"strconv"
 
+	"github.com/skycoin/skycoin/src/coin"
 	"github.com/skycoin/skycoin/src/visor"
 )
 
-var ErrJSONUnmarshal = errors.New("json unmarshal failed")
+// ErrJSONUnmarshal is returned if JSON unmarshal fails
+var ErrJSONUnmarshal = errors.New("JSON unmarshal failed")
 
+// Client is an RPC client
 type Client struct {
 	Addr     string
-	reqIdCtr int
+	reqIDCtr int
 }
 
+// Do makes an RPC request
 func (c *Client) Do(obj interface{}, method string, params interface{}) error {
-	c.reqIdCtr++
-	req, err := NewRequest(method, params, strconv.Itoa(c.reqIdCtr))
+	c.reqIDCtr++
+	req, err := NewRequest(method, params, strconv.Itoa(c.reqIDCtr))
 	if err != nil {
 		return err
 	}
@@ -34,9 +39,11 @@ func (c *Client) Do(obj interface{}, method string, params interface{}) error {
 		return rsp.Error
 	}
 
-	return decodeJson(rsp.Result, obj)
+	return decodeJSON(rsp.Result, obj)
 }
 
+// GetUnspentOutputs returns unspent outputs for a set of addresses
+// TODO -- what is the difference between this and GetAddressUxOuts?
 func (c *Client) GetUnspentOutputs(addrs []string) (*OutputsResult, error) {
 	outputs := OutputsResult{}
 	if err := c.Do(&outputs, "get_outputs", addrs); err != nil {
@@ -46,8 +53,8 @@ func (c *Client) GetUnspentOutputs(addrs []string) (*OutputsResult, error) {
 	return &outputs, nil
 }
 
-// Returns TxId
-func (c *Client) InjectTransaction(rawtx string) (string, error) {
+// InjectTransactionString injects a hex-encoded transaction string to the network
+func (c *Client) InjectTransactionString(rawtx string) (string, error) {
 	params := []string{rawtx}
 	rlt := TxIDJson{}
 
@@ -58,6 +65,14 @@ func (c *Client) InjectTransaction(rawtx string) (string, error) {
 	return rlt.Txid, nil
 }
 
+// InjectTransaction injects a *coin.Transaction to the network
+func (c *Client) InjectTransaction(tx *coin.Transaction) (string, error) {
+	d := tx.Serialize()
+	rawTx := hex.EncodeToString(d)
+	return c.InjectTransactionString(rawTx)
+}
+
+// GetStatus returns status info for a skycoin node
 func (c *Client) GetStatus() (*StatusResult, error) {
 	status := StatusResult{}
 	if err := c.Do(&status, "get_status", nil); err != nil {
@@ -67,6 +82,7 @@ func (c *Client) GetStatus() (*StatusResult, error) {
 	return &status, nil
 }
 
+// GetTransactionByID returns a transaction given a txid
 func (c *Client) GetTransactionByID(txid string) (*TxnResult, error) {
 	txn := TxnResult{}
 	if err := c.Do(&txn, "get_transaction", []string{txid}); err != nil {
@@ -76,6 +92,8 @@ func (c *Client) GetTransactionByID(txid string) (*TxnResult, error) {
 	return &txn, nil
 }
 
+// GetAddressUxOuts returns unspent outputs for a set of addresses
+// TODO -- what is the difference between this and GetUnspentOutputs?
 func (c *Client) GetAddressUxOuts(addrs []string) ([]AddrUxoutResult, error) {
 	uxouts := []AddrUxoutResult{}
 	if err := c.Do(&uxouts, "get_address_uxouts", addrs); err != nil {
@@ -85,6 +103,7 @@ func (c *Client) GetAddressUxOuts(addrs []string) ([]AddrUxoutResult, error) {
 	return uxouts, nil
 }
 
+// GetBlocks returns a range of blocks
 func (c *Client) GetBlocks(start, end uint64) (*visor.ReadableBlocks, error) {
 	param := []uint64{start, end}
 	blocks := visor.ReadableBlocks{}
@@ -96,6 +115,7 @@ func (c *Client) GetBlocks(start, end uint64) (*visor.ReadableBlocks, error) {
 	return &blocks, nil
 }
 
+// GetBlocksBySeq returns blocks for a set of block sequences (heights)
 func (c *Client) GetBlocksBySeq(ss []uint64) (*visor.ReadableBlocks, error) {
 	blocks := visor.ReadableBlocks{}
 
@@ -106,6 +126,7 @@ func (c *Client) GetBlocksBySeq(ss []uint64) (*visor.ReadableBlocks, error) {
 	return &blocks, nil
 }
 
+// GetLastBlocks returns the last n blocks
 func (c *Client) GetLastBlocks(n uint64) (*visor.ReadableBlocks, error) {
 	param := []uint64{n}
 	blocks := visor.ReadableBlocks{}
@@ -135,7 +156,7 @@ func Do(req *Request, rpcAddress string) (*Response, error) {
 	return &res, nil
 }
 
-func decodeJson(data []byte, obj interface{}) error {
+func decodeJSON(data []byte, obj interface{}) error {
 	if err := json.NewDecoder(bytes.NewBuffer(data)).Decode(obj); err != nil {
 		return ErrJSONUnmarshal
 	}

src/api/webrpc/client_test.go

@@ -94,17 +94,17 @@ func testClientGetUnspentOutputs(t *testing.T, c *Client, s *WebRPC, gw *fakeGat
 
 func testClientInjectTransaction(t *testing.T, c *Client, s *WebRPC, gw *fakeGateway) {
 	gw.injectRawTxMap = map[string]bool{
-		rawTxId: true,
+		rawTxID: true,
 	}
 	require.Empty(t, gw.injectedTransactions)
 
-	txID, err := c.InjectTransaction(rawTxStr)
+	txID, err := c.InjectTransactionString(rawTxStr)
 	require.NoError(t, err)
 	require.NotEmpty(t, txID)
 
 	log.Println(gw.injectedTransactions)
 	require.Len(t, gw.injectedTransactions, 1)
-	require.Contains(t, gw.injectedTransactions, rawTxId)
+	require.Contains(t, gw.injectedTransactions, rawTxID)
 }
 
 func testClientGetStatus(t *testing.T, c *Client, s *WebRPC, gw *fakeGateway) {
@@ -128,15 +128,15 @@ func testClientGetTransactionByID(t *testing.T, c *Client, s *WebRPC, gw *fakeGa
 
 	// Valid txn id, txn does not exist
 	// TODO
-	txn, err = c.GetTransactionByID(rawTxId)
+	txn, err = c.GetTransactionByID(rawTxID)
 	require.Nil(t, txn)
 	require.Error(t, err)
 
 	// Txn exists
 	gw.transactions = map[string]string{
-		rawTxId: rawTxStr,
+		rawTxID: rawTxStr,
 	}
-	txn, err = c.GetTransactionByID(rawTxId)
+	txn, err = c.GetTransactionByID(rawTxID)
 	require.NoError(t, err)
 	expectedTxn := decodeRawTransaction(rawTxStr)
 	rbTx, err := visor.NewReadableTransaction(expectedTxn)

src/api/webrpc/transaction.go

@@ -70,7 +70,7 @@ func injectTransactionHandler(req Request, gateway Gatewayer) Response {
 		return makeErrorResponse(errCodeInvalidParams, fmt.Sprintf("invalid raw transaction:%v", err))
 	}
 
-	txn, err := deserializeTx(b)
+	txn, err := coin.TransactionDeserialize(b)
 	if err != nil {
 		return makeErrorResponse(errCodeInvalidParams, fmt.Sprintf("%v", err))
 	}
@@ -81,13 +81,3 @@ func injectTransactionHandler(req Request, gateway Gatewayer) Response {
 
 	return makeSuccessResponse(req.ID, TxIDJson{txn.Hash().Hex()})
 }
-
-func deserializeTx(b []byte) (tx coin.Transaction, err error) {
-	defer func() {
-		if r := recover(); r != nil {
-			err = fmt.Errorf("%v", r)
-		}
-	}()
-	tx = coin.TransactionDeserialize(b)
-	return
-}

src/api/webrpc/transaction_test.go

@@ -12,7 +12,7 @@ import (
 
 const (
 	rawTxStr    = "dc00000000a8558b814926ed0062cd720a572bd67367aa0d01c0769ea4800adcc89cdee524010000008756e4bde4ee1c725510a6a9a308c6a90d949de7785978599a87faba601d119f27e1be695cbb32a1e346e5dd88653a97006bf1a93c9673ac59cf7b5db7e07901000100000079216473e8f2c17095c6887cc9edca6c023afedfac2e0c5460e8b6f359684f8b020000000060dfa95881cdc827b45a6d49b11dbc152ecd4de640420f00000000000000000000000000006409744bcacb181bf98b1f02a11e112d7e4fa9f940f1f23a000000000000000000000000"
-	rawTxId     = "bdc4a85a3e9d17a8fe00aa7430d0347c7f1dd6480a16da7147b6e43905057d43"
+	rawTxID     = "bdc4a85a3e9d17a8fe00aa7430d0347c7f1dd6480a16da7147b6e43905057d43"
 	txHeight    = uint64(103)
 	txConfirmed = true
 )
@@ -27,7 +27,7 @@ func decodeRawTransaction(rawTxStr string) *visor.Transaction {
 		panic(fmt.Sprintf("invalid raw transaction:%v", err))
 	}
 
-	tx := coin.TransactionDeserialize(rawTx)
+	tx := coin.MustTransactionDeserialize(rawTx)
 	return &visor.Transaction{
 		Txn: tx,
 		Status: visor.TransactionStatus{
@@ -67,10 +67,10 @@ func Test_getTransactionHandler(t *testing.T) {
 					ID:      "1",
 					Jsonrpc: jsonRPC,
 					Method:  "get_transaction",
-					Params:  []byte(fmt.Sprintf(`["%s"]`, rawTxId)),
+					Params:  []byte(fmt.Sprintf(`["%s"]`, rawTxID)),
 				},
 				gateway: &fakeGateway{transactions: map[string]string{
-					rawTxId: rawTxStr,
+					rawTxID: rawTxStr,
 				}},
 			},
 			makeSuccessResponse("1", TxnResult{&txRlt}),

src/coin/transactions.go

@@ -3,6 +3,7 @@ package coin
 import (
 	"bytes"
 	"errors"
+	"fmt"
 	"math"
 	"sort"
 
@@ -269,13 +270,22 @@ func (txn *Transaction) Serialize() []byte {
 	return encoder.Serialize(*txn)
 }
 
+// MustTransactionDeserialize deserialize transaction, panics on error
+func MustTransactionDeserialize(b []byte) Transaction {
+	t, err := TransactionDeserialize(b)
+	if err != nil {
+		logger.Panicf("Failed to deserialize transaction: %v", err)
+	}
+	return t
+}
+
 // TransactionDeserialize deserialize transaction
-func TransactionDeserialize(b []byte) Transaction {
+func TransactionDeserialize(b []byte) (Transaction, error) {
 	t := Transaction{}
 	if err := encoder.DeserializeRaw(b, &t); err != nil {
-		logger.Panic("Failed to deserialize transaction")
+		return t, fmt.Errorf("Invalid transaction: %v", err)
 	}
-	return t
+	return t, nil
 }
 
 // OutputHours returns the coin hours sent as outputs. This does not include the fee.