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

Update twap response #2963

Closed
wants to merge 9 commits into from
Closed

Update twap response #2963

wants to merge 9 commits into from

Conversation

hieuvubk
Copy link
Contributor

@hieuvubk hieuvubk commented Oct 7, 2022

Closes: #2622

What is the purpose of the change

Add a bool field for "is response unstable" to the twap queries

Brief Changelog

(for example:)

  • Update proto
  • Check the err in the queries to return the appropriate response
  • Add tests

@hieuvubk hieuvubk requested a review from a team October 7, 2022 10:52
@github-actions github-actions bot added C:CLI C:x/twap Changes to the twap module labels Oct 7, 2022
@hieuvubk hieuvubk requested a review from ValarDragon October 10, 2022 04:48
mattverse
mattverse reviewed Oct 11, 2022
View reviewed changes
Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

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

Oh i see the is_response_unstable fields is for when the twap query fails.

Don't know how others feel about this, but my suggestion would be changing the field name from is_response_unstable to success

@hieuvubk
Copy link
Contributor Author

Oh i see the is_response_unstable fields is for when the twap query fails.

Don't know how others feel about this, but my suggestion would be changing the field name from is_response_unstable to success

need to change some logic but yess

@hieuvubk hieuvubk requested a review from mattverse October 14, 2022 09:39
mattverse
mattverse approved these changes Oct 14, 2022
View reviewed changes
Copy link
Member

@mattverse mattverse 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 @hieuvubk ! This LGTM!

@github-actions
Copy link
Contributor

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!

@github-actions github-actions bot added the Stale label Oct 29, 2022
@mattverse mattverse added V:state/compatible/backport State machine compatible PR, should be backported and removed Stale labels Oct 29, 2022
czarcas7ic
czarcas7ic approved these changes Oct 31, 2022
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.

I believe the intent was to still provide the last known TWAP despite it erroring. If that is true, this LGTM!

@mattverse
Copy link
Member

@hieuvubk Can you look into proto gen ( if it's correctly updated) before we merge this? (Upon seeing check proto CI failing)

@hieuvubk
Copy link
Contributor Author

hieuvubk commented Nov 1, 2022

@hieuvubk Can you look into proto gen ( if it's correctly updated) before we merge this? (Upon seeing check proto CI failing)

I already ran both make proto-all & make proto-gen but it still happen. Seem we got this CI err before.

@mattverse
Copy link
Member

@hieuvubk interesting, can you try merging with current main on ur branch and then running it?

@hieuvubk hieuvubk force-pushed the update_twap_response branch from f9d92d0 to 460f106 Compare November 1, 2022 09:54
@hieuvubk
Copy link
Contributor Author

hieuvubk commented Nov 1, 2022

@hieuvubk interesting, can you try merging with current main on ur branch and then running it?

Still got the check-proto err

@github-actions
Copy link
Contributor

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!

@github-actions github-actions bot added Stale and removed Stale labels Nov 25, 2022
@hieuvubk hieuvubk force-pushed the update_twap_response branch from 571b017 to 38d4c37 Compare December 1, 2022 14:56
@github-actions
Copy link
Contributor

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!

@github-actions github-actions bot added the Stale label Dec 16, 2022
@faddat
Copy link
Member

faddat commented Dec 18, 2022

trying a quick make proto-all here

@faddat
Copy link
Member

faddat commented Dec 18, 2022

hey @hieuvubk -- I suspect that you may have found a bug with this one.

Like you, when I try it, I end up in the same spot that you did.

@github-actions github-actions bot removed the Stale label Dec 19, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jan 3, 2023

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!

@github-actions github-actions bot added the Stale label Jan 3, 2023
@github-actions github-actions bot closed this Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattverse mattverse mattverse approved these changes

@czarcas7ic czarcas7ic czarcas7ic approved these changes

@ValarDragon ValarDragon Awaiting requested review from ValarDragon

Assignees
No one assigned
Labels
C:CLI C:x/twap Changes to the twap module Stale V:state/compatible/backport State machine compatible PR, should be backported
Projects
Osmosis Chain Development
Status: Done ✅
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update TWAP responses, to make errors for incorrect error in time range, exposed in response
4 participants
@hieuvubk @mattverse @faddat @czarcas7ic