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

Migrate 0.7.2 to cosmos-sdk goz-phase-3 #132

Closed
wants to merge 1 commit into from
Closed

Conversation

alpe
Copy link
Member

@alpe alpe commented Jun 5, 2020

See #128

Reviewer note

The --validate-signatures is not supported anymore in cli_test.go so I replaced failing tests with the matching ones from https://github.com/cosmos/cosmos-sdk/blob/goz-phase-3/x/auth/client/cli/cli_test.go


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)

@alpe alpe requested a review from ethanfrey as a code owner June 5, 2020 15:57
@codecov-commenter
Copy link

codecov-commenter commented Jun 5, 2020

Codecov Report

Merging #132 into update-gaia will increase coverage by 0.05%.
The diff coverage is 93.75%.

Impacted file tree graph

@@               Coverage Diff               @@
##           update-gaia     #132      +/-   ##
===============================================
+ Coverage        30.88%   30.93%   +0.05%     
===============================================
  Files               20       21       +1     
  Lines             2678     2686       +8     
===============================================
+ Hits               827      831       +4     
- Misses            1776     1780       +4     
  Partials            75       75              
Impacted Files Coverage Δ
app/genesis.go 0.00% <0.00%> (ø)
lcd_test/helpers.go 74.45% <ø> (-1.46%) ⬇️
x/wasm/types/codec.go 100.00% <ø> (ø)
app/app.go 93.22% <100.00%> (-0.04%) ⬇️
app/codec.go 100.00% <100.00%> (ø)
x/wasm/keeper/test_common.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 015802e...20bd2c9. Read the comment docs.

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.

looks good

app/codec.go Show resolved Hide resolved
genDoc, err := tmtypes.GenesisDocFromFile(f.GenesisFile())
require.NoError(f.T, err)

var appState simapp.GenesisState
require.NoError(f.T, cdc.UnmarshalJSON(genDoc.AppState, &appState))
require.NoError(f.T, f.cdc.UnmarshalJSON(genDoc.AppState, &appState))
Copy link
Member

Choose a reason for hiding this comment

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

Good check here

@@ -262,12 +257,12 @@ func NewWasmApp(

// Create IBC Keeper
app.ibcKeeper = ibc.NewKeeper(
app.cdc, keys[ibc.StoreKey], app.stakingKeeper, scopedIBCKeeper,
app.cdc, appCodec, keys[ibc.StoreKey], app.stakingKeeper, scopedIBCKeeper,
Copy link
Member

Choose a reason for hiding this comment

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

I am getting confused.

Why does this need both *std.Codec and *codec.Codec and what are these for?

(Not your issue, but if you can give me a 1 paragraph issue, I would be grateful)

Copy link
Member Author

Choose a reason for hiding this comment

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

I have not looked into details but migration to protobuf is not done for this module

@@ -56,7 +56,9 @@ func CreateTestInput(t *testing.T, tempDir string) (sdk.Context, auth.AccountKee
isCheckTx := false
ctx := sdk.NewContext(ms, abci.Header{}, isCheckTx, log.NewNopLogger())
cdc := MakeTestCodec()
appCodec := codecstd.NewAppCodec(cdc)
interfaceRegistry := codectypes.NewInterfaceRegistry()
Copy link
Member

Choose a reason for hiding this comment

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

Should we just pull in the MakeCodec from app to this file and use it?

It seems it did a number of register commands that might be needed to some infrastructure later. I did spend ~2 hours hunting down a bug that was caused by not registering some amino codec in my test code.

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea but this creates cyclic dependencies:

package github.com/cosmwasm/wasmd/app
	imports github.com/cosmwasm/wasmd/x/wasm
	imports github.com/cosmwasm/wasmd/x/wasm/client/cli
	imports github.com/cosmwasm/wasmd/x/wasm/keeper
	imports github.com/cosmwasm/wasmd/app

Copy link
Member

Choose a reason for hiding this comment

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

I actually meant cut-and-paste that code 😬

Too many circular deps in the sdk. Anyway, good to merge as is, or with this cut and paste.

@ethanfrey
Copy link
Member

One note... this seems to close #128 not #127

(you are updating the sdk, not merging v0.8 into this)

@alpe alpe mentioned this pull request Jun 10, 2020
10 tasks
@ethanfrey
Copy link
Member

#138 is definitely superior. Closing this to avoid confusion

@ethanfrey ethanfrey closed this Jun 10, 2020
@alpe alpe deleted the upgrade-sdk-0.7.2 branch July 1, 2020 12:13
zemyblue pushed a commit to Finschia/wasmd that referenced this pull request Jan 2, 2023
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.

3 participants