AutoCLI improve default value handling

[julienrbrt]

Improve the handling of default values. Currently, the default value of a parameter is set to the default go.
Which is, when that default value a valid value for a query, leads to some confusing behavior:

$ cosmcli evmos query auth account-address-by-id 
{
  "account_address":  "evmos1rtj2r4eaz0v68mxjt5jleynm85yjfu2uxm7pxx"
}
$ cosmcli evmos query auth account-address-by-id --id 0
{
  "account_address":  "evmos1rtj2r4eaz0v68mxjt5jleynm85yjfu2uxm7pxx"
}

ref: #14696 (comment)

[julienrbrt]

It seems like the solution is to mark the flags as required. However, we need a way to know which flag is required for a specific query. Is this something we want to add in the protos @aaronc (this does not look supported by our tooling however)?

Something naive like this unfortunately will lead having too many flags required. Especially that it can differ per chain.

	binder, err := b.AddMessageFlags(cmd.Context(), cmd.Flags(), inputType, options)
	if err != nil {
		return nil, err
	}

	// mark all flags as required except optional
	cmd.Flags().VisitAll(func(f *pflag.Flag) {
		if f.Name == "pagination" || strings.HasPrefix(f.Name, "page-") {
			return
		}

		cmd.MarkFlagRequired(f.Name)
	})

[aaronc]

We could look into required annotations, maybe https://github.com/bufbuild/protoc-gen-validate. Or start using proto3 optional to make the distinction. The latter would be more refactoring because it would make everything default required by autocli if it doesn't specify optional. Whereas assuming optional by default is closer to our existing semantics.

Maybe we create a simple required field option and start our own set of validation annotations

[alexanderbez]

SGTM. If the field is nil, we can assume it was unset, right?

[aaronc]

SGTM. If the field is nil, we can assume it was unset, right?

If a scalar field is marked as optional then we can know if it is nil and therefore unset, otherwise we can't.

[aaronc]

For fields in Msg's (as opposed to queries), it seems like maybe the default assumption should probably be that all fields are required and thus positional parameters.

Generally for autocli, it feels like it would be cleanest to assume things are required and thus positional unless explicitly marked as optional. The challenge is that we have so many existing queries with effectively optional fields that are not marked as such... and changing them is breaking: https://docs.buf.build/breaking/rules#field_same_label.

What if we added an autocli module option like assume_required to indicate whether the .proto files are properly using optional to indicate not required? For existing proto files that don't set this, we can assume everything is optional and and positional_args can be set to make things required. Then for newer proto files, we can set assume_required and optional will be needed to turn something into a flag.

[JeancarloBarrios]

Generally for autocli, it feels like it would be cleanest to assume things are required and thus positional unless explicitly marked as optional. The challenge is that we have so many existing queries with effectively optional fields that are not marked as such... and changing them is breaking: https://docs.buf.build/breaking/rules#field_same_label.

I agree with this personally I also feel that better to be explicit with optional rather than with mandatory. I vote for this change!

[tac0turtle]

@julienrbrt is this solved?

[julienrbrt]

@julienrbrt is this solved?

No, but I think to achieve a better UX you should just use positional argument instead.
Flags should be by definition option, so the default value shouldn't matter.