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

Implement SIGN_MODE_DIRECT_JSON #9320

Closed
1 of 3 tasks
aaronc opened this issue May 12, 2021 · 8 comments
Closed
1 of 3 tasks

Implement SIGN_MODE_DIRECT_JSON #9320

aaronc opened this issue May 12, 2021 · 8 comments
Assignees
Labels
S:blocked Status: Blocked

Comments

@aaronc
Copy link
Member

aaronc commented May 12, 2021

Implementation details based on design agreed upon in #6513.

@amaury1093
Copy link
Contributor

The 2nd step here is "create deterministic proto JSON serializer", and it's unclear to me what's the best way to start working on this.

My understanding is the determinstic JSON serializer will be incorporated into our custom codegen at some point, but that might not be ready for 0.44. So we'll need to write this serializer for the current codebase anyways.

@aaronc should we start writing a proto-reflection-based serializer for the current api v1 codebase?

@aaronc
Copy link
Member Author

aaronc commented Jul 19, 2021

@AmauryM, @ruhatch is working on the serializer. We still need someone to work on the sign mode handlers and that's somewhat orthogonal.

@amaury1093
Copy link
Contributor

amaury1093 commented Jul 19, 2021

yeah I know, I'm asking because during our standup we assigned #8476 to Ru, but it was unclear what the next step after that was. In this case i'll let the 2 of two sync on this 👍

@aaronc
Copy link
Member Author

aaronc commented Jul 19, 2021

Okay, maybe I'm not understanding. @ruhatch let's sync on this.

@ruhatch ruhatch changed the title Implement SIGN_MODE_TEXTUAL Implement SIGN_MODE_DIRECT_JSON Jul 21, 2021
@clevinson
Copy link
Collaborator

I think there's a lot of good reasons for us prioritizing SIGN_MODE_TEXTUAL specification for hardware wallets being discussed in #6513, but want to better understand if this really means that we should not do SIGN_MODE_DIRECT_JSON at all...

From #6513 (@aaronc):

We can always do proto JSON signing first and then add textual signing later, but given that proto JSON signing is actually a lot of work for three teams, I want to invite us to consider whether we're maybe better off scraping proto JSON and just doing sign mode textual like we originally intended. My fear is that proto JSON will have felt like such an effort for so little pay off that nobody will have much enthusiasm to do the work for actual textual signing.

@aaronc Do you mean scrapping proto JSON entirely or just for the hardware wallet use case?

IMO there are still some valid use cases for a JSON based sign-mode that's a hybrid of "dev-friendly human readable" & machine readable. Interacting with the CLI through the --generate-only flag comes to mind. Or maybe we should just be folding those requirements (like the ability to aggregate single-message tx's into a single tx with multiple messages...) into SIGN_MODE_TEXTUAL.

The other reason why I see us still potentially wanting SIGN_MODE_DIRECT_JSON is that this sign mode would "come for free" without any additional requirements of chain developers. If the current direction for SIGN_MODE_TEXTUAL treats it as required of all sdk.Msg implementations, then we should be cognizant of the additional work this puts on chain developers. If its required - would that be for only "English" versions of SIGN_MODE_TEXTUAL? Or how would we distinguish btw different localized implementations satisfying/not satisfying the "Requirement" of a SIGN_MODE_TEXTUAL format string?

@aaronc
Copy link
Member Author

aaronc commented Jul 28, 2021

@aaronc Do you mean scrapping proto JSON entirely or just for the hardware wallet use case?

That's sort of what I'm proposing to optimize developer time across several projects...

The other reason why I see us still potentially wanting SIGN_MODE_DIRECT_JSON is that this sign mode would "come for free" without any additional requirements of chain developers. If the current direction for SIGN_MODE_TEXTUAL treats it as required of all sdk.Msg implementations, then we should be cognizant of the additional work this puts on chain developers.

Yes, you're right that it sort of "comes for free" for chain developers, but it doesn't come for free for the SDK, CosmJs or Ledger developers. Having both JSON signing + textual signing will probably take us twice the effort. Also, it doesn't really come for free in the sense that users that really see very many benefits of sign mode JSON unless the ledger app specifically adapts to each case which seems much more complex than SIGN_MODE_TEXTUAL. I think many of us have experience signing messages on Osmosis with the Ledger and I can never tell what I'm signing with the default JSON rendering of Osmosis specific messages. It all feels like blind trust in the Osmosis UI.

So yes, it is additional overhead for chain developers but IMHO it's additional work that brings a lot of value. I think we should reduce the overhead with a good set of linting tools and best practices provided by the SDK as I tried to outline above.

If its required - would that be for only "English" versions of SIGN_MODE_TEXTUAL? Or how would we distinguish btw different localized implementations satisfying/not satisfying the "Requirement" of a SIGN_MODE_TEXTUAL format string?

One unfortunate limitation of ledger devices is that they only support the renderable subset of ASCII characters. So localization is not impossible but will be somewhat ugly and have the unfortunate side effect of sometimes telling users cuantos anos some contract will be valid for. Maybe people can live with that but they'll get a bit of a laugh. I actually find it strange and would be surprised if Ledger doesn't have Unicode in their roadmap (would you happen to know @jleni ?). They're based in France right? And I know that little screen can sometimes render images.

@jleni
Copy link
Member

jleni commented Jul 28, 2021

side effect of sometimes telling users cuantos anos some contract will be valid for. Maybe people can live with that but they'll get a bit of a laugh. I actually find it strange and would be surprised if Ledger doesn't have Unicode in their roadmap (would you happen to know @jleni ?). They're based in France right? And I know that little screen can sometimes render images.

I don't think this is something in their roadmap... while extended ascii "could" be possible... (solving problems like contrato valido por 2 anos 🙃) ... current Ledger devices no definitely way of supporting Unicode and more complex character sets such as korean/chinese/japanese/etc... there is not enough storage to do that..

@amaury1093
Copy link
Contributor

We're not pursuing the DIRECT_JSON sign mode anymore, because of limitations of using JSON in ledger devices. For more info, see the meeting notes of the 22.09.2021 meeting with Juan in https://hackmd.io/G4mjmz7YRJ-5_rE12Y8uYQ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S:blocked Status: Blocked
Projects
None yet
Development

No branches or pull requests

5 participants