search

cosmos / cosmos-sdk

:chains: A Framework for Building High Value Public Blockchains :sparkles:
https://cosmos.network/
Apache License 2.0
6.2k stars 3.58k forks source link

Implement SIGN_MODE_DIRECT_JSON #9320

Closed aaronc closed 2 years ago

aaronc commented 3 years ago

Implementation details based on design agreed upon in #6513.

amaury1093 commented 3 years ago

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 commented 3 years ago

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

amaury1093 commented 3 years ago

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 commented 3 years ago

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

clevinson commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 2 years ago

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