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

chore: making changes based on nits from 02-client refactor audit #1585

Conversation

chatton
Copy link
Contributor

@chatton chatton commented Jun 27, 2022

Description

closes: #1581


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

[]metrics.Label{telemetry.NewLabel(types.LabelClientType, clientState.ClientType())},
)
}()
defer telemetry.IncrCounterWithLabels(
Copy link
Contributor Author

@chatton chatton Jun 27, 2022

Choose a reason for hiding this comment

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

this was not mentioned in the audit however we are wrapping these calls with an anonymous function which is not required unless we want to not evaluate clientState.ClientType() until function execution for some reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, nice catch. I think then this improvement could be applied to other places of the codebase as well.

@@ -26,7 +28,13 @@ func EmitCreateClientEvent(ctx sdk.Context, clientID string, clientState exporte
}

// EmitUpdateClientEvent emits an update client event
func EmitUpdateClientEvent(ctx sdk.Context, clientID string, clientType string, consensusHeights []exported.Height, clientMsgStr string) {
func EmitUpdateClientEvent(ctx sdk.Context, clientID string, clientType string, consensusHeights []exported.Height, cdc codec.BinaryCodec, clientMsg exported.ClientMessage) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cdc codec.BinaryCodec was added as an argument to prevent this function being added to the Keeper or accepting a keeper as an argument

@chatton chatton marked this pull request as ready for review June 27, 2022 10:21
@chatton chatton requested review from damiannolan and seantking June 27, 2022 10:22
Copy link
Contributor

@crodriguezvega crodriguezvega left a comment

Choose a reason for hiding this comment

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

LGTM!

[]metrics.Label{telemetry.NewLabel(types.LabelClientType, clientState.ClientType())},
)
}()
defer telemetry.IncrCounterWithLabels(
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, nice catch. I think then this improvement could be applied to other places of the codebase as well.

@@ -198,6 +178,9 @@ func (cs ClientState) pruneOldestConsensusState(ctx sdk.Context, cdc codec.Binar
// this error should never occur
if !found {
pruneError = sdkerrors.Wrapf(clienttypes.ErrConsensusStateNotFound, "failed to retrieve consensus state at height: %s", height)
if pruneError != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can safely remove the if condition because pruneError will always be not nil here, right?

Copy link
Member

@damiannolan damiannolan left a comment

Choose a reason for hiding this comment

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

Thanks @chatton! 🚀

modules/core/02-client/keeper/events.go Outdated Show resolved Hide resolved
Co-authored-by: Damian Nolan <damiannolan@gmail.com>
@chatton chatton merged commit aab9ca2 into 02-client-refactor Jun 30, 2022
@chatton chatton deleted the cian/issue#1581-nits-from-first-02-client-refactor-audit- branch June 30, 2022 14:32
oshorefueled pushed a commit to ComposableFi/ibc-go that referenced this pull request Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants