[Feature]: add MaxOptionsLen to gov configs

[marcello33]

Summary

Adding a MaxOptionsLen alongside the other configs in gov module to help define the maximum number of options a proposal can have.

Problem Definition

In our use case (here at Polygon Labs), for example, we use that to enforce the MaxOptionsLen to be 1 when we do not want to allow multiple options per proposals, by doing something like

	if config.MaxOptionsLen != 1 {
		config.MaxOptionsLen = types.DefaultConfig().MaxOptionsLen
	}

where default configs are

// DefaultConfig returns the default config for gov.
func DefaultConfig() Config {
	return Config{
		MaxMetadataLen: 255,
		MaxOptionsLen:  1,
	}
}

In Polygon use case, this is then verified while tallying or adding a vote by asserting the options length:

// assertOptionsLength returns an error if given options length
// is greater than a pre-defined MaxOptionsLen.
func (k Keeper) assertOptionsLength(options v1.WeightedVoteOptions) error {
	if uint64(len(options)) > k.config.MaxOptionsLen {
		return types.ErrTooManyVoteOptions.Wrapf("got %d options, maximum allowed is %d", len(options), k.config.MaxOptionsLen)
	}
	return nil
}

Proposed Feature

Change the gov configs from

// Config is a config struct used for initializing the gov module to avoid using globals.
type Config struct {
	// MaxTitleLen defines the amount of characters that can be used for proposal title
	MaxTitleLen uint64
	// MaxMetadataLen defines the amount of characters that can be used for proposal metadata
	MaxMetadataLen uint64
	// MaxSummaryLen defines the amount of characters that can be used for proposal summary
	MaxSummaryLen uint64
	// CalculateVoteResultsAndVotingPowerFn is a function signature for calculating vote results and voting power
	// Keeping it nil will use the default implementation
	CalculateVoteResultsAndVotingPowerFn CalculateVoteResultsAndVotingPowerFn
}

to

// Config is a config struct used for initializing the gov module to avoid using globals.
type Config struct {
	// MaxTitleLen defines the amount of characters that can be used for proposal title
	MaxTitleLen uint64
	// MaxMetadataLen defines the amount of characters that can be used for proposal metadata
	MaxMetadataLen uint64
	// MaxSummaryLen defines the amount of characters that can be used for proposal summary
	MaxSummaryLen uint64
        // MaxOptionsLen defines the maximum number of options a proposal can have.
        MaxOptionsLen uint64
	// CalculateVoteResultsAndVotingPowerFn is a function signature for calculating vote results and voting power
	// Keeping it nil will use the default implementation
	CalculateVoteResultsAndVotingPowerFn CalculateVoteResultsAndVotingPowerFn
}

[julienrbrt]

I guess this could make sense (expect let's call it MaxVoteOptionsLen instead), but why not disabling MsgVoteWeighted instead if you allow one option.
I am trying to understand if there is a more general use case as well.

[marcello33]

Yeah in our case we wanted to minimize changes with upstream, so we disabled MsgVoteWeighted related commands, but to enforce this check on the server side, we introduced this variable.
We forked v0.50.x, and changed it against that version, where configs are a bit different, but raised the issue for main anyway

[marcello33]

Btw, if you guys plan to release other minor or patch versions on top of v0.50, I can raise the issue there, or create a PR from my fork.
Thank you

[julienrbrt]

If the default is 0 / -1 which would mean all supported options (current state) then we can backport to v0.50.
Otherwise that won't be possible. The flow is correct however, the PR will have to target main and we'll backport if necessary.

Concerning disabling MsgVoteWeighted on the server side you can simply use the x/circuit module and block MsgVoteWeighted. This way you don't need this logic.

I am not opposed to such feature personally, maybe other have other opinions (cc @tac0turtle), but I am still failing to understand the use case (why would you want to limit the number of option).

[marcello33]

Hey @julienrbrt
Yeah I can set the value to 0 and keep 1 in my fork.
Also, we are not using the x/circuit module, and that is exactly what made us do this change.
As a general purpose SDK, I believe you can leave the choice to devs to enable/disable modules, and have this config will help achieving such behaviour without x/circuit module.
Of course this is the behaviour we wanted to achieve with our fork, but if you guys find it helpful for others too, we can make the changes available to backport

[julienrbrt]

Okay, are you willing to tackle this then? Happy to review.

[marcello33]

Sure. I'll make the relevant changes targeting main. Then - if approved and merged - it'd be great if you can backport to v0.50.x for us to get rid of that divergent portion of the code compared to the upstream.
Also, @julienrbrt I'll be only raising the changes to add a new config, right? No server side validation enforcing

[julienrbrt]

That will be already server side if you add the check in the message handler / keeper.

[marcello33]

Created a PR @julienrbrt