-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rename EIP191 Sign Mode #11833
Comments
@fedekunze what tx encoding are you using for this 191 sign mode? We should rename |
Agreed. @fedekunze would you or anyone from your team be willing to make amendments to #11533? |
I'm fine renaming it, I won't be able to look into it until EOW most likely so happy if someone can give me a hand here. |
I don't think we can rename it though, since it would be a proto-breaking change. We can simply deprecate the existing enum option, and add new ones. |
Enum names are proto-breaking? |
I can work on this, but I have a few questions:
|
#11533 introduced a new SignMode enum variant called simply
EIP191
. However, this name is not precise enough. 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.
For this reason, the current sign mode called EIP191 needs to be renamed to avoid confusion. If renaming it is not possible, then it should be deprecated and replaced with a properly named variant.
@AmauryM @fedekunze @alexanderbez
For Admin Use
The text was updated successfully, but these errors were encountered: