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

interchain accounts #793

Closed
wants to merge 17 commits into from
Closed

interchain accounts #793

wants to merge 17 commits into from

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Mar 23, 2022

Hi, this PR upgrades wasmd to ibcv3

@codecov
Copy link

codecov bot commented Mar 23, 2022

Codecov Report

Merging #793 (8dc296f) into master (57ead1a) will decrease coverage by 0.00%.
The diff coverage is 80.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #793      +/-   ##
==========================================
- Coverage   58.76%   58.76%   -0.01%     
==========================================
  Files          50       50              
  Lines        5862     5856       -6     
==========================================
- Hits         3445     3441       -4     
+ Misses       2164     2162       -2     
  Partials      253      253              
Impacted Files Coverage Δ
app/test_access.go 0.00% <0.00%> (ø)
app/test_helpers.go 0.75% <0.00%> (ø)
x/wasm/keeper/handler_plugin.go 85.14% <ø> (ø)
x/wasm/keeper/handler_plugin_encoders.go 80.88% <ø> (ø)
x/wasm/keeper/ibc.go 77.77% <ø> (ø)
x/wasm/keeper/query_plugins.go 80.06% <ø> (ø)
app/export.go 12.12% <4.16%> (ø)
x/wasm/ibc.go 68.09% <55.55%> (+0.39%) ⬆️
app/app.go 88.77% <98.76%> (-0.17%) ⬇️
app/ante.go 62.50% <100.00%> (ø)

@faddat faddat marked this pull request as ready for review March 23, 2022 03:16
@faddat faddat requested a review from alpe as a code owner March 23, 2022 03:16
@faddat
Copy link
Contributor Author

faddat commented Apr 11, 2022

I think this one is:

  • up to date
  • ready to go

Could you let me know if this is something that you're interested in reviewing / accepting?

Thanks,

-Jacob

@ethanfrey
Copy link
Member

Awesome! I was wondering about pulling this out of the 0.46 upgrade and just noticed this PR.
I was off since the end of March, but I will look into this PR this week and give you full feedback

@ethanfrey ethanfrey added this to the v0.27.0 milestone Apr 26, 2022
@ethanfrey
Copy link
Member

Note: Resolves #806

@ethanfrey ethanfrey mentioned this pull request Apr 28, 2022
Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Thank you for this PR.

I would pull it out into a few different ones. I see:

  1. some osmosis-specific stuff that should be removed
  2. Renaming lots of WasmApp members to make them public
  3. Importing ibcv3 and exposing SetChannel

I see (2) and (3) as independent and not necessarily both needed.

I will try to cut out the ibcv3 commits alone and see if I can make a new PR.

You could also pull out the "make all app members public" code into a separate PR. I have no strong opinion on this either way, but it does make some testing easier. I would like @alpe to give feedback on that part.

@@ -170,25 +178,20 @@ var (
mint.AppModuleBasic{},
distr.AppModuleBasic{},
gov.NewAppModuleBasic(
append(
wasmclient.ProposalHandlers,
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove wasmclient.ProposalHandlers here? This is an essential part (and a slice, thus the reason for append)

@@ -123,6 +123,10 @@ require (
replace (
// Use the cosmos-flavored keyring library
github.com/99designs/keyring => github.com/cosmos/keyring v1.1.7-0.20210622111912-ef00f8ac3d76
// Use osmosis version of the SDK. Our branch: v0.45.0x-osmo-v7, current tag: v0.45.0x-osmo-v7.2
github.com/cosmos/cosmos-sdk => github.com/osmosis-labs/cosmos-sdk v0.45.1-0.20220220082240-1b4ae82744ea
Copy link
Member

Choose a reason for hiding this comment

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

We can't merge this (even if it is a better fork). Wasmd sticks with mainstream cosmos-sdk.

I would just cut 7ec06a0 out of the commit history

Copy link
Member

Choose a reason for hiding this comment

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

And 22c91a1

crisisKeeper crisiskeeper.Keeper
upgradeKeeper upgradekeeper.Keeper
paramsKeeper paramskeeper.Keeper
AccountKeeper authkeeper.AccountKeeper
Copy link
Member

Choose a reason for hiding this comment

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

Definitely makes testing easier and I was happy to see this in osmosis codebase.

@alpe any reason we shouldn't make these public?

@@ -686,13 +682,13 @@ func NewWasmApp(

app.ScopedIBCKeeper = scopedIBCKeeper
app.ScopedTransferKeeper = scopedTransferKeeper
app.ScopedICAControllerKeeper = scopedICAControllerKeeper
app.ScopedICAHostKeeper = scopedICAHostKeeper
app.ScopedICAControllerKeeper = app.ScopedICAControllerKeeper
Copy link
Member

Choose a reason for hiding this comment

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

Huh. This makes no sense? (Remove app. on the right)

@@ -135,6 +135,7 @@ func (i IBCHandler) OnChanOpenAck(
return sdkerrors.Wrapf(channeltypes.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID)
}
channelInfo.Counterparty.ChannelId = counterpartyChannelID
i.channelKeeper.SetChannel(ctx, portID, channelID, channelInfo)
Copy link
Member

Choose a reason for hiding this comment

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

This is the critical change in our codebase?

@@ -16,6 +16,7 @@ type MockChannelKeeper struct {
ChanCloseInitFn func(ctx sdk.Context, portID, channelID string, chanCap *capabilitytypes.Capability) error
GetAllChannelsFn func(ctx sdk.Context) []channeltypes.IdentifiedChannel
IterateChannelsFn func(ctx sdk.Context, cb func(channeltypes.IdentifiedChannel) bool)
SetChannelFn func(ctx sdk.Context, portID, channelID string, channel channeltypes.Channel)
Copy link
Member

Choose a reason for hiding this comment

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

Nice addition

This was referenced Apr 28, 2022
@ethanfrey
Copy link
Member

Closing in favor of #824 and #837

@ethanfrey ethanfrey closed this May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants