Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: fix estimateXX dex queries #NTRN-318 #543

Closed
wants to merge 13 commits into from
+296 −447
Closed

feat: fix estimateXX dex queries #NTRN-318 #543

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
e3ac929
don't check bank balance for estimateXX queries
jcompagni10 May 29, 2024
33b5549
misc cleanup
jcompagni10 May 29, 2024
9083cde
Merge remote-tracking branch 'origin/feat/sdk-50' into feat/fix_estim…
jcompagni10 May 29, 2024
8ff24d2
Add tests for EstimatePlaceLimitOrder
jcompagni10 May 30, 2024
cf0ccf6
lint
jcompagni10 May 30, 2024
fe1ab28
keep wire numbers consistent
jcompagni10 May 30, 2024
fcf9feb
fix query wire numbers
jcompagni10 May 31, 2024
4d67e6f
Merge remote-tracking branch 'origin/feat/sdk-50' into feat/fix_estim…
jcompagni10 May 31, 2024
d236ce9
add comment explaining random address in EstimateXX
jcompagni10 May 31, 2024
4d540e1
Merge remote-tracking branch 'origin/main' into feat/fix_estimateXX
jcompagni10 Jul 23, 2024
8a9aed7
use execute fns for simulation queries
jcompagni10 Jul 23, 2024
ffcec4f
add err return
jcompagni10 Jul 23, 2024
cd8d9e6
proto regen
jcompagni10 Jul 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions proto/neutron/dex/query.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,8 +260,6 @@ message QueryGetPoolReservesResponse {
}

message QueryEstimateMultiHopSwapRequest {
string creator = 1;
string receiver = 2;
repeated MultiHopRoute routes = 3;
string amount_in = 4 [
(gogoproto.moretags) = "yaml:\"amount_in\"",
Expand All @@ -275,7 +273,6 @@ message QueryEstimateMultiHopSwapRequest {
(gogoproto.nullable) = false,
(gogoproto.jsontag) = "exit_limit_price"
];

// If pickBestRoute == true then all routes are run and the route with the
// best price is chosen otherwise, the first succesful route is used.
bool pick_best_route = 6;
Expand All @@ -290,8 +287,6 @@ message QueryEstimateMultiHopSwapResponse {
}

message QueryEstimatePlaceLimitOrderRequest {
string creator = 1;
string receiver = 2;
string token_in = 3;
string token_out = 4;
int64 tick_index_in_to_out = 5;
Expand All @@ -302,13 +297,12 @@ message QueryEstimatePlaceLimitOrderRequest {
(gogoproto.jsontag) = "amount_in"
];
LimitOrderType order_type = 7;

// expirationTime is only valid iff orderType == GOOD_TIL_TIME.
google.protobuf.Timestamp expiration_time = 8 [
google.protobuf.Timestamp expiration_time = 9 [
jcompagni10 marked this conversation as resolved.
Show resolved Hide resolved
(gogoproto.stdtime) = true,
(gogoproto.nullable) = true
];
string maxAmount_out = 9 [
string max_amount_out = 8 [
(gogoproto.moretags) = "yaml:\"max_amount_out\"",
(gogoproto.customtype) = "cosmossdk.io/math.Int",
(gogoproto.nullable) = true,
Expand Down
2 changes: 0 additions & 2 deletions wasmbinding/queries.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,6 @@ func (qp *QueryPlugin) DexQuery(ctx sdk.Context, query bindings.DexQuery) (data
data, err = dexQuery(ctx, query.EstimateMultiHopSwap, qp.dexKeeper.EstimateMultiHopSwap)
case query.EstimatePlaceLimitOrder != nil:
q := dextypes.QueryEstimatePlaceLimitOrderRequest{
Creator: query.EstimatePlaceLimitOrder.Creator,
Receiver: query.EstimatePlaceLimitOrder.Receiver,
TokenIn: query.EstimatePlaceLimitOrder.TokenIn,
TokenOut: query.EstimatePlaceLimitOrder.TokenOut,
TickIndexInToOut: query.EstimatePlaceLimitOrder.TickIndexInToOut,
Expand Down
105 changes: 105 additions & 0 deletions x/dex/keeper/grpc_estimate_place_limit_order_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
package keeper_test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


import (
"cosmossdk.io/math"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"

"github.com/neutron-org/neutron/v4/x/dex/types"
)

func (s *DexTestSuite) TestEstimatePlaceLimitOrderGTC() {
// GIVEN liquidity A<>B
s.SetupMultiplePools(
NewPoolSetup("TokenA", "TokenB", 0, 1, 0, 1),
NewPoolSetup("TokenA", "TokenB", 0, 1, 1, 1),
)

// WHEN estimate GTC Limit order selling "TokenA"
resp, err := s.App.DexKeeper.EstimatePlaceLimitOrder(s.Ctx, &types.QueryEstimatePlaceLimitOrderRequest{
TokenIn: "TokenA",
TokenOut: "TokenB",
TickIndexInToOut: 4,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

AmountIn: math.NewInt(3_000_000),
OrderType: types.LimitOrderType_GOOD_TIL_CANCELLED,
})
s.NoError(err)

// Then estimate is 3 BIG TokenA in with 2 BIG TokenB out from swap
s.True(math.NewInt(3_000_000).Equal(resp.TotalInCoin.Amount), "Got %v", resp.TotalInCoin.Amount)
s.Equal("TokenA", resp.TotalInCoin.Denom)

s.True(math.NewInt(2_000_301).Equal(resp.SwapInCoin.Amount), "Got %v", resp.SwapInCoin.Amount)
s.Equal("TokenA", resp.SwapInCoin.Denom)

s.True(math.NewInt(2_000_000).Equal(resp.SwapOutCoin.Amount), "Got %v", resp.SwapOutCoin.Amount)
s.Equal("TokenB", resp.SwapOutCoin.Denom)

// AND state is not altered
s.assertDexBalanceWithDenom("TokenA", 0)
s.assertDexBalanceWithDenom("TokenB", 2)

// No events are emitted
s.AssertEventValueNotEmitted(types.TickUpdateEventKey, "Expected no events")

// Subsequent transactions use the original BankKeeper
// ie. The simulation bankkeeper is not retained giving users unlimited funds
s.assertBobLimitSellFails(sdkerrors.ErrInsufficientFunds, "TokenA", -400_000, 100_000_000)
}

func (s *DexTestSuite) TestEstimatePlaceLimitOrderFoK() {
// GIVEN liquidity TokenB
s.SetupMultiplePools(
NewPoolSetup("TokenA", "TokenB", 0, 1, 0, 1),
NewPoolSetup("TokenA", "TokenB", 0, 1, 1, 1),
)

// WHEN estimate FoK Limit order selling "TokenA"
resp, err := s.App.DexKeeper.EstimatePlaceLimitOrder(s.Ctx, &types.QueryEstimatePlaceLimitOrderRequest{
TokenIn: "TokenA",
TokenOut: "TokenB",
TickIndexInToOut: 4,
AmountIn: math.NewInt(1_500_000),
OrderType: types.LimitOrderType_FILL_OR_KILL,
})
s.NoError(err)

// Then estimate is 1.5 BIG TokenA in with 1.5 BIG TokenB out from swap
s.True(math.NewInt(1_500_000).Equal(resp.TotalInCoin.Amount), "Got %v", resp.TotalInCoin.Amount)
s.Equal("TokenA", resp.TotalInCoin.Denom)

s.True(math.NewInt(1_500_000).Equal(resp.SwapInCoin.Amount), "Got %v", resp.SwapInCoin.Amount)
s.Equal("TokenA", resp.SwapInCoin.Denom)

s.True(math.NewInt(1_499_800).Equal(resp.SwapOutCoin.Amount), "Got %v", resp.SwapOutCoin.Amount)
s.Equal("TokenB", resp.SwapOutCoin.Denom)

// AND state is not altered
s.assertDexBalanceWithDenom("TokenA", 0)
s.assertDexBalanceWithDenom("TokenB", 2)

// No events are emitted
s.AssertEventValueNotEmitted(types.TickUpdateEventKey, "Expected no events")

// Subsequent transactions use the original BankKeeper
s.assertBobLimitSellFails(sdkerrors.ErrInsufficientFunds, "TokenA", -400_000, 100_000_000)
}

func (s *DexTestSuite) TestEstimatePlaceLimitOrderFoKFails() {
// GIVEN no liquidity

// WHEN estimate placeLimitOrder
resp, err := s.App.DexKeeper.EstimatePlaceLimitOrder(s.Ctx, &types.QueryEstimatePlaceLimitOrderRequest{
TokenIn: "TokenA",
TokenOut: "TokenB",
TickIndexInToOut: 4,
AmountIn: math.NewInt(1_500_000),
OrderType: types.LimitOrderType_IMMEDIATE_OR_CANCEL,
})

// THEN error is returned
s.ErrorIs(err, types.ErrLimitPriceNotSatisfied)
s.Nil(resp)

// Subsequent transactions use the original BankKeeper
s.assertBobLimitSellFails(sdkerrors.ErrInsufficientFunds, "TokenA", -400_000, 100_000_000)
}
17 changes: 8 additions & 9 deletions x/dex/keeper/grpc_query_estimate_multi_hop_swap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,13 @@ import (
"github.com/neutron-org/neutron/v4/x/dex/types"
)

// TODO: This doesn't run ValidateBasic() checks.
func (k Keeper) EstimateMultiHopSwap(
goCtx context.Context,
req *types.QueryEstimateMultiHopSwapRequest,
) (*types.QueryEstimateMultiHopSwapResponse, error) {
msg := types.MsgMultiHopSwap{
Creator: req.Creator,
Receiver: req.Receiver,
Creator: "neutron1dft8nwxzr0u27wvr2cknpermjkreqvp9fdy0uz",
Receiver: "neutron1dft8nwxzr0u27wvr2cknpermjkreqvp9fdy0uz",
Comment on lines +17 to +18
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this address?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably need to create another message QueryMultiHopSwap with it's own validate and reusing validate logic somehow if possible.
Otherwise kinda ugly

Routes: req.Routes,
AmountIn: req.AmountIn,
ExitLimitPrice: req.ExitLimitPrice,
Expand All @@ -28,23 +27,23 @@ func (k Keeper) EstimateMultiHopSwap(
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)
Copy link
Contributor

@sotnikov-s sotnikov-s Jun 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more or less the same for PlaceLimitOrderCore

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, i prefer a pure function that does estimation.

coinOut, err := k.MultiHopSwapCore(
cacheCtx,
req.AmountIn,
req.Routes,
req.ExitLimitPrice,
req.PickBestRoute,
callerAddr,
receiverAddr,
[]byte("caller"),
[]byte("receiver"),
)
if err != nil {
return nil, err
}

// NB: Critically, we do not write the best route's buffered state context since this is only an estimate.
//nolint:staticcheck // Should be unnecessary but out of an abundance of caution we restore the old bankkeeper
k.bankKeeper = oldBk

return &types.QueryEstimateMultiHopSwapResponse{CoinOut: coinOut}, nil
}
Loading
Loading