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.

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

MultihopEstimateInGivenExactAmountOut and MultihopEstimateOutGivenExactAmountIn panic recovering #4248

Merged
merged 19 commits into from
Feb 8, 2023

Conversation

pysel
Copy link
Member

@pysel pysel commented Feb 7, 2023

Closes: #4180

What is the purpose of the change

Panic recovery in MultihopEstimateInGivenExactAmountOut and MultihopEstimateOutGivenExactAmountIn

Brief Changelog

Adds a defer statement that cathes panic and returns empty struct corresponding to function's return type. Similar to
https://github.com/osmosis-labs/osmosis/blob/main/x/gamm/keeper/swap.go#L39-L44

Testing and Verifying

Adds unit tests

@pysel pysel changed the title EstimateMultihopIn/Out panic recovering MultihopEstimateInGivenExactAmountOut and MultihopEstimateOutGivenExactAmountIn panic recovering Feb 7, 2023
@pysel pysel added V:state/compatible/backport State machine compatible PR, should be backported V:state/compatible/no_backport State machine compatible PR, depends on prior breaks and removed V:state/compatible/backport State machine compatible PR, should be backported labels Feb 7, 2023
@pysel pysel marked this pull request as draft February 7, 2023 14:18
@github-actions github-actions bot added the C:app-wiring Changes to the app folder label Feb 7, 2023
@pysel pysel force-pushed the ruslan/panic-recovering-multihop branch from 3f3a16b to 04be5ff Compare February 7, 2023 14:26
@github-actions github-actions bot removed the C:app-wiring Changes to the app folder label Feb 7, 2023
@github-actions github-actions bot added the C:app-wiring Changes to the app folder label Feb 7, 2023
x/poolmanager/router_test.go Outdated Show resolved Hide resolved
x/poolmanager/router_test.go Outdated Show resolved Hide resolved
x/poolmanager/router_test.go Outdated Show resolved Hide resolved
x/poolmanager/router_test.go Outdated Show resolved Hide resolved
@pysel pysel marked this pull request as ready for review February 7, 2023 17:29
x/poolmanager/router_test.go Outdated Show resolved Hide resolved
x/poolmanager/router_test.go Outdated Show resolved Hide resolved
Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

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

Nice work

@p0mvn p0mvn added V:state/breaking State machine breaking PR and removed V:state/compatible/no_backport State machine compatible PR, depends on prior breaks labels Feb 7, 2023
@p0mvn
Copy link
Member

p0mvn commented Feb 7, 2023

@pysel please add a changelog entry prior to merge

Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

Nice LGTM! Echo the needs changelog

@pysel
Copy link
Member Author

pysel commented Feb 8, 2023

added changelog entry

@p0mvn p0mvn added the A:backport/v15.x backport patches to v15.x branch label Feb 8, 2023
@p0mvn p0mvn merged commit a33893d into main Feb 8, 2023
@p0mvn p0mvn deleted the ruslan/panic-recovering-multihop branch February 8, 2023 09:15
mergify bot pushed a commit that referenced this pull request Feb 8, 2023
…ExactAmountIn` panic recovering (#4248)

* add panic recovering to multihop estimate functions

* unit test for making sure recovery works as expected

* change error message

* separate tests

* refactoring

* refactor

* add panic catching to RouteExactAmountOut

* add panic catching assertions to existing tests

* delete old unit tests

* remove fmt

* hardcoded stableswap to enum

* getSetupPoolsStrategy as a helper function

* remove callback return

* remove redundant swap fee variable

* .String rm

* docs fix/rename

* typo

* typo2

* changelog entry

(cherry picked from commit a33893d)
p0mvn pushed a commit that referenced this pull request Feb 8, 2023
…ExactAmountIn` panic recovering (#4248) (#4258)

* add panic recovering to multihop estimate functions

* unit test for making sure recovery works as expected

* change error message

* separate tests

* refactoring

* refactor

* add panic catching to RouteExactAmountOut

* add panic catching assertions to existing tests

* delete old unit tests

* remove fmt

* hardcoded stableswap to enum

* getSetupPoolsStrategy as a helper function

* remove callback return

* remove redundant swap fee variable

* .String rm

* docs fix/rename

* typo

* typo2

* changelog entry

(cherry picked from commit a33893d)

Co-authored-by: Ruslan Akhtariev <[email protected]>
@github-actions github-actions bot mentioned this pull request May 1, 2024
@github-actions github-actions bot mentioned this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:backport/v15.x backport patches to v15.x branch C:app-wiring Changes to the app folder C:x/poolmanager V:state/breaking State machine breaking PR
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Panic Handling In MultihopEstimateOutGivenExactAmountIn borks Protorev
3 participants