cosmos / ibc-go

Inter-Blockchain Communication Protocol (IBC) implementation in Golang.
https://ibc.cosmos.network/
MIT License
542 stars 580 forks source link

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

Open DimitrisJim opened 4 months ago

DimitrisJim commented 4 months ago

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:

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

DimitrisJim commented 4 months ago

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

damiannolan commented 4 months ago

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)
DimitrisJim commented 4 months ago

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).

damiannolan commented 4 months ago

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?

DimitrisJim commented 4 months ago

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.