fix: revert proto changes from #12610

[likhita-809]

Description

ref:#12610 (comment)

---

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)

[likhita-809]

@AmauryM should I remove the changelog of #12610 (from State Machine Breaking) and add another one for new feature ?

[amaury1093]

LGTM

[amaury1093]

@AmauryM should I remove the changelog of #12610 (from State Machine Breaking) and add another one for new feature ?

No that changelog entry looks good. We changed ValidateBasic, so it's still a State Machine Breaking change

[julienrbrt]

Just a general question, (for @AmauryM ?) -> when do we actually change the protobuf and upgrade its version?

[Amaury]

Just a general question, (for @Amaury ?) -> when do we actually change the protobuf and upgrade its version?

Still not the right "Amaury" ^^'
Don't forget the "m" at the end...

[julienrbrt]

Just a general question, (for @Amaury ?) -> when do we actually change the protobuf and upgrade its version?

Still not the right "Amaury" ^^'

Don't forget the "m" at the end...

Oops 😅 @AmauryM

[amaury1093]

when do we actually change the protobuf and upgrade its version?

Whenever a change is "proto-breaking", we need to bump the version. The definition of "proto-breaking" is in ADR044, which is basically what buf's linting guide recommends. BTW I just re-read it, and made some small tweaks #12683.

Generally when the "Protobuf / break-check" is red, it's a sign that some proto pkg should be bumped.

[julienrbrt]

lgtm

[codecov]

Codecov Report

Merging #12670 (8a4a21b) into main (5e1ff06) will increase coverage by 0.06%.
The diff coverage is 85.26%.

@@            Coverage Diff             @@
##             main   #12670      +/-   ##
==========================================
+ Coverage   65.33%   65.39%   +0.06%     
==========================================
  Files         689      693       +4     
  Lines       70680    70965     +285     
==========================================
+ Hits        46178    46407     +229     
- Misses      21944    21993      +49     
- Partials     2558     2565       +7     

Impacted Files	Coverage Δ
x/bank/keeper/msg_server.go	19.27% <0.00%> (-0.24%)	⬇️
x/bank/keeper/send.go	82.03% <82.35%> (ø)
x/bank/simulation/operations.go	79.01% <88.67%> (+2.36%)	⬆️
x/bank/types/msgs.go	92.52% <100.00%> (+3.52%)	⬆️
x/distribution/simulation/operations.go	80.85% <0.00%> (-9.58%)	⬇️
errors/stacktrace.go	84.14% <0.00%> (ø)
errors/abci.go	87.23% <0.00%> (ø)
errors/errors.go	83.46% <0.00%> (ø)
errors/handle.go	0.00% <0.00%> (ø)
... and 3 more

[alexanderbez]

Generally when the "Protobuf / break-check" is red, it's a sign that some proto pkg should be bumped.

This happens all the time though, e.g. a new field. That doesn't mean it's breaking AFAIU.

[alexanderbez]

Also, what's the plan here WRT to the actual desired change (#12601)...are we just not doing it?

[julienrbrt]

Also, what's the plan here WRT to the actual desired change (#12601)...are we just not doing it?

It has been implemented in #12610. This PR only reverts the protobuf breaking changes but ValidateBasic won't allow more that one sender anyway (resolving the issue).

[alexanderbez]

I see. IMO keeping the proto fields unchanged, i.e. it remaining []Input, is not the cleanest approach. But if we're really trying to avoid proto breaking changes, then I suppose it's fine. I would at least consider adding a new field and marking that one deprecated.

cc @AmauryM

[amaury1093]

I would at least consider adding a new field and marking that one deprecated.

Yes that's fine too. Let's make sure to make it as backwards-compatible as possible.