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

Panic Handling In MultihopEstimateOutGivenExactAmountIn borks Protorev #4180

Closed
Tracked by #4170
NotJeremyLiu opened this issue Jan 31, 2023 · 1 comment · Fixed by #4248
Closed
Tracked by #4170

Panic Handling In MultihopEstimateOutGivenExactAmountIn borks Protorev #4180

NotJeremyLiu opened this issue Jan 31, 2023 · 1 comment · Fixed by #4248
Assignees

Comments

@NotJeremyLiu
Copy link
Contributor

Background

Panics are caught when swapping (https://github.com/osmosis-labs/osmosis/blob/main/x/gamm/keeper/swap.go#L39-L44) but not when estimating a swap (

func (k Keeper) MultihopEstimateOutGivenExactAmountIn(
).

The specific panic that brought this to our attention for Protorev was in stableswap solveCFMMBinarySearchMulti: (

panic("cannot input more than pool reserves")
), but generally any of these panics could likely cause an issue if MultihopEstimateOutGivenExactAmountIn does not catch the panics.

Suggested Design

We are open to different forms of resolution, whether that may be core team adding panic catching in MultihopEstimateOutGivenExactAmountIn or us catching panics in Protorev, but we are waiting for core team decision on best way forward.

Acceptance Criteria

All current protorev tests pass, and adding new tests to purposefully trigger the panics does not result in the actual panic to cause a halt.

@ValarDragon
Copy link
Member

I think we should definitely just catch this panic -- quite silly for estimate to panic haha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
4 participants