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-0020: Transaction messages/comments metadata #100

Merged
merged 21 commits into from
Aug 17, 2021

Conversation

gitmachtl
Copy link
Contributor

No description provided.

@gitmachtl gitmachtl changed the title Transaction messages/comments metadata CIP-0020 - Transaction messages/comments metadata Jun 14, 2021
@gitmachtl gitmachtl changed the title CIP-0020 - Transaction messages/comments metadata CIP-0020: Transaction messages/comments metadata Jun 14, 2021
@SebastienGllmt
Copy link
Contributor

This seems conceptually similar to #44 so there may be ideas in the discussion of that CIP you're interested in

@gitmachtl
Copy link
Contributor Author

gitmachtl commented Jun 29, 2021

This seems conceptually similar to #44 so there may be ideas in the discussion of that CIP you're interested in

thx, i checked #44 but thats a really specific CIP for the SPO to delegator communication. the #100 is from wallet owner to wallet owner as part of the actual transaction detail. like when you pay a bill you wanna attach the invoice number for example. i think we have room for some specific labels here, i don't think that we have to make all "messaging/description" related schemes into one label here. the #100 was created as a fast simple solution without much "overhead" in the keys of the json to keep the fees as low as possible.

@gitmachtl
Copy link
Contributor Author

Implementation-Update: The CardanoCommunityWallet https://ccwallet.io now also implemented the transaction metadata/comment CIP0020 into the wallet.

@gitmachtl
Copy link
Contributor Author

Implementation-Update: https://cardanoscan.io now also implemented the transaction metadata/comment CIP0020 🥰

image

@gitmachtl
Copy link
Contributor Author

Implementation-Update: https://adastat.net now also implemented the transaction metadata/comment CIP0020 🥰

image

@gitmachtl
Copy link
Contributor Author

Implementation-Update: CardanoWall https://cardanowall.com now also implemented the transaction metadata/comment CIP0020.

@mmahut
Copy link
Contributor

mmahut commented Jul 4, 2021

This opens a new and pretty dangerous phishing vector, as it allows scammers messaging wallet end users directly and presenting them phishing information without a way to filter it out. I think the final wallet implementation should be carefully considered (how are the wallets going to display the link? is there even a way to shield domain names?).

Are there any thoughts on how to deal with these threats?

@gitmachtl
Copy link
Contributor Author

gitmachtl commented Jul 4, 2021

What link? Metadata is already displayed in Daedalus for example and in the Explorer. I think it would be a good thing to hide it behind a "show unmoderated note" button like already implemented in the Explorer. The message is currently just plain-text, not a clickable link. As with everything, there is room for abuse yes, auto-deleting/hiding urls would be possible i think.

@gitmachtl
Copy link
Contributor Author

gitmachtl commented Jul 4, 2021

Are there any thoughts on how to deal with these threats?

Thx for the reply, i added in these thoughts in the draft. Autodeleting/Autohiding URLs is a possible solution for such comments i guess. Or limiting it to a selected range of chars for the beginning. The messaging could be improved over time when identity is working and fully in place for transactions too.

@mmahut
Copy link
Contributor

mmahut commented Jul 4, 2021

Yes, I think especially wallet interfaces needs to be very careful about this - the link even if it is not clickable will result in some number of people scammed, just by sending malicious instructions to non-technical end users.

How is the messaging solved in solutions like Atala Prism that includes identity as well?

@gitmachtl
Copy link
Contributor Author

Some positive signals from the Daedalus Wallet side about an implementation: input-output-hk/daedalus#2613 (comment)

Also from the Adalite Wallet Team: vacuumlabs/adalite#1059 (comment)

@crptmppt
Copy link
Contributor

crptmppt commented Jul 28, 2021

PR100 is set as a REVIEW item for next week, 8/3 Editor meeting 27: if the issue is of relevance to you, consider attending the meeting or adding to the conversation here.

@gitmachtl
Copy link
Contributor Author

PR100 is set as a REVIEW item for next week, 8/3 Editor meeting 27: if the issue is of relevance to you, consider attending the meeting or adding to the conversation here.

Thx!

@gitmachtl
Copy link
Contributor Author

