From 5e7b3efcbb3f15df29abc24b00b9868dfb3fb4e0 Mon Sep 17 00:00:00 2001 From: Jernej Kos Date: Sun, 21 Apr 2024 20:16:38 +0200 Subject: [PATCH] go/governance: Add support for proposal metadata --- .changelog/5650.breaking.md | 1 + .../cometbft/apps/governance/transactions.go | 12 +-- .../apps/governance/transactions_test.go | 33 ++++++++ go/genesis/genesis_test.go | 8 -- go/governance/api/api.go | 83 +++++++++++++++++-- go/governance/api/api_test.go | 22 ++++- go/governance/api/sanity_check.go | 5 +- go/oasis-test-runner/scenario/e2e/upgrade.go | 6 ++ go/upgrade/migrations/consensus_240.go | 1 + 9 files changed, 146 insertions(+), 25 deletions(-) create mode 100644 .changelog/5650.breaking.md diff --git a/.changelog/5650.breaking.md b/.changelog/5650.breaking.md new file mode 100644 index 00000000000..ead1d772796 --- /dev/null +++ b/.changelog/5650.breaking.md @@ -0,0 +1 @@ +go/governance: Add support for proposal metadata diff --git a/go/consensus/cometbft/apps/governance/transactions.go b/go/consensus/cometbft/apps/governance/transactions.go index 1dedec8531b..b75fbcfd77b 100644 --- a/go/consensus/cometbft/apps/governance/transactions.go +++ b/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 diff --git a/go/consensus/cometbft/apps/governance/transactions_test.go b/go/consensus/cometbft/apps/governance/transactions_test.go index 860f1ddd513..90c5344a625 100644 --- a/go/consensus/cometbft/apps/governance/transactions_test.go +++ b/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, diff --git a/go/genesis/genesis_test.go b/go/genesis/genesis_test.go index 7efff02f346..255cffd0ea1 100644 --- a/go/genesis/genesis_test.go +++ b/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") diff --git a/go/governance/api/api.go b/go/governance/api/api.go index b59fd3ac7aa..04a280d6557 100644 --- a/go/governance/api/api.go +++ b/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. diff --git a/go/governance/api/api_test.go b/go/governance/api/api_test.go index 6777847da20..417167b9bf4 100644 --- a/go/governance/api/api_test.go +++ b/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 diff --git a/go/governance/api/sanity_check.go b/go/governance/api/sanity_check.go index 23486ee82c5..926095246c9 100644 --- a/go/governance/api/sanity_check.go +++ b/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. diff --git a/go/oasis-test-runner/scenario/e2e/upgrade.go b/go/oasis-test-runner/scenario/e2e/upgrade.go index b777111c142..892cb0c1c77 100644 --- a/go/oasis-test-runner/scenario/e2e/upgrade.go +++ b/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 } diff --git a/go/upgrade/migrations/consensus_240.go b/go/upgrade/migrations/consensus_240.go index a4b731148a2..9234f8ae38b 100644 --- a/go/upgrade/migrations/consensus_240.go +++ b/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)