Emit event upon setting upgrade consensus state #1741
Conversation
chatton
requested review from
colin-axner,
fedekunze,
AdityaSripal,
crodriguezvega,
seantking,
charleenfei and
damiannolan
as code owners
| ctx.EventManager().EmitEvents(sdk.Events{ | ||
| sdk.NewEvent( | ||
| types.EventTypeUpgradeChain, | ||
| sdk.NewAttribute(types.AttributeKeyUpgradePlanHeight, strconv.Itoa(int(height))), |
There was a problem hiding this comment.
In this PR Fede recommended to use strconv.FormatUint for converting uint64 to string. Could we not use strconv.FormatInt instead here? It's not like I have a strong opinion for using one or the other way, but I guess it would be nice to settle in one (if possible) and use that always. :)
There was a problem hiding this comment.
The type is int64 rather than uint64 but there is also a strconv.FormatInt(height, 10) we can use instead!
modules/core/02-client/abci_test.go
Outdated
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| "github.com/stretchr/testify/suite" | ||
| abci "github.com/tendermint/tendermint/abci/types" | ||
| tmproto "github.com/tendermint/tendermint/proto/tendermint/types" | ||
|
|
||
| upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types" |
There was a problem hiding this comment.
nit: should we group upgradetypes with the above?
modules/core/02-client/abci_test.go
Outdated
| @@ -74,3 +77,55 @@ func (suite *ClientTestSuite) TestBeginBlockerConsensusState() { | |||
| suite.Require().NoError(err) | |||
| suite.Require().Equal(bz, consState) | |||
| } | |||
|
|
|||
| func (suite *ClientTestSuite) TestBeginBlockerWithUpgradePlan_EmitsUpgradeChainEvent() { | |||
There was a problem hiding this comment.
nit: I don't think underscores in test names are convention in Go, right?
Could use TestBeginBlockerUpgradeEvents and TestBeginBlockerUpgradeEventsAbsence?
But I mean its probably fine to be honest, the names are meaningful as is!
There was a problem hiding this comment.
yeah no harm simplifying the name a bit, great suggestion!
chatton
deleted the
cian/issue#270-emit-event-upon-setting-upgrade-consensus-state-2
branch
| cacheCtx, writeCache := suite.chainA.GetContext().CacheContext() | ||
|
|
||
| client.BeginBlocker(cacheCtx, suite.chainA.App.GetIBCKeeper().ClientKeeper) | ||
| writeCache() | ||
|
|
||
| suite.requireContainsEvent(cacheCtx.EventManager().Events(), types.EventTypeUpgradeChain, true) |
There was a problem hiding this comment.
just as a note, the caching of the context should be unnecessary in this situation. You should be able to get the event via suite.chainA.GetContext() after calling begin blocker on that context
crodriguezvega
deleted the
cian/issue#270-emit-event-upon-setting-upgrade-consensus-state-2
branch
Description
This PR updates
BeginBlockerto update anupgrade_chainevent.closes: #270
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes