fix[CL]: codec changes #3811
fix[CL]: codec changes #3811
Conversation
| registry.RegisterInterface( | ||
| "osmosis.swaprouter.v1beta1.PoolI", | ||
| (*swaproutertypes.PoolI)(nil), | ||
| &Pool{}, | ||
| ) |
There was a problem hiding this comment.
Why do we marshal an interface though? I think this could have been avoided if we marshaled a pool struct directly
There was a problem hiding this comment.
The reason we marshal an interface in gamm is that there are 2 implementations - balancer and stableswap. Here, we only have one pool model. So we should be able to marshal its concrete type directly
There was a problem hiding this comment.
@p0mvn wdym by marshalling a pool struct directly?
There was a problem hiding this comment.
I'm pretty sure he means not marshaling into proto any and instead directly marshaling into the struct itself, but sometimes this isnt possible due to circular dependencies in the proto files
There was a problem hiding this comment.
Hrmm, it potentially makes sense to register this, if we wanted a swaprouter query for underlying structs as an interface tho
There was a problem hiding this comment.
FWIW, I don't really care if we serialize into state as concrete struct vs interface.
Concrete is slightly more space efficient, but I doubt this efficiency ever matters. Interface could plausibly give more ease of switching in the future, but if we didn't do an interface I don't think it'd be that hard to change in the future either.
| cdc.RegisterConcrete(&Pool{}, "osmosis/cl-pool", nil) | ||
| cdc.RegisterConcrete(&MsgCreateConcentratedPool{}, "osmosis/cl-create-pool", nil) |
There was a problem hiding this comment.
did we hit the 40 char limit before or smth?
There was a problem hiding this comment.
Matt was just telling me to be weary of the lengths of these, so I made it as short as possible to get the point across
|
Agreed, I think we need this for generalized pool queries, even if we do concrete serialization within CL |
Closes: #XXX
What is the purpose of the change
Small codec change I left out. It is needed for pool queries