fix ListPoolsByDenom method
#7341
Conversation
doggystylez
requested review from
czarcas7ic,
p0mvn,
mattverse and
AlpinYukseloglu
as code owners
|
Hey @doggystylez would you be able to add a changelog entry for this? |
| @@ -577,7 +577,10 @@ func (k Keeper) ListPoolsByDenom( | |||
|
|
|||
| var poolsByDenom []types.PoolI | |||
| for _, pool := range currentModulePools { | |||
| poolDenoms := pool.GetPoolDenoms(ctx) | |||
| poolDenoms, err := poolModule.GetPoolDenoms(ctx, pool.GetId()) | |||
There was a problem hiding this comment.
can we add test case where we have error from this?
There was a problem hiding this comment.
I have been thinking about this logic and I wonder if instead of returning an error should any pools that dont expose the denoms be skipped? considering the case for future cosm wasm pools, we would be relying that every single potential contract has the query implemented (correctly)
There was a problem hiding this comment.
Yeah, I think they should be skipped, filing as a new issue!
|
@doggystylez please add changelog entry |
|
changelog is added. sorry about that |
|
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
Closes: #XXX
What is the purpose of the change
fixes the query for list pools by denom, it was panicking because you can't get the denom from cosmwasm pool directly
(E.g.: This pull request improves documentation of area A by adding ....
Testing and Verifying
added a test case for cw pool
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)