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

refactor: remove header type from NewContext #17426

Merged
merged 12 commits into from
Aug 21, 2023
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (types) [#17348](https://github.com/cosmos/cosmos-sdk/pull/17348) Remove the `WrapServiceResult` function.
* The `*sdk.Result` returned by the msg server router will not contain the `.Data` field.
* (x/staking) [#17335](https://github.com/cosmos/cosmos-sdk/pull/17335) Remove usage of `"github.com/cosmos/cosmos-sdk/x/staking/types".Infraction_*` in favour of `"cosmossdk.io/api/cosmos/staking/v1beta1".Infraction_` in order to remove dependency between modules on staking
* (types) [#17426](https://github.com/cosmos/cosmos-sdk/pull/17426) `NewContext` does not take a `cmtproto.Header{}` any longer.
* `WithChainID` / `WithBlockHeight` / `WithBlockHeader` must be used to set values on the context

### CLI Breaking Changes

Expand Down
12 changes: 5 additions & 7 deletions baseapp/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -549,9 +549,8 @@ func (app *BaseApp) ProcessProposal(req *abci.RequestProcessProposal) (resp *abc
func (app *BaseApp) ExtendVote(_ context.Context, req *abci.RequestExtendVote) (resp *abci.ResponseExtendVote, err error) {
// Always reset state given that ExtendVote and VerifyVoteExtension can timeout
// and be called again in a subsequent round.
emptyHeader := cmtproto.Header{ChainID: app.chainID, Height: req.Height}
ms := app.cms.CacheMultiStore()
ctx := sdk.NewContext(ms, emptyHeader, false, app.logger).WithStreamingManager(app.streamingManager)
ctx := sdk.NewContext(ms, false, app.logger).WithStreamingManager(app.streamingManager).WithChainID(app.chainID).WithBlockHeight(req.Height)

if app.extendVote == nil {
return nil, errors.New("application ExtendVote handler not set")
Expand Down Expand Up @@ -611,9 +610,8 @@ func (app *BaseApp) VerifyVoteExtension(req *abci.RequestVerifyVoteExtension) (r
return nil, errors.New("application VerifyVoteExtension handler not set")
}

emptyHeader := cmtproto.Header{ChainID: app.chainID, Height: req.Height}
ms := app.cms.CacheMultiStore()
ctx := sdk.NewContext(ms, emptyHeader, false, app.logger).WithStreamingManager(app.streamingManager)
ctx := sdk.NewContext(ms, false, app.logger).WithStreamingManager(app.streamingManager).WithChainID(app.chainID).WithBlockHeight(req.Height)

// If vote extensions are not enabled, as a safety precaution, we return an
// error.
Expand Down Expand Up @@ -1016,7 +1014,7 @@ func (app *BaseApp) getContextForProposal(ctx sdk.Context, height int64) sdk.Con
ctx, _ = app.finalizeBlockState.ctx.CacheContext()

// clear all context data set during InitChain to avoid inconsistent behavior
ctx = ctx.WithBlockHeader(cmtproto.Header{}).WithHeaderInfo(coreheader.Info{})
ctx = ctx.WithHeaderInfo(coreheader.Info{}).WithBlockHeader(cmtproto.Header{})
return ctx
}

Expand Down Expand Up @@ -1120,10 +1118,10 @@ func (app *BaseApp) CreateQueryContext(height int64, prove bool) (sdk.Context, e
}

// branch the commit multi-store for safety
ctx := sdk.NewContext(cacheMS, app.checkState.ctx.BlockHeader(), true, app.logger).
ctx := sdk.NewContext(cacheMS, true, app.logger).
WithMinGasPrices(app.minGasPrices).
WithBlockHeight(height).
WithGasMeter(storetypes.NewGasMeter(app.queryGasLimit))
WithGasMeter(storetypes.NewGasMeter(app.queryGasLimit)).WithBlockHeader(app.checkState.ctx.BlockHeader())

if height != lastBlockHeight {
rms, ok := app.cms.(*rootmulti.Store)
Expand Down
2 changes: 1 addition & 1 deletion baseapp/baseapp.go
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ func (app *BaseApp) setState(mode execMode, header cmtproto.Header) {
ms := app.cms.CacheMultiStore()
baseState := &state{
ms: ms,
ctx: sdk.NewContext(ms, header, false, app.logger).WithStreamingManager(app.streamingManager),
ctx: sdk.NewContext(ms, false, app.logger).WithStreamingManager(app.streamingManager).WithBlockHeader(header),
}

switch mode {
Expand Down
8 changes: 4 additions & 4 deletions baseapp/test_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,11 @@ func (app *BaseApp) SimTxFinalizeBlock(txEncoder sdk.TxEncoder, tx sdk.Tx) (sdk.
// NewContextLegacy returns a new sdk.Context with the provided header
func (app *BaseApp) NewContextLegacy(isCheckTx bool, header cmtproto.Header) sdk.Context {
if isCheckTx {
return sdk.NewContext(app.checkState.ms, header, true, app.logger).
WithMinGasPrices(app.minGasPrices)
return sdk.NewContext(app.checkState.ms, true, app.logger).
WithMinGasPrices(app.minGasPrices).WithBlockHeader(header)
}

return sdk.NewContext(app.finalizeBlockState.ms, header, false, app.logger)
return sdk.NewContext(app.finalizeBlockState.ms, false, app.logger).WithBlockHeader(header)
}

// NewContext returns a new sdk.Context with a empty header
Expand All @@ -66,7 +66,7 @@ func (app *BaseApp) NewContext(isCheckTx bool) sdk.Context {
}

func (app *BaseApp) NewUncachedContext(isCheckTx bool, header cmtproto.Header) sdk.Context {
return sdk.NewContext(app.cms, header, isCheckTx, app.logger)
return sdk.NewContext(app.cms, isCheckTx, app.logger).WithBlockHeader(header)
}

func (app *BaseApp) GetContextForFinalizeBlock(txBytes []byte) sdk.Context {
Expand Down
3 changes: 1 addition & 2 deletions tests/integration/bank/keeper/deterministic_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package keeper_test
import (
"testing"

cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
"gotest.tools/v3/assert"
"pgregory.net/rapid"

Expand Down Expand Up @@ -70,7 +69,7 @@ func initDeterministicFixture(t *testing.T) *deterministicFixture {
logger := log.NewTestLogger(t)
cms := integration.CreateMultiStore(keys, logger)

newCtx := sdk.NewContext(cms, cmtproto.Header{}, true, logger)
newCtx := sdk.NewContext(cms, true, logger)

authority := authtypes.NewModuleAddress("gov")

Expand Down
2 changes: 1 addition & 1 deletion tests/integration/distribution/keeper/msg_server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func initFixture(tb testing.TB) *fixture {
logger := log.NewTestLogger(tb)
cms := integration.CreateMultiStore(keys, logger)

newCtx := sdk.NewContext(cms, types.Header{}, true, logger)
newCtx := sdk.NewContext(cms, true, logger)

authority := authtypes.NewModuleAddress("gov")

Expand Down
3 changes: 1 addition & 2 deletions tests/integration/evidence/keeper/infraction_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"time"

abci "github.com/cometbft/cometbft/abci/types"
cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
"gotest.tools/v3/assert"

"cosmossdk.io/collections"
Expand Down Expand Up @@ -90,7 +89,7 @@ func initFixture(tb testing.TB) *fixture {
logger := log.NewTestLogger(tb)
cms := integration.CreateMultiStore(keys, logger)

newCtx := sdk.NewContext(cms, cmtproto.Header{}, true, logger)
newCtx := sdk.NewContext(cms, true, logger)

authority := authtypes.NewModuleAddress("gov")

Expand Down
3 changes: 1 addition & 2 deletions tests/integration/gov/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package keeper_test
import (
"testing"

cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
"gotest.tools/v3/assert"

"cosmossdk.io/core/appmodule"
Expand Down Expand Up @@ -58,7 +57,7 @@ func initFixture(tb testing.TB) *fixture {
logger := log.NewTestLogger(tb)
cms := integration.CreateMultiStore(keys, logger)

newCtx := sdk.NewContext(cms, cmtproto.Header{}, true, logger)
newCtx := sdk.NewContext(cms, true, logger)

authority := authtypes.NewModuleAddress(types.ModuleName)

Expand Down
3 changes: 1 addition & 2 deletions tests/integration/slashing/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"testing"
"time"

cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
"github.com/stretchr/testify/require"
"gotest.tools/v3/assert"

Expand Down Expand Up @@ -60,7 +59,7 @@ func initFixture(tb testing.TB) *fixture {
logger := log.NewTestLogger(tb)
cms := integration.CreateMultiStore(keys, logger)

newCtx := sdk.NewContext(cms, cmtproto.Header{}, true, logger)
newCtx := sdk.NewContext(cms, true, logger)

authority := authtypes.NewModuleAddress("gov")

Expand Down
3 changes: 1 addition & 2 deletions tests/integration/staking/keeper/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"math/big"
"testing"

cmtprototypes "github.com/cometbft/cometbft/proto/tendermint/types"
"gotest.tools/v3/assert"

"cosmossdk.io/core/appmodule"
Expand Down Expand Up @@ -103,7 +102,7 @@ func initFixture(tb testing.TB) *fixture {
logger := log.NewTestLogger(tb)
cms := integration.CreateMultiStore(keys, logger)

newCtx := sdk.NewContext(cms, cmtprototypes.Header{}, true, logger)
newCtx := sdk.NewContext(cms, true, logger)

authority := authtypes.NewModuleAddress("gov")

Expand Down
2 changes: 1 addition & 1 deletion tests/integration/staking/keeper/deterministic_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ func initDeterministicFixture(t *testing.T) *deterministicFixture {
logger := log.NewTestLogger(t)
cms := integration.CreateMultiStore(keys, logger)

newCtx := sdk.NewContext(cms, cmtproto.Header{}, true, logger)
newCtx := sdk.NewContext(cms, true, logger)

authority := authtypes.NewModuleAddress("gov")

Expand Down
7 changes: 3 additions & 4 deletions testutil/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"testing"
"time"

cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
dbm "github.com/cosmos/cosmos-db"
"github.com/stretchr/testify/assert"

Expand All @@ -26,7 +25,7 @@ func DefaultContext(key, tkey storetypes.StoreKey) sdk.Context {
if err != nil {
panic(err)
}
ctx := sdk.NewContext(cms, cmtproto.Header{}, false, log.NewNopLogger())
ctx := sdk.NewContext(cms, false, log.NewNopLogger())

return ctx
}
Expand Down Expand Up @@ -58,7 +57,7 @@ func DefaultContextWithKeys(
panic(err)
}

return sdk.NewContext(cms, cmtproto.Header{}, false, log.NewNopLogger())
return sdk.NewContext(cms, false, log.NewNopLogger())
}

type TestContext struct {
Expand All @@ -76,7 +75,7 @@ func DefaultContextWithDB(tb testing.TB, key, tkey storetypes.StoreKey) TestCont
err := cms.LoadLatestVersion()
assert.NoError(tb, err)

ctx := sdk.NewContext(cms, cmtproto.Header{Time: time.Now()}, false, log.NewNopLogger())
ctx := sdk.NewContext(cms, false, log.NewNopLogger()).WithBlockTime(time.Now())

return TestContext{ctx, db, cms}
}
5 changes: 2 additions & 3 deletions testutil/integration/example_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"fmt"
"io"

cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
"github.com/google/go-cmp/cmp"

"cosmossdk.io/core/appmodule"
Expand Down Expand Up @@ -38,7 +37,7 @@ func Example() {
logger := log.NewNopLogger()

cms := integration.CreateMultiStore(keys, logger)
newCtx := sdk.NewContext(cms, cmtproto.Header{}, true, logger)
newCtx := sdk.NewContext(cms, true, logger)

accountKeeper := authkeeper.NewAccountKeeper(
encodingCfg.Codec,
Expand Down Expand Up @@ -127,7 +126,7 @@ func Example_oneModule() {
logger := log.NewLogger(io.Discard)

cms := integration.CreateMultiStore(keys, logger)
newCtx := sdk.NewContext(cms, cmtproto.Header{}, true, logger)
newCtx := sdk.NewContext(cms, true, logger)

accountKeeper := authkeeper.NewAccountKeeper(
encodingCfg.Codec,
Expand Down
18 changes: 12 additions & 6 deletions types/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,21 +121,24 @@ func (c Context) Err() error {
}

// create a new context
func NewContext(ms storetypes.MultiStore, header cmtproto.Header, isCheckTx bool, logger log.Logger) Context {
// https://github.com/gogo/protobuf/issues/519
header.Time = header.Time.UTC()
func NewContext(ms storetypes.MultiStore, isCheckTx bool, logger log.Logger) Context {
h := cmtproto.Header{}
h.Time = h.Time.UTC()
return Context{
baseCtx: context.Background(),
ms: ms,
header: header,
chainID: header.ChainID,
header: h,
chainID: h.ChainID,
checkTx: isCheckTx,
logger: logger,
gasMeter: storetypes.NewInfiniteGasMeter(),
minGasPrice: DecCoins{},
eventManager: NewEventManager(),
kvGasConfig: storetypes.KVGasConfig(),
transientKVGasConfig: storetypes.TransientGasConfig(),
headerInfo: header.Info{
Time: h.Time.UTC(),
},
}
}

Expand All @@ -156,6 +159,9 @@ func (c Context) WithBlockHeader(header cmtproto.Header) Context {
// https://github.com/gogo/protobuf/issues/519
header.Time = header.Time.UTC()
c.header = header

// when calling withBlockheader on a new context chainID in the struct is empty
c.chainID = header.ChainID
Copy link
Member

Choose a reason for hiding this comment

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

Do we expect WithChainID to have the same behavior? (Set the chainID to the header?).
Currently in all SDK versions, it does not do it.

Copy link
Member Author

Choose a reason for hiding this comment

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

tbh this is weird, the chainID is in the header and on the struct, but they werent updated together, I think this is two bugs, first storing it twice second not updating or checking the other when updating one of them

Copy link
Member

@julienrbrt julienrbrt Aug 21, 2023

Choose a reason for hiding this comment

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

+1, I think this is a valid fix of WithBlockHeader / WithChainID that we can add to in least v0.50

return c
}

Expand Down Expand Up @@ -303,7 +309,7 @@ func (c Context) WithCometInfo(cometInfo comet.BlockInfo) Context {

// WithHeaderInfo returns a Context with an updated header info
func (c Context) WithHeaderInfo(headerInfo header.Info) Context {
// Settime to UTC
// Set time to UTC
headerInfo.Time = headerInfo.Time.UTC()
c.headerInfo = headerInfo
return c
Expand Down
13 changes: 6 additions & 7 deletions types/context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ func (s *contextTestSuite) TestContextWithCustom() {
ctrl := gomock.NewController(s.T())
s.T().Cleanup(ctrl.Finish)

header := cmtproto.Header{}
height := int64(1)
chainid := "chainid"
ischeck := true
Expand All @@ -97,8 +96,8 @@ func (s *contextTestSuite) TestContextWithCustom() {
headerHash := []byte("headerHash")
zeroGasCfg := storetypes.GasConfig{}

ctx = types.NewContext(nil, header, ischeck, logger)
s.Require().Equal(header, ctx.BlockHeader())
ctx = types.NewContext(nil, ischeck, logger)
s.Require().Equal(cmtproto.Header{}, ctx.BlockHeader())

ctx = ctx.
WithBlockHeight(height).
Expand Down Expand Up @@ -152,7 +151,7 @@ func (s *contextTestSuite) TestContextHeader() {
addr := secp256k1.GenPrivKey().PubKey().Address()
proposer := types.ConsAddress(addr)

ctx = types.NewContext(nil, cmtproto.Header{}, false, nil)
ctx = types.NewContext(nil, false, nil)

ctx = ctx.
WithBlockHeight(height).
Expand All @@ -166,7 +165,7 @@ func (s *contextTestSuite) TestContextHeader() {

func (s *contextTestSuite) TestWithBlockTime() {
now := time.Now()
ctx := types.NewContext(nil, cmtproto.Header{}, false, nil)
ctx := types.NewContext(nil, false, nil)
ctx = ctx.WithBlockTime(now)
cmttime2 := cmttime.Canonical(now)
s.Require().Equal(ctx.BlockTime(), cmttime2)
Expand Down Expand Up @@ -215,7 +214,7 @@ func (s *contextTestSuite) TestContextHeaderClone() {
for name, tc := range cases {
tc := tc
s.T().Run(name, func(t *testing.T) {
ctx := types.NewContext(nil, tc.h, false, nil)
ctx := types.NewContext(nil, false, nil).WithBlockHeader(tc.h)
s.Require().Equal(tc.h.Height, ctx.BlockHeight())
s.Require().Equal(tc.h.Time.UTC(), ctx.BlockTime())

Expand All @@ -229,7 +228,7 @@ func (s *contextTestSuite) TestContextHeaderClone() {
}

func (s *contextTestSuite) TestUnwrapSDKContext() {
sdkCtx := types.NewContext(nil, cmtproto.Header{}, false, nil)
sdkCtx := types.NewContext(nil, false, nil)
ctx := types.WrapSDKContext(sdkCtx)
sdkCtx2 := types.UnwrapSDKContext(ctx)
s.Require().Equal(sdkCtx, sdkCtx2)
Expand Down
5 changes: 2 additions & 3 deletions types/mempool/mempool_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"math/rand"
"testing"

cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"
protov2 "google.golang.org/protobuf/proto"
Expand Down Expand Up @@ -132,7 +131,7 @@ func fetchTxs(iterator mempool.Iterator, maxBytes int64) []sdk.Tx {

func (s *MempoolTestSuite) TestDefaultMempool() {
t := s.T()
ctx := sdk.NewContext(nil, cmtproto.Header{}, false, log.NewNopLogger())
ctx := sdk.NewContext(nil, false, log.NewNopLogger())
accounts := simtypes.RandomAccounts(rand.New(rand.NewSource(0)), 10)
txCount := 1000
var txs []testTx
Expand Down Expand Up @@ -224,7 +223,7 @@ func TestMempoolTestSuite(t *testing.T) {
}

func (s *MempoolTestSuite) TestSampleTxs() {
ctxt := sdk.NewContext(nil, cmtproto.Header{}, false, log.NewNopLogger())
ctxt := sdk.NewContext(nil, false, log.NewNopLogger())
t := s.T()
s.resetMempool()
mp := s.mempool
Expand Down
Loading