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: generalize query response with height #4573

Merged
merged 19 commits into from
Jul 1, 2019
Merged
Show file tree
Hide file tree
Changes from 16 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/improvements/sdk/4573-adds-height-in-
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
#4573 Returns height in response for query endpoints.
35 changes: 35 additions & 0 deletions types/rest/rest.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
package rest

import (
"encoding/json"
"errors"
"fmt"
"io/ioutil"
Expand Down Expand Up @@ -222,9 +223,17 @@ func ParseQueryHeightOrReturnBadRequest(w http.ResponseWriter, cliCtx context.CL
}

// PostProcessResponse performs post processing for a REST response.
// If the height is greater than zero it will be injected into the body
// of the response. An internal server error is written to the response
// if the height is negative or an encoding/decoding error occurs.
func PostProcessResponse(w http.ResponseWriter, cliCtx context.CLIContext, response interface{}) {
var output []byte

if cliCtx.Height < 0 {
WriteErrorResponse(w, http.StatusInternalServerError, fmt.Errorf("negative height in response").Error())
return
}

switch response.(type) {
case []byte:
output = response.([]byte)
Expand All @@ -236,6 +245,32 @@ func PostProcessResponse(w http.ResponseWriter, cliCtx context.CLIContext, respo
} else {
output, err = cliCtx.Codec.MarshalJSON(response)
}

if err != nil {
WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
return
}
}

// inject the height into the response by:
// - decoding into a map
// - adding the height to the map
// - encoding using standard JSON library
if cliCtx.Height > 0 {
m := make(map[string]interface{})
err := json.Unmarshal(output, &m)
if err != nil {
WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
return
}

m["height"] = cliCtx.Height

if cliCtx.Indent {
output, err = json.MarshalIndent(m, "", " ")
} else {
output, err = json.Marshal(m)
}
if err != nil {
WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
return
Expand Down
98 changes: 97 additions & 1 deletion types/rest/rest_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,21 @@
package rest

import (
"encoding/json"
"io"
"io/ioutil"
"net/http"
"net/http/httptest"
"strconv"
"testing"

"github.com/cosmos/cosmos-sdk/client/context"
"github.com/stretchr/testify/require"
"github.com/tendermint/tendermint/crypto/secp256k1"

"github.com/cosmos/cosmos-sdk/client/context"
"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/types"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
Copy link
Contributor

Choose a reason for hiding this comment

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

Top level packages must not depend on x sub packages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Offf, good catch -- I missed this!

Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably remove this test all together @colin-axner and move it to the LCD tests in Gaia.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We don’t need LCD tests for queries as those are covered by the contract tests

Copy link
Contributor Author

@colin-axner colin-axner Jul 1, 2019

Choose a reason for hiding this comment

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

Makes sense, should I remove and move the tests elsewhere or should I refactor the test to use a mock version of account?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the latter

)

type mockResponseWriter struct{}
Expand Down Expand Up @@ -138,6 +144,96 @@ func TestParseQueryHeight(t *testing.T) {
}
}

func TestProcessPostResponse(t *testing.T) {
// setup
ctx := context.NewCLIContext()
height := int64(194423)

privKey := secp256k1.GenPrivKey()
pubKey := privKey.PubKey()
addr := types.AccAddress(pubKey.Address())
coins := types.NewCoins(types.NewCoin("atom", types.NewInt(100)), types.NewCoin("tree", types.NewInt(125)))
accNumber := uint64(104)
sequence := uint64(32)

acc := authtypes.NewBaseAccount(addr, coins, pubKey, accNumber, sequence)
cdc := codec.New()
authtypes.RegisterBaseAccount(cdc)
ctx = ctx.WithCodec(cdc)

// setup expected json responses with zero height
jsonNoHeight, err := cdc.MarshalJSON(acc)
require.Nil(t, err)
require.NotNil(t, jsonNoHeight)
jsonIndentNoHeight, err := cdc.MarshalJSONIndent(acc, "", " ")
require.Nil(t, err)
require.NotNil(t, jsonIndentNoHeight)

// decode into map to order alphabetically
m := make(map[string]interface{})
err = json.Unmarshal(jsonNoHeight, &m)
require.Nil(t, err)
jsonMap, err := json.Marshal(m)
require.Nil(t, err)
jsonWithHeight := append(append([]byte(`{"height":`), []byte(strconv.Itoa(int(height))+",")...), jsonMap[1:]...)
jsonIndentMap, err := json.MarshalIndent(m, "", " ")
jsonIndentWithHeight := append(append([]byte(`{`+"\n "+` "height": `), []byte(strconv.Itoa(int(height))+",")...), jsonIndentMap[1:]...)

// check that negative height writes an error
w := httptest.NewRecorder()
ctx = ctx.WithHeight(-1)
PostProcessResponse(w, ctx, acc)
require.Equal(t, http.StatusInternalServerError, w.Code)

// check that zero height returns expected response
ctx = ctx.WithHeight(0)
runPostProcessResponse(t, ctx, acc, jsonNoHeight, false)
// chcek zero height with indent
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
runPostProcessResponse(t, ctx, acc, jsonIndentNoHeight, true)
// check that height returns expected response
ctx = ctx.WithHeight(height)
runPostProcessResponse(t, ctx, acc, jsonWithHeight, false)
// check height with indent
runPostProcessResponse(t, ctx, acc, jsonIndentWithHeight, true)
}

// asserts that ResponseRecorder returns the expected code and body
// runs PostProcessResponse on the objects regular interface and on
// the marshalled struct.
func runPostProcessResponse(t *testing.T, ctx context.CLIContext, obj interface{},
expectedBody []byte, indent bool,
) {
if indent {
ctx.Indent = indent
}

// test using regular struct
w := httptest.NewRecorder()
PostProcessResponse(w, ctx, obj)
require.Equal(t, http.StatusOK, w.Code, w.Body)
resp := w.Result()
body, err := ioutil.ReadAll(resp.Body)
require.Nil(t, err)
require.Equal(t, expectedBody, body)

var marshalled []byte
if indent {
marshalled, err = ctx.Codec.MarshalJSONIndent(obj, "", " ")
} else {
marshalled, err = ctx.Codec.MarshalJSON(obj)
}
require.Nil(t, err)

// test using marshalled struct
w = httptest.NewRecorder()
PostProcessResponse(w, ctx, marshalled)
require.Equal(t, http.StatusOK, w.Code, w.Body)
resp = w.Result()
body, err = ioutil.ReadAll(resp.Body)
require.Nil(t, err)
require.Equal(t, expectedBody, body)
}

func mustNewRequest(t *testing.T, method, url string, body io.Reader) *http.Request {
req, err := http.NewRequest(method, url, body)
require.NoError(t, err)
Expand Down
13 changes: 3 additions & 10 deletions x/auth/client/rest/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,9 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/rest"
"github.com/cosmos/cosmos-sdk/x/auth/client/utils"
"github.com/cosmos/cosmos-sdk/x/auth/exported"
"github.com/cosmos/cosmos-sdk/x/auth/types"
)

// AccountWithHeight wraps the embedded Account with the height it was queried
// at.
type AccountWithHeight struct {
exported.Account `json:"account"`
Height int64 `json:"height"`
}

// query accountREST Handler
func QueryAccountRequestHandlerFn(storeName string, cliCtx context.CLIContext) http.HandlerFunc {

Expand All @@ -47,13 +39,14 @@ func QueryAccountRequestHandlerFn(storeName string, cliCtx context.CLIContext) h
return
}

account, err := accGetter.GetAccount(addr)
account, height, err := accGetter.GetAccountWithHeight(addr)
if err != nil {
rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
return
}

rest.PostProcessResponse(w, cliCtx, AccountWithHeight{account, cliCtx.Height})
cliCtx = cliCtx.WithHeight(height)
rest.PostProcessResponse(w, cliCtx, account)
}
}

Expand Down
18 changes: 13 additions & 5 deletions x/auth/types/account_retriever.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,22 +29,30 @@ func NewAccountRetriever(querier NodeQuerier) AccountRetriever {
// GetAccount queries for an account given an address and a block height. An
// error is returned if the query or decoding fails.
func (ar AccountRetriever) GetAccount(addr sdk.AccAddress) (exported.Account, error) {
account, _, err := ar.GetAccountWithHeight(addr)
return account, err
}

// GetAccountWithHeight queries for an account given an address. Returns the
// height of the query with the account. An error is returned if the query
// or decoding fails.
func (ar AccountRetriever) GetAccountWithHeight(addr sdk.AccAddress) (exported.Account, int64, error) {
bs, err := ModuleCdc.MarshalJSON(NewQueryAccountParams(addr))
if err != nil {
return nil, err
return nil, 0, err
}

res, _, err := ar.querier.QueryWithData(fmt.Sprintf("custom/%s/%s", QuerierRoute, QueryAccount), bs)
res, height, err := ar.querier.QueryWithData(fmt.Sprintf("custom/%s/%s", QuerierRoute, QueryAccount), bs)
if err != nil {
return nil, err
return nil, 0, err
}

var account exported.Account
if err := ModuleCdc.UnmarshalJSON(res, &account); err != nil {
return nil, err
return nil, 0, err
}

return account, nil
return account, height, nil
}

// EnsureExists returns an error if no account exists for the given address else nil.
Expand Down
4 changes: 3 additions & 1 deletion x/bank/client/rest/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,14 @@ func QueryBalancesRequestHandlerFn(cliCtx context.CLIContext) http.HandlerFunc {
return
}

res, _, err := cliCtx.QueryWithData("custom/bank/balances", bz)
res, height, err := cliCtx.QueryWithData("custom/bank/balances", bz)
if err != nil {
rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
return
}

cliCtx = cliCtx.WithHeight(height)

// the query will return empty if there is no data for this account
if len(res) == 0 {
rest.PostProcessResponse(w, cliCtx, sdk.Coins{})
Expand Down
9 changes: 6 additions & 3 deletions x/distribution/client/rest/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,12 +115,13 @@ func delegatorWithdrawalAddrHandlerFn(cliCtx context.CLIContext, queryRoute stri
}

bz := cliCtx.Codec.MustMarshalJSON(types.NewQueryDelegatorWithdrawAddrParams(delegatorAddr))
res, _, err := cliCtx.QueryWithData(fmt.Sprintf("custom/%s/withdraw_addr", queryRoute), bz)
res, height, err := cliCtx.QueryWithData(fmt.Sprintf("custom/%s/withdraw_addr", queryRoute), bz)
if err != nil {
rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
return
}

cliCtx = cliCtx.WithHeight(height)
rest.PostProcessResponse(w, cliCtx, res)
}
}
Expand Down Expand Up @@ -232,7 +233,7 @@ func communityPoolHandler(cliCtx context.CLIContext, queryRoute string) http.Han
return
}

