refactor: add title, summary and proposer to proposal struct of gov #14390
Conversation
github-prbot
requested review from
a team,
JeancarloBarrios and
atheeshp
and removed request for
a team
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
do we need migration for this? from 0.46 -> 0.47? |
|
I feel like we should update CLI (submit-proposal (and draft-proposal)) to check on title being set and as well actually set it, when parsing a proposal json. cosmos-sdk/x/gov/client/cli/util.go Line 88 in f1062c7 |
x/gov/client/cli/prompt.go
Outdated
| @@ -155,6 +155,10 @@ func (p *proposalType) Prompt(cdc codec.Codec) (*proposal, types.ProposalMetadat | |||
| return nil, metadata, fmt.Errorf("failed to set proposal deposit: %w", err) | |||
| } | |||
|
|
|||
| // set title and summary from metadata in the proposal | |||
There was a problem hiding this comment.
Nit actually we can move that up to L147, just after we set the proposal.Metadata. IMHO it makes more sense to group that.
Maybe even assign all three of them in the struct directly after the metadata prompt and remove L138.
proposal := &proposal{
Metadata: "IPFS", // comment
Title: metadata.Title,
Summary: metadata.Summary,
}| err := keeper.assertMetadataLength(metadata) | ||
| if err != nil { | ||
| return v1.Proposal{}, err | ||
| } | ||
|
|
||
| // assert summary is no longer than predefined max length of metdata | ||
| err = keeper.assertMetadataLength(summary) |
There was a problem hiding this comment.
I think it warrants adding this in the documentation.
x/gov/keeper/msg_server.go
Outdated
| if msg.Title == "" { | ||
| return nil, goerrors.New("proposal title cannot be empty") | ||
| } | ||
| if msg.Summary == "" { | ||
| return nil, goerrors.New("proposal summary cannot be empty") | ||
| } |
There was a problem hiding this comment.
As I can see, these checks are already present in ValidateBasic, do we still need here?
tac0turtle
added
the
backport/v0.47.x
tac0turtle
force-pushed
the
marko/addgov
branch
from
38b565d
to
fc93b4f
Compare
I don't think you can. You cannot safely infer title and summary from the (unstructured) metadata. |
x/gov/README.md
Outdated
| } | ||
| ``` | ||
|
|
||
| > Note: By default the metadata and the proposal are both limited by 255 characters, this can be overridden by the application developer |
There was a problem hiding this comment.
| > Note: By default the metadata and the proposal are both limited by 255 characters, this can be overridden by the application developer | |
| :::note | |
| By default the metadata and the proposal are both limited by 255 characters, this can be overridden by the application developer. | |
| ::: |
Nit: it shows nicer in docusaurus.
x/gov/client/cli/prompt.go
Outdated
| proposal.Metadata = "ipfs://CID" | ||
|
|
||
| proposal := &proposal{ | ||
| Metadata: "ipfs://CID", |
There was a problem hiding this comment.
| Metadata: "ipfs://CID", | |
| Metadata: "ipfs://CID", // the metadata must be saved on IPFS, set placeholder |
I think the comment still has a value.
Description
add title, summary (limited in characters) and proposer to the proposal struct. Would like to here if proposer is needed here?
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking change