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

feat: emitting an event when handling a client upgrade proposal #1570

Merged
merged 7 commits into from
Jun 27, 2022
Merged
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (app/29-fee) [\#1305](https://github.com/cosmos/ibc-go/pull/1305) Change version string for fee module to `ics29-1`
* (app/29-fee) [\#1341](https://github.com/cosmos/ibc-go/pull/1341) Check if the fee module is locked and if the fee module is enabled before refunding all fees
* (testing/simapp) [\#1397](https://github.com/cosmos/ibc-go/pull/1397) Adding mock module to maccperms and adding check to ensure mock module is not a blocked account address.
* (core/02-client) [\#1570](https://github.com/cosmos/ibc-go/pull/1570) Emitting an event when handling an upgrade client proposal.

### Features

Expand Down
13 changes: 13 additions & 0 deletions modules/core/02-client/keeper/events.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package keeper

import (
"fmt"

sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/cosmos/ibc-go/v3/modules/core/02-client/types"
Expand Down Expand Up @@ -68,6 +70,17 @@ func EmitUpdateClientProposalEvent(ctx sdk.Context, clientID string, clientState
)
}

// EmitUpgradeClientProposalEvent emits an upgrade client proposal event
func EmitUpgradeClientProposalEvent(ctx sdk.Context, title string, height int64) {
ctx.EventManager().EmitEvent(
sdk.NewEvent(
types.EventTypeUpgradeClientProposal,
sdk.NewAttribute(types.AttributeKeyUpgradePlanTitle, title),
sdk.NewAttribute(types.AttributeKeyUpgradePlanHeight, fmt.Sprintf("%d", height)),
),
)
}

// EmitSubmitMisbehaviourEvent emits a client misbehaviour event
func EmitSubmitMisbehaviourEvent(ctx sdk.Context, clientID string, clientState exported.ClientState) {
ctx.EventManager().EmitEvent(
Expand Down
9 changes: 8 additions & 1 deletion modules/core/02-client/keeper/proposal.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,5 +98,12 @@ func (k Keeper) HandleUpgradeProposal(ctx sdk.Context, p *types.UpgradeProposal)

// sets the new upgraded client in last height committed on this chain is at plan.Height,
// since the chain will panic at plan.Height and new chain will resume at plan.Height
return k.upgradeKeeper.SetUpgradedClient(ctx, p.Plan.Height, bz)
if err = k.upgradeKeeper.SetUpgradedClient(ctx, p.Plan.Height, bz); err != nil {
return err
}

// emitting an event for handling client upgrade proposal
EmitUpgradeClientProposalEvent(ctx, p.Title, p.Plan.Height)
Copy link
Contributor

Choose a reason for hiding this comment

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

I really don't know what the right answer is, but should we emit the event only if k.upgradeKeeper.SetUpgradedClient(ctx, p.Plan.Height, bz) succeeds?

If not, should we first execute err := k.upgradeKeeper.SetUpgradedClient(ctx, p.Plan.Height, bz) and add to the event a string with the error from err in case this call fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea 🤝

Copy link
Contributor Author

@seantking seantking Jun 24, 2022

Choose a reason for hiding this comment

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

I think we should just emit the event if there is no error returned. The hermes relayer will only try to do the upgrade after picking up the event so I don't see why they would want to do that if the upgrade won't happen?

I am not 100% clear on how this upgrade flow operates though. I see we first call ScheduleUpgrade and then SetUpgradedClient. I'm assuming if SetUpgradeClient fails the upgrade won't go ahead?

cc: @AdityaSripal

Copy link
Member

Choose a reason for hiding this comment

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

Yes if the handler errors, then the state updates made during the handler gets reverted:

https://github.com/cosmos/cosmos-sdk/blob/main/x/gov/abci.go#L73


return nil
}
23 changes: 13 additions & 10 deletions modules/core/02-client/types/events.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,23 @@ import (

// IBC client events
const (
AttributeKeyClientID = "client_id"
AttributeKeySubjectClientID = "subject_client_id"
AttributeKeyClientType = "client_type"
AttributeKeyConsensusHeight = "consensus_height"
AttributeKeyHeader = "header"
AttributeKeyClientID = "client_id"
AttributeKeySubjectClientID = "subject_client_id"
AttributeKeyClientType = "client_type"
AttributeKeyConsensusHeight = "consensus_height"
AttributeKeyHeader = "header"
AttributeKeyUpgradePlanTitle = "title"
AttributeKeyUpgradePlanHeight = "height"
)

// IBC client events vars
var (
EventTypeCreateClient = "create_client"
EventTypeUpdateClient = "update_client"
EventTypeUpgradeClient = "upgrade_client"
EventTypeSubmitMisbehaviour = "client_misbehaviour"
EventTypeUpdateClientProposal = "update_client_proposal"
EventTypeCreateClient = "create_client"
EventTypeUpdateClient = "update_client"
EventTypeUpgradeClient = "upgrade_client"
EventTypeSubmitMisbehaviour = "client_misbehaviour"
EventTypeUpdateClientProposal = "update_client_proposal"
EventTypeUpgradeClientProposal = "upgrade_client_proposal"

AttributeValueCategory = fmt.Sprintf("%s_%s", host.ModuleName, SubModuleName)
)