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

fix!: update ABCI query to use request height #9879

Merged
merged 12 commits into from
Aug 12, 2021
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Client Breaking Changes

* [\#9879](https://github.com/cosmos/cosmos-sdk/pull/9879) Modify ABCI Queries to use `abci.QueryRequest` Height field instead of context height.
* [\#9859](https://github.com/cosmos/cosmos-sdk/pull/9859) The `default` pruning strategy now keeps the last 362880 blocks instead of 100. 362880 equates to roughly enough blocks to cover the entire unbonding period assuming a 21 day unbonding period and 5s block time.
* [\#9594](https://github.com/cosmos/cosmos-sdk/pull/9594) Remove legacy REST API. Please see the [REST Endpoints Migration guide](https://docs.cosmos.network/master/migrations/rest.html) to migrate to the new REST endpoints.
* [\#9781](https://github.com/cosmos/cosmos-sdk/pull/9781) Improve`withdraw-all-rewards` UX when broadcast mode `async` or `async` is used.
Expand Down
2 changes: 1 addition & 1 deletion client/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ func (ctx Context) queryABCI(req abci.RequestQuery) (abci.ResponseQuery, error)
}

opts := rpcclient.ABCIQueryOptions{
Height: ctx.Height,
Height: req.Height,
Copy link
Contributor

Choose a reason for hiding this comment

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

In CLI (with --height) and grpc queries (with the height header), iirc we populate the client ctx.Height. So I think we should use req.Height if that's set, or fallback to ctx.Height.

I believe that's the cause of all failing tests

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. Is there a reason we don't populate the req in CLI and API pathways, instead of ctx? Just curious.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we don't populate the req in CLI and API pathways

Yeah that could work too, where do you see this logic happening?

Copy link
Contributor

Choose a reason for hiding this comment

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

So Context#queryABCI gets call from 2-3 places, one of which doesn't have a height to provide, so we cannot solely rely on req.Height.

I would do two things:

  1. Update RunGRPCQuery to set Height in abciReq.
  2. Update your logic to fallback on ctx.Height if req.Height is zero.

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM!

Prove: req.Prove,
}

Expand Down
35 changes: 35 additions & 0 deletions client/query_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// +build norace

package client_test

import (
"fmt"

abci "github.com/tendermint/tendermint/abci/types"

banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
)

func (s *IntegrationTestSuite) TestQueryABCIHeight() {
// test ABCI query uses request height argument
// instead of client context height
contextHeight := 1 // query at height 1 or 2 would cause an error
reqHeight := int64(10)

val := s.network.Validators[0]
clientCtx := val.ClientCtx
clientCtx = clientCtx.WithHeight(contextHeight)

req := abci.RequestQuery{
Path: fmt.Sprintf("store/%s/key", banktypes.StoreKey),
Height: reqHeight,
Data: banktypes.CreateAccountBalancesPrefix(val.Address),
Prove: true,
}

res, err := clientCtx.QueryABCI(req)
s.Require().NoError(err)

s.Require().Equal(reqHeight, res.Height)

colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}