feat: fix estimateXX dex queries #NTRN-318 #543
Conversation
also general cleanup for estimateXX
jcompagni10
force-pushed
the
feat/fix_estimateXX
branch
from
faddb43 to
e3ac929
Compare
| Creator: "neutron1dft8nwxzr0u27wvr2cknpermjkreqvp9fdy0uz", | ||
| Receiver: "neutron1dft8nwxzr0u27wvr2cknpermjkreqvp9fdy0uz", |
There was a problem hiding this comment.
Just a random valid address. Will add a comment. But let me know if there's a more preferable way to do this. I agree it's a bit odd.
There was a problem hiding this comment.
Probably need to create another message QueryMultiHopSwap with it's own validate and reusing validate logic somehow if possible.
Otherwise kinda ugly
joldie777
changed the title
| ctx := sdk.UnwrapSDKContext(goCtx) | ||
| cacheCtx, _ := ctx.CacheContext() | ||
|
|
||
| callerAddr := sdk.MustAccAddressFromBech32(req.Creator) | ||
| receiverAddr := sdk.MustAccAddressFromBech32(req.Receiver) | ||
|
|
||
| oldBk := k.bankKeeper | ||
| k.bankKeeper = NewSimulationBankKeeper(k.bankKeeper) |
There was a problem hiding this comment.
do we do that bank keeper substitution only because of these module<>bankKeeper interactions? also I can see that we use a cached context to not update the dex keeper's state. if yes, I think we should rethink the way we implemented the code.
for example, speaking of the MultiHopSwapCore method: the logic for a simulation and for a real swap is mostly the same:
1. calc the best route (common logic for both cases)
2. write route context (mutate dex storage as far as I understand)
3. send coins between receiver, caller and module account
4. emit events
why don't we create a separate method that does only the 1. step and is used in both simulation and real swap? in simulation we only call this method and return the result, but in the real swap we follow it up with the remaining necessary steps?
There was a problem hiding this comment.
more or less the same for PlaceLimitOrderCore
There was a problem hiding this comment.
Agree, i prefer a pure function that does estimation.
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // NB: We're only using a cache context so we don't expect any writes to happen. | ||
| //nolint:staticcheck // Should be unnecessary but out of an abundance of caution we restore the old bankkeeper | ||
| k.bankKeeper = oldBk |
There was a problem hiding this comment.
What happens if we do an early exit at the line 54 and do not put the real keeper back?
Even if you do a defer to retern the keeper back, since a query is not transactional, you can brake the dex keeper with 2 concurrent queries
| q1 | q2 |
|---|---|
| oldBk := k.bankKeeper | |
| k.bankKeeper = NewSimulationBankKeeper | |
| oldBk := k.bankKeeper (actually it's NewSimulationBankKeeper) | |
| k.bankKeeper = NewSimulationBankKeeper | |
| k.bankKeeper = oldBk | |
| k.bankKeeper = oldBk (NewSimulationBankKeeper) |
and now we have dex keeper configured with SimulationBankKeeper, dont we?
if i got it right, my point is - it's pretty dangerous to reconfigure keeper during a query
| ctx := sdk.UnwrapSDKContext(goCtx) | ||
| cacheCtx, _ := ctx.CacheContext() | ||
|
|
||
| callerAddr := sdk.MustAccAddressFromBech32(req.Creator) | ||
| receiverAddr := sdk.MustAccAddressFromBech32(req.Receiver) | ||
|
|
||
| oldBk := k.bankKeeper | ||
| k.bankKeeper = NewSimulationBankKeeper(k.bankKeeper) |
There was a problem hiding this comment.
Agree, i prefer a pure function that does estimation.
jcompagni10
force-pushed
the
feat/fix_estimateXX
branch
from
e57b3d5 to
8a9aed7
Compare
| @@ -0,0 +1,105 @@ | |||
| package keeper_test | |||
There was a problem hiding this comment.
lets rename the file to grpc_query_estimate_place_limit_order_test to get exact before grpc_query_estimate_place_limit_order.go in listing
| resp, err := s.App.DexKeeper.EstimatePlaceLimitOrder(s.Ctx, &types.QueryEstimatePlaceLimitOrderRequest{ | ||
| TokenIn: "TokenA", | ||
| TokenOut: "TokenB", | ||
| TickIndexInToOut: 4, |
There was a problem hiding this comment.
it's out of scope of the pr, but EstimatePlaceLimitOrder looks outdated, TickIndexInToOut is not marked as deprecated and limit_sell_price is missed.
What do you think about to unify these quries with something like
message QueryEstimatePlaceLimitOrderRequest {
MsgPlaceLimitOrder msg = 1;
}to not copypaste whole bunch of fields. The only downside i can see you can get rid of creator and receiver in a query
Task: https://hadronlabs.atlassian.net/browse/NTRN-318
Related PRs:
neutron-org/neutron-sdk#149
neutron-org/neutron-dev-contracts#55
neutron-org/neutron-integration-tests#315
List of changes: