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

Show detailed message info in Keplr #1982

Closed
mhagel opened this issue Oct 3, 2022 · 6 comments
Closed

Show detailed message info in Keplr #1982

mhagel opened this issue Oct 3, 2022 · 6 comments
Assignees
Labels

Comments

@mhagel
Copy link
Contributor

mhagel commented Oct 3, 2022

As a user, when I am confirming a transaction in Keplr, I want to see the details of what I am signing.

RIght now, we see the Msg type, and value: >--xnxahafbs... (some encoded string).

Screen Shot 2022-11-30 at 3.08.40 PM.png

Desired behavior:
the details in "value" should be human-readable, so user knows what they are signing.

Details:

Per Aaron, re: the encoded value...

It will be base64 if the proto message types aren't registered with the json encoder

So, presumably, we just need to make sure the proto message types are registered correctly in regen-js.

@mhagel mhagel self-assigned this Nov 23, 2022
@blushi
Copy link
Member

blushi commented Nov 23, 2022

@mhagel do you have an estimate for this task?

@mhagel
Copy link
Contributor Author

mhagel commented Nov 29, 2022

There may not be a way around this in the regen-js code. This is a known issue with Keplr, as noted in their codebase: chainapsis/keplr-wallet@f86844d#diff-0f59f6b531470706ceb26dfd437a9f532268c7db41145bbbc60c48d566bbc633

because protobuf sign doc is binary formatted, msgs not natively supported by Keplr may not be human-readable.

If you’d like to enforce the use of Amino, you can use the following APIs: keplr.getOfflineSignerOnlyAmino(chainId) or window.getOfflineSignerOnlyAmino(chainId: string). Because this will always return an Amino compatible signer, any CosmJS requested msg that is Amino compatible will request an Amino SignDoc to Keplr.

We can get a human-readable message by forcing keplr to use Amino in regen-web.

However, I'm not sure if we would want to do that, since Amino may go away eventually.

@mhagel
Copy link
Contributor Author

mhagel commented Nov 30, 2022

@clevinson @aaronc @blushi @flagrede

See above comment. We may not be able to fix this in regen-js, but we can patch it in regen-web by forcing keplr to use their amino API.

This draft PR shows the change and results: #1628

This feature has been requested a lot for keplr, and so far this is the only resolution I've seen. There is still an open issue: chainapsis/keplr-wallet#248

WDYT? Would it make sense to do this until Keplr fixes custom proto messages?

@flagrede
Copy link
Collaborator

flagrede commented Dec 1, 2022

If it improves the UX for users, even temporarily, I'm in favor of doing it.

@blushi
Copy link
Member

blushi commented Dec 15, 2022

The only concern that we had with using amino signing by default is that x/group doesn't support it as part of cosmos-sdk 0.46 (that's a bug) and regen ledger v5.0 will be using v0.46.
But the ledger team forked cosmos-sdk v0.46.7 and cherry-picked the PRs that fixed x/group amino signing: regen-network/cosmos-sdk#49 and this forked version will be used in v5.0 release.
So after discussing this with @S4mmyb and @clevinson, the plan is to have @haveanicedavid test groups-ui with regen-ledger v5.0 release to make sure everything works as expected before we move forward with using amino signing by default on our app.

@ryanchristo
Copy link
Member

ryanchristo commented Jul 25, 2023

As mentioned above, this is resolved with application developers using getOfflineSignerOnlyAmino.

I'm going to transfer this issue to the regen-web repository to track switching to only amino within the marketplace application. The groups-ui is already using only amino but not without its challenges (regen-network/groups-ui#105).

I think it makes sense to say this is blocked by v1.0.0 and the root of the problem is fixing the amino converters generated by telescope, which is likely related to regen-network/regen-js#84.

@ryanchristo ryanchristo transferred this issue from regen-network/regen-js Jul 25, 2023
@blushi blushi closed this as completed Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants