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

Bind Transfer Port on InitChain #5954

Merged
merged 9 commits into from
Apr 8, 2020
Merged

Bind Transfer Port on InitChain #5954

merged 9 commits into from
Apr 8, 2020

Conversation

AdityaSripal
Copy link
Member

@AdityaSripal AdityaSripal commented Apr 8, 2020 •
edited
Loading

Resolves: cosmos/relayer#80 (comment)

Description

#5888 did not bind the "bank" port for the transfer module on setup. This corrects that issue by calling BindPort on InitChain

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)

@AdityaSripal AdityaSripal changed the base branch from master to ibc-alpha April 8, 2020 03:03
AdityaSripal
AdityaSripal commented Apr 8, 2020
View reviewed changes
types/module/module.go Outdated Show resolved Hide resolved
@AdityaSripal AdityaSripal added x/ibc and removed R4R labels Apr 8, 2020
cwgoes
cwgoes approved these changes Apr 8, 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.

utACK

types/module/module.go Outdated Show resolved Hide resolved
@jackzampolin
Copy link
Member

This looks great, and @AdityaSripal you are AWESOME! Some failing tests and a failing sim.

@jackzampolin
Copy link
Member

This hasn't fully fixed the issue and is also causing a number of test failures around genesis behavior on both the SDK and gaia.

@jackzampolin
Copy link
Member

Ok debugging now. We have found an issue where failed transactions still grab the capability. We need to roll that back as well if the transaction fails.

golangcibot
golangcibot reviewed Apr 8, 2020
View reviewed changes
x/ibc/20-transfer/keeper/keeper.go Outdated Show resolved Hide resolved
x/ibc/20-transfer/keeper/keeper.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 8, 2020 •
edited
Loading

Codecov Report

Merging #5954 into ibc-alpha will increase coverage by 0.04%.
The diff coverage is 84.37%.

@@              Coverage Diff              @@
##           ibc-alpha    #5954      +/-   ##
=============================================
+ Coverage      58.37%   58.42%   +0.04%     
=============================================
  Files            396      396              
  Lines          23510    23527      +17     
=============================================
+ Hits           13725    13745      +20     
+ Misses          8827     8821       -6     
- Partials         958      961       +3

AdityaSripal
AdityaSripal commented Apr 8, 2020
View reviewed changes
simapp/app.go Outdated Show resolved Hide resolved
x/capability/keeper/keeper.go Show resolved Hide resolved
x/capability/keeper/keeper_test.go Show resolved Hide resolved
x/capability/keeper/keeper_test.go Show resolved Hide resolved
x/ibc/20-transfer/genesis.go Show resolved Hide resolved
@AdityaSripal AdityaSripal requested a review from cwgoes April 8, 2020 20:44
fedekunze
fedekunze suggested changes Apr 8, 2020
View reviewed changes
Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

utACK. Minor changes. Pending lint fixes and godoc comments

simapp/app.go Outdated Show resolved Hide resolved
x/ibc/20-transfer/genesis.go Show resolved Hide resolved
x/ibc/20-transfer/types/keys.go
@@ -18,7 +18,7 @@ const (
Version = "ics20-1"

// PortID that transfer module binds to
PortID = "bank"
PortID = "transfer"
Copy link
Collaborator

Choose a reason for hiding this comment

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

double-checking if is this correct?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@AdityaSripal ^

Copy link
Member

Choose a reason for hiding this comment

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

Yes this is good. I prefer transfer to bank tbh.

x/ibc/keeper/keeper.go Show resolved Hide resolved
@AdityaSripal AdityaSripal requested a review from fedekunze April 8, 2020 21:44
fedekunze
fedekunze approved these changes Apr 8, 2020
View reviewed changes
Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

utACK

@fedekunze fedekunze added R4R and removed WIP labels Apr 8, 2020
golangcibot
golangcibot reviewed Apr 8, 2020
View reviewed changes
x/ibc/20-transfer/keeper/keeper.go Outdated
bankKeeper types.BankKeeper
supplyKeeper types.SupplyKeeper
scopedKeeper capability.ScopedKeeper
channelKeeper types.ChannelKeeper
Copy link
Contributor

Choose a reason for hiding this comment

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

File is not gofmt-ed with -s (from gofmt)

jackzampolin
jackzampolin approved these changes Apr 8, 2020
View reviewed changes
Copy link
Member

@jackzampolin jackzampolin left a comment

Choose a reason for hiding this comment

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

Great debugging session @AdityaSripal and thank you for cleaning up that branch! LGTM (going to pull the trigger here)

@jackzampolin jackzampolin merged commit b85f82e into ibc-alpha Apr 8, 2020
@jackzampolin jackzampolin deleted the aditya/bind-transfer branch April 8, 2020 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@golangcibot golangcibot golangcibot left review comments

@fedekunze fedekunze fedekunze approved these changes

@jackzampolin jackzampolin jackzampolin approved these changes

@alessio alessio Awaiting requested review from alessio

@alexanderbez alexanderbez Awaiting requested review from alexanderbez

@hschoenburg hschoenburg Awaiting requested review from hschoenburg

@nylira nylira Awaiting requested review from nylira

@cwgoes cwgoes Awaiting requested review from cwgoes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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