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

Off-chain Messages #22

Merged
merged 9 commits into from
Nov 14, 2023
Merged

Off-chain Messages #22

merged 9 commits into from
Nov 14, 2023

Conversation

JulianToledano
Copy link
Contributor

No description provided.

JulianToledano and others added 2 commits October 11, 2023 11:50
* add: offchain cip

* lint

* Update cips/cip-offchain_message_signature.md

Co-authored-by: Ezequiel Raynaudo <[email protected]>

* improvements

* fix

* fix

* fix: list of questions

---------

Co-authored-by: Ezequiel Raynaudo <[email protected]>
@JulianToledano JulianToledano requested a review from a team as a code owner October 11, 2023 10:00
cips/cip-X.md Show resolved Hide resolved
cips/cip-X.md Show resolved Hide resolved
cips/cip-X.md Outdated Show resolved Hide resolved
Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍾❤️🚀

cips/cip-X.md Outdated Show resolved Hide resolved
cips/cip-X.md Outdated Show resolved Hide resolved
cips/cip-X.md Show resolved Hide resolved
cips/cip-X.md Outdated Show resolved Hide resolved
cips/cip-X.md Show resolved Hide resolved
cips/cip-X.md Show resolved Hide resolved
cips/cip-X.md Show resolved Hide resolved
cips/cip-X.md Outdated Show resolved Hide resolved
cips/cip-X.md Outdated Show resolved Hide resolved
cips/cip-X.md Outdated Show resolved Hide resolved
@JulianToledano
Copy link
Contributor Author

@tac0turtle @webmaster128
Hey guys! I did commit most of your suggestions. One thing I don't have clear is:
should we go with just one message or both?
As commented above the main problem with ADR-036 was that the message defined wasn't human readable.

@webmaster128
Copy link
Member

should we go with just one message or both?

I like the separate MsgProveIdentity. Sounds specific and flexible at the same time.

For the arbitrary content signing I'd go with arbitrary valid UTF-8 text. This is readable by default, encourages the design of readable solutions and can host arbitrary data as well. E.g. any JSON document is a printable string and embedding raw data using some kind of encoding is just as good as a bytes blob that the UI has to show as base64 anyways. Given that I don't see a reason for arbitrary non-string data signing.

cips/cip-X.md Outdated Show resolved Hide resolved
cips/cip-X.md Outdated Show resolved Hide resolved
@testinginprod testinginprod self-requested a review November 14, 2023 10:13
Copy link

@testinginprod testinginprod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I would sync with Keplr since I saw they have a MsgSignData they use for off-chain signing, and understand how this might impact them.

Thank you.

@tac0turtle
Copy link
Member

merging for now, but we will still sync with wallets and clients

@tac0turtle tac0turtle merged commit 5f06ca8 into cosmos:main Nov 14, 2023
3 checks passed
@dogemos
Copy link
Member

dogemos commented Dec 1, 2023

one minor feedback from @Thunnini.

could an optional chainID field be added?

@webmaster128
Copy link
Member

could an optional chainID field be added?

This was discussed somwhere in here. Where and why do you need this? Since off-chain messages are outside of any chain, I don't see how this belongs in here. In MsgSignArbitraryData you can add application specific context (which might include a chain ID). MsgProveIdentity proved a keypair ownership independent of a chain.

@JulianToledano
Copy link
Contributor Author

could an optional chainID field be added?

This was discussed somwhere in here.

Here is the discussion.

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

Successfully merging this pull request may close these issues.

6 participants