Gov proposals should should trim empty Title and Description

[fedekunze]

Summary of Bug

Governance proposal's Title and Description (L37 and L40 in msgs.go respectively) should handle whitespaces (blanks) by using strings.TrimSpace(msg.Title) and strings.TrimSpace(msg.Description).

Steps to Reproduce

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[sunnya97]

This should probably be at the UI level, right? (so for now, the CLI)

[fedekunze]

@sunnya97 @ValarDragon I think it should be done in both the UI and in the backend, just as it's a standard to do form validations in both frontend and backend. Imagine if someone submits a blank proposal to the state. The proposal will still be valid (i.e not breaking anything) but no one will be able to see it. I strongly suggest trimming whitespaces in Msg in general

[cwgoes]

But what if they wanted to add extra whitespace (the action was intentional)?

[fedekunze]

@cwgoes That's ok as the whole value is not all containing whitespaces. This is what I meant:

// ValidateBasic Implements Msg
func (msg SubmitProposalMsg) ValidateBasic() sdk.Error {
	if len(msg.Submitter) == 0 {
		return sdk.ErrInvalidAddress("Invalid address: " + msg.Submitter.String())
	}
	if len(strings.TrimSpace(msg.Title)) == 0 {
		return ErrInvalidTitle("Cannot submit a proposal with empty title")
	}
	if len(strings.TrimSpace(msg.Description)) == 0 {
		return ErrInvalidDescription("Cannot submit a proposal with empty description")
	}
	if !msg.Deposit.IsValid() {
		return sdk.ErrInvalidCoins("Deposit is not valid")
	}
	if !msg.Deposit.IsPositive() {
		return sdk.ErrInvalidCoins("Deposit cannot be negative")
	}
	return nil
}

[ValarDragon]

I'm not entirely sure why we need the state machine to ban empty message strings. I think it should just be a UI layer thing, for simplicity of the state machine. Its not providing any security, and governance proposals are going to have non-negligible deposits, so making empty titles is going to be disincentivized. (As you lose the deposit due to it being marked as spam / veto). (Method of marking spam prevention is up for discussion)

[fedekunze]

@ValarDragon I get your point of the deposits and incentives. Nevertheless, I think an empty title will be confusing for the user (validators/delegators) that are voting on the proposal, so from a UX point of view I propose only to ban empty Titles and allow empty Descriptions

[liamsi]

This should probably be at the UI level, right? (so for now, the CLI)

Chiming in as I was discussing this with @fedekunze: If we agree to ban empty titles (or whitespaces or whatever), we shouldn't only handle this in the UI but also in the backend. Someone will create a UI that doesn't handle the cases correctly and things will get messy eventually.

[faboweb]

I support @fedekunze and @liamsi on this

[rigelrozanski]

+1 for the ban in the backend

[ValarDragon]

proposal cost is planned to be non-neglible. Because of that, we can assume that the only proposals like this were done intentionally. (As any rational person would double check) It seems to me that marking this as spam won't be hard for validators/delegators.

@ValarDragon I get your point of the deposits and incentives. Nevertheless, I think an empty title will be confusing for the user (validators/delegators) that are voting on the proposal, so from a UX point of view I propose only to ban empty

Totally agree that no empty titles. Thats already the case though. Just looked at the code, empty is already banned. You're proposing to ban a title of pure spaces. However there are other characters that render weirdly, like newlines and etc. Why aren't those being banned as well here? In general, I think we should attempt to minimize the amount of parameters like these we have, to simplify implementations in other languages. I don't understand why we're worried about UI's breaking if the message is just spaces. (Disclaimer, I have no experience in UI, I would appreciate an explanation if I'm missing something)

I don't have strong feelings about this, but it seems like we're going halfway. I'd rather we made this a new governance boolean, and then banned all leading and trailing escape characters, rather then just spaces.

[faboweb]

I'd rather we made this a new governance boolean, and then banned all leading and trailing escape characters, rather then just spaces.

Sounds like even an improvement to the initial proposal.

[fedekunze]

@rigelrozanski @ValarDragon @cwgoes @sunnya97 @faboweb @liamsi What about banning by default and have a parameter to change it? I think setting the default value as true will provide a better UX since most users usually stick to the default value

Please vote with thumbs up for setting the default value as true or thumbs down for false
Note: strings.TrimSpace also trims tabs (\t) and newlines (\n)

[rigelrozanski]

I'm so confused as to what I'm supposed to vote for - I think a ban on default values (aka empty string) for gov. proposals makes sense

[cwgoes]

Also confused on the options, I voted "thumbs up" to indicate "support banning whitespace by default".

[fedekunze]

lets vote if we want to implement this to avoid spam proposals:
👍: banning whitespaces + empty strings by default
👎: don't do anything in the backend

cc: @cwgoes @ValarDragon @rigelrozanski @sunnya97 @alexanderbez @alessio @jackzampolin

[ValarDragon]

avoid spam proposals

This does nothing to stop spam proposals. There is ongoing discussion in #2256 for things that do stop spam proposals. The reason this doesn't stop spam is that a single letter "a" or "_" repeatedly still works.

But regardless, if we go through with this, I'd prefer we do what I mentioned in my prior message regarding banning more weirdly rendered chars:

Totally agree that no empty titles. Thats already the case though. Just looked at the code, empty is already banned. You're proposing to ban a title of pure spaces. However there are other characters that render weirdly, like newlines and etc. Why aren't those being banned as well here? In general, I think we should attempt to minimize the amount of parameters like these we have, to simplify implementations in other languages. I don't understand why we're worried about UI's breaking if the message is just spaces. (Disclaimer, I have no experience in UI, I would appreciate an explanation if I'm missing something)

This still feels like an odd thing to have in the state machine, but I think if we're doing it, we should ban all strangely rendered things. Also I retract my prior statement about wanting this to be a parameter, if we do go through with this, a parameter seems unnecessary.

Unless this is going to significantly simplify all downstream UI's and/or CLI's, I don't really like the idea of the state machine enforcing this. Empty string seems sufficient to me (as is already implemented) since thats the default value that may get sent by accident.

[ValarDragon]

I think we'll end up with a better outcome if we increase both the gas and deposit of submit proposals. Most uis will likely have separate screens for proposals with sufficient deposit and insufficient deposit. To avoid clogging up the insufficient deposit screen, we can make proposal submitting still cost a non-negligible amount of gas. I don't see why a string of spaces is that bad on the not enough deposit screen. (It's costly to do as well)

Empty strings are already banned, to avoid accidents.

[alexanderbez]

we can make proposal submitting still cost a non-negligible amount of gas

++