res, _, err := cliCtx.QueryWithData(fmt.Sprintf("custom/%s/community_pool", queryRoute), nil)
res, height, err := cliCtx.QueryWithData(fmt.Sprintf("custom/%s/community_pool", queryRoute), nil)
if err != nil {
rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
return
Expand All @@ -244,6 +245,7 @@ func communityPoolHandler(cliCtx context.CLIContext, queryRoute string) http.Han
return
}

cliCtx = cliCtx.WithHeight(height)
rest.PostProcessResponse(w, cliCtx, result)
}
}
Expand All @@ -262,12 +264,13 @@ func outstandingRewardsHandlerFn(cliCtx context.CLIContext, queryRoute string) h
}

bin := cliCtx.Codec.MustMarshalJSON(types.NewQueryValidatorOutstandingRewardsParams(validatorAddr))
res, _, err := cliCtx.QueryWithData(fmt.Sprintf("custom/%s/validator_outstanding_rewards", queryRoute), bin)
res, height, err := cliCtx.QueryWithData(fmt.Sprintf("custom/%s/validator_outstanding_rewards", queryRoute), bin)
if err != nil {
rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
return
}

cliCtx = cliCtx.WithHeight(height)
rest.PostProcessResponse(w, cliCtx, res)
}
}
Expand Down
2 changes: 1 addition & 1 deletion x/distribution/types/delegator.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (
type DelegatorStartingInfo struct {
PreviousPeriod uint64 `json:"previous_period"` // period at which the delegation should withdraw starting from
Stake sdk.Dec `json:"stake"` // amount of staking token delegated
Height uint64 `json:"height"` // height at which delegation was created
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
Height uint64 `json:"creation_height"` // height at which delegation was created
}

// create a new DelegatorStartingInfo
Expand Down
12 changes: 8 additions & 4 deletions x/gov/client/rest/rest.go
Original file line number Diff line number Diff line change
Expand Up @@ -201,12 +201,13 @@ func queryParamsHandlerFn(cliCtx context.CLIContext) http.HandlerFunc {
return
}

res, _, err := cliCtx.QueryWithData(fmt.Sprintf("custom/gov/%s/%s", types.QueryParams, paramType), nil)
res, height, err := cliCtx.QueryWithData(fmt.Sprintf("custom/gov/%s/%s", types.QueryParams, paramType), nil)
if err != nil {
rest.WriteErrorResponse(w, http.StatusNotFound, err.Error())
return
}

cliCtx = cliCtx.WithHeight(height)
rest.PostProcessResponse(w, cliCtx, res)
}
}
Expand Down Expand Up @@ -240,12 +241,13 @@ func queryProposalHandlerFn(cliCtx context.CLIContext) http.HandlerFunc {
return
}

res, _, err := cliCtx.QueryWithData("custom/gov/proposal", bz)
res, height, err := cliCtx.QueryWithData("custom/gov/proposal", bz)
if err != nil {
rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
return
}

cliCtx = cliCtx.WithHeight(height)
rest.PostProcessResponse(w, cliCtx, res)
}
}
Expand Down Expand Up @@ -607,12 +609,13 @@ func queryProposalsWithParameterFn(cliCtx context.CLIContext) http.HandlerFunc {
return
}

res, _, err := cliCtx.QueryWithData("custom/gov/proposals", bz)
res, height, err := cliCtx.QueryWithData("custom/gov/proposals", bz)
if err != nil {
rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
return
}

cliCtx = cliCtx.WithHeight(height)
rest.PostProcessResponse(w, cliCtx, res)
}
}
Expand Down Expand Up @@ -647,12 +650,13 @@ func queryTallyOnProposalHandlerFn(cliCtx context.CLIContext) http.HandlerFunc {
return
}

res, _, err := cliCtx.QueryWithData("custom/gov/tally", bz)
res, height, err := cliCtx.QueryWithData("custom/gov/tally", bz)
if err != nil {
rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
return
}

cliCtx = cliCtx.WithHeight(height)
rest.PostProcessResponse(w, cliCtx, res)
}
}
Loading