refactor: remove gogoproto stringer annotations and MarshalYAML functions

[julienrbrt]

Description

Follow-up of cosmos/gogoproto#24 / #10965
Fix tests mentioning #10965 by using .String() instead of marshalling and unmarshalling for comparing value.

---

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

• included the correct type prefix in the PR title
• added ! to the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• followed the guidelines for building modules
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• included comments for documenting Go code
• updated the relevant documentation or specification
• reviewed "Files changed" and left comments if necessary
• confirmed all CI checks have passed

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)

[julienrbrt]

My main concerns of removing some .String() methods, is that the generated String() methods, is sometimes less human-readable. Are we okay with that?
cc @AmauryM

[amaury1093]

Thanks! Removing this proto annotation feels great.

My main concerns of removing some .String() methods, is that the generated String() methods, is sometimes less human-readable. Are we okay with that?

Yes, I think the consensus in #12908 was that the trade-off is reasonable. Morever, if people want more human-friendliness, they can always go the JSON->YAML route.

[julienrbrt]

Yes, I think the consensus in #12908 was that the trade-off is reasonable. Morever, if people want more human-friendliness, they can always go the JSON->YAML route.

Okay, I can continue to do that across the codebase if 64db167 is acceptable.

[tac0turtle]

LGTM, thank for this cleanup

[sonarcloud]

[Cosmos SDK] SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

[amaury1093]

LGTM

[tac0turtle]

have we checked if the string versions were compatible? we should document some of the breaking formats, if there are any(?).

I think the stable query checks would have caught on some of the strings in staking.

[julienrbrt]

have we checked if the string versions were compatible? we should document some of the breaking formats, if there are any(?).

I think the stable query checks would have caught on some of the strings in staking.

String results are different because they are differently stringified. Here they however contain all same values.

I can look further into staking, but note that not all Stringer have been removed for that exact reason (because sometimes the amount of returned contains less information than using the proto generated String method - I need to look at what is the impact of that).

Are chains dev using Strings methods for any other reason than display?

[tac0turtle]

It should be fine, worried mainly that it breaks an endpoint for users and we aren't aware of it.

[amaury1093]

I'm not worried about endpoints, we generally use protobuf (not .String) for them. I'm slightly more worried about state-machine, like @julienrbrt pointed out:

Are chains dev using Strings methods for any other reason than display?

I think in the SDK we're fine, we have 2-3 instances like this one, where we use String for comparison, potentially in state-machine code. I think it's a terrible pattern, and should not be encouraged.

Maybe we can add a small note in the changelog/upgrading guide to tell chain devs to double-check they are not doing this anymore?