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

CIP-0083 | Adding encrypted messages to CIP-0020 #409

Merged
merged 31 commits into from
Jan 17, 2023

Conversation

gitmachtl
Copy link
Contributor

@gitmachtl gitmachtl commented Dec 8, 2022

CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
@gitmachtl
Copy link
Contributor Author

Update - Eternl.io Wallet has now also an active implementation

@rphair
Copy link
Collaborator

rphair commented Jan 16, 2023

FYI this is on the agenda for the CIP meeting tomorrow & I've noted for us to decide on whether the currently documented implementation status should classify this as Proposed or Active. So @gitmachtl @AndrewWestberg if there are any feelings this should be merged as Active then please post here soon.

@gitmachtl
Copy link
Contributor Author

FYI this is on the agenda for the CIP meeting tomorrow & I've noted for us to decide on whether the currently documented implementation status should classify this as Proposed or Active. So @gitmachtl @AndrewWestberg if there are any feelings this should be merged as Active then please post here soon.

thx for the info, i think current implementations are enough to set it to active. ashish will also introduce it to cardanoscan by the end of january as the next implementation. i think we don't have a strict rule here about numbers. imo its ok to set it to active. @AndrewWestberg ?

@AndrewWestberg
Copy link
Contributor

It's active

CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
- removed &nbsp;<p> entries
- shifted captions one level down
CIP-????/README.md Outdated Show resolved Hide resolved
"674":
{
"enc": "<encryption-method>",
"msg":
Copy link
Member

Choose a reason for hiding this comment

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

So, this format doesn't allow mixing non-encrypted and encrypted messages. That's perhaps okay by design, but thought I'd still raise this? Maybe that's a possible addition to the design?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment was added about that in the rational

* has property "enc".
* enc property contains a supported method like `basic`
* has property "msg".
* msg property contains an array of strings, even for a single-line message.
Copy link
Member

Choose a reason for hiding this comment

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

This (as well as the base64 encoding of each string) sounds like a bit of a unfortunate design decision? On-chain, metadata are actually encoded as bytes using CBOR; which means that each message can actually be encoded as a plain byte-array, saving 33% size over the base64 encoding.

If anything, I'd at least like to see this design decision (choosing UTF-8 text string over plain bytes) explained in the rationale section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment was added about that in the rational

Copy link
Member

@KtorZ KtorZ Jan 17, 2023

Choose a reason for hiding this comment

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

I am now pleased with CIP editor's hat; but not with my technical expert's one 😬. I am not convinced by the justification of using base64-encoded text strings and I would strongly advocate against that.

@gitmachtl
Copy link
Contributor Author

gitmachtl commented Jan 17, 2023

I want to remember everyone, that this is an addendum to the existing CIP-0020, which is active and implemented by many tools out there. This addendum takes advantage of the original format and extends functions in a backwards compatible way. This CIP is not here to wipe out the current CIP-0020 implementations in tools, its an optional addition for those who wants to implement it. There might be completely new and breaking messaging implementations in the future, this current design was choosen to make the feature available in a backwards compatible and 'easy-to-integrate' way. Which was well received from all implementors so far.

@@ -202,7 +202,8 @@ Which results in the original content of the **msg** key:
## Rationale

This design is simple, so many tools on the cardano blockchain can adopt it easily and a few have already started to implement it.
The original CIP-0020 design allowed the addition of new entries like the `"enc":` key for encrypted messages in this CIP.
The original CIP-0020 design allowed the addition of new entries like the `"enc":` key for encrypted messages in this CIP. Therefore the encoding format of the encrypted message was choosen to be UTF-8 instead of bytearrays, because it would break the backwards compatibility to CIP-0020. But maybe more important, it gives the user a simple text-format to handle such messages. Users can copy and paste the base64 encoded string(s) using there own tools for creation and verification. For example, a user can simply copy the encrypted format from an explorer and verify it with an external own local tool. Such messages are usally pretty short. Yes, the benefit of using bytearrays is to have less data (around -33% over base64), but the decision was made to sacrifice this benefit in favor of the base64 format for the reasons pointed out before.
Copy link
Member

@KtorZ KtorZ Jan 17, 2023

Choose a reason for hiding this comment

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

I'd argue that frontend applications can still display the data as base64-encoded text string; while the on-chain data is itself being encoded as a plain byte array. Showing that data in a "human-readable" way is really a UI concern. As for backward compatibility; given that enc already specifies the behavior that a server needs to adopt in order to decode the msg payload; this may as well indicates that the data is specified as a sequence of bytes and not a sequence of UTF-8 text.

---
CIP: ????
Title: Encrypted Transaction message/comment metadata (Addendum to CIP-0020)
Status: Proposed
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Status: Proposed
Status: Active

Copy link
Member

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

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

Approved from an editing standpoint. I'd still strongly suggest to consider using plain bytes for the on-chain representation instead of base64 text strings.

Will assign a number and merge as active as soon as you confirm your final stand regarding base64 @gitmachtl :)

@gitmachtl
Copy link
Contributor Author

gitmachtl commented Jan 17, 2023

Will assign a number and merge as active as soon as you confirm your final stand regarding base64 @gitmachtl :)

Thx for your response, as this CIP was intentionally designed as a simple addendum to CIP-0020 the design decision is base64 for this CIP. However, i fully understand your point and we even did some testing about the byte usage before we pushed that PR. With all the other metadata stuff out there - especially all the 721 stuff for nfts - we decided that its worth it to stay on base64 encoding to preserve the backwards compatibility with existing tooling. But, we'll be happy to work on another CIP for the future to tackle the raised conserns - most likely also pick up some design ideas from CIP-0008 for that too to present a breaking-change solution. I am technical as you know, but for this design decision the importance of bringing user experience benefits to cardano in a reasonable timeframe was more important than squezzing out the last bytes from the storage needed.

@KtorZ KtorZ changed the title CIP-???? | Adding encrypted messages to CIP-0020 CIP-0083? | Adding encrypted messages to CIP-0020 Jan 17, 2023
CIP-????/README.md Outdated Show resolved Hide resolved
@rphair
Copy link
Collaborator

rphair commented Jan 17, 2023

thanks @gitmachtl - please let us know when done with the latest round of commits so we can merge this.

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

OK I see that we are "done" as per #409 (comment) so merging this.

@rphair rphair merged commit e201fd2 into cardano-foundation:master Jan 17, 2023
@rphair rphair changed the title CIP-0083? | Adding encrypted messages to CIP-0020 CIP-0083 | Adding encrypted messages to CIP-0020 Jan 17, 2023
Ryun1 pushed a commit to Ryun1/CIPs that referenced this pull request Nov 17, 2023
…#409)

* Create README.md

* Create democode-NODEJS.js

* Create democode-PHP.php

* Create democode-BASH.sh

* Create normal-message-metadata.json

* Create encrypted-message-metadata.json

* Removed markdown in the header preamble

* Update README.md

* grammer correction

* grammer correction

* Create democode-WEB.js

* Added 'Path to Active' section

* Update AdaStat.net Screenshots

* Update README.md

* Adding Eternl.io - Now active implementation

- Eternl.io Wallet has now done an active implementation
- Added Screenshot

* removed '&nbsp;<p>'

* Updating formatting

- removed &nbsp;<p> entries
- shifted captions one level down

* moved integration examples into section 'path to active'

* added comment about future mixed style option

* added comment why base64 format was choosen

* Create format.cddl

* Update README.md

* Changed status to 'Active'

* Update and rename CIP-????/README.md to CIP-0083/README.md

* Rename CIP-????/format.cddl to CIP-0083/format.cddl

* Rename CIP-????/codesamples/democode-BASH.sh to CIP-0083/codesamples/democode-BASH.sh

* Rename CIP-????/codesamples/democode-NODEJS.js to CIP-0083/codesamples/democode-NODEJS.js

* Rename CIP-????/codesamples/democode-PHP.php to CIP-0083/codesamples/democode-PHP.php

* Rename CIP-????/codesamples/democode-WEB.js to CIP-0083/codesamples/democode-WEB.js

* Rename CIP-????/codesamples/encrypted-message-metadata.json to CIP-0083/codesamples/encrypted-message-metadata.json

* Rename CIP-????/codesamples/normal-message-metadata.json to CIP-0083/codesamples/normal-message-metadata.json
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.

None yet

4 participants