-
Notifications
You must be signed in to change notification settings - Fork 400
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
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)) |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
#138 is definitely superior. Closing this to avoid confusion |
See #128
Reviewer note
The
--validate-signatures
is not supported anymore incli_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.goFor admin use:
WIP
,R4R
,docs
, etc)