Implementation-Update: https://ccwallet.io supports it now on the sending-page via an input field (one liner for sending, max. 64char length), and also on the transactions-page for the incoming and outgoing transactions (including multiline support):
image
image
image

Copy link
Contributor

@SebastienGllmt SebastienGllmt left a comment

Choose a reason for hiding this comment

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

Technically speaking nothing wrong with this specification and it's implemented in places so I see no reason not to merge it

@gitmachtl
Copy link
Contributor Author

@KtorZ review/approval request

@crptmppt
Copy link
Contributor

This PR was REVIEWED last week's Editor meeting (27)- see notes.
It is sensible, already used by some and ready to be merged.
PR100 is set as a Last Check item for next week's 8/17 Editor meeting 28.
If the issue is of relevance to you, consider attending the meeting.

@gitmachtl
Copy link
Contributor Author

gitmachtl commented Aug 11, 2021

StakePool Operator Tools Integration Example: It works on the commandline like any other script of the collection by just adding the "msg: ..." parameter to a transaction. This automatically generates the needed metadata.json structure and attaches it to the transaction itself. (https://github.com/gitmachtl/scripts)

image

@Scitz0
Copy link
Contributor

Scitz0 commented Aug 12, 2021

This CIP has been supported in CNTools since the draft was originally put together.
This is how it looks when adding a message when sending funds by requesting normal text input through the default Linux text editor and automatically generating the correct JSON schema with 674 label.

image

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.

Looks fine. I'd perhaps like to see a note about what implementors should do when receiving an ill-formed 674 metadata. For example, one that does not contain a "msg" key or, contains more than one key, or contains something else than a list of strings. I'd argue that in such case, the client application should ignore the metadata, or perhaps present it as "invalid" without displaying it.

@Scitz0
Copy link
Contributor

Scitz0 commented Aug 12, 2021

Looks fine. I'd perhaps like to see a note about what implementors should do when receiving an ill-formed 674 metadata. For example, one that does not contain a "msg" key or, contains more than one key, or contains something else than a list of strings. I'd argue that in such case, the client application should ignore the metadata, or perhaps present it as "invalid" without displaying it.

My view on it is as follows.

The message should be considered valid if the following apply:

  • Label = 674.
  • has property "msg".
  • msg property contains an array of strings, even for a single-line message.
  • Each line has a maximum length of 64 characters.
  • If there are additional properties I don't think this should invalidate the message. They can just be ignored.

If any of the above is not met, ignore the metadata as a transaction message. Can still be displayed as general metadata to the transaction.

Optional to consider for the implementer:

  • For message creation both single-line and multi-line input should be considered valid. The wallet/tool isn't required to support multi-line input.
  • Message display in explorers/wallets should however preferably support multi-line messages even if it only supports single-line on creation. Not a requirement but should at least indicate that there are more data if only the first line is displayed. Maybe a link to explorer etc in the case it's not possible to solve in UI in a good way.
  • Message sanitation. Up to the implementer for how to handle and on what level. Should at least treat as text only and prevent code injection.

added some notes on how to handle ill-formed 674 metadata
@gitmachtl
Copy link
Contributor Author

I agree here, have already added some notes to the draft itself and also added your comments too.

@OliHericium
Copy link

What is the point of having a separate CIP for this? Should not this be just an entry in CIP10? Thank you.

@gitmachtl
Copy link
Contributor Author

gitmachtl commented Aug 13, 2021

What is the point of having a separate CIP for this? Should not this be just an entry in CIP10? Thank you.

The update of the CIP0010 registry.json is only a side commit. You should take a look at https://github.com/cardano-foundation/CIPs/blob/80fd709163481528b1cbf22193a18afb179ee9bc/CIP-0020/CIP-0020.md

The entries in CIP0010 are only the label reservations, but no format/usage specification.

@KtorZ
Copy link
Member

KtorZ commented Aug 17, 2021

Indeed @gitmachtl. I feel like we might need to update CIP-0010 to clarify this. We do expect people to open a CIP for a new entry in CIP-0010 to explain the purpose of that new entry. Even if short, a dedicated CIP is always useful.

@crptmppt crptmppt merged commit ca04875 into cardano-foundation:master Aug 17, 2021
@gitmachtl gitmachtl deleted the patch-2 branch August 17, 2021 09:37
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.

7 participants