gctcli: Improve Order Prompts & Struct Handling #1965

samuael · 2025-07-03T21:15:29Z

Shaloooooom 🕊️ ፐeam

This PR improves the gctcli user prompts for order submission, modification, and cancellation. Key updates include:

Enhanced CLI prompts based on newly updated input parameters.
Regeneration of the proto file and related Go code.
Updates to Submit, Modify, and Cancel structs for improved efficiency.
Adjustments to exchange logic reflecting struct changes.
Added updates and unit tests for order-related types such as PriceType and TrackingMode.

Fixes # (issue)

Type of change

Please delete options that are not relevant and add an x in [] as item is complete.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How has this been tested

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration and
also consider improving test coverage whilst working on a certain feature or package.

go test ./... -race
golangci-lint run
Test X

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation and regenerated documentation via the documentation tool
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally and on Github Actions with my changes
Any dependent changes have been merged and published in downstream modules

… into gctcli_update

… added unit tests

… into gctcli_update

shazbert

Just a suggestion for everybody.

shazbert · 2025-07-03T23:12:21Z

cmd/gctcli/commands.go

 			Name:  "amount",
 			Usage: "the amount for the order",
 		},
+		&cli.StringFlag{


Before I do a full review I have one suggestion which will need to be agreed by everyone. I am having a real hard time justiying the amount of toil when doing stuff like this and also tracking arg position it just adds in so much bloat. Suggestion we use structs with tags to define cli flags which will cut down time on implementation. cli tags can be updated with required and usage flags in future. @thrasher- @gbjk

var submitOrderCommand = &cli.Command{ Name: "submitorder", Usage: "submit order submits an exchange order", ArgsUsage: "<exchange> <pair> <side> <type> <amount> <price> <client_id>", Action: submitOrder, Flags: flagsFromStruct(submitOrderRequest), } type SubmitOrderParams struct { Exchange string `cli:"exchange"` Pair string `cli:"pair"` Side string `cli:"side"` Type string `cli:"type"` Amount float64 `cli:"amount"` Asset string `cli:"asset"` Price float64 `cli:"price"` Leverage float64 `cli:"leverage"` ClientOrderID string `cli:"client_order_id"` MarginType string `cli:"margin_type"` TimeInForce string `cli:"time_in_force"` QuoteAmount float64 `cli:"quote_amount"` ClientID string `cli:"client_id"` TriggerPrice float64 `cli:"trigger_price"` TriggerLimitPrice float64 `cli:"trigger_limit_price"` TriggerPriceType string `cli:"trigger_price_type"` TpPrice float64 `cli:"tp_price"` TpLimitPrice float64 `cli:"tp_limit_price"` TpPriceType string `cli:"tp_price_type"` SlPrice float64 `cli:"sl_price"` SlLimitPrice float64 `cli:"sl_limit_price"` SlPriceType string `cli:"sl_price_type"` TrackingMode string `cli:"tracking_mode"` TrackingValue float64 `cli:"tracking_value"` Hidden bool `cli:"hidden"` Iceberg bool `cli:"iceberg"` AutoBorrow bool `cli:"auto_borrow"` ReduceOnly bool `cli:"reduce_only"` RetrieveFees bool `cli:"retrieve_fees"` RetrieveFeeDelayMs int64 `cli:"retrieve_fee_delay_ms"` } var submitOrderRequest = &SubmitOrderParams{} func flagsFromStruct(params any) []cli.Flag { var flags []cli.Flag val := reflect.ValueOf(params).Elem() typ := val.Type() for i := 0; i < typ.NumField(); i++ { field := typ.Field(i) flagName := field.Tag.Get("cli") if flagName == "" { continue } switch field.Type.Kind() { case reflect.String: flags = append(flags, &cli.StringFlag{ Name: flagName, Usage: "the " + flagName + " for the order", }) case reflect.Float64: flags = append(flags, &cli.Float64Flag{ Name: flagName, Usage: "the " + flagName + " for the order", }) case reflect.Bool: flags = append(flags, &cli.BoolFlag{ Name: flagName, Usage: "the " + flagName + " for the order", }) case reflect.Int64: flags = append(flags, &cli.Int64Flag{ Name: flagName, Usage: "the " + flagName + " for the order", }) } } return flags } func submitOrder(c *cli.Context) error { if c.NArg() == 0 && c.NumFlags() == 0 { return cli.ShowSubcommandHelp(c) } assetType := strings.ToLower(submitOrderRequest.Asset) if !validAsset(assetType) { return errInvalidAsset } marginType := strings.ToLower(submitOrderRequest.MarginType) if !margin.IsValidString(marginType) { return margin.ErrInvalidMarginType } p, err := currency.NewPairDelimiter(submitOrderRequest.Pair, pairDelimiter) if err != nil { return err } conn, cancel, err := setupClient(c) if err != nil { return err } defer closeConn(conn, cancel) client := gctrpc.NewGoCryptoTraderServiceClient(conn) result, err := client.SubmitOrder(c.Context, &gctrpc.SubmitOrderRequest{ Exchange: submitOrderRequest.Exchange, Pair: &gctrpc.CurrencyPair{ Delimiter: p.Delimiter, Base: p.Base.String(), Quote: p.Quote.String(), }, Amount: submitOrderRequest.Amount, Price: submitOrderRequest.Price, Leverage: submitOrderRequest.Leverage, Side: submitOrderRequest.Side, OrderType: submitOrderRequest.Type, AssetType: assetType, MarginType: marginType, ClientId: submitOrderRequest.ClientID, ClientOrderId: submitOrderRequest.ClientOrderID, QuoteAmount: submitOrderRequest.QuoteAmount, TimeInForce: submitOrderRequest.TimeInForce, TriggerPrice: submitOrderRequest.TriggerPrice, TriggerPriceType: submitOrderRequest.TriggerPriceType, TriggerLimitPrice: submitOrderRequest.TriggerLimitPrice, StopLoss: &gctrpc.RiskManagement{ Price: submitOrderRequest.SlPrice, LimitPrice: submitOrderRequest.SlLimitPrice, PriceType: submitOrderRequest.SlPriceType, }, TakeProfit: &gctrpc.RiskManagement{ Price: submitOrderRequest.TpPrice, LimitPrice: submitOrderRequest.TpLimitPrice, PriceType: submitOrderRequest.TpPriceType, }, Hidden: submitOrderRequest.Hidden, Iceberg: submitOrderRequest.Iceberg, ReduceOnly: submitOrderRequest.ReduceOnly, AutoBorrow: submitOrderRequest.AutoBorrow, RetrieveFees: submitOrderRequest.RetrieveFees, RetrieveFeeDelayMs: submitOrderRequest.RetrieveFeeDelayMs, TrackingMode: submitOrderRequest.TrackingMode, TrackingValue: submitOrderRequest.TrackingValue, }) if err != nil { return err } jsonOutput(result) return nil }

Forgot to set destination for flag can do something like this

Destination: reflect.ValueOf(params).Elem().FieldByName(field.Name).Addr().Interface().(*string), // For strings etc

@thrasher- @gbjk There seems to be no prior agreement on direction here and this was pushed through, when you get some time could you quickly review?

I had applied this change with different additions. Could you please look at it and let me know your thoughts?

… into gctcli_update

…dling update

… into gctcli_update

shazbert

Thanks for the changes, definately consolidates the code a bit, suggestions and requires a go mod tidy.

shazbert · 2025-08-03T22:50:33Z

go.sum

 github.com/grpc-ecosystem/go-grpc-middleware v1.4.0/go.mod h1:g5qyo/la0ALbONm6Vbp88Yd8NsDy6rZz+RcrMPxvld8=
 github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0/go.mod h1:8NvIoxWQoOIhqOTXgfV/d3M/q6VIi02HzZEHgUlZvzk=
 github.com/grpc-ecosystem/grpc-gateway v1.9.0/go.mod h1:vNeuVxBJEsws4ogUvrchl83t/GYV9WGTSLVdBhOQFDY=
+github.com/grpc-ecosystem/grpc-gateway/v2 v2.27.0 h1:+epNPbD5EqgpEMm5wrl4Hqts3jZt8+kYaqUisuuIGTk=


Need to run go mod tidy

shazbert · 2025-08-03T23:00:29Z

cmd/gctcli/commands.go

 }
+
+// UnmarshalCLIFields unmarshals field values from command line as *cli.Context to an interface.
+func UnmarshalCLIFields(c *cli.Context, params any) error {


suggestion: If you set a global you don't need to do this.

Can you clarify what you mean by this comment?
We want to move away from globals, don't we?

shazbert · 2025-08-03T23:15:15Z

cmd/gctcli/commands.go

+}
+
+// FlagsFromStruct returns list of cli flags from exported flags
+func FlagsFromStruct(params any, usageInfo map[string]string) []cli.Flag {


suggestion: can use specific tags for field.Tag.Get as name,usage, required for field and set destination for value, if no usage can set up a global map for default values if no name default to using the field name and try to standardise field names.

… into gctcli_update

gbjk · 2025-09-18T04:11:18Z

cmd/gctcli/commands.go

-			return errInvalidAsset
+		in = &gctrpc.WithdrawalEventsByExchangeRequest{
+			Exchange:  arg.Exchange,
+			Limit:     int32(arg.Limit), //nolint:gosec // TODO: SQL boiler's QueryMode limit only accepts the int type


🚧

Pretty sure we mean QueryMod

Confused why we say int type, but then give it int32. Especially since it accepts int.

Can you check if this would work with int? Seems like we're going through the client first anyway...
🌴 Please make this change in similar comments below too

Suggested change

Limit: int32(arg.Limit), //nolint:gosec // TODO: SQL boiler's QueryMode limit only accepts the int type

Limit: int32(arg.Limit), //nolint:gosec // TODO: SQL boiler's QueryMod limit only accepts the int type

gbjk · 2025-09-18T08:05:52Z

cmd/gctcli/commands.go

 }
+
+// UnmarshalCLIFields unmarshals field values from command line as *cli.Context to an interface.
+func UnmarshalCLIFields(c *cli.Context, params any) error {


Can you clarify what you mean by this comment?
We want to move away from globals, don't we?

gbjk

Thanks for those changes

I've been considering the impact radius of our CLI processing here.
I've used viper to handle automatic flags and config files with merge-folding of defaults, user config, and test config from files, and then overlaying env vars and flags.
It doesn't need any scaffolding with tags, but it also doesn't handle requires or usage for you. OTOH, cli doesn't handle it for us either, so it's no worse off.
I guess the first question would be: Would the import list of viper be acceptable to even consider it. If it's not, then closed-case.
Sorry - If I was you I wouldn't want someone throwing this gasoline on my PR 🔥

cmd/gctcli/commands_types.go

gbjk · 2025-09-20T10:34:37Z

cmd/gctcli/commands.go

+		flagNames := strings.Split(field.Tag.Get("name"), ",")
+		if len(flagNames) == 0 || flagNames[0] == "" {
+			continue
+		}
+		flagName := flagNames[0]


🚧 Looking at the amount of code we have to support them, and and the amount of definitions, could we just drop the name entirely, use the field name?
This assumes we can strip - out of params coming in to match them, which I'm expecting we could.
It feels like that'd make all the multiple name and shortened name complexity go away.

If what we need is to have a single name that would make sense, and I support that. However, the current implementation allows multiple alias names, and using a structure that supports this—such as the name tag—is the most appropriate approach.

By the way, Not all field Names and cli tag name are similar.

cmd/gctcli/commands_types.go

gbjk · 2025-09-20T10:37:45Z

cmd/gctcli/commands_types.go

+	RetrieveFeeDelayMs int64   `name:"retrieve_fee_delay_ms"`
+}
+
+// GetOrderParams defines command-line flags for exchange order retrieval and unmarshal their values.


🐒 🌴 This concept applies throughout... exchange XYZ sounds weird

Suggested change

// GetOrderParams defines command-line flags for exchange order retrieval and unmarshal their values.

// GetOrderParams defines command-line flags for retrieving exchange orders

gbjk · 2025-09-20T10:52:04Z

cmd/gctcli/futures.go

-	if !validPair(currencyPair) {
-		return fmt.Errorf("%w currencypair:%v", errInvalidPair, currencyPair)
+	if !validPair(arg.Pair) {
+		return fmt.Errorf("%w currencypair:%v", errInvalidPair, arg.Pair)


Suggested change

return fmt.Errorf("%w currencypair:%v", errInvalidPair, arg.Pair)

return fmt.Errorf("%w: %q", errInvalidPair, arg.Pair)

gbjk · 2025-09-20T10:56:31Z

cmd/gctcli/futures.go

-	if !validPair(currencyPair) {
-		return fmt.Errorf("%w currencypair:%v", errInvalidPair, currencyPair)
+	if !validPair(arg.Pair) {
+		return fmt.Errorf("%w currencypair:%v", errInvalidPair, arg.Pair)


🚧

Suggested change

return fmt.Errorf("%w currencypair:%v", errInvalidPair, arg.Pair)

return fmt.Errorf("%w: %q", errInvalidPair, arg.Pair)

gbjk · 2025-09-20T10:56:53Z

cmd/gctcli/futures.go

-	if !validPair(currencyPair) {
-		return fmt.Errorf("%w currencypair:%v", errInvalidPair, currencyPair)
+	if !validPair(arg.Pair) {
+		return fmt.Errorf("%w currencypair:%v", errInvalidPair, arg.Pair)


🚧

Suggested change

return fmt.Errorf("%w currencypair:%v", errInvalidPair, arg.Pair)

return fmt.Errorf("%w: %q", errInvalidPair, arg.Pair)

gbjk · 2025-09-20T10:57:07Z

cmd/gctcli/futures.go

-	if !margin.IsValidString(marginType) {
-		return fmt.Errorf("%w margintype:%v", margin.ErrInvalidMarginType, marginType)
+	if !margin.IsValidString(arg.MarginType) {
+		return fmt.Errorf("%w margintype:%v", margin.ErrInvalidMarginType, arg.MarginType)


🚧

Suggested change

return fmt.Errorf("%w margintype:%v", margin.ErrInvalidMarginType, arg.MarginType)

return fmt.Errorf("%w: %q", margin.ErrInvalidMarginType, arg.MarginType)

gbjk · 2025-09-20T10:57:27Z

cmd/gctcli/futures.go

-			return fmt.Errorf("%w currencypair:%v", errInvalidPair, currencyPair)
+	if arg.Pair != "" {
+		if !validPair(arg.Pair) {
+			return fmt.Errorf("%w currencypair:%v", errInvalidPair, arg.Pair)


🚧

Suggested change

return fmt.Errorf("%w currencypair:%v", errInvalidPair, arg.Pair)

return fmt.Errorf("%w: %q", errInvalidPair, arg.Pair)

… into gctcli_update

This reverts commit 1de0f6b.

…ader; branch 'master' of http://github.com/thrasher-corp/gocryptotrader into gctcli_update

… into gctcli_update

samuael added 8 commits June 23, 2025 00:41

start adding fields to proto and investigating gctcli

5a230e6

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

58d4ab3

… into gctcli_update

update and generate order submit request proto and related files, and…

621d8f3

… added unit tests

update order submit, modify, and cancel proto and command handling

f3ae934

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

1be30be

… into gctcli_update

update submit order and update order parameters and testing

8d2f788

restore config example

afd5ddf

rm unncessary comment

a1c292a

shazbert requested changes Jul 3, 2025

View reviewed changes

shazbert requested review from gbjk and thrasher- July 3, 2025 23:13

shazbert assigned samuael Jul 3, 2025

shazbert added the review me This pull request is ready for review label Jul 3, 2025

gbjk mentioned this pull request Jul 7, 2025

exchanges: Update exchange templates #1777

Merged

15 tasks

samuael added 2 commits July 8, 2025 17:21

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

67c5525

… into gctcli_update

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

c948b75

… into gctcli_update

samuael requested a review from shazbert July 10, 2025 13:52

samuael added 9 commits July 11, 2025 01:33

update on order submit, modify, and cancel params processing

d72aa91

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

840cda8

… into gctcli_update

format .proto file

f9926b6

cli params unmarshaler and unit tests, types update, and commands han…

69a65f0

…dling update

update on unmarshal and FlagFromStruct method call and commands handling

04f9ae7

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

6b51735

… into gctcli_update

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

2e6a6c4

… into gctcli_update

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

bc5fc0e

… into gctcli_update

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

54037a9

… into gctcli_update

shazbert requested changes Aug 3, 2025

View reviewed changes

samuael added 3 commits August 8, 2025 14:25

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

fb0e913

… into gctcli_update

minor linter issues fix

44ec1bb

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

8afc223

… into gctcli_update

samuael requested a review from gbjk September 18, 2025 08:08

gbjk reviewed Sep 18, 2025

View reviewed changes

currency state fields handling update and unmarshaler function rename

163780d

samuael requested a review from gbjk September 19, 2025 05:44

gbjk requested changes Sep 20, 2025

View reviewed changes

shazbert added reconstructing Based on PR feedback, this is currently being reworked and is not to be merged and removed review me This pull request is ready for review labels Sep 23, 2025

samuael added 2 commits October 3, 2025 23:55

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

4b59adf

… into gctcli_update

comment, types, and minor handler functions

87e2b54

shazbert added review me This pull request is ready for review and removed reconstructing Based on PR feedback, this is currently being reworked and is not to be merged labels Oct 9, 2025

samuael added 5 commits October 24, 2025 12:07

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

eb607e2

… into gctcli_update

mod file tidy

e3e9ff9

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

8783e2e

… into gctcli_update

regenerate pb.go files from .proto file

5d334cf

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

399249f

… into gctcli_update

samuael requested a review from gbjk November 16, 2025 08:44

samuael added 13 commits November 16, 2025 23:16

action functions code structure update

9c83586

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

c879552

… into gctcli_update

minor fix to client cli handlers

82131b0

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

e4f10fe

… into gctcli_update

added test coverage for POST with a slice

1de0f6b

Revert "added test coverage for POST with a slice"

6e02eb0

This reverts commit 1de0f6b.

Merge branch 'gctcli_update' of https://github.com/samuael/gocryptotr…

c67951e

…ader; branch 'master' of http://github.com/thrasher-corp/gocryptotrader into gctcli_update

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

d9b1ac3

… into gctcli_update

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

805943f

… into gctcli_update

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

1f98dfa

… into gctcli_update

minor update

4bf7e11

miscellaneous checks fix

a4b66bc

Merge branch 'master' of http://github.com/thrasher-corp/gocryptotrader…

d67c5a2

… into gctcli_update

	Limit: int32(arg.Limit), //nolint:gosec // TODO: SQL boiler's QueryMode limit only accepts the int type
	Limit: int32(arg.Limit), //nolint:gosec // TODO: SQL boiler's QueryMod limit only accepts the int type

	// GetOrderParams defines command-line flags for exchange order retrieval and unmarshal their values.
	// GetOrderParams defines command-line flags for retrieving exchange orders

	return fmt.Errorf("%w currencypair:%v", errInvalidPair, arg.Pair)
	return fmt.Errorf("%w: %q", errInvalidPair, arg.Pair)

	return fmt.Errorf("%w margintype:%v", margin.ErrInvalidMarginType, arg.MarginType)
	return fmt.Errorf("%w: %q", margin.ErrInvalidMarginType, arg.MarginType)

gctcli: Improve Order Prompts & Struct Handling #1965

Are you sure you want to change the base?

gctcli: Improve Order Prompts & Struct Handling #1965

Uh oh!

Conversation

samuael commented Jul 3, 2025

Type of change

How has this been tested

Checklist

Uh oh!

shazbert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shazbert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gbjk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants