Merge pull request #5650 from oasisprotocol/kostko/feature/gov-propos…

…al-names

go/governance: Add support for proposal metadata

.changelog/5650.breaking.md

@@ -0,0 +1 @@
+go/governance: Add support for proposal metadata

go/consensus/cometbft/apps/governance/transactions.go

@@ -26,8 +26,13 @@ func (app *governanceApplication) submitProposal(
 		"proposal_content", proposalContent,
 	)
 
+	params, err := state.ConsensusParameters(ctx)
+	if err != nil {
+		return nil, fmt.Errorf("failed to fetch consensus parameters: %w", err)
+	}
+
 	// Validate proposal content basics.
-	if err := proposalContent.ValidateBasic(); err != nil {
+	if err = proposalContent.ValidateBasic(params); err != nil {
 		ctx.Logger().Debug("governance: malformed proposal content",
 			"content", proposalContent,
 			"err", err,
@@ -39,11 +44,6 @@ func (app *governanceApplication) submitProposal(
 		return nil, nil
 	}
 
-	params, err := state.ConsensusParameters(ctx)
-	if err != nil {
-		return nil, fmt.Errorf("failed to fetch consensus parameters: %w", err)
-	}
-
 	// To not violate the consensus, change parameters proposals should be ignored when disabled.
 	if proposalContent.ChangeParameters != nil && !params.EnableChangeParametersProposal {
 		return nil, governance.ErrInvalidArgument

go/consensus/cometbft/apps/governance/transactions_test.go

@@ -58,6 +58,9 @@ func TestSubmitProposal(t *testing.T) {
 		VotingPeriod:              beacon.EpochTime(50),
 	}
 
+	allowPropMetaParams := *baseConsParams
+	allowPropMetaParams.AllowProposalMetadata = true
+
 	for _, tc := range []struct {
 		msg             string
 		params          *governance.ConsensusParameters
@@ -157,6 +160,36 @@ func TestSubmitProposal(t *testing.T) {
 			func() {},
 			nil,
 		},
+		{
+			"should fail with metadata when metadata is not allowed",
+			baseConsParams,
+			pk1,
+			&governance.ProposalContent{
+				Metadata: &governance.ProposalMetadata{
+					Title: "Proposal that should fail",
+				},
+				Upgrade: &governance.UpgradeProposal{
+					Descriptor: baseAtEpoch(200),
+				},
+			},
+			func() {},
+			governance.ErrInvalidArgument,
+		},
+		{
+			"should work with metadata when metadata is allowed",
+			&allowPropMetaParams,
+			pk1,
+			&governance.ProposalContent{
+				Metadata: &governance.ProposalMetadata{
+					Title: "Proposal that should work",
+				},
+				Upgrade: &governance.UpgradeProposal{
+					Descriptor: baseAtEpoch(200),
+				},
+			},
+			func() {},
+			nil,
+		},
 		{
 			"should work with valid cancel upgrade proposal",
 			baseConsParams,

go/genesis/genesis_test.go

@@ -814,14 +814,6 @@ func TestGenesisSanityCheck(t *testing.T) {
 	d.Governance.Proposals[0].Submitter = staking.CommonPoolAddress
 	require.Error(d.SanityCheck(), "proposal submitter reserved address")
 
-	d.Governance.Proposals = validTestProposals()
-	d.Governance.Proposals[0].Content.Upgrade = nil
-	require.Error(d.SanityCheck(), "proposal invalid content")
-
-	d.Governance.Proposals = validTestProposals()
-	d.Governance.Proposals[0].Content.Upgrade.Target = version.ProtocolVersions{}
-	require.Error(d.SanityCheck(), "proposal upgrade invalid target")
-
 	d.Governance.Proposals = validTestProposals()
 	d.Governance.Proposals[0].ClosesAt = 5
 	require.Error(d.SanityCheck(), "active proposal with past closing epoch")

go/governance/api/api.go

@@ -7,7 +7,6 @@ import (
 	"encoding/base64"
 	"fmt"
 	"io"
-	"reflect"
 
 	beacon "github.com/oasisprotocol/oasis-core/go/beacon/api"
 	"github.com/oasisprotocol/oasis-core/go/common/cbor"
@@ -60,21 +59,43 @@ var (
 
 // ProposalContent is a consensus layer governance proposal content.
 type ProposalContent struct {
+	// Metadata contains optional proposal metadata which is ignored during proposal execution.
+	Metadata *ProposalMetadata `json:"metadata,omitempty"`
+
 	Upgrade          *UpgradeProposal          `json:"upgrade,omitempty"`
 	CancelUpgrade    *CancelUpgradeProposal    `json:"cancel_upgrade,omitempty"`
 	ChangeParameters *ChangeParametersProposal `json:"change_parameters,omitempty"`
 }
 
 // ValidateBasic performs basic proposal content validity checks.
-func (p *ProposalContent) ValidateBasic() error {
-	numProposals := 0
-	values := reflect.ValueOf(*p)
-	for i := 0; i < values.NumField(); i++ {
-		if !values.Field(i).IsNil() {
-			numProposals++
+func (p *ProposalContent) ValidateBasic(params *ConsensusParameters) error {
+	// Validate metadata if present.
+	switch params.AllowProposalMetadata {
+	case true:
+		if p.Metadata == nil {
+			return fmt.Errorf("%w: proposal metadata required", ErrInvalidArgument)
+		}
+		if err := p.Metadata.ValidateBasic(); err != nil {
+			return err
+		}
+	case false:
+		if p.Metadata != nil {
+			return ErrInvalidArgument // Must be consistent with error during unmarshal.
 		}
 	}
 
+	// Validate content.
+	var numProposals int
+	if p.Upgrade != nil {
+		numProposals++
+	}
+	if p.CancelUpgrade != nil {
+		numProposals++
+	}
+	if p.ChangeParameters != nil {
+		numProposals++
+	}
+
 	switch {
 	case numProposals > 1:
 		return fmt.Errorf("proposal content has multiple fields set")
@@ -121,6 +142,9 @@ func (p *ProposalContent) Equals(other *ProposalContent) bool {
 // PrettyPrint writes a pretty-printed representation of ProposalContent to the
 // given writer.
 func (p ProposalContent) PrettyPrint(ctx context.Context, prefix string, w io.Writer) {
+	if p.Metadata != nil {
+		p.Metadata.PrettyPrint(ctx, prefix, w)
+	}
 	if p.Upgrade != nil {
 		fmt.Fprintf(w, "%sUpgrade:\n", prefix)
 		p.Upgrade.PrettyPrint(ctx, prefix+"  ", w)
@@ -141,6 +165,48 @@ func (p ProposalContent) PrettyType() (interface{}, error) {
 	return p, nil
 }
 
+const (
+	// MinProposalTitleLength is the minimum length of a proposal's title.
+	MinProposalTitleLength = 3
+	// MaxProposalTitleLength is the maximum length of a proposal's title.
+	MaxProposalTitleLength = 100
+)
+
+// ProposalMetadata contains metadata about a proposal.
+type ProposalMetadata struct {
+	// Title is the human-readable proposal title.
+	Title string `json:"title"`
+	// Description is the human-readable description.
+	Description string `json:"description,omitempty"`
+}
+
+// ValidateBasic performs basic proposal metadata validity checks.
+func (p *ProposalMetadata) ValidateBasic() error {
+	if len(p.Title) < MinProposalTitleLength {
+		return fmt.Errorf("%w: proposal title too short", ErrInvalidArgument)
+	}
+	if len(p.Title) > MaxProposalTitleLength {
+		return fmt.Errorf("%w: proposal title too long", ErrInvalidArgument)
+	}
+	return nil
+}
+
+// PrettyPrint writes a pretty-printed representation of ProposalMetadata to the given writer.
+func (p ProposalMetadata) PrettyPrint(_ context.Context, prefix string, w io.Writer) {
+	if len(p.Title) > 0 {
+		fmt.Fprintf(w, "%sTitle: %s\n", prefix, p.Title)
+	}
+	if len(p.Description) > 0 {
+		fmt.Fprintf(w, "%sDescription:\n", prefix)
+		fmt.Fprintf(w, "%s%s\n", prefix, p.Description)
+	}
+}
+
+// PrettyType returns a representation of ProposalMetadata that can be used for pretty printing.
+func (p ProposalMetadata) PrettyType() (interface{}, error) {
+	return p, nil
+}
+
 // UpgradeProposal is an upgrade proposal.
 type UpgradeProposal struct {
 	upgrade.Descriptor
@@ -379,6 +445,9 @@ type ConsensusParameters struct {
 
 	// AllowVoteWithoutEntity is true iff casting votes without a registered entity is allowed.
 	AllowVoteWithoutEntity bool `json:"allow_vote_without_entity,omitempty"`
+
+	// AllowProposalMetadata is true iff proposals are allowed to contain metadata.
+	AllowProposalMetadata bool `json:"allow_proposal_metadata,omitempty"`
 }
 
 // ConsensusParameterChanges are allowed governance consensus parameter changes.

go/governance/api/api_test.go

@@ -18,6 +18,7 @@ func TestValidateBasic(t *testing.T) {
 	for _, tc := range []struct {
 		msg       string
 		p         *ProposalContent
+		params    ConsensusParameters
 		shouldErr bool
 	}{
 		{
@@ -61,8 +62,27 @@ func TestValidateBasic(t *testing.T) {
 			},
 			shouldErr: false,
 		},
+		{
+			msg: "proposal with metadata should fail when metadata not enabled",
+			p: &ProposalContent{
+				Metadata:      &ProposalMetadata{},
+				CancelUpgrade: &CancelUpgradeProposal{},
+			},
+			shouldErr: true,
+		},
+		{
+			msg: "proposal with metadata should not fail when metadata is enabled",
+			p: &ProposalContent{
+				Metadata:      &ProposalMetadata{Title: "My nice proposal"},
+				CancelUpgrade: &CancelUpgradeProposal{},
+			},
+			params: ConsensusParameters{
+				AllowProposalMetadata: true,
+			},
+			shouldErr: false,
+		},
 	} {
-		err := tc.p.ValidateBasic()
+		err := tc.p.ValidateBasic(&tc.params) //nolint: gosec
 		if tc.shouldErr {
 			require.NotNil(t, err, tc.msg)
 			continue

go/governance/api/sanity_check.go

@@ -57,9 +57,8 @@ func SanityCheckProposals(proposals []*Proposal, epoch beacon.EpochTime, governa
 		if !p.Submitter.IsValid() {
 			return fmt.Errorf("proposal %v: invalid proposal submitter", p.ID)
 		}
-		if err := p.Content.ValidateBasic(); err != nil {
-			return fmt.Errorf("proposal %v: basic validation failure: %w", p.ID, err)
-		}
+		// Since validation rules can change through time, we cannot run basic validation checks on
+		// past proposals based on current rules (and past rules may not be available).
 
 		// XXX: There are actually other possible error states that are not covered here.
 		// e.g. for cancel upgrade proposal a pending upgrade should exist.

go/oasis-test-runner/scenario/e2e/upgrade.go

@@ -108,6 +108,9 @@ func (c *upgrade240Checker) PreUpgradeFn(ctx context.Context, ctrl *oasis.Contro
 	if govParams.AllowVoteWithoutEntity {
 		return fmt.Errorf("voting without entity is allowed")
 	}
+	if govParams.AllowProposalMetadata {
+		return fmt.Errorf("proposal metadata is allowed")
+	}
 
 	return nil
 }
@@ -157,6 +160,9 @@ func (c *upgrade240Checker) PostUpgradeFn(ctx context.Context, ctrl *oasis.Contr
 	if !govParams.AllowVoteWithoutEntity {
 		return fmt.Errorf("voting without entity is not allowed")
 	}
+	if !govParams.AllowProposalMetadata {
+		return fmt.Errorf("proposal metadata is not allowed")
+	}
 
 	return nil
 }

go/upgrade/migrations/consensus_240.go

@@ -79,6 +79,7 @@ func (h *Handler240) ConsensusUpgrade(privateCtx interface{}) error {
 			return fmt.Errorf("failed to load governance consensus parameters: %w", err)
 		}
 		govParams.AllowVoteWithoutEntity = true
+		govParams.AllowProposalMetadata = true
 
 		if err = govState.SetConsensusParameters(abciCtx, govParams); err != nil {
 			return fmt.Errorf("failed to update governance consensus parameters: %w", err)