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

x/ibc-transfer: move ICS20 out from x/ibc #6222

Merged
merged 6 commits into from
May 15, 2020
Merged

Conversation

fedekunze
Copy link
Collaborator

@fedekunze fedekunze commented May 14, 2020

Description

closes: #6092

@auto-comment
Copy link

auto-comment bot commented May 15, 2020

👋 Thanks for creating a PR!

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.


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)

Thank you for your contribution to the Cosmos-SDK! 🚀

@codecov
Copy link

codecov bot commented May 15, 2020

Codecov Report

Merging #6222 into master will decrease coverage by 0.07%.
The diff coverage is 94.59%.

@@            Coverage Diff             @@
##           master    #6222      +/-   ##
==========================================
- Coverage   54.83%   54.75%   -0.08%     
==========================================
  Files         444      447       +3     
  Lines       26797    26894      +97     
==========================================
+ Hits        14693    14726      +33     
- Misses      11061    11124      +63     
- Partials     1043     1044       +1     

@fedekunze fedekunze added the A:automerge Automatically merge PR once all prerequisites pass. label May 15, 2020
@tac0turtle
Copy link
Member

should this be a submodule of bank? As a user at first glance it is hard to kow what the difference between bank and transfer is. will comment in the issue as well

@alexanderbez
Copy link
Contributor

I don't think a sub-module of x/bank is the best idea, but I do agree x/transfer is generic and would cause a great deal of confusion with the already existing x/bank. I'd recommend something like x/ibc-transfer.

@fedekunze fedekunze changed the title x/transfer: move ICS20 out from x/ibc x/ibc-transfer: move ICS20 out from x/ibc May 15, 2020
@fedekunze
Copy link
Collaborator Author

renamed to ibc-transfer

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.

LGTM

@@ -0,0 +1,311 @@
package transfer_test

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a bit of commented-out code in this test file. I'd recommend removing it if it's not intended to be re-enabled.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, there are a few places with that code commented. We can do that cleanup in #5558

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.

LGTM, thanks for changing to ibc-transfer

@mergify mergify bot merged commit 3dcdc58 into master May 15, 2020
@mergify mergify bot deleted the fedekunze/6019-move-transfer branch May 15, 2020 20:36
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
* x/transfer: move ICS20 out from x/ibc

* rename to ibc-transfer
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

move transfer outside x/ibc as a standalone module
3 participants