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

E2EE text messaging example. #3754

Closed
wants to merge 2 commits into from
Closed

E2EE text messaging example. #3754

wants to merge 2 commits into from

Conversation

RhinoDevel
Copy link

@RhinoDevel RhinoDevel commented Sep 27, 2023

Shows how to prepare an existing room for end-to-end-encryption (E2EE) and send a text message.

Addresses issues #437 and #1156.

Signed-off-by: Marcel Timm [email protected]


Here's what your changelog entry will look like:

✨ Features

Shows how to prepare an existing room for end-to-end-encryption (E2EE) and send a text message.
@RhinoDevel RhinoDevel requested a review from a team as a code owner September 27, 2023 09:04
@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Sep 27, 2023
@richvdh richvdh marked this pull request as draft December 5, 2023 13:49
@richvdh richvdh marked this pull request as ready for review December 5, 2023 13:50
@richvdh richvdh requested a review from dbkr March 21, 2024 10:21
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

A few specific points but I think this would need a bit of work generally before being an example we could accept. Some more general points:

  • Comments, comments, comments! This is documentation so it needs to be explaining what it's doing and why in detail so the reader understands the process. eg. why does it need to wait for the 'PREPARED' sync state?
  • It really ought to conform to our code style. In fact it would be nice if it got checked by the linters.
  • It would be even nicer if it got run as part of CI to make sure it still worked every time
  • We don't tend to use the 'mjs' extension - is it necessary? Why?
  • You've use MemoryCryptoStore: this is great for this example but again, needs a comment to say why it's appropriate here and what the limitations are.
  • Some doc of how to run it would feel useful too.

@@ -0,0 +1,126 @@
// Description:
Copy link
Member

Choose a reason for hiding this comment

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

Don't forget license header and copyright attribution (to yourself)

//
// - NodeJS v18.17.1
// - matrix-js-sdk v28.2.0
// - olm v3.1.5
Copy link
Member

Choose a reason for hiding this comment

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

This probably wants to be a block comment?

function log(msg)
{
console.log(`${sLogPrefix}${msg}`);
};
Copy link
Member

Choose a reason for hiding this comment

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

This whole log wrapper and prefix feels weird and distracting for an example, especially since the prefix is hardcoded.

import olm from 'olm';

const sLogPrefix = '*** MAIN ***: ',
sC = {
Copy link
Member

Choose a reason for hiding this comment

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

What does sC mean?

process.exit(1);
}

// Marking all devices as verified:
Copy link
Member

Choose a reason for hiding this comment

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

Okay, but... why?

@RhinoDevel RhinoDevel closed this by deleting the head repository Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Enhancement Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants