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

ibc: protobuf v1 #7432

Merged
merged 3 commits into from
Oct 1, 2020
Merged

ibc: protobuf v1 #7432

merged 3 commits into from
Oct 1, 2020

Conversation

fedekunze
Copy link
Collaborator

@fedekunze fedekunze commented Oct 1, 2020

closes #7338

@codecov
Copy link

codecov bot commented Oct 1, 2020

Codecov Report

Merging #7432 into master will decrease coverage by 0.10%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master    #7432      +/-   ##
==========================================
- Coverage   55.17%   55.07%   -0.11%     
==========================================
  Files         435      588     +153     
  Lines       28783    36760    +7977     
==========================================
+ Hits        15881    20245    +4364     
- Misses      11302    14417    +3115     
- Partials     1600     2098     +498     

@colin-axner
Copy link
Contributor

we need to update these too right?

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

lgtm for now, yeah would be good to update the interfaces too to cosmos.*

@fedekunze
Copy link
Collaborator Author

@colin-axner @amaurymartiny updated the codec too 👍

@@ -11,11 +11,11 @@ import (
// Any.
func RegisterInterfaces(registry codectypes.InterfaceRegistry) {
registry.RegisterInterface(
"cosmos_sdk.ibc.v1.connection.ConnectionI",
"ibc.core.connection.v1.ConnectionI",
Copy link
Collaborator Author

@fedekunze fedekunze Oct 1, 2020

Choose a reason for hiding this comment

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

not sure if we should rename this to just Connection instead of ConnectionI

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be fine renaming. exported.Connection should imply it is an interface

Copy link
Contributor

Choose a reason for hiding this comment

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

though ideally we would do this with all interfaces since it seems the channel has the same format

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

nice!

@fedekunze fedekunze added the A:automerge Automatically merge PR once all prerequisites pass. label Oct 1, 2020
@colin-axner
Copy link
Contributor

could we wait to merge 🙏 I don't want conflicts stacking up on my handshake pr. The connection handshake changes should be r4r

Copy link
Collaborator

@anilcse anilcse left a comment

Choose a reason for hiding this comment

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

utACK

@@ -9,19 +9,19 @@ import (
// RegisterInterfaces registers the commitment interfaces to protobuf Any.
func RegisterInterfaces(registry codectypes.InterfaceRegistry) {
registry.RegisterInterface(
"cosmos.ibc.commitment.Root",
"ibc.core.commitment.v1.Root",
Copy link
Collaborator

Choose a reason for hiding this comment

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

😨

@mergify mergify bot merged commit 53f8aec into master Oct 1, 2020
@mergify mergify bot deleted the fedekunze/7338-proto-ibc-v1 branch October 1, 2020 19:23
@anilcse
Copy link
Collaborator

anilcse commented Oct 1, 2020

could we wait to merge I don't want conflicts stacking up on my handshake pr. The connection handshake changes should be r4r

Ah sorry, I didn't check the automerge label. Should we revert this? @fedekunze
cc @colin-axner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:Encoding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Determine proto definition versioning for IBC
4 participants