feat: emitting an event when handling a client upgrade proposal

[seantking]

Description

closes: #1558

---

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.

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer
• Review Codecov Report in the comment section below once CI passes

[crodriguezvega]

Thanks for picking this up, @seantking!

[crodriguezvega]

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?

[seantking]

Good idea 🤝

[seantking]

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

[AdityaSripal]

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

[damiannolan]

Just left a small nit about formatting and code style.

[damiannolan]

nit: sprintf formatting

Suggested change

	sdk.NewAttribute(types.AttributeKeyUpgradePlanHeight, fmt.Sprint(height)),
	sdk.NewAttribute(types.AttributeKeyUpgradePlanHeight, fmt.Sprintf("%d", height)),

[damiannolan]

I think this would be more "idiomatic" go to return early with the err and then we could emit the event before returning nil.

Something like below:

Suggested change

	err = k.upgradeKeeper.SetUpgradedClient(ctx, p.Plan.Height, bz)
	if err == nil {
	// emitting an event for handling client upgrade proposal
	EmitUpgradeClientProposalEvent(ctx, p.Title, p.Plan.Height)
	}
	return err
	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)
	return nil