Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

R4R: Tx Query return values #3642

Merged
merged 4 commits into from
Feb 15, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions PENDING.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
BREAKING CHANGES

* Gaia REST API
* [\#3642](https://github.com/cosmos/cosmos-sdk/pull/3642) `GET /tx/{hash}` now returns `404` instead of `500` if the transaction is not found

* Gaia CLI

Expand Down
12 changes: 12 additions & 0 deletions client/lcd/lcd_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"net/http"
"os"
"regexp"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -503,6 +504,17 @@ func TestTxs(t *testing.T) {
txs = getTransactions(t, port, fmt.Sprintf("recipient=%s", receiveAddr.String()))
require.Len(t, txs, 1)
require.Equal(t, resultTx.Height, txs[0].Height)

// query transaction that doesn't exist
validTxHash := "9ADBECAAD8DACBEC3F4F535704E7CF715C765BDCEDBEF086AFEAD31BA664FB0B"
res, body := getTransactionRequest(t, port, validTxHash)
require.True(t, strings.Contains(body, validTxHash))
require.Equal(t, http.StatusNotFound, res.StatusCode)

// bad query string
res, body = getTransactionRequest(t, port, "badtxhash")
require.True(t, strings.Contains(body, "encoding/hex"))
require.Equal(t, http.StatusInternalServerError, res.StatusCode)
}

func TestPoolParamsQuery(t *testing.T) {
Expand Down
6 changes: 5 additions & 1 deletion client/lcd/test_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -504,14 +504,18 @@ func getValidatorSets(t *testing.T, port string, height int, expectFail bool) rp
// GET /txs/{hash} get tx by hash
func getTransaction(t *testing.T, port string, hash string) sdk.TxResponse {
var tx sdk.TxResponse
res, body := Request(t, port, "GET", fmt.Sprintf("/txs/%s", hash), nil)
res, body := getTransactionRequest(t, port, hash)
require.Equal(t, http.StatusOK, res.StatusCode, body)

err := cdc.UnmarshalJSON([]byte(body), &tx)
require.NoError(t, err)
return tx
}

func getTransactionRequest(t *testing.T, port, hash string) (*http.Response, string) {
return Request(t, port, "GET", fmt.Sprintf("/txs/%s", hash), nil)
}

// POST /txs broadcast txs

// GET /txs search transactions
Expand Down
66 changes: 35 additions & 31 deletions client/tx/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"encoding/hex"
"fmt"
"net/http"
"strings"

"github.com/gorilla/mux"
"github.com/spf13/cobra"
Expand All @@ -26,18 +27,18 @@ func QueryTxCmd(cdc *codec.Codec) *cobra.Command {
Short: "Matches this txhash over all committed blocks",
Args: cobra.ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
// find the key to look up the account
hashHexStr := args[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why delete?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it's only used in one place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it add meaning! Otherwise, you'd have to guess what it is


cliCtx := context.NewCLIContext().WithCodec(cdc)

output, err := queryTx(cdc, cliCtx, hashHexStr)
output, err := queryTx(cdc, cliCtx, args[0])
if err != nil {
return err
}

fmt.Println(string(output))
return nil
if output.Empty() {
return fmt.Errorf("No transaction found with hash %s", args[0])
}

return cliCtx.PrintOutput(output)
},
}

Expand All @@ -48,50 +49,44 @@ func QueryTxCmd(cdc *codec.Codec) *cobra.Command {
return cmd
}

func queryTx(cdc *codec.Codec, cliCtx context.CLIContext, hashHexStr string) ([]byte, error) {
func queryTx(cdc *codec.Codec, cliCtx context.CLIContext, hashHexStr string) (out sdk.TxResponse, err error) {
hash, err := hex.DecodeString(hashHexStr)
if err != nil {
return nil, err
return out, err
}

node, err := cliCtx.GetNode()
if err != nil {
return nil, err
return out, err
}

res, err := node.Tx(hash, !cliCtx.TrustNode)
if err != nil {
return nil, err
return out, err
}

if !cliCtx.TrustNode {
err := ValidateTxResult(cliCtx, res)
if err != nil {
return nil, err
}
if err = ValidateTxResult(cliCtx, res); !cliCtx.TrustNode && err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not equal to what was before. Before we didn't execute ValidateTxResult if TrustNode == true.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch.

return out, err
}

info, err := formatTxResult(cdc, res)
if err != nil {
return nil, err
if out, err = formatTxResult(cdc, res); err != nil {
return out, err
}

if cliCtx.Indent {
return cdc.MarshalJSONIndent(info, "", " ")
}
return cdc.MarshalJSON(info)
return out, nil
}

// ValidateTxResult performs transaction verification
func ValidateTxResult(cliCtx context.CLIContext, res *ctypes.ResultTx) error {
check, err := cliCtx.Verify(res.Height)
if err != nil {
return err
}

err = res.Proof.Validate(check.Header.DataHash)
if err != nil {
return err
if !cliCtx.TrustNode {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if cliCtx.TrustNode {
  return nil
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code does this. Slightly confused what you mean here? If we trust the node, we don't want to validate the results.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant instead of wrapping big chunk of code into if condition, you can return early!

check, err := cliCtx.Verify(res.Height)
if err != nil {
return err
}
err = res.Proof.Validate(check.Header.DataHash)
if err != nil {
return err
}
}
return nil
}
Expand All @@ -118,17 +113,26 @@ func parseTx(cdc *codec.Codec, txBytes []byte) (sdk.Tx, error) {

// REST

// transaction query REST handler
// QueryTxRequestHandlerFn transaction query REST handler
func QueryTxRequestHandlerFn(cdc *codec.Codec, cliCtx context.CLIContext) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
vars := mux.Vars(r)
hashHexStr := vars["hash"]

output, err := queryTx(cdc, cliCtx, hashHexStr)
if err != nil {
if strings.Contains(err.Error(), hashHexStr) {
rest.WriteErrorResponse(w, http.StatusNotFound, err.Error())
return
}
rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
return
}

if output.Empty() {
rest.WriteErrorResponse(w, http.StatusNotFound, fmt.Sprintf("no transaction found with hash %s", hashHexStr))
}

rest.PostProcessResponse(w, cdc, output, cliCtx.Indent)
}
}
9 changes: 9 additions & 0 deletions types/result.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ type TxResponse struct {
Tx Tx `json:"tx,omitempty"`
}

// NewResponseResultTx returns a TxResponse given a ResultTx from tendermint
func NewResponseResultTx(res *ctypes.ResultTx, tx Tx) TxResponse {
if res == nil {
return TxResponse{}
Expand All @@ -73,6 +74,7 @@ func NewResponseResultTx(res *ctypes.ResultTx, tx Tx) TxResponse {
}
}

// NewResponseFormatBroadcastTxCommit returns a TxResponse given a ResultBroadcastTxCommit from tendermint
func NewResponseFormatBroadcastTxCommit(res *ctypes.ResultBroadcastTxCommit) TxResponse {
if res == nil {
return TxResponse{}
Expand All @@ -95,8 +97,10 @@ func NewResponseFormatBroadcastTxCommit(res *ctypes.ResultBroadcastTxCommit) TxR
Tags: TagsToStringTags(res.DeliverTx.Tags),
Codespace: res.DeliverTx.Codespace,
}

}

// NewResponseFormatBroadcastTx returns a TxResponse given a ResultBroadcastTx from tendermint
func NewResponseFormatBroadcastTx(res *ctypes.ResultBroadcastTx) TxResponse {
if res == nil {
return TxResponse{}
Expand Down Expand Up @@ -156,3 +160,8 @@ func (r TxResponse) String() string {

return strings.TrimSpace(sb.String())
}

// Empty returns true if the response is empty
func (r TxResponse) Empty() bool {
return r.TxHash == "" && r.Log == ""
}