Added support for saving message #2409
Conversation
|
This also will allow for a standard and unified implementation for exchanges, payment processors and merchants where users need to be uniquely identified. |
|
@garyyu @yeastplume @ignopeverell Could you review my PR? |
|
Works great 👏 |
| if slate.participant_data.len() > 0 { | ||
| for data in slate.participant_data.iter() { | ||
| if message.len() > 0 { | ||
| message.push_str(&"|".to_string()); |
There was a problem hiding this comment.
What if the message contains a pipe?
There was a problem hiding this comment.
I don't know the details of how it all works. What I've tested until now there were always 2 participants. Can the recipient add the message to the slate as well, to his part of participant_data?
About message containing pipe, I didn't account for that since it's not my use case but since message supports any utf8 character there could also be newlines, correct? I'm open to suggestions for a better delimiter.
Edit because of another comment:
According to your other comment, the message could be stored with participant identifier (I don't know yet what that would be) and the contents could be JSON encoded. This would simplify the delimiter issue, right?
There was a problem hiding this comment.
Yes, and there will ultimately need multiple participants. A JSON structure would work, the best way to do that would be to serialise a struct that contains an id, public key, signature and the message itself
| @@ -109,6 +118,7 @@ where | |||
| let log_id = batch.next_tx_log_id(&parent_key_id)?; | |||
| let mut t = TxLogEntry::new(parent_key_id.clone(), TxLogEntryType::TxSent, log_id); | |||
| t.tx_slate_id = Some(slate_id); | |||
| t.message = Some(message); | |||
There was a problem hiding this comment.
This should be None if there are no messages rather than an empty string.
There was a problem hiding this comment.
Thanks for looking into this! Not against this in theory, but I think one or two small things still need to be thought out. A few comments above, but also since a slate could potentially have many participants, how is each stored message associated with a particular participant? Their public keys would also probably have to be stored somewhere to make this at least somewhat useful.
@mmitech This is not going to be sufficient to uniquely identify participants after the fact. In the best case, all the message could prove is that the person signing the message had the keys for their outputs at the time of signing, but even to do that we'd have to do quite a bit more than what PR is currently providing.
| @@ -205,6 +225,7 @@ where | |||
| let mut t = TxLogEntry::new(parent_key_id.clone(), TxLogEntryType::TxReceived, log_id); | |||
| t.tx_slate_id = Some(slate_id); | |||
| t.amount_credited = amount; | |||
| t.message = Some(message); | |||
There was a problem hiding this comment.
Same, should be None if there's no message
|
Sorry, just one other thing, this only seems to store the message when outputs are being created. I think it would also need to update the stored messages with all participant messages at each part of the exchange (so during finalize as well, which would update the sender's database with all of the participant messages) |
|
Would this be necessary in order to merge this PR or could it be done separately? Above I've suggested a JSON encoded message store in order not to over complicate things due to the possibility of multiple participants. I'm just not sure what would be the identifier of each participant. Any suggestions? EDIT: Actually instead of JSON encoded data it could be an array/object with key/value pairs with key being the participant identifier. |
|
Since mainnet has been launched, we need to be very careful about what we add in now since we'll now be required to support it indefinitely. Unfortunately this means we need to move a bit more slowly and thoughtfully than we did before launch. So I think something like this could work:
And again, this isn't intended to be definitive proof of participation. @ignopeverell @antiochp Does this seem reasonable to you |
How not? we don't need a cryptographical proof, if a user, pool or an exchange provide a unique message with their tx it would be enough for us to help us identify to whom it is intended! because right now exchanges and pools are doing this by including a unique string in their URLs, not sure if that is remotely better than this not to mention the security implications, I would rather see this message packed in a tx, what do you think? |
|
@mmitech it already is in the TX and exchanges can already use it. This PR is just about how to sensibly store it in the wallet alongside the rest of the transaction info. |
|
yes, chainrift.com is using this feature, I was just replying to the part about it not being definitive proof of participation. |
|
@yeastplume please help me understand the use case of multiple participants. Currently since there are always two one is obviously the sender and one participant. Is it possible, even in this case, that they would both be sender and recipient via multiple slate exchanges in one tx? In case of 3 or more participants is there always one sender or can there be multiple senders? If this even makes sense. How would message be set on recipients side. Currently there's no way to do this, correct? The amount in a slate represents the total transferred amount, even in case of 3 or more participants? When there are multiple participants, is it intended to be possible to send a different message to each recipient? |
|
Agree with @yeastplume comments. Not sure if that's already there, but as always when accepting user inputs, the content must be checked. In this particular case, length limit? |
Input limit should be checked where it's entered, I don't think this PR should have anything to do with it since it's just saving the message. |
|
@alesito85 Unfortunately I think it will be necessary to check length before saving, because it will be data that comes in from internet, potentially directly into an open IP:port, meaning someone can try to fill up your disk, spam with unfriendly unicode, put in data that is later read by non-rust code and cause that code to crash (ask Bobby Tables). Are you using this on an internet-accessible listener, i.e. the If chainrift is already using the message feature, we must take care deserializing any message, or only deserialize string to json (struct) if the message begins with something, say "{". The immediate next step seems to be adding a ParticipantMessage struct.in slate.rs |
|
Thanks for getting this going, but I'm going to close this in favor of #2441.. please feel free to review there and/or continue the discussion. |
This PR intends to add support for saving messages that are sent with Grin transactions. In Grin's current state it is possible to store messages but nothing is done with them. In order to be able to use messages and streamline implementation into 3rd party services (merchant or exchange) the team at ChainRift exchange has come up with these basic changes.
What the PR does is save all messages from participants and stores them into the local database, separated by pipe, only extending existing features.
The messages are currently only shown when fetching transactions through owner API. If necessary the txs command can be extended to show messages as well.
Since Grin designers may have intended to save/show messages in a different way I welcome any and all comments in order to meet their intent. Since this was my first touch with Rust feel free to make code suggestions if necessary.