fix(08-wasm/test): register mockVm querier callbacks against TimestampAtHeightMsg to avoid recursive loops #5878
Conversation
…o avoid recursive loops
damiannolan
requested review from
colin-axner,
AdityaSripal,
charleenfei,
chatton,
DimitrisJim,
srdtrk and
crodriguezvega
as code owners
| clientState.Status(suite.chainA.GetContext(), clientStore, suite.chainA.App.AppCodec()) | ||
|
|
||
| // 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 |
There was a problem hiding this comment.
due to Status being used in the VerifyMembership grpc implementation?
There was a problem hiding this comment.
do we need to protect against this somehow? Or are avoiding recursive loops up to contract developers?
There was a problem hiding this comment.
due to Status being used in the VerifyMembership grpc implementation?
Yes, exactly!
do we need to protect against this somehow? Or are avoiding recursive loops up to contract developers?
I feel like there is only a certain amount we can do to protect against this tbh. Any wasm client devs should be testing their code before anything like this can ever happen. I think it should be extremely rare to occur. The recursive loop scenario here imo is more due to how we use the mockVM and the fact that we are making callback into the same client state which has invoked the call.
| clientState.Status(suite.chainA.GetContext(), clientStore, suite.chainA.App.AppCodec()) | ||
|
|
||
| // 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 |
There was a problem hiding this comment.
do we need to protect against this somehow? Or are avoiding recursive loops up to contract developers?
|
should |
crodriguezvega
added
backport-to-v8.2.x
backport-wasm-v0.2.x+ibc-go-v8.2.x-wasmvm-v2.0.x
and removed
backport-to-v8.2.x
labels
Yeah, its down to the implementation of the contract right? and which one its calling. But specifying its own clientID seems like something that shouldn't ever need to happen - we can probably add a defensive check here. Happy to chat about this in eng call next week. |
Description
Register querier callbacks in mockVM against
TimestampAtHeightMsginstead ofStatusMsg.This avoids recursive loops introduced by yours truly in #5870, adding the client status check to the rpc handler of
VerifyMembership.If anyone would rather a different approach to fixing this issue, we could additional create an entirely separate test func for
success: verify membership queryand only useTimestampAtHeightMsgfor that one, but it makes no difference to me.Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.