Fix: Remove balance check from EstimateSwap #2764

mattverse · 2022-09-16T04:32:11Z

Closes: #2337

What is the purpose of the change

EstimateSwap currently checks the balance of the query sender, not because it actually moves the assets on an actual swap, but beacuse it uses the same API with the actual multi swaps that happen.

This PR separates out the API needed for the estimation so that it does not check balance of the query sender.

(Special thanks to @vuong177 the original author of this PR!)

Brief Changelog

Remove balance check from EstimateSwap

Testing and Verifying

This change added tests.

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? yes
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.mxd? yes
How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

wasmbinding/message_plugin.go

ValarDragon

Reviewed all of the wasmbinding and proto changes, looks great to me! Reviewing keeper logic now

x/gamm/keeper/estimate_swap.go

ValarDragon · 2022-09-20T09:49:17Z

(Had a call to rename the internal estimate methods to better indicate that they can really change state, and move cacheCtx from query into the go API for public Estimate methods

x/gamm/keeper/estimate_swap.go

wasmbinding/queries.go

x/gamm/keeper/grpc_query_test.go

x/gamm/keeper/estimate_swap.go

x/gamm/keeper/swap.go

github-actions · 2022-10-07T00:05:55Z

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!

czarcas7ic

Left some comments, main thing is changing the multi-hop logic to match the new design here #3352

Also, unclear what is going on with the proto gen CI check

wasmbinding/message_plugin.go

czarcas7ic · 2022-11-14T18:53:50Z

wasmbinding/queries.go

+	if swap == nil {
+		return nil, wasmvmtypes.InvalidRequest{Err: "gamm perform swap null swap"}
+	}
+	if swap.Amount.ExactIn != nil {


Shouldn't we double check swap.Amount.ExactIn != nil && swap.Amount.ExactOut == nil here?

Why do you think that's the case? AFAIU, i think swap.Amount.ExactIn != nil && swap.Amount.ExactOut == nil can be a possible scenario, for example, when a pool is 1:1 ratio(10 uosmo, 10 uatom), you could be expecting 1 in 1out or similar

Right, not only CAN it be a possible scenario, it should be the only scenario for the one sided argument. What I am trying to prevent here is if someone provides a SwapMsg with both a swap.Amount.ExactIn and swap.Amount.ExactOut, with this current code instead of erroring they would go through the first for loop. EstimatePerformSwap should only return a valid response if one side is given, not both sides

AFAIU, we should never call an estimate method with both the in and the out. We either give it the in to calculate the out or we give it the out to calculate the in

I still feel this is the case

x/gamm/keeper/estimate_swap.go

x/gamm/keeper/estimate_swap_test.go

czarcas7ic · 2022-11-15T16:29:01Z

Wait, but wouldn't it be as simple as adding a "useCacheCtx" boolean here

osmosis/x/gamm/keeper/multihop.go

Lines 13 to 19 in bef8a4b

    
           func (k Keeper) MultihopSwapExactAmountIn( 
        
           	ctx sdk.Context, 
        
           	sender sdk.AccAddress, 
        
           	routes []types.SwapAmountInRoute, 
        
           	tokenIn sdk.Coin, 
        
           	tokenOutMinAmount sdk.Int, 
        
           ) (tokenOutAmount sdk.Int, err error) {

and then we could get rid of the entire estimate_swap.go file?

mattverse · 2022-11-16T03:27:55Z

@czarcas7ic I actaully think that's a great idea. Removed the whole file of estimate swap in my last commit and changed the existing swap methods to have a boolean filed for the estimate swaps.

I think this requires detailed, in-depth review as it touches the core swap logic

czarcas7ic · 2022-11-16T03:29:04Z

@mattverse will review in depth tomorrow!

p0mvn · 2022-11-17T21:37:27Z

Is this v13 blocking?

p0mvn · 2022-11-17T21:38:40Z

NVM, I see that we went with the code duplication approach for safety here: #3424

ValarDragon · 2022-11-18T18:46:34Z

@mattverse should we close this PR, and re-PR with elements you want added in still?

ValarDragon · 2022-12-02T21:35:44Z

Closing per last comment

estimate swap

05aaad1

mattverse requested a review from a team September 16, 2022 04:32

github-actions bot added C:CLI C:docs Improvements or additions to documentation C:x/gamm Changes, features and bugs related to the gamm module. labels Sep 16, 2022

mattverse added 2 commits September 16, 2022 17:05

Add tests

88c0634

Update refelct wasm binary

4a88c3f

mattverse mentioned this pull request Sep 16, 2022

Remove sender from estimate swap osmosis-labs/bindings#38

Open

mattverse added this to the V12 Blockers milestone Sep 16, 2022

ValarDragon reviewed Sep 19, 2022

View reviewed changes

wasmbinding/message_plugin.go Show resolved Hide resolved

ValarDragon reviewed Sep 19, 2022

View reviewed changes

wasmbinding/message_plugin.go Show resolved Hide resolved

ValarDragon reviewed Sep 19, 2022

View reviewed changes

x/gamm/keeper/estimate_swap.go Outdated Show resolved Hide resolved

mattverse added the A:backport/v12.x backport patches to v12.x branch label Sep 20, 2022

Change fn name + comment

1bae96f

Further seperate out methods

e67f26a

mattverse commented Sep 20, 2022

View reviewed changes

x/gamm/keeper/estimate_swap.go Outdated Show resolved Hide resolved

p0mvn reviewed Sep 20, 2022

View reviewed changes

mattverse added 2 commits September 21, 2022 15:38

Add cache ctx argument, add test cases

2fd212e

Remove update pool

d243468

p0mvn removed this from the V12 Blockers milestone Sep 21, 2022

p0mvn removed the A:backport/v12.x backport patches to v12.x branch label Sep 21, 2022

mattverse added this to the V13 milestone Sep 22, 2022

mattverse mentioned this pull request Oct 5, 2022

Cannot Estimate Swap in v12 #2943

Closed

github-actions bot added the Stale label Oct 7, 2022

github-actions bot closed this Oct 14, 2022

ValarDragon reopened this Oct 26, 2022

github-actions bot removed the C:docs Improvements or additions to documentation label Oct 26, 2022

github-actions bot removed the Stale label Oct 27, 2022

mattverse requested review from ValarDragon and p0mvn November 1, 2022 07:20

Merge branch 'concentrated-liquidity-main' into mattverse/cl-pool

9b4161b

mattverse added the V:state/breaking State machine breaking PR label Nov 14, 2022

Merge branch 'main' into mattverse/estimate-swap

5fe4089

czarcas7ic reviewed Nov 14, 2022

View reviewed changes

mattverse added 3 commits November 15, 2022 12:59

Adams review

d1ec07f

Merge branch 'main' into mattverse/estimate-swap

08b120d

Fix merge conflicts

c5c582a

mattverse mentioned this pull request Nov 15, 2022

De-duplicate code from MultihopSwapExactAmountIn and EstimateMultihopSwapExactAmountIn #3386

Open

Proto update

6f8066d

Just remove estimate swap:)

1af43f9

github-actions bot added C:app-wiring Changes to the app folder C:x/txfees labels Nov 16, 2022

Merge main

d427b0d

p0mvn removed this from the V13 milestone Nov 17, 2022

ValarDragon closed this Dec 2, 2022

czarcas7ic deleted the mattverse/estimate-swap branch August 7, 2023 23:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Remove balance check from EstimateSwap #2764

Fix: Remove balance check from EstimateSwap #2764

mattverse commented Sep 16, 2022

ValarDragon left a comment

ValarDragon commented Sep 20, 2022

github-actions bot commented Oct 7, 2022

czarcas7ic left a comment •

edited

Loading

czarcas7ic Nov 14, 2022

mattverse Nov 15, 2022

czarcas7ic Nov 15, 2022 •

edited

Loading

czarcas7ic Nov 15, 2022

czarcas7ic Nov 18, 2022

czarcas7ic commented Nov 15, 2022

mattverse commented Nov 16, 2022

czarcas7ic commented Nov 16, 2022

p0mvn commented Nov 17, 2022

p0mvn commented Nov 17, 2022

ValarDragon commented Nov 18, 2022

ValarDragon commented Dec 2, 2022

Fix: Remove balance check from EstimateSwap #2764

Fix: Remove balance check from EstimateSwap #2764

Conversation

mattverse commented Sep 16, 2022

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

ValarDragon left a comment

Choose a reason for hiding this comment

ValarDragon commented Sep 20, 2022

github-actions bot commented Oct 7, 2022

czarcas7ic left a comment • edited Loading

Choose a reason for hiding this comment

czarcas7ic Nov 14, 2022

Choose a reason for hiding this comment

mattverse Nov 15, 2022

Choose a reason for hiding this comment

czarcas7ic Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

czarcas7ic Nov 15, 2022

Choose a reason for hiding this comment

czarcas7ic Nov 18, 2022

Choose a reason for hiding this comment

czarcas7ic commented Nov 15, 2022

mattverse commented Nov 16, 2022

czarcas7ic commented Nov 16, 2022

p0mvn commented Nov 17, 2022

p0mvn commented Nov 17, 2022

ValarDragon commented Nov 18, 2022

ValarDragon commented Dec 2, 2022

czarcas7ic left a comment •

edited

Loading

czarcas7ic Nov 15, 2022 •

edited

Loading