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

Jump to bottom

fix: {overflow} bug when querying cosmwasmpool spot price #6710

Merged
merged 3 commits into from
Oct 25, 2023
Merged

fix: {overflow} bug when querying cosmwasmpool spot price #6710

merged 3 commits into from
Oct 25, 2023

Conversation

iboss-ptk
Copy link
Contributor

@iboss-ptk iboss-ptk commented Oct 17, 2023 •
edited
Loading

What is the purpose of the change

Add a description of the overall background and high level changes that this PR introduces

Fix {overflow} bug when querying cosmwasmpool spot price.

This is caused by mapping sdk gas to wasmvm gas which normal gas limit will likely cause overflow due to high number.

The fix is to use queryGasLimit from wasmkeeper to keep the vm gas under uint64.

Testing and Verifying

Verified that querying spotprice from transmuter v2.0.0 cosmwasmpool has no issue.

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes?
  • Changelog entry added to Unreleased section of CHANGELOG.md?

@github-actions
Copy link
Contributor

Important Notice

This PR modifies an in-repo Go module. It is one of:

  • osmomath
  • osmoutils
  • x/ibc-hooks
  • x/epochs

The dependent Go modules, especially the root one, will have to be
updated to reflect the changes. Failing to do so might cause e2e to fail.

Please follow the instructions below:

  1. Open https://github.com/osmosis-labs/osmosis/actions/workflows/go-mod-auto-bump.yml
  2. Provide the current branch name
  3. On success, confirm if an automated commit corretly updated the go.mod and go.sum files

Please let us know if you need any help.

@iboss-ptk iboss-ptk changed the title Fix {overflow} bug when querying cosmwasmpool spot price fix: {overflow} bug when querying cosmwasmpool spot price Oct 17, 2023
@iboss-ptk iboss-ptk marked this pull request as ready for review October 17, 2023 08:17
@iboss-ptk iboss-ptk added the V:state/compatible/backport State machine compatible PR, should be backported label Oct 17, 2023
czarcas7ic
czarcas7ic approved these changes Oct 17, 2023
View reviewed changes
Copy link
Member

@czarcas7ic czarcas7ic left a comment

Choose a reason for hiding this comment

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

high level ack, would like one other as well before merge. thanks boss 👍

ValarDragon
ValarDragon reviewed Oct 20, 2023
View reviewed changes
osmoutils/cosmwasm/helpers.go
@@ -51,6 +54,7 @@ func Query[T any, K any](ctx sdk.Context, wasmKeeper WasmKeeper, contractAddress
return response, err
}

ctx = ctx.WithGasMeter(storetypes.NewGasMeter(wasmKeeper.QueryGasLimit()))
Copy link
Member

@ValarDragon ValarDragon Oct 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If this is in consensus, then this is state breaking. I think protorev handled this, let me see if I can find a link. (Basically what happens if this query out of gas's)

Copy link
Member

Choose a reason for hiding this comment

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

This is only used in cosmwasmpool for CalcInAmountGivenOut (and the other dir)

can someone sanity check those are only used in queries? If so looks good to merge imo

Copy link
Member

Choose a reason for hiding this comment

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

Im pretty sure this is called within consensus :/

SwapExactAmountOut -> RouteExactAmountOut -> createMultihopExpectedSwapOuts -> swapModule.CalcInAmtGivenOut

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I think so to. And transmuter pools are active

nicolaslara
nicolaslara approved these changes Oct 20, 2023
View reviewed changes
Copy link
Contributor

@nicolaslara nicolaslara left a comment

Choose a reason for hiding this comment

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

The code looks good to merge with the caveat that dev mentioned that this is state breaking

@ValarDragon ValarDragon added V:state/breaking State machine breaking PR and removed V:state/compatible/backport State machine compatible PR, should be backported labels Oct 20, 2023
@ValarDragon ValarDragon merged commit ca49a1d into main Oct 25, 2023
1 check passed
@ValarDragon ValarDragon deleted the fix/wasm-query-gas-error branch October 25, 2023 15:36
ValarDragon pushed a commit that referenced this pull request Oct 26, 2023
@iboss-ptk @ValarDragon
fix: {overflow} bug when querying cosmwasmpool spot price (#6710)
11205e0 
* fix cosmwasm query helper gas limit

* update osmoutils version

* update changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ValarDragon ValarDragon ValarDragon left review comments

@czarcas7ic czarcas7ic czarcas7ic approved these changes

@nicolaslara nicolaslara nicolaslara approved these changes

@p0mvn p0mvn Awaiting requested review from p0mvn

Assignees
No one assigned
Labels
V:state/breaking State machine breaking PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@iboss-ptk @nicolaslara @ValarDragon @czarcas7ic