Use reflection + Generics to delete most CLI boilerplate for queries #3611

ValarDragon · 2022-12-03T17:36:25Z

What is the purpose of the change

Cleans up some CLI boilerplate. Mostly was using it as an exercise to play around with ChatGPT

Brief Changelog

Less lines of code to create an index command
Make a generic to create our query commands
Make a generic to create our tx commands
Make a generics + reflection approach to automatically get code for "simple" queries
Index commands have more succinct outputs:
Before:

After:

Testing and Verifying

Manually verified index commands still work, just more succinctly.
Manually verified all query commands altered work as before. Two modules not changed (Superlfuid and one other), because they do a non-standard unwrapping of the proto response.
Other TWAP commands just had code moves.

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)

ValarDragon · 2022-12-05T04:14:01Z

Limitation for certain classes of further tx deletion:

We should be able to parse ValAddr just fine as it stands, but I don't have a convenient way to test out the superfluid txs commands on testnet, to ensure they work if I did that. So holding off for now
Need to just add sdk.Int parsing support, then we can knock out ~half of gamm txs
A lot of commands use flags when they really shouldn't. Need more generic custom "use flag" support. EDIT: Added this!
Gov commands (needs a third, related, cmd scaffolding). I'm thinking separate issue/PR.

p0mvn

Looks great to me! Amazing work.

General suggestions/questions:

Although most CLI functions are self-explanatory, godocs would still be helpful in many cases. E.g: ParseField. Also, for consistency with the repo.
What is the testing strategy for this?

osmoutils/generic_helper.go

osmoutils/osmocli/index_cmd.go

p0mvn · 2022-12-05T16:12:50Z

osmoutils/osmocli/parsers.go

+	fVal := v.Field(fieldIndex)
+	fType := t.Field(fieldIndex)
+
+	// fmt.Printf("Field %d: %s %s %s\n", fieldIndex, fType.Name, fType.Type, fType.Type.Kind())


Since this is turning into a discussion, I would like to chime in with some thoughts as well.

I agree that these are valuable for debugging purposes. However, I don't think that leaving commented-out prints should be the direction.

I agree with @nicolaslara that we should be using a toggle. I would even go beyond that to suggest using various log levels - debug, info etc. Each log should be analyzed in the review whether it adds value, including its log level. Otherwise, we will eventually have a system full of misc logs at the info level.

In my opinion, raw commented-out prints only add additional complexity to a system. So we should be using a logger with a configured tracing level.

If we don't take proper care to analyze the value of each in the review, the system will eventually become polluted with comments.

osmoutils/osmocli/parsers.go

p0mvn · 2022-12-05T16:23:25Z

osmoutils/osmocli/parsers.go

+
+// TODO: Make this able to read from some local alias file for denoms.
+func ParseCoins(arg string, fieldName string) (sdk.Coins, error) {
+	coins, err := sdk.ParseCoinsNormalized(arg)


There are some cases where sdk.ParseCoinsNormalized doesn't work. For example, here:

osmosis/x/gamm/client/cli/tx.go

Line 487 in da6b7d1

deposit, err := ParseCoinsNoSort(flags.InitialDeposit)

Is that case handled in this PR?

Took a quick peek. Looks like ParseCoinsNoSort is still used in the right place

yeah, explicitly didn't replace those for now. Need to add a way to set a custom per-field parser to get those replaced

ValarDragon · 2022-12-05T16:48:14Z

Testing strategy:

Existing queries and CLI commands all still work at feature parity right now
- All of the (admittedly bad) old tests still work
We can add some golang tests for the parse methods

ValarDragon · 2022-12-05T16:52:01Z

Thanks for the reviews! Will get godocs added onto those primary parse methods

czarcas7ic · 2022-12-05T21:38:01Z

x/epochs/module.go

@@ -84,7 +84,7 @@ func (AppModuleBasic) RegisterGRPCGatewayRoutes(clientCtx client.Context, mux *r

 // GetTxCmd returns the capability module's root tx command.
 func (a AppModuleBasic) GetTxCmd() *cobra.Command {
-	return cli.GetTxCmd()
+	return nil


What is the long term plan for this?

wdym, it has no TX command?

Long term plan is that you don't have to implement this function for interface compat, if you don't use a tx cli

Ah, was trying to figure out why we were returning GetTxCmd before, makes sense

czarcas7ic · 2022-12-05T21:39:21Z

x/gamm/client/cli/tx.go

 	return cmd
 }

+// TODO: Change these flags to args. Required flags don't make that much sense.


…3611) Makes new, far more succinct methods, to build tx and query CLI's (cherry picked from commit ce98bbb) # Conflicts: # x/valset-pref/client/cli/query.go # x/valset-pref/client/cli/tx.go

…(backport #3611) (#3633) Makes new, far more succinct methods, to build tx and query CLI's Co-authored-by: Dev Ojha <[email protected]> Co-authored-by: Dev Ojha <[email protected]>

ValarDragon added 2 commits December 3, 2022 11:30

More CLI boilerplate cleanup

92ea1a3

Update the usage line

8abe76b

github-actions bot added C:CLI C:x/incentives C:x/twap Changes to the twap module labels Dec 3, 2022

ValarDragon added V:state/compatible/backport State machine compatible PR, should be backported A:backport/v13.x backport patches to v13.x branch labels Dec 3, 2022

Make params query via generics

d4357ca

github-actions bot added the C:x/tokenfactory label Dec 3, 2022

ValarDragon mentioned this pull request Dec 3, 2022

Make an SDK function for getting base module query command command cosmos/cosmos-sdk#11867

Closed

4 tasks

ValarDragon added 2 commits December 3, 2022 12:10

Generics trick to remove argument

fe1ff60

Apply these query helpers to modules

6507812

github-actions bot added C:x/epochs C:x/gamm Changes, features and bugs related to the gamm module. C:x/lockup C:x/mint C:x/pool-incentives C:x/superfluid C:x/txfees labels Dec 3, 2022

ValarDragon added 4 commits December 3, 2022 13:45

Add generic way to create simple Commands!

36ab3ea

More deletions

dda6074

Remove extra type arg

4bff851

Simplifty more of the query

b352cfe

ValarDragon changed the title ~~More CLI boilerplate cleanup~~ Use reflection + Generics to delete most CLI boilerplate for queries Dec 3, 2022

ValarDragon added 2 commits December 3, 2022 16:40

More deletion!

f4e1395

More deletes

ed0d4a9

ValarDragon requested review from hieuvubk, czarcas7ic and p0mvn December 3, 2022 23:01

More code deletion

cbf1edc

ValarDragon added 6 commits December 4, 2022 21:11

Replace ChangeAdmin, remove need for NumArgs

98fc1ef

Make tx index commands

8fbc7bb

more index command integration

f9e0d5e

Add sdk.Coin parsing, migrate all of tokenfactory :)

f2176fe

Add Coins parser

a5e7050

Delete superfluid tx boilerplate

5eaac43

ValarDragon added 5 commits December 4, 2022 22:23

Migrate add-to-gauge (checked on testnet)

9f458f9

Succesfully make a custom flag override

f217cd7

Migrate remaining lockup txs (tested all but force unlock)

9dc769f

Cut more CLI boilerplate from gamm

48ec370

Delete empty epochs cli/tx.go

a346000

github-actions bot added the C:app-wiring Changes to the app folder label Dec 5, 2022

p0mvn approved these changes Dec 5, 2022

View reviewed changes

ValarDragon added 5 commits December 5, 2022 13:53

More parsing deletion!

83a0358

make switch statement more succinct, godocs

2282506

DRY params query code

ce02e61

Add query flag support

6628058

Add missing, requested godoc entry

3ccb04e

czarcas7ic approved these changes Dec 5, 2022

View reviewed changes

ValarDragon merged commit ce98bbb into main Dec 5, 2022

ValarDragon deleted the dev/more_cli_boilerplate_cleanup branch December 5, 2022 21:59

mergify bot mentioned this pull request Dec 5, 2022

Use reflection + Generics to delete most CLI boilerplate for queries (backport #3611) #3633

Merged

ValarDragon mentioned this pull request Dec 6, 2022

osmocli epic: #3641

Closed

7 tasks

github-actions bot mentioned this pull request Mar 15, 2024

Broken Links Detected #7752

Closed

github-actions bot mentioned this pull request Apr 1, 2024

Broken Links Detected #7903

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use reflection + Generics to delete most CLI boilerplate for queries #3611

Use reflection + Generics to delete most CLI boilerplate for queries #3611

ValarDragon commented Dec 3, 2022 •

edited

Loading

ValarDragon commented Dec 5, 2022 •

edited

Loading

p0mvn left a comment

p0mvn Dec 5, 2022

p0mvn Dec 5, 2022

p0mvn Dec 5, 2022

ValarDragon Dec 5, 2022

ValarDragon commented Dec 5, 2022

ValarDragon commented Dec 5, 2022

czarcas7ic Dec 5, 2022

ValarDragon Dec 5, 2022

czarcas7ic Dec 5, 2022

czarcas7ic Dec 5, 2022

Use reflection + Generics to delete most CLI boilerplate for queries #3611

Use reflection + Generics to delete most CLI boilerplate for queries #3611

Conversation

ValarDragon commented Dec 3, 2022 • edited Loading

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

ValarDragon commented Dec 5, 2022 • edited Loading

p0mvn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon commented Dec 5, 2022

ValarDragon commented Dec 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon commented Dec 3, 2022 •

edited

Loading

ValarDragon commented Dec 5, 2022 •

edited

Loading