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: rename 07-tendermint type Misbehaviour to ConflictingHeaders #1090

Closed

Conversation

colin-axner
Copy link
Contributor

@colin-axner colin-axner commented Mar 9, 2022

Description

ref: #879


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

message Misbehaviour {
// ConflictingHeaders is a wrapper over two conflicting Headers
// that implements Header interface expected by ICS-02
message ConflictingHeaders {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AdityaSripal thoughts on the naming here?

We could potentially leave this as Misbehaviour?

I'm still have some concerns about merging the Misbehaviour and Header interface, mostly around naming. It'd be nice if we could align with the spec as well

We know a normal Header can lead to a valid client update, or be evidence of misbehaviour. We also know there can be another structure which can show misbehaviour (in this case duplicate header), but I imagine a light client could have multiple of these structures. My main concern is that Header is usually associated with the header produced by a blockchain, but the interface we are using expects potentially one or more headers to be passed in to update the IBC client (either to a valid state or to freeze it)

Copy link
Member

Choose a reason for hiding this comment

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

We can just leave this as misbehavior in my opinion.

Yes this is odd naming, especially given that it conflicts with the semantic definition of Header in the regular blockchain case.

I would prefer we give a different name to the superset interface that encompasses the regular headers, batched headers, and misbehaviour.

Perhaps something generic like 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.

Oh that's great. I'm in favor!

@colin-axner
Copy link
Contributor Author

Leaving as misbehaviour as @AdityaSripal suggests

@colin-axner colin-axner deleted the colin/879-misbehaviour-renaming branch March 10, 2022 11:26
CosmosCar pushed a commit to caelus-labs/ibc-go that referenced this pull request Nov 6, 2023
<!--
Please read and fill out this form before submitting your PR.

Please make sure you have reviewed our contributors guide before
submitting your
first PR.
-->

## Overview

<!-- 
Please provide an explanation of the PR, including the appropriate
context,
background, goal, and rationale. If there is an issue with this
information,
please provide a tl;dr and link the issue. 
-->

## Checklist

<!-- 
Please complete the checklist to ensure that the PR is ready to be
reviewed.

IMPORTANT:
PRs should be left in Draft until the below checklist is completed.
-->

- [ ] New and updated code has appropriate documentation
- [ ] New and updated code has new and/or updated testing
- [ ] Required CI checks are passing
- [ ] Visual proof for any user facing features like CLI or
documentation updates
- [ ] Linked issues closed with keywords

Co-authored-by: Ganesha Upadhyaya <[email protected]>
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.

2 participants