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

fix(hubl): revert f235aae #16534

Merged
merged 6 commits into from
Jun 15, 2023
Merged

fix(hubl): revert f235aae #16534

merged 6 commits into from
Jun 15, 2023

Conversation

julienrbrt
Copy link
Member

@julienrbrt julienrbrt commented Jun 13, 2023

Description

Closes: #16533

reverts f235aae.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@julienrbrt julienrbrt requested a review from a team as a code owner June 13, 2023 21:57
@github-prbot github-prbot requested review from a team and kocubinski and removed request for a team June 13, 2023 21:57
@github-actions github-actions bot added the C:Hubl Tool: Hubl label Jun 13, 2023
@julienrbrt julienrbrt added backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release C:Hubl Tool: Hubl and removed C:Hubl Tool: Hubl labels Jun 13, 2023
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

what is the reason for this revert, do we know why its broken?

@julienrbrt
Copy link
Member Author

what is the reason for this revert, do we know why its broken?

See the issue. It breaks hubl currently because apps do not support reflection v1.

@tac0turtle
Copy link
Member

could we do something that new apps use v1 and old apps use v1alpha. I think its good to try to migrate users to v1 instead of keeping them on v1alpha

@julienrbrt
Copy link
Member Author

could we do something that new apps use v1 and old apps use v1alpha. I think its good to try to migrate users to v1 instead of keeping them on v1alpha

I'll add a fallback here then.

@julienrbrt julienrbrt changed the title fix: revert f235aae fix(hubl): fallback to reflection v1beta1 Jun 14, 2023
@julienrbrt julienrbrt removed the backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release label Jun 14, 2023
@julienrbrt
Copy link
Member Author

Can't convert to draft on a phone, I'll do it when I grab my laptop.

@tac0turtle tac0turtle marked this pull request as draft June 14, 2023 09:19
@julienrbrt
Copy link
Member Author

julienrbrt commented Jun 14, 2023

could we do something that new apps use v1 and old apps use v1alpha. I think its good to try to migrate users to v1 instead of keeping them on v1alpha

So I have looked a bit more at this and I do not think it is useful, using grpc reflection like that is a fallback for apps that do not support cosmos.reflection.v1 endpoints, so new apps will not have this problem anyway.
Is that a good assumption @aaronc?

@julienrbrt julienrbrt changed the title fix(hubl): fallback to reflection v1beta1 fix: revert f235aae Jun 14, 2023
@julienrbrt julienrbrt added the backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release label Jun 14, 2023
@julienrbrt julienrbrt marked this pull request as ready for review June 14, 2023 11:23
@github-prbot github-prbot requested a review from a team June 14, 2023 11:23
@tac0turtle
Copy link
Member

i dont quite follow, if its not a problem with new chains then why revert?

@julienrbrt
Copy link
Member Author

i dont quite follow, if its not a problem with new chains then why revert?

Because it is a problem with chains that support only grpc relfection v1alpha1 and not the cosmos.reflection.v1.
No chains support grpc reflection v1, so this is why imho we do not need a fallback but simply revert, as if a chain supports cosmos.reflection.v1, we use it instead of grpc reflection v1alpha (so v1alpha1 is already the fallback).

@tac0turtle
Copy link
Member

im leaning more towards leaving this and we can backport something for hubl in particular for older chains. It would be nice to use v1 instead here, otherwise we create an issue and add tech debt for future cleanup

@julienrbrt julienrbrt changed the title fix: revert f235aae fix(hubl): revert f235aae Jun 14, 2023
@julienrbrt
Copy link
Member Author

im leaning more towards leaving this and we can backport something for hubl in particular for older chains. It would be nice to use v1 instead here, otherwise we create an issue and add tech debt for future cleanup

Right, this makes sense, reverted for hubl only here. Hubl will any use cosmos.reflection.v1 for chains that use grpc reflection v1.

@julienrbrt julienrbrt removed the backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release label Jun 14, 2023
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

utACK

@julienrbrt julienrbrt added this pull request to the merge queue Jun 15, 2023
Merged via the queue into main with commit 822e715 Jun 15, 2023
50 of 52 checks passed
@julienrbrt julienrbrt deleted the julien/revert-f235aae branch June 15, 2023 12:54
@julienrbrt
Copy link
Member Author

Hubl works again from this merge commit (822e715):

$ hubl version
hubl version: v0.0.0-20230615125140-822e71585288

@bizk
Copy link
Contributor

bizk commented Jun 16, 2023

Amazing!

@julienrbrt julienrbrt mentioned this pull request Jun 26, 2023
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:Hubl Tool: Hubl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: HUBL "unknown service grpc.reflection.v1.ServerReflection" panic error
3 participants