refactor: add title and summary to groups proposal #14465

tac0turtle · 2023-01-03T09:04:01Z

Description

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...

Reviewers 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...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

proto/cosmos/group/v1/tx.proto

julienrbrt

lgtm, I'll try it locally after the nits.

other nits: the same can be done in group prompt.go that what you've done in gov prompt.

x/group/client/cli/util.go

x/group/keeper/msg_server.go

amaury1093

LGTM, just have a question about max length

proto/cosmos/gov/v1/gov.proto

amaury1093 · 2023-01-03T13:45:57Z

proto/cosmos/group/v1/tx.proto

+  string title = 6;
+
+  // summary is the summary of the proposal.
+  string summary = 7;


Do we want to add a char limit to these 2 fields? IIRC we had that set to 10000 chars

(same q for gov)

I was thinking of reusing the same max length as metadata. I did the check on summary but can add to title. Went with metadata length because it shouldn't be long

Went with metadata length because it shouldn't be long

I think I didn't follow the whole conversation for these changes, so if there was already consensus on this, then I can approve.

But my concern is: summary is not the long description, right? Where would summary be shown/used btw? Also, iiuc the initial problem was that without these 2 news fields, it breaks explorers (for gov). But if we introduce a summary with a 255-char lenghth, it still break explorers, right? e.g. https://www.mintscan.io/cosmos/proposals/93, the "Details" field would still need to be resolved with IPFS.

I can rename it description to not break, that would be better. The character limit is there to ask people to only write a short description/summary, but not the entire one. Its kind of a middle ground to storing it off chain

OK, talked offline, let's keep title+summary.

Can we add the max length check to title too? To keep the same behavior as before. (and to gov too)

likhita-809

everything lgtm, not really needed but you can add more testcases to msgs_test.go with empty title and summary

likhita-809

one small nit

x/group/msgs_test.go

Co-authored-by: Likhita Polavarapu <78951027+likhita-809@users.noreply.github.com>

sonarcloud · 2023-01-04T13:38:55Z

[Cosmos SDK] Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

x/group/types.pb.go

julienrbrt · 2023-01-04T17:41:36Z

x/group/client/cli/prompt.go

@@ -31,15 +31,18 @@ type proposalType struct {

 // Prompt the proposal type values and return the proposal and its metadata.
 func (p *proposalType) Prompt(cdc codec.Codec) (*Proposal, govtypes.ProposalMetadata, error) {
-	proposal := &Proposal{}

 	// set metadata
 	metadata, err := govcli.Prompt(govtypes.ProposalMetadata{}, "proposal")
 	if err != nil {
 		return nil, metadata, fmt.Errorf("failed to set proposal metadata: %w", err)
 	}
 	// the metadata must be saved on IPFS, set placeholder


Suggested change

// the metadata must be saved on IPFS, set placeholder

Will do in as follow-up as I need to touch this file for supporting slice in prompts

julienrbrt

tACK

(cherry picked from commit 7b12333)

…14482) Co-authored-by: Marko <marbar3778@yahoo.com> Co-authored-by: Julien Robert <julien@rbrt.fr>

tac0turtle added 2 commits January 3, 2023 10:02

add proto file changes

5d61e54

Merge branch 'main' into marko/groups_proposal

c309a84

github-actions bot added the C:x/group label Jan 3, 2023

add title and summary to groups

5d3c6b3

github-actions bot added C:CLI C:Simulations labels Jan 3, 2023

fix tests

702ce1e

amaury1093 reviewed Jan 3, 2023

View reviewed changes

proto/cosmos/group/v1/tx.proto Show resolved Hide resolved

julienrbrt added the backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release label Jan 3, 2023

tac0turtle added 3 commits January 3, 2023 11:17

fix tests

061482d

add since

2670273

Merge branch 'main' into marko/groups_proposal

9689600

tac0turtle marked this pull request as ready for review January 3, 2023 10:29

tac0turtle requested a review from a team as a code owner January 3, 2023 10:29

julienrbrt reviewed Jan 3, 2023

View reviewed changes

x/group/client/cli/util.go Show resolved Hide resolved

x/group/keeper/msg_server.go Show resolved Hide resolved

amaury1093 reviewed Jan 3, 2023

View reviewed changes

tac0turtle added 3 commits January 3, 2023 14:56

add since

fa6497a

fix since and add to test case

c6e347d

Merge branch 'main' into marko/groups_proposal

ebf1ae0

tac0turtle assigned facundomedica and julienrbrt Jan 3, 2023

tac0turtle requested a review from julienrbrt January 3, 2023 18:42

tac0turtle added 2 commits January 3, 2023 19:42

Merge branch 'main' into marko/groups_proposal

a02af40

regenerate

2fcad41

github-actions bot added the C:x/gov label Jan 3, 2023

likhita-809 reviewed Jan 4, 2023

View reviewed changes

tac0turtle added 2 commits January 4, 2023 12:23

Merge branch 'main' into marko/groups_proposal

9c3cef7

add test cases

8d2345e

likhita-809 approved these changes Jan 4, 2023

View reviewed changes

x/group/msgs_test.go Outdated Show resolved Hide resolved

Update x/group/msgs_test.go

3941d43

Co-authored-by: Likhita Polavarapu <78951027+likhita-809@users.noreply.github.com>

tac0turtle added 2 commits January 4, 2023 16:36

Merge branch 'main' into marko/groups_proposal

605f2e4

add length checks and extra lines to docs

f2d069f

amaury1093 self-assigned this Jan 4, 2023

address comments

553de26

julienrbrt reviewed Jan 4, 2023

View reviewed changes

x/group/types.pb.go Show resolved Hide resolved

regenerate

ab2c6ce

julienrbrt reviewed Jan 4, 2023

View reviewed changes

julienrbrt approved these changes Jan 4, 2023

View reviewed changes

julienrbrt enabled auto-merge (squash) January 4, 2023 19:43

julienrbrt merged commit 7b12333 into main Jan 4, 2023

julienrbrt deleted the marko/groups_proposal branch January 4, 2023 20:01

mergify bot pushed a commit that referenced this pull request Jan 4, 2023

refactor: add title and summary to groups proposal (#14465)

cc23b66

(cherry picked from commit 7b12333)

mergify bot mentioned this pull request Jan 4, 2023

refactor: add title and summary to groups proposal (backport #14465) #14482

Merged

julienrbrt mentioned this pull request Jan 4, 2023

feat: Add support for []string and []int in draft-proposal prompt. #14483

Merged

19 tasks

julienrbrt added a commit that referenced this pull request Jan 5, 2023

refactor: add title and summary to groups proposal (backport #14465) (#…

a1a6258

…14482) Co-authored-by: Marko <marbar3778@yahoo.com> Co-authored-by: Julien Robert <julien@rbrt.fr>

0xstepit mentioned this pull request Jul 1, 2024

chore(cachemulti): add Copy method evmos/cosmos-sdk#52

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: add title and summary to groups proposal #14465

refactor: add title and summary to groups proposal #14465

tac0turtle commented Jan 3, 2023

julienrbrt left a comment •

edited

Loading

amaury1093 left a comment

amaury1093 Jan 3, 2023

tac0turtle Jan 3, 2023

amaury1093 Jan 4, 2023

tac0turtle Jan 4, 2023

amaury1093 Jan 4, 2023

likhita-809 left a comment

likhita-809 left a comment

sonarcloud bot commented Jan 4, 2023

julienrbrt Jan 4, 2023

julienrbrt Jan 4, 2023

julienrbrt left a comment •

edited

Loading

refactor: add title and summary to groups proposal #14465

refactor: add title and summary to groups proposal #14465

Conversation

tac0turtle commented Jan 3, 2023

Description

Author Checklist

Reviewers Checklist

julienrbrt left a comment • edited Loading

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 Jan 3, 2023

Choose a reason for hiding this comment

tac0turtle Jan 3, 2023

Choose a reason for hiding this comment

amaury1093 Jan 4, 2023

Choose a reason for hiding this comment

tac0turtle Jan 4, 2023

Choose a reason for hiding this comment

amaury1093 Jan 4, 2023

Choose a reason for hiding this comment

likhita-809 left a comment

Choose a reason for hiding this comment

likhita-809 left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jan 4, 2023

julienrbrt Jan 4, 2023

Choose a reason for hiding this comment

julienrbrt Jan 4, 2023

Choose a reason for hiding this comment

julienrbrt left a comment • edited Loading

Choose a reason for hiding this comment

julienrbrt left a comment •

edited

Loading

julienrbrt left a comment •

edited

Loading