ibc-go icon indicating copy to clipboard operation
ibc-go copied to clipboard

Further reduce function arguments for 08-wasm contract calling methods.

Open DimitrisJim opened this issue 10 months ago • 5 comments

Basically, this comment https://github.com/cosmos/ibc-go/pull/6103#discussion_r1566519731. Noticed that we have a couple of ways of accessing client store/state:

  • Grab store with storeProvider and use types.GetClientState to grab client state.
  • Use the client keeper stored on the wasm keeper to access these (requiring a cast to WasmClientState)

Current pattern for most of the light client module impls is:

  1. grab store
  2. grab wasm client state
  3. create payload
  4. call sudo/query/migrate etc

Seems like these arguments can just be constructed inside the methods we call instead of passing them as args. Open this issue here to keep track, would be nice to address before v9.


For Admin Use

  • [ ] Not duplicate issue
  • [ ] Appropriate labels applied
  • [ ] Appropriate contributors tagged/assigned

DimitrisJim avatar Apr 17 '24 16:04 DimitrisJim

Haven't tried doing the refactor currently, unsure if something might pop up and make this not possible.

DimitrisJim avatar Apr 17 '24 16:04 DimitrisJim

Seems like these arguments can just be constructed inside the methods we call instead of passing them as args. Open this issue here to keep track, would be nice to address before v9.

are you suggesting we do something more like below, and get the store and client state at the keeper level?

l.keeper.WasmQuery(ctx, clientID, payload)

damiannolan avatar Apr 18 '24 12:04 damiannolan

yup! I think that for light clients with a keeper that can hold references to a clientKeeper, the store provider can not be used? I'm unsure if this was the end goal you and Colin had in mind (add internal keepers to lcm's for other clients and just use them to access needed state).

DimitrisJim avatar Apr 18 '24 12:04 DimitrisJim

Off the top of my head, I'm thinking we could potentially replace the wasm keeper's clientKeeper entirely with store provider? Would need to look at it more and come back with a fresh answer

edit: If I remember correctly, the only reason wasm keeper has this ref to client keeper is for contract migrations, right?

damiannolan avatar Apr 18 '24 12:04 damiannolan

the only reason wasm keeper has this ref to client keeper is for contract migrations

yea! That's where I encountered its usage. Though would be nice before v9 I guess it isn't as pressing since these are mostly called internally for the time being. We could wait until we see what we do for other light clients and just normalize the way we do things across the board.

DimitrisJim avatar Apr 18 '24 12:04 DimitrisJim