-
Notifications
You must be signed in to change notification settings - Fork 318
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
CIP-0083 | Adding encrypted messages to CIP-0020 #409
Conversation
- Eternl.io Wallet has now done an active implementation - Added Screenshot
Update - Eternl.io Wallet has now also an active implementation |
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 |
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 ? |
It's active |
- removed <p> entries - shifted captions one level down
CIP-????/README.md
Outdated
"674": | ||
{ | ||
"enc": "<encryption-method>", | ||
"msg": |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
CIP-????/README.md
Outdated
* 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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. |
CIP-????/README.md
Outdated
@@ -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. |
There was a problem hiding this comment.
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-????/README.md
Outdated
--- | ||
CIP: ???? | ||
Title: Encrypted Transaction message/comment metadata (Addendum to CIP-0020) | ||
Status: Proposed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Status: Proposed | |
Status: Active |
There was a problem hiding this 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 :)
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. |
…s/democode-NODEJS.js
…83/codesamples/encrypted-message-metadata.json
…codesamples/normal-message-metadata.json
thanks @gitmachtl - please let us know when done with the latest round of commits so we can merge this. |
There was a problem hiding this 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.
…#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 ' <p>' * Updating formatting - removed <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
(rendered latest version in branch)