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.

Sign up for GitHub

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

Jump to bottom

x/ibc: proto migration pt 1 #6097

Merged
merged 33 commits into from
May 6, 2020
Merged

x/ibc: proto migration pt 1 #6097

merged 33 commits into from
May 6, 2020

Conversation

fedekunze
Copy link
Collaborator

@fedekunze fedekunze commented Apr 29, 2020 •
edited
Loading

Description

Splits #5704 to migrate the following IBC packages that are not affected by tendermint breaking changes:

  • x/ibc/03-connection
  • x/ibc/04-channel
  • x/ibc/05-port
  • x/ibc/20-transfer
  • x/ibc/23-commitment
  • x/ibc/types

For contributor use:

  • 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

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@codecov
Copy link

codecov bot commented Apr 29, 2020 •
edited
Loading

Codecov Report

Merging #6097 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #6097   +/-   ##
=======================================
  Coverage   54.69%   54.69%           
=======================================
  Files         441      441           
  Lines       26576    26576           
=======================================
  Hits        14537    14537           
  Misses      11020    11020           
  Partials     1019     1019

@fedekunze fedekunze marked this pull request as ready for review April 29, 2020 17:51
alexanderbez
alexanderbez reviewed Apr 29, 2020
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
x/ibc/03-connection/keeper/keeper.go Show resolved Hide resolved
x/ibc/03-connection/keeper/keeper_test.go Show resolved Hide resolved
x/ibc/03-connection/keeper/verify.go Show resolved Hide resolved
x/ibc/03-connection/types/types.proto Show resolved Hide resolved
aaronc
aaronc requested changes Apr 29, 2020
View reviewed changes
std/codec.proto Outdated Show resolved Hide resolved
buf.yaml Outdated Show resolved Hide resolved
@fedekunze fedekunze requested a review from clevinson as a code owner May 4, 2020 13:20
@fedekunze fedekunze requested a review from aaronc May 4, 2020 13:33
@fedekunze
Copy link
Collaborator Author

I need to fix a few tests

@fedekunze fedekunze added WIP and removed R4R labels May 4, 2020
@fedekunze fedekunze marked this pull request as draft May 4, 2020 16:23
@jackzampolin
Copy link
Member

Want to try getting a relayer running with this code.

aaronc
aaronc reviewed May 5, 2020
View reviewed changes
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

Just for readability's sake, we should have comments on every field that takes an Any in the .proto files indicating what the interface type is. Also on the implementations, we should have comments that indicate which interfaces they satisfy

alexanderbez
alexanderbez reviewed May 5, 2020
View reviewed changes
x/ibc/02-client/exported/codec.go Outdated Show resolved Hide resolved
x/ibc/03-connection/keeper/keeper.go Show resolved Hide resolved
x/ibc/03-connection/keeper/keeper.go Show resolved Hide resolved
x/ibc/03-connection/keeper/keeper.go Outdated Show resolved Hide resolved
x/ibc/03-connection/types/codec.go Outdated Show resolved Hide resolved
x/ibc/03-connection/types/codec.go Outdated Show resolved Hide resolved
x/ibc/04-channel/keeper/keeper.go Outdated Show resolved Hide resolved
@fedekunze
Copy link
Collaborator Author

@alexanderbez @aaronc Addressed all the comments and removed the single interface proto type

cwgoes
cwgoes reviewed May 6, 2020
View reviewed changes
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

I'm not super familiar with the latest proto work, but the basic structure here looks fine to me.

Just a note that the proofs will need to change to be non-SDK-specific once the ICS 23 work is ready.

alexanderbez
alexanderbez approved these changes May 6, 2020
View reviewed changes
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

utACK. Great work @fedekunze!

@alexanderbez alexanderbez added the A:automerge Automatically merge PR once all prerequisites pass. label May 6, 2020
@fedekunze fedekunze merged commit d7ebee7 into master May 6, 2020
@fedekunze fedekunze deleted the ibc-proto-1 branch May 6, 2020 15:17
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
@fedekunze @alexanderbez @aaronc
x/ibc: proto migration pt 1 (cosmos#6097)
773881a 
* x/ibc: proto migration pt 1

* fix tests

* fixes and godoc

* yaml tags

* rm changelog

* address comments from review

* fix some tests

* fix tests

* add _UNSPECIFIED suffix for default enums

* Update app

* protobuf Any fixes

* use gogoproto

* fix tests

* wrap all messages

* address @alexanderbez comments

* update proto files

Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
Co-authored-by: Aleksandr Bezobchuk <aleks.bezobchuk@gmail.com>
Co-authored-by: Aaron Craelius <aaron@regen.network>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cwgoes cwgoes cwgoes left review comments

@alexanderbez alexanderbez alexanderbez approved these changes

@alessio alessio Awaiting requested review from alessio

@hschoenburg hschoenburg Awaiting requested review from hschoenburg

@nylira nylira Awaiting requested review from nylira

@AdityaSripal AdityaSripal Awaiting requested review from AdityaSripal

@clevinson clevinson Awaiting requested review from clevinson

@jackzampolin jackzampolin Awaiting requested review from jackzampolin

@zmanian zmanian Awaiting requested review from zmanian

@aaronc aaronc Awaiting requested review from aaronc

Assignees

@fedekunze fedekunze

Labels
A:automerge Automatically merge PR once all prerequisites pass. C:Encoding
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fedekunze @alexanderbez @jackzampolin @aaronc @cwgoes