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

osmocli: parse Use field's arguments dynamically from a message #6005

Merged
merged 24 commits into from
Aug 30, 2023

Conversation

pysel
Copy link
Member

@pysel pysel commented Aug 9, 2023

Closes: #XXX

What is the purpose of the change

In current osmocli implementation, in order to create a cli command you need to create a descriptor of this command (example here). The problem with this approach is that it is easy to forget to pass some arguments into Use field of descriptor, which leads to inconsistent Usages of cli commands (example PR which fixes mistakenly dropped arguments).

In this PR I propose to attach arguments dynamically during command construction. It will help avoid the mentioned inconsistencies. If this gets merged, there will be no need to provide message's and query's arguments to descriptors. Ex:

return &osmocli.QueryDescriptor{
	Use:   "position-by-id",  // was <Use:   "position-by-id [positionID]">
        ---- snip ----
},

To reviewers: right now I only implemented it for queries. If folks like the idea of this PR, I will continue to do the same for txs and migrate the repo to the new usage of osmocli

Testing and Verifying

Manual + added test

@pysel
Copy link
Member Author

pysel commented Aug 9, 2023

devbot add changelog misc osmocli: parse Use field's arguments dynamically

@osmosis-labs osmosis-labs deleted a comment from devbot-wizard Aug 9, 2023
@pysel pysel added V:state/compatible/no_backport State machine compatible PR, depends on prior breaks osmocli labels Aug 9, 2023
@pysel pysel force-pushed the pysel/dynamic-use-field branch from ed949f2 to 9bd8cdf Compare August 9, 2023 16:48
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.

Hey @pysel love this change, I think this is a great idea!

I was manually testing out things, and I noticed that arguments have trouble getting dynamically parsed methods using custom parser or custom logic. Here's an example

(base) matt@MacBook-Pro build % ./osmosisd q gamm estimate-swap-exact-amount-in --help
Query estimate-swap-exact-amount-in.

Example:
$ osmosisd q gamm estimate-swap-exact-amount-in 1 osm11vmx8jtggpd9u7qr0t8vxclycz85u925sazglr7 1000stake --swap-route-pool-ids=2 --swap-route-pool-ids=3

Usage:
  osmosisd query gamm estimate-swap-exact-amount-in [flags]

notice how it failed to dynamically place the usage.
After we get this fixed I think we should be good to go :) Nice work!

@pysel
Copy link
Member Author

pysel commented Aug 14, 2023

hey @mattverse! weird, I just tried it on the same command, seems to be working fine:

ruslanakhtariev@Ruslans-MacBook-Air osmosis % osmosisd q gamm estimate-swap-exact-amount-in -h
Query estimate-swap-exact-amount-in.

Example:
$ <appd> q gamm estimate-swap-exact-amount-in 1 osm11vmx8jtggpd9u7qr0t8vxclycz85u925sazglr7 1000stake --swap-route-pool-ids=2 --swap-route-pool-ids=3

Usage:
  osmosisd query gamm estimate-swap-exact-amount-in [pool-id] [token-in] [routes] [flags]

Flags:
      --height int                   Use a specific height to query state at (this can error if the node is pruning state)
  -h, --help                         help for estimate-swap-exact-amount-in
      --node string                  <host>:<port> to Tendermint RPC interface for this chain (default "tcp://localhost:26657")
  -o, --output string                Output format (text|json) (default "text")
      --swap-route-denoms string     swap route amount
      --swap-route-pool-ids string   swap route pool id

Global Flags:
      --chain-id string     The network chain ID
      --home string         directory for config and data (default "/Users/ruslanakhtariev/.osmosisd")
      --log_format string   The logging format (json|plain) (default "plain")
      --log_level string    The logging level (trace|debug|info|warn|error|fatal|panic) (default "info")
      --trace               print out full stack trace on errors

could you please confirm you used an updated binary? thanks!

@mattverse
Copy link
Member

@pysel What is the binary version you see? The current one I have is

(base) matt@MacBook-Pro build % ./osmosisd version
15.8.0-333-gc63b8e409

@pysel
Copy link
Member Author

pysel commented Aug 14, 2023

@mattverse have you installed the binary via go install ./... or via some other way?

@mattverse
Copy link
Member

@pysel I use make build and directly use the binary file itself. Maybe it's worth looking into it

@pysel
Copy link
Member Author

pysel commented Aug 24, 2023

@pysel I use make build and directly use the binary file itself. Maybe it's worth looking into it

hey @mattverse, I fixed the problem (it was outdated osmoutils version in go.mod), it should be good now.

@pysel
Copy link
Member Author

pysel commented Aug 24, 2023

btw, this osmoutils version problem took some time to be figured out. I think we might like to revisit PR that was closed recently, which idea was to update versions of workspace's submodules in go.mod when anything in them changes. if we had this functionality, the mentioned problem wouldn't have occurred, wdyt?

EDIT: another option is to maybe add this functionality to devbot. cc: @ValarDragon

cc: @mattverse @p0mvn @czarcas7ic

@mattverse
Copy link
Member

I'm down to re-reviewing #4667, sorry for the delay on the review for that one

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.

LGTM, nice work 🌮

@p0mvn p0mvn added V:state/compatible/backport State machine compatible PR, should be backported A:backport/v18.x backport patches to v18.x branch A:backport/v19.x backport patches to v19.x branch and removed V:state/compatible/no_backport State machine compatible PR, depends on prior breaks labels Aug 28, 2023
@p0mvn p0mvn merged commit c9cbb9c into main Aug 30, 2023
@p0mvn p0mvn deleted the pysel/dynamic-use-field branch August 30, 2023 20:11
mergify bot pushed a commit that referenced this pull request Aug 30, 2023
)

* dynamically attach fields' values to use of cobra cmd

* update changelog

* add comment

* support both tx/q

* fix use overwriting + owner to non-attachable fields

* change QueryDescriptor

* go fmt

* remove params in SimpleQueryCmd

* refactor TxCliDesc to not pass hardcoded args

* field names to kebab

* add test for descs

* reset statik

* Update x/concentrated-liquidity/tick_test.go

* Update x/concentrated-liquidity/client/grpc/grpc_query.go

* Update x/concentrated-liquidity/client/grpc/grpc_query.go

* reset grpc

* reset grpc dt-det

* reset all grpcs

* remove testing function

* self review

* update osmoutils version

---------

Co-authored-by: devbot-wizard <[email protected]>
(cherry picked from commit c9cbb9c)

# Conflicts:
#	CHANGELOG.md
#	go.mod
#	go.sum
mergify bot pushed a commit that referenced this pull request Aug 30, 2023
)

* dynamically attach fields' values to use of cobra cmd

* update changelog

* add comment

* support both tx/q

* fix use overwriting + owner to non-attachable fields

* change QueryDescriptor

* go fmt

* remove params in SimpleQueryCmd

* refactor TxCliDesc to not pass hardcoded args

* field names to kebab

* add test for descs

* reset statik

* Update x/concentrated-liquidity/tick_test.go

* Update x/concentrated-liquidity/client/grpc/grpc_query.go

* Update x/concentrated-liquidity/client/grpc/grpc_query.go

* reset grpc

* reset grpc dt-det

* reset all grpcs

* remove testing function

* self review

* update osmoutils version

---------

Co-authored-by: devbot-wizard <[email protected]>
(cherry picked from commit c9cbb9c)

# Conflicts:
#	CHANGELOG.md
#	go.mod
#	go.sum
p0mvn added a commit that referenced this pull request Aug 30, 2023
p0mvn added a commit that referenced this pull request Aug 30, 2023
…ckport #6005) (#6240)

* osmocli: parse Use field's arguments dynamically from a message (backport #6005)

* go sum

---------

Co-authored-by: Roman <[email protected]>
nicolaslara pushed a commit that referenced this pull request Aug 31, 2023
)

* dynamically attach fields' values to use of cobra cmd

* update changelog

* add comment

* support both tx/q

* fix use overwriting + owner to non-attachable fields

* change QueryDescriptor

* go fmt

* remove params in SimpleQueryCmd

* refactor TxCliDesc to not pass hardcoded args

* field names to kebab

* add test for descs

* reset statik

* Update x/concentrated-liquidity/tick_test.go

* Update x/concentrated-liquidity/client/grpc/grpc_query.go

* Update x/concentrated-liquidity/client/grpc/grpc_query.go

* reset grpc

* reset grpc dt-det

* reset all grpcs

* remove testing function

* self review

* update osmoutils version

---------

Co-authored-by: devbot-wizard <[email protected]>
@github-actions github-actions bot mentioned this pull request Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:backport/v18.x backport patches to v18.x branch A:backport/v19.x backport patches to v19.x branch C:CLI C:x/concentrated-liquidity C:x/gamm Changes, features and bugs related to the gamm module. C:x/incentives C:x/lockup C:x/pool-incentives C:x/poolmanager C:x/superfluid C:x/tokenfactory osmocli T:dev-UX V:state/compatible/backport State machine compatible PR, should be backported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants