Deprecate unused pool id field in swap estimate #6530
Conversation
mattverse
requested review from
czarcas7ic,
p0mvn and
AlpinYukseloglu
as code owners
mattverse
added
V:state/compatible/no_backport
|
This field was unused any where so should be state compatible, but happy to include it in the next major upgrade just in case there was an oversight |
|
doesn't this need to be retracted? Something like |
|
@nicolaslara Both ways work, deprecation or reserving, I feel like they have updated the proto gen policy in the latest docker image for creating proto bufs, it wouldn't allow me to create the reserved field for some reason 🤔 |
| TokenOut: req.TokenOut, | ||
| Routes: types.SwapAmountOutRoutes{{PoolId: req.PoolId, TokenInDenom: req.TokenInDenom}}, | ||
| Routes: types.SwapAmountOutRoutes{{TokenInDenom: req.TokenInDenom}}, |
There was a problem hiding this comment.
This change doesn't follow for me.. I can see why you wouldn't need the PoolId field, but the PoolId within the Routes feels necessary, unless I am missing something?
There was a problem hiding this comment.
Ah I do see the point, we're deprecating pool id field due to it being in the routes for normal swaps, but for single swaps, this isn't the case, we're getting pool id as input and constructing it as routes within query proto wrap so we should indeed be having the pool id field only for single swap estimate
There was a problem hiding this comment.
Changes introduced in 600209d
There was a problem hiding this comment.
Right but we were removing BOTH pool ids, you need at least one of them to determine what pool we are in, otherwise we would just have a denom. I think your change addresses this tho
|
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
Deprecates unused and unnecessary pool id field in swap estimate queries.
Testing and Verifying
This change is a trivial rework / code cleanup without any test coverage.
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)