feat: EIP191 sign mode

[fedekunze]

Description

Adds EIP-191 as a supported sign mode.

Ref: #10553 (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)

[amaury1093]

Would love to see some docs on this, in a follow-up!

[amaury1093]

Let's also wait for Aaron's approval

[codecov]

Codecov Report

Merging #11533 (0b971b5) into master (44a1293) will decrease coverage by 1.05%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master   #11533      +/-   ##
==========================================
- Coverage   66.09%   65.03%   -1.06%     
==========================================
  Files         715      679      -36     
  Lines       72876    70814    -2062     
==========================================
- Hits        48165    46052    -2113     
- Misses      21762    22091     +329     
+ Partials     2949     2671     -278     

Impacted Files	Coverage Δ
client/cmd.go	55.67% <ø> (ø)
client/flags/flags.go	20.68% <ø> (ø)
client/tx/factory.go	25.86% <0.00%> (-0.23%)	⬇️
x/bank/types/send_authorization.go	55.55% <0.00%> (ø)
x/group/client/cli/tx.go	0.00% <0.00%> (ø)
x/group/client/cli/util.go	6.97% <40.00%> (ø)
crypto/keyring/keyring.go	62.39% <50.00%> (ø)
x/auth/tx/config.go	89.65% <100.00%> (+0.76%)	⬆️
x/authz/client/testutil/tx.go	98.25% <100.00%> (+0.02%)	⬆️
x/staking/types/authz.go	87.37% <100.00%> (+0.37%)	⬆️
... and 47 more

[alexanderbez]

I think this is fine.

[amaury1093]

Got an offline ack from Aaron, I just added some proto comments to clarify. Thanks @fedekunze for this PR!

[sunnya97]

I don't think this should be called simply Sign Mode "EIP191". EIP 191 simply refers to a method of encoding a string data to be signed.

However, the Sign Mode concept in Cosmos SDK also refers to how to encode a tx into a string in the first place.

For this reason, in the osmosis-labs version of this, we specifically call this "EIP191LegacyJSON" to signal that it is the EIP191 re-encoded version of the Amino JSON string.

In the future, we will probably want more EIP191 sign modes, such as an EIP191Textual which wraps SignModeTextual.

I think any new sign modes should have an associated ADR for them. For example, in the case of EIP191LegacyJSON, we would also need to standardize how to format the amino json (such as adding new lines and indents) before putting it through the EIP191 conversion.

[amaury1093]

Ah I wasn't aware of this. That's unfortunate, we just released this in v0.45.2. I don't think we'll be able to rename, but we can always deprecate and create new enum variants.

I think any new sign modes should have an associated ADR for them. For example, in the case of EIP191LegacyJSON, we would also need to standardize how to format the amino json (such as adding new lines and indents) before putting it through the EIP191 conversion.

Agreed. The current PR doesn't introduce EIP-191, just technically a new enum variant, and any chain can use it and put whatever sign mode handler they want in this enum.

The day we decide that the SDK should fully support a EIP191-based sign mode, we should definitely create an ADR, and define the payload format.

[alexanderbez]

Let's be sure to create an issue for this @AmauryM

[amaury1093]

Let's be sure to create an issue for this @AmauryM

@sunnya97 would you like to create this issue, and explain how osmosis is doing it currently?