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

Add TWAP CLI #3214

Merged
merged 6 commits into from
Nov 2, 2022
Merged

Add TWAP CLI #3214

merged 6 commits into from
Nov 2, 2022

Conversation

ValarDragon
Copy link
Member

What is the purpose of the change

Add CLI query support for TWAP. I've eye ball-tested the values for correctness against imperator API. Ideally we add this logic to our querygen, or use proto-reflect from SDK to automate CLI boilerplate going for.

Testing and Verifying

Manually tested

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? yes
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? yes
  • How is the feature or change documented? CLI help

@ValarDragon ValarDragon added A:backport/v12.x backport patches to v12.x branch V:state/compatible/backport State machine compatible PR, should be backported labels Nov 2, 2022
@ValarDragon ValarDragon requested a review from a team November 2, 2022 07:10
@github-actions github-actions bot added C:app-wiring Changes to the app folder C:CLI C:x/twap Changes to the twap module labels Nov 2, 2022
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.

Overall LGTM, left some nit comments!

x/twap/client/cli/query.go Show resolved Hide resolved
}

// GetCmdAssetMultiplier returns multiplier of an asset by denom.
func GetQueryTwapCommand() *cobra.Command {
Copy link
Member

Choose a reason for hiding this comment

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

nit: don't know if its worth overhead, but would love to have some test cases on this since this includes lot of internal parsing. We could also make it a separate issue for this if u agree with the needs of testing for this

Copy link
Member Author

Choose a reason for hiding this comment

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

Happy if someone else wants to write tests for this, I think of it as pretty low priority.

I'd much rather allocate those mental cycles to getting most CLI code killed / better testing structures. (I believe SDK has progress on better CLI testing structure)

x/twap/client/cli/query.go Show resolved Hide resolved
ValarDragon and others added 2 commits November 2, 2022 09:29
Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>
@mergify mergify bot merged commit e3e8e27 into main Nov 2, 2022
@mergify mergify bot deleted the dev/add_twap_cli branch November 2, 2022 08:57
mergify bot pushed a commit that referenced this pull request Nov 2, 2022
## What is the purpose of the change

Add CLI query support for TWAP. I've eye ball-tested the values for correctness against imperator API. Ideally we add this logic to our querygen, or use proto-reflect from SDK to automate CLI boilerplate going for.

## Testing and Verifying

Manually tested

## Documentation and Release Note

  - Does this pull request introduce a new feature or user-facing behavior changes? yes
  - Is a relevant changelog entry added to the `Unreleased` section in `CHANGELOG.md`? yes
  - How is the feature or change documented? CLI help

(cherry picked from commit e3e8e27)

# Conflicts:
#	CHANGELOG.md
@mergify mergify bot mentioned this pull request Nov 2, 2022
ValarDragon added a commit that referenced this pull request Nov 2, 2022
* Add TWAP CLI (#3214)

## What is the purpose of the change

Add CLI query support for TWAP. I've eye ball-tested the values for correctness against imperator API. Ideally we add this logic to our querygen, or use proto-reflect from SDK to automate CLI boilerplate going for.

## Testing and Verifying

Manually tested

## Documentation and Release Note

  - Does this pull request introduce a new feature or user-facing behavior changes? yes
  - Is a relevant changelog entry added to the `Unreleased` section in `CHANGELOG.md`? yes
  - How is the feature or change documented? CLI help

(cherry picked from commit e3e8e27)

# Conflicts:
#	CHANGELOG.md

* Fix changelog conflict

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
Co-authored-by: Dev Ojha <dojha@berkeley.edu>
@github-actions github-actions bot mentioned this pull request Jan 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:automerge A:backport/v12.x backport patches to v12.x branch C:app-wiring Changes to the app folder C:CLI C:x/twap Changes to the twap module V:state/compatible/backport State machine compatible PR, should be backported
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants