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

Twap Wasm Bindings #2212

Merged
merged 10 commits into from
Aug 5, 2022
Merged

Twap Wasm Bindings #2212

merged 10 commits into from
Aug 5, 2022

Conversation

mattverse
Copy link
Member

Sub component of : #2187

What is the purpose of the change

This PR adds wasm bindings for query on two APIs GetArithmeticTWAP and GetArithmeticTwapToNow.

This PR invlolves adding twap keeper to the wasm query plugin to use the API provided by the twap keeper.

Brief Changelog

  • Add wasm bindings for twap APIs(GetArithmeticTWAP, GetArithmeticTWAPToNow)

Testing and Verifying

Testing is TBD and to be sorted out, as of right now, do not have exact test cases for testing.

Documentation and Release Note

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

@mattverse mattverse requested a review from a team July 23, 2022 16:04
@github-actions github-actions bot added the C:app-wiring Changes to the app folder label Jul 23, 2022
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.

utACK

@mattverse
Copy link
Member Author

Figuring plans for testing w/ exact numbers for TWAP

@mattverse mattverse marked this pull request as draft July 25, 2022 14:31
@mattverse
Copy link
Member Author

Converting to draft.

Current status for this PR is that I've added test codes for new queries in custom bindings test, whilst getting error Error parsing into type osmo_reflect::msg::QueryMsg: unknown variant arithmetic_twap, expected one of full_denom, pool_state, spot_price, estimate_swap: query wasm contract failed. Further investigation needed in order to figure out where this is coming from / why / where this is happening

@mattverse mattverse requested a review from ethanfrey July 25, 2022 14:34
@mattverse mattverse closed this Jul 30, 2022
@mattverse mattverse reopened this Aug 2, 2022
@mattverse
Copy link
Member Author

Confirmed that this is happening bc we don't have the .wasm files thats used for tests updated with the bindings(rust side). Replacing those files with the updated .wasm(after the rust side code gets reviewed and merged) files should fix the broken tests

@ValarDragon
Copy link
Member

This LGTM!

@mattverse mattverse marked this pull request as ready for review August 4, 2022 09:45
@nicolaslara
Copy link
Contributor

This looks good to me.

My only comment is that it's not clear which queries should have their own bindings and which should go through StargateQuery with convenience wrappers in https://github.com/osmosis-labs/osmosis-rust. I don't see manipulation of the query result to avoid non-determinism, in which case adding it to the stargate query whitelist should have the same effect.

@mattverse mattverse mentioned this pull request Aug 4, 2022
5 tasks
@mattverse mattverse force-pushed the mattverse/twap-go-bindings branch from 6b92ff0 to cb64a5b Compare August 4, 2022 15:28
reflect := instantiateReflectContract(t, ctx, osmosis, actor)
require.NotEmpty(t, reflect)

// TODO: figure out why it errors when we use start time without second
Copy link
Member

Choose a reason for hiding this comment

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

wdym

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see, the query errors in this case, interesting

Copy link
Member

Choose a reason for hiding this comment

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

here's the error message: looking for a time thats too old, not in the historical index. Try storing the accumulator value.

Copy link
Member

Choose a reason for hiding this comment

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

ah the error is correct, this is a test error here =p

Committing fix here.

Copy link
Member

Choose a reason for hiding this comment

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

Hrmm actually its not so clean, this is actually a deeper issue with the binding using Unix time for the time passing here.

The reason its not working at the start time is because its being passed in unix time in miliseocnds. However the state record has greater precision.

Trying to see more about how to fix this

Copy link
Member

Choose a reason for hiding this comment

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

So I think what we need to do is make the TWAP binding parse from UnixMiliseconds, and then in the state machine we write at UnixMilisecond precision.

For this PR, can we change the binding to be from Unix milisecond and then call it good from that? Then in the test if we set ctx.BlockTime to be at a milisecond boundary, this should be fixed. Once we do #2209 should also get fixed state machine side as well.

@ValarDragon ValarDragon mentioned this pull request Aug 5, 2022
20 tasks
@github-actions github-actions bot added the C:x/gamm Changes, features and bugs related to the gamm module. label Aug 5, 2022
@ValarDragon
Copy link
Member

ValarDragon commented Aug 5, 2022

@mattverse please check my latest commit to fix the time threading.

if you agree, please make docs for the binding, for start time being in Unix time miliseconds. (and more importantly make the docs rust side)

Also communicate to Mars that we can change the time input API to be whatever they'd prefer.

Right now there will be an edge case with StartTime for a TWAP, that will get fixed in a subsequent commit pre-release. Namely that if start time = pool creation time, and the block time is not milisecond aligned, it may not work. Setting it to be 1 milisecond after pool creation time would fix this for now as workaround, until next week we change it state machine to not be nanosecond precise in state machine storage.

(The state machine computes the correct result, its that this binding API doesn't offer sufficient precision)

@mattverse
Copy link
Member Author

Agreed with using Unix time millisecond for now for precision, updated documentation

Base automatically changed from dev/twap_wip to main August 5, 2022 19:53
@ValarDragon ValarDragon force-pushed the mattverse/twap-go-bindings branch from e905c3b to af28fad Compare August 5, 2022 19:56
@github-actions github-actions bot removed the C:x/gamm Changes, features and bugs related to the gamm module. label Aug 5, 2022
@ValarDragon
Copy link
Member

(I rebased this after I merged the twap branch)

poolId := arithmeticTwap.PoolId
quoteAssetDenom := arithmeticTwap.QuoteAssetDenom
baseAssetDenom := arithmeticTwap.BaseAssetDenom
startTime := time.Unix(arithmeticTwap.StartTime, 0)
Copy link
Member

Choose a reason for hiding this comment

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

ah we forgot to make this milli in the version we sent to mars

Copy link
Member

Choose a reason for hiding this comment

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

I can spin one up fairly quickly if this needs to be changed

@ValarDragon ValarDragon merged commit 2c85c41 into main Aug 5, 2022
@ValarDragon ValarDragon deleted the mattverse/twap-go-bindings branch August 5, 2022 20:29
@ValarDragon ValarDragon restored the mattverse/twap-go-bindings branch August 5, 2022 20:29
@czarcas7ic czarcas7ic mentioned this pull request Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:app-wiring Changes to the app folder
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants