Skip to content
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

Support bytes signer fields #15740

Closed
2 tasks
aaronc opened this issue Apr 7, 2023 · 2 comments · Fixed by #21935
Closed
2 tasks

Support bytes signer fields #15740

aaronc opened this issue Apr 7, 2023 · 2 comments · Fixed by #21935

Comments

@aaronc
Copy link
Member

aaronc commented Apr 7, 2023

Summary

Support using bytes fields for message signers.

Problem Definition

As discussed in #13140, it would be nice if modules didn't have to deal with converting bech32 address strings to bytes. Sometimes to avoid making the conversion, modules store addresses in state as bytes which is inefficient and unneeded. The reason addresses are strings in messages is so that they render correctly on the ledger with amino JSON. Now that we have SIGN_MODE_TEXTUAL, we can create a value renderer specifically to do this rendering so that as far as modules are concerned addresses are bytes.

Proposal

  • Support bytes fields in x/tx GetSignersContext
  • Add a value renderer to SIGN_MODE_TEXTUAL for the cosmos_proto.scalar cosmos.AddressBytes which renders it as a bech32 address string
@robert-zaremba
Copy link
Collaborator

Since Amino JSON is going to stay there for a while, and technically every msg should support it, does it mean that we are going to accept address bytes in Amino JSON as a part of the deprecation process?

@aaronc
Copy link
Member Author

aaronc commented Apr 13, 2023

Yes amino JSON would support bytes signers but unfortunately they would likely get rendered as base64 unless we can figure out some special behavior with an annotation. That actually might not be too complex now that we render amino json with protoreflect. We'd just need to coordinate with js implementations. What do you think @kocubinski ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants