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

Remove usage of globals from 08-wasm #6103

Merged
merged 24 commits into from
Apr 10, 2024
Merged

Remove usage of globals from 08-wasm #6103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
6dc7fa9
Move functionality from client state methods to light client module m…
DimitrisJim Apr 3, 2024
8ec8bf6
Remove global VM, introspection to retrieve client identifier.
DimitrisJim Apr 3, 2024
c3ddc51
Move migrateContract from clientState to keeper method.
DimitrisJim Apr 3, 2024
6e9d4a6
Move state management to keeper.
DimitrisJim Apr 4, 2024
5cc45a4
Make InitializePinnedCodes a method on the keeper.
DimitrisJim Apr 4, 2024
139c806
Remove pending todo in test for migrate contract.
DimitrisJim Apr 4, 2024
f3a7aae
Move vm entrypoint functions in keeper/ package.
DimitrisJim Apr 5, 2024
6f5616a
feat: move vm entry points to keeper.
DimitrisJim Apr 7, 2024
7373de0
chore: simplify signatures.
DimitrisJim Apr 7, 2024
1692f8b
test: add amended tests for contact keeper.
DimitrisJim Apr 7, 2024
5127126
refactor: move querier to keeper package.
DimitrisJim Apr 8, 2024
57c2235
lint: you'll never fix me.
DimitrisJim Apr 8, 2024
12c34cb
refactor: remove queryRouter global variable
DimitrisJim Apr 8, 2024
151a546
refactor: remove global for query plugins.
DimitrisJim Apr 8, 2024
f679ccf
nit: rename to queryPlugins.
DimitrisJim Apr 8, 2024
e726ce9
refactor: remove QueryPluginsI interface.
DimitrisJim Apr 8, 2024
480fcf8
refactor: make queryHandler a private type.
DimitrisJim Apr 8, 2024
d614a93
Make migrateContractCode a private function, clean up uneeded export …
DimitrisJim Apr 8, 2024
8e6b567
refactor: Move vm entrypoints to keeper.
DimitrisJim Apr 8, 2024
2aea36f
chore: address some testing todos. Move testing unmarshal failure cas…
DimitrisJim Apr 8, 2024
23decca
chore: additional tiny nits.
DimitrisJim Apr 8, 2024
f23bc7c
chore: restructure definitions to make diff more readable.
DimitrisJim Apr 9, 2024
2836073
d'oh: rm vm arg from instantiate.
DimitrisJim Apr 9, 2024
76d8055
nit: Space out keeper fields. Remove TODO for method name.
DimitrisJim Apr 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions modules/light-clients/08-wasm/internal/ibcwasm/expected_interfaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
wasmvmtypes "github.com/CosmWasm/wasmvm/v2/types"

"github.com/cosmos/cosmos-sdk/baseapp"
sdk "github.com/cosmos/cosmos-sdk/types"
)

type WasmEngine interface {
Expand Down Expand Up @@ -123,8 +122,3 @@ type QueryRouter interface {
// if not found
Route(path string) baseapp.GRPCQueryHandler
}

type QueryPluginsI interface {
Copy link
Member

Choose a reason for hiding this comment

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

This interface isn't needed anymore? what was it being used for previously?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#5345 (comment)

The main problem previously was the Set/Get for query plugins being globally accessible through types. Currently these are moved (and exported) on Keeper which in cosmos-sdk context would require someone explicitly hold a ref to keeper to modify, hence, not as unexpected (was my thinking).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, think I could make them private though! I'll do a follow up here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in general my plan was do refactor -> merge, move as much as possible into internal/make symbols private.

// HandleQuery will route the query to the correct plugin and return the result
HandleQuery(ctx sdk.Context, caller string, request wasmvmtypes.QueryRequest) ([]byte, error)
}
77 changes: 0 additions & 77 deletions modules/light-clients/08-wasm/internal/ibcwasm/wasm.go
View file
Open in desktop

This file was deleted.

160 changes: 68 additions & 92 deletions modules/light-clients/08-wasm/types/vm.go → ...clients/08-wasm/keeper/contract_keeper.go
View file
Open in desktop

Large diffs are not rendered by default.

46 changes: 18 additions & 28 deletions ...es/light-clients/08-wasm/types/vm_test.go → ...ts/08-wasm/keeper/contract_keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package types_test
package keeper_test

import (
"encoding/json"
Expand All @@ -15,7 +15,7 @@ import (
localhost "github.com/cosmos/ibc-go/v8/modules/light-clients/09-localhost"
)

func (suite *TypesTestSuite) TestWasmInstantiate() {
func (suite *KeeperTestSuite) TestWasmInstantiate() {
testCases := []struct {
name string
malleate func()
Expand Down Expand Up @@ -165,17 +165,19 @@ func (suite *TypesTestSuite) TestWasmInstantiate() {
tc := tc
suite.Run(tc.name, func() {
suite.SetupWasmWithMockVM()
checksum := storeWasmCode(suite, wasmtesting.Code)

DimitrisJim marked this conversation as resolved.
Show resolved Hide resolved
tc.malleate()

initMsg := types.InstantiateMessage{
ClientState: clienttypes.MustMarshalClientState(suite.chainA.App.AppCodec(), wasmtesting.MockTendermitClientState),
ConsensusState: clienttypes.MustMarshalConsensusState(suite.chainA.App.AppCodec(), wasmtesting.MockTendermintClientConsensusState),
Checksum: suite.checksum,
Checksum: checksum,
}

clientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), defaultWasmClientID)
err := types.WasmInstantiate(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), clientStore, &types.ClientState{Checksum: suite.checksum}, initMsg)
wasmClientKeeper := GetSimApp(suite.chainA).WasmClientKeeper
err := wasmClientKeeper.WasmInstantiate(suite.chainA.GetContext(), defaultWasmClientID, clientStore, &types.ClientState{Checksum: checksum}, initMsg)

expPass := tc.expError == nil
if expPass {
Expand All @@ -187,7 +189,7 @@ func (suite *TypesTestSuite) TestWasmInstantiate() {
}
}

func (suite *TypesTestSuite) TestWasmMigrate() {
func (suite *KeeperTestSuite) TestWasmMigrate() {
testCases := []struct {
name string
malleate func()
Expand Down Expand Up @@ -305,6 +307,7 @@ func (suite *TypesTestSuite) TestWasmMigrate() {
tc := tc
suite.Run(tc.name, func() {
suite.SetupWasmWithMockVM()
_ = storeWasmCode(suite, wasmtesting.Code)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

many of the type tests were written to assume a checksum exists in state, keeper tests don't. This should require some refactoring at some point.

endpoint := wasmtesting.NewWasmEndpoint(suite.chainA)
err := endpoint.CreateClient()
Expand All @@ -313,7 +316,8 @@ func (suite *TypesTestSuite) TestWasmMigrate() {
tc.malleate()

clientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), defaultWasmClientID)
err = types.WasmMigrate(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), clientStore, &types.ClientState{}, defaultWasmClientID, []byte("{}"))
wasmClientKeeper := GetSimApp(suite.chainA).WasmClientKeeper
err = wasmClientKeeper.WasmMigrate(suite.chainA.GetContext(), clientStore, &types.ClientState{}, defaultWasmClientID, []byte("{}"))

expPass := tc.expError == nil
if expPass {
Expand All @@ -325,7 +329,7 @@ func (suite *TypesTestSuite) TestWasmMigrate() {
}
}

func (suite *TypesTestSuite) TestWasmQuery() {
func (suite *KeeperTestSuite) TestWasmQuery() {
var payload types.QueryMsg

testCases := []struct {
Expand Down Expand Up @@ -368,21 +372,13 @@ func (suite *TypesTestSuite) TestWasmQuery() {
},
types.ErrWasmContractCallFailed,
},
{
"failure: response fails to unmarshal",
func() {
DimitrisJim marked this conversation as resolved.
Show resolved Hide resolved
suite.mockVM.RegisterQueryCallback(types.StatusMsg{}, func(_ wasmvm.Checksum, _ wasmvmtypes.Env, _ []byte, _ wasmvm.KVStore, _ wasmvm.GoAPI, _ wasmvm.Querier, _ wasmvm.GasMeter, _ uint64, _ wasmvmtypes.UFraction) (*wasmvmtypes.QueryResult, uint64, error) {
return &wasmvmtypes.QueryResult{Ok: []byte("invalid json")}, wasmtesting.DefaultGasUsed, nil
})
},
types.ErrWasmInvalidResponseData,
},
}

for _, tc := range testCases {
tc := tc
suite.Run(tc.name, func() {
suite.SetupWasmWithMockVM()
_ = storeWasmCode(suite, wasmtesting.Code)

endpoint := wasmtesting.NewWasmEndpoint(suite.chainA)
err := endpoint.CreateClient()
Expand All @@ -398,7 +394,8 @@ func (suite *TypesTestSuite) TestWasmQuery() {

tc.malleate()

res, err := types.WasmQuery[types.StatusResult](suite.chainA.GetContext(), clientStore, wasmClientState, payload)
wasmClientKeeper := GetSimApp(suite.chainA).WasmClientKeeper
res, err := wasmClientKeeper.WasmQuery(suite.chainA.GetContext(), endpoint.ClientID, clientStore, wasmClientState, payload)

expPass := tc.expError == nil
if expPass {
Expand All @@ -411,7 +408,7 @@ func (suite *TypesTestSuite) TestWasmQuery() {
}
}

func (suite *TypesTestSuite) TestWasmSudo() {
func (suite *KeeperTestSuite) TestWasmSudo() {
var payload types.SudoMsg

testCases := []struct {
Expand Down Expand Up @@ -487,15 +484,6 @@ func (suite *TypesTestSuite) TestWasmSudo() {
},
types.ErrWasmAttributesNotAllowed,
},
{
"failure: response fails to unmarshal",
func() {
suite.mockVM.RegisterSudoCallback(types.UpdateStateMsg{}, func(_ wasmvm.Checksum, _ wasmvmtypes.Env, _ []byte, _ wasmvm.KVStore, _ wasmvm.GoAPI, _ wasmvm.Querier, _ wasmvm.GasMeter, _ uint64, _ wasmvmtypes.UFraction) (*wasmvmtypes.ContractResult, uint64, error) {
return &wasmvmtypes.ContractResult{Ok: &wasmvmtypes.Response{Data: []byte("invalid json")}}, wasmtesting.DefaultGasUsed, nil
})
},
types.ErrWasmInvalidResponseData,
},
{
"failure: invalid clientstate type",
func() {
Expand Down Expand Up @@ -561,6 +549,7 @@ func (suite *TypesTestSuite) TestWasmSudo() {
tc := tc
suite.Run(tc.name, func() {
suite.SetupWasmWithMockVM()
_ = storeWasmCode(suite, wasmtesting.Code)

endpoint := wasmtesting.NewWasmEndpoint(suite.chainA)
err := endpoint.CreateClient()
Expand All @@ -576,7 +565,8 @@ func (suite *TypesTestSuite) TestWasmSudo() {

tc.malleate()

res, err := types.WasmSudo[types.UpdateStateResult](suite.chainA.GetContext(), suite.chainA.App.AppCodec(), clientStore, wasmClientState, payload)
wasmClientKeeper := GetSimApp(suite.chainA).WasmClientKeeper
res, err := wasmClientKeeper.WasmSudo(suite.chainA.GetContext(), endpoint.ClientID, clientStore, wasmClientState, payload)

expPass := tc.expError == nil
if expPass {
Expand Down
8 changes: 8 additions & 0 deletions modules/light-clients/08-wasm/keeper/export_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package keeper

import sdk "github.com/cosmos/cosmos-sdk/types"

// MigrateContractCode is a wrapper around k.migrateContractCode to allow the method to be directly called in tests.
func (k Keeper) MigrateContractCode(ctx sdk.Context, clientID string, newChecksum, migrateMsg []byte) error {
return k.migrateContractCode(ctx, clientID, newChecksum, migrateMsg)
}
9 changes: 4 additions & 5 deletions modules/light-clients/08-wasm/keeper/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,14 @@ import (

sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/cosmos/ibc-go/modules/light-clients/08-wasm/internal/ibcwasm"
"github.com/cosmos/ibc-go/modules/light-clients/08-wasm/types"
)

// InitGenesis initializes the 08-wasm module's state from a provided genesis
// state.
func (k Keeper) InitGenesis(ctx sdk.Context, gs types.GenesisState) error {
storeFn := func(code wasmvm.WasmCode, _ uint64) (wasmvm.Checksum, uint64, error) {
checksum, err := ibcwasm.GetVM().StoreCodeUnchecked(code)
checksum, err := k.GetVM().StoreCodeUnchecked(code)
return checksum, 0, err
}

Expand All @@ -28,16 +27,16 @@ func (k Keeper) InitGenesis(ctx sdk.Context, gs types.GenesisState) error {

// ExportGenesis returns the 08-wasm module's exported genesis. This includes the code
// for all contracts previously stored.
func (Keeper) ExportGenesis(ctx sdk.Context) types.GenesisState {
checksums, err := types.GetAllChecksums(ctx)
func (k Keeper) ExportGenesis(ctx sdk.Context) types.GenesisState {
checksums, err := k.GetAllChecksums(ctx)
if err != nil {
panic(err)
}

// Grab code from wasmVM and add to genesis state.
var genesisState types.GenesisState
for _, checksum := range checksums {
code, err := ibcwasm.GetVM().GetCode(checksum)
code, err := k.GetVM().GetCode(checksum)
if err != nil {
panic(err)
}
Expand Down
2 changes: 1 addition & 1 deletion modules/light-clients/08-wasm/keeper/genesis_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func (suite *KeeperTestSuite) TestInitGenesis() {
suite.Require().NoError(err)

var storedHashes []string
checksums, err := types.GetAllChecksums(suite.chainA.GetContext())
checksums, err := GetSimApp(suite.chainA).WasmClientKeeper.GetAllChecksums(suite.chainA.GetContext())
suite.Require().NoError(err)

for _, hash := range checksums {
Expand Down
11 changes: 5 additions & 6 deletions modules/light-clients/08-wasm/keeper/grpc_query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,13 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
sdkquery "github.com/cosmos/cosmos-sdk/types/query"

"github.com/cosmos/ibc-go/modules/light-clients/08-wasm/internal/ibcwasm"
"github.com/cosmos/ibc-go/modules/light-clients/08-wasm/types"
)

var _ types.QueryServer = (*Keeper)(nil)

// Code implements the Query/Code gRPC method
func (Keeper) Code(goCtx context.Context, req *types.QueryCodeRequest) (*types.QueryCodeResponse, error) {
func (k Keeper) Code(goCtx context.Context, req *types.QueryCodeRequest) (*types.QueryCodeResponse, error) {
if req == nil {
return nil, status.Error(codes.InvalidArgument, "empty request")
}
Expand All @@ -31,11 +30,11 @@ func (Keeper) Code(goCtx context.Context, req *types.QueryCodeRequest) (*types.Q
}

// Only return checksums we previously stored, not arbitrary checksums that might be stored via e.g Wasmd.
if !types.HasChecksum(sdk.UnwrapSDKContext(goCtx), checksum) {
if !k.HasChecksum(sdk.UnwrapSDKContext(goCtx), checksum) {
return nil, status.Error(codes.NotFound, errorsmod.Wrap(types.ErrWasmChecksumNotFound, req.Checksum).Error())
}

code, err := ibcwasm.GetVM().GetCode(checksum)
code, err := k.GetVM().GetCode(checksum)
if err != nil {
return nil, status.Error(codes.NotFound, errorsmod.Wrap(types.ErrWasmChecksumNotFound, req.Checksum).Error())
}
Expand All @@ -46,10 +45,10 @@ func (Keeper) Code(goCtx context.Context, req *types.QueryCodeRequest) (*types.Q
}

// Checksums implements the Query/Checksums gRPC method. It returns a list of hex encoded checksums stored.
func (Keeper) Checksums(goCtx context.Context, req *types.QueryChecksumsRequest) (*types.QueryChecksumsResponse, error) {
func (k Keeper) Checksums(goCtx context.Context, req *types.QueryChecksumsRequest) (*types.QueryChecksumsResponse, error) {
checksums, pageRes, err := sdkquery.CollectionPaginate(
goCtx,
ibcwasm.Checksums,
k.GetChecksums(),
req.Pagination,
func(key []byte, value collections.NoValue) (string, error) {
return hex.EncodeToString(key), nil
Expand Down
Loading
Loading