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

Refactor 08-wasm with improvements after 02-client refactor #6088

Merged
merged 10 commits into from
Apr 15, 2024
1 change: 0 additions & 1 deletion modules/light-clients/08-wasm/keeper/querier.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ type queryHandler struct {
}

// newQueryHandler returns a default querier that can be used in the contract.
// TODO(jim): Make private and use export_test?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

stale

func newQueryHandler(ctx sdk.Context, plugins QueryPlugins, callerID string) *queryHandler {
return &queryHandler{
Ctx: ctx,
Expand Down
5 changes: 3 additions & 2 deletions modules/light-clients/08-wasm/keeper/querier_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -328,13 +328,14 @@ func (suite *KeeperTestSuite) TestStargateQuery() {
// NOTE: we register query callbacks against: types.TimestampAtHeightMsg{}
// in practise, this can against any client state msg, however registering against types.StatusMsg{} introduces recursive loops
// due to test case: "success: verify membership query"
// TODO(Jim): Sanity check that it unmarshals correctly?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just checked the typical res is nil/not nil.

_, err = wasmClientKeeper.WasmQuery(suite.chainA.GetContext(), endpoint.ClientID, clientStore, clientState, payload)
res, err := wasmClientKeeper.WasmQuery(suite.chainA.GetContext(), endpoint.ClientID, clientStore, clientState, payload)

expPass := tc.expError == nil
if expPass {
suite.Require().NoError(err)
suite.Require().NotNil(res)
} else {
suite.Require().Nil(res)
// use error contains as wasmvm errors do not implement errors.Is method
suite.Require().ErrorContains(err, tc.expError.Error())
}
Expand Down
4 changes: 1 addition & 3 deletions modules/light-clients/08-wasm/light_client_module.go
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@

clientMessage, ok := clientMsg.(*types.ClientMessage)
if !ok {
panic(fmt.Errorf("expected type %T, got %T", &types.ClientMessage{}, clientMsg))

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods Warning

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
}

payload := types.SudoMsg{
Expand All @@ -196,12 +196,12 @@

res, err := l.keeper.WasmSudo(ctx, clientID, clientStore, clientState, payload)
if err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods Warning

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
}

var result types.UpdateStateResult
if err := json.Unmarshal(res, &result); err != nil {
panic(errorsmod.Wrap(types.ErrWasmInvalidResponseData, err.Error()))

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods Warning

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
}

heights := []exported.Height{}
Expand Down Expand Up @@ -438,13 +438,11 @@
}

substituteClientStore := l.storeProvider.ClientStore(ctx, substituteClientID)
substituteClient, found := types.GetClientState(substituteClientStore, cdc)
substituteClientState, found := types.GetClientState(substituteClientStore, cdc)
if !found {
return errorsmod.Wrap(clienttypes.ErrClientNotFound, substituteClientID)
}

substituteClientState := substituteClient
Copy link
Contributor Author

Choose a reason for hiding this comment

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

dunno why tf I had done this 😅


// check that checksums of subject client state and substitute client state match
// changing the checksum is only allowed through the migrate contract RPC endpoint
if !bytes.Equal(clientState.Checksum, substituteClientState.Checksum) {
Expand Down
27 changes: 12 additions & 15 deletions modules/light-clients/08-wasm/light_client_module_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -854,7 +854,9 @@ func (suite *WasmTestSuite) TestVerifyUpgradeAndUpdateState() {
},
clienttypes.ErrClientNotFound,
},
/* TODO(jim): Get back to these.
/* NOTE(jim): This can't fail on unmarshalling, it appears. Any consensus type
Copy link
Contributor Author

Choose a reason for hiding this comment

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

does it make sense to keep this in to note for covering this edge case?

Copy link
Contributor

Choose a reason for hiding this comment

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

is there something we should do about this commented out code for a follow up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unsure tbh. On the one hand I think we should be aware of this quirk, on the other hand, can't seem to best find a place to add it.

we attempt to unmarshal just creates a Wasm ConsensusState that has a
Data field empty.
{
"failure: upgraded consensus state is not wasm consensus state",
func() {
Expand Down Expand Up @@ -1422,8 +1424,7 @@ func (suite *WasmTestSuite) TestRecoverClient() {
bz, err := json.Marshal(types.EmptyResult{})
suite.Require().NoError(err)

// TODO(jim): re-arrange export_test.go contents
prefixedKey := []byte("subject/")
prefixedKey := types.SubjectPrefix
prefixedKey = append(prefixedKey, host.ClientStateKey()...)
expectedClientStateBz = wasmtesting.CreateMockClientStateBz(suite.chainA.Codec, suite.checksum)
store.Set(prefixedKey, expectedClientStateBz)
Expand Down Expand Up @@ -1462,24 +1463,20 @@ func (suite *WasmTestSuite) TestRecoverClient() {
},
clienttypes.ErrClientNotFound,
},
/* TODO(jim): We don't pass a client state directly now so we need to modify how this is tested.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

first test was removed, was already checked. Second test was amended to cover the case where the substitute has a diff checksum

{
"failure: invalid substitute client state",
func() {
substituteClientState = &ibctm.ClientState{}
},
clienttypes.ErrInvalidClient,
},
{
"failure: checksums do not match",
func() {
substituteClientState = &types.ClientState{
Checksum: []byte("invalid"),
}
substituteClientState, found := GetSimApp(suite.chainA).IBCKeeper.ClientKeeper.GetClientState(suite.chainA.GetContext(), substituteClientID)
suite.Require().True(found)

wasmSubstituteClientState, ok := substituteClientState.(*types.ClientState)
suite.Require().True(ok)

wasmSubstituteClientState.Checksum = []byte("invalid")
GetSimApp(suite.chainA).IBCKeeper.ClientKeeper.SetClientState(suite.chainA.GetContext(), substituteClientID, wasmSubstituteClientState)
},
clienttypes.ErrInvalidClient,
},
*/
{
"failure: vm returns error",
func() {
Expand Down
Loading