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.

Sign up for GitHub

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

Jump to bottom

basic callback on secret stream #34

Merged
merged 22 commits into from
Oct 11, 2023
Merged

basic callback on secret stream #34

merged 22 commits into from
Oct 11, 2023

Conversation

BillCarsonFr
Copy link
Member

@BillCarsonFr BillCarsonFr commented Oct 4, 2023 •
edited
Loading

Adds support for secret stream in wasm.

@BillCarsonFr BillCarsonFr mentioned this pull request Oct 4, 2023
Merged
3 tasks
@BillCarsonFr BillCarsonFr requested review from richvdh and poljar October 4, 2023 14:03
@BillCarsonFr BillCarsonFr marked this pull request as ready for review October 4, 2023 14:58
richvdh
richvdh requested changes Oct 4, 2023
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

Any chance of a test?

src/machine.rs Outdated Show resolved Hide resolved
src/machine.rs Outdated Show resolved Hide resolved
src/machine.rs Outdated Show resolved Hide resolved
src/machine.rs Outdated Show resolved Hide resolved
src/machine.rs Outdated Show resolved Hide resolved
src/machine.rs Show resolved Hide resolved
src/machine.rs Outdated Show resolved Hide resolved
src/machine.rs Outdated Show resolved Hide resolved
richvdh
richvdh reviewed Oct 5, 2023
View reviewed changes
src/machine.rs Outdated Show resolved Hide resolved
@BillCarsonFr BillCarsonFr requested a review from richvdh October 5, 2023 11:43
richvdh
richvdh reviewed Oct 5, 2023
View reviewed changes
src/machine.rs Outdated Show resolved Hide resolved
@BillCarsonFr BillCarsonFr requested a review from richvdh October 10, 2023 07:08
richvdh
richvdh reviewed Oct 10, 2023
View reviewed changes
Copy link
Member

@richvdh richvdh 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 nits & suggestions; generally seems good

src/machine.rs Outdated Show resolved Hide resolved
src/machine.rs Outdated Show resolved Hide resolved
src/machine.rs Outdated Show resolved Hide resolved
src/machine.rs Outdated Show resolved Hide resolved
src/machine.rs Outdated Show resolved Hide resolved
src/machine.rs Outdated Show resolved Hide resolved
src/machine.rs Show resolved Hide resolved
src/machine.rs Outdated Show resolved Hide resolved
src/machine.rs
Comment on lines +1100 to +1101
/// Register a callback which will be called whenever a secret
/// (`m.secret.send`) is received.
Copy link
Member

Choose a reason for hiding this comment

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

is it true that it is called for all m.secret.send events? I got the impression that some secrets (the cross-signing keys?) were handled internally? I might be wrong though

Copy link
Member Author

@BillCarsonFr BillCarsonFr Oct 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

No you are right for now it will only return is the backup decryption key. The cross signing keys are handled internally and custom secret are not yet supported.
Added a comment here and in get_secrets

@BillCarsonFr
Copy link
Member Author

Any chance of a test?

There is no API to inject a secret to test the signaling. So no simple way to test it, but there is a full integrated test on the js-sdk side to test it

BillCarsonFr and others added 5 commits October 10, 2023 15:51
@BillCarsonFr @richvdh
Update src/machine.rs
4e710e0 
Co-authored-by: Richard van der Hoff <[email protected]>
@BillCarsonFr @richvdh
Update src/machine.rs
ead4f39 
Co-authored-by: Richard van der Hoff <[email protected]>
@BillCarsonFr @richvdh
Update src/machine.rs
e214730 
Co-authored-by: Richard van der Hoff <[email protected]>
@BillCarsonFr @richvdh
Update src/machine.rs
f888a3a 
Co-authored-by: Richard van der Hoff <[email protected]>
@BillCarsonFr @richvdh
Update src/machine.rs
8b264ab 
Co-authored-by: Richard van der Hoff <[email protected]>
BillCarsonFr and others added 3 commits October 10, 2023 15:53
@BillCarsonFr @richvdh
Update src/machine.rs
5bd4ffe 
Co-authored-by: Richard van der Hoff <[email protected]>
@BillCarsonFr @richvdh
Update src/machine.rs
b60063a 
Co-authored-by: Richard van der Hoff <[email protected]>
@BillCarsonFr
code review
91eaf8b
@BillCarsonFr BillCarsonFr requested a review from richvdh October 10, 2023 14:12
richvdh
richvdh approved these changes Oct 11, 2023
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

LGTM otherwise

src/machine.rs
Comment on lines +1117 to +1118
/// `callback` should be a function that takes 2 arguments: the secret name
/// (string) and value (string).
Copy link
Member

Choose a reason for hiding this comment

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

does it not matter what it returns?

src/machine.rs
@@ -1100,19 +1100,26 @@ impl OlmMachine {
/// Register a callback which will be called whenever a secret
/// (`m.secret.send`) is received.
///
/// The only secret this will currently broadcast is the
/// `m.megolm_backup.v1` (the cross signing secrets are handled internaly).
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
/// `m.megolm_backup.v1` (the cross signing secrets are handled internaly).
/// `m.megolm_backup.v1` (the cross signing secrets are handled internally).

src/machine.rs
Comment on lines +1139 to +1140
/// The only secret this will currently be returned is the
/// `m.megolm_backup.v1` secret.
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
/// The only secret this will currently be returned is the
/// `m.megolm_backup.v1` secret.
/// The only secret this will currently return is the
/// `m.megolm_backup.v1` secret.

@richvdh
Copy link
Member

richvdh commented Oct 11, 2023

(Looks like we have a flaky test)

@BillCarsonFr
Copy link
Member Author

(Looks like we have a flaky test)

Yes, I see that we create OlmMachine but never close them?

@BillCarsonFr BillCarsonFr merged commit df37f7f into main Oct 11, 2023
3 checks passed
@BillCarsonFr BillCarsonFr deleted the valere/secret_stream_support branch October 11, 2023 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richvdh richvdh richvdh approved these changes

@poljar poljar Awaiting requested review from poljar

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@BillCarsonFr @richvdh