-
Notifications
You must be signed in to change notification settings - Fork 33
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
Encrypt and Decrypt support #39
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,7 +29,7 @@ pub fn create_outbound_session( | |
sending_handle: &str, | ||
receiving_handle: &str, | ||
message: &str, | ||
) -> Result<String, JsValue> { | ||
) -> Result<Box<[JsValue]>, JsValue> { | ||
// Look up both handles in INSTANCE_MAP | ||
let instances = INSTANCE_MAP.lock().unwrap(); | ||
let mut sending_instance = instances | ||
|
@@ -53,8 +53,13 @@ pub fn create_outbound_session( | |
.get(receiving_handle) | ||
.unwrap() | ||
.set(receiving_instance); | ||
|
||
match result { | ||
Ok((_, ciphertext_json)) => Ok(ciphertext_json), | ||
Ok((session_id, ciphertext_json)) => Ok(vec![ | ||
JsValue::from_str(&session_id), | ||
JsValue::from_str(&ciphertext_json), | ||
] | ||
Comment on lines
+52
to
+55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using the Vec here changes the memory structure. Given the size cannot be known at compile time. Imagine using a tuple would be easier to use and would avoid the need for boxing of the values. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately, |
||
.into_boxed_slice()), | ||
Err(e) => Err(JsValue::from_str(&e.to_string())), | ||
} | ||
} | ||
|
@@ -64,7 +69,7 @@ pub fn create_inbound_session( | |
sending_handle: &str, | ||
receiving_handle: &str, | ||
message: &str, | ||
) -> Result<String, JsValue> { | ||
) -> Result<Box<[JsValue]>, JsValue> { | ||
// Look up both handles in INSTANCE_MAP | ||
let instances = INSTANCE_MAP.lock().unwrap(); | ||
let sending_instance = instances | ||
|
@@ -90,7 +95,52 @@ pub fn create_inbound_session( | |
.set(receiving_instance); | ||
|
||
match result { | ||
Ok((_, plaintext)) => Ok(plaintext), | ||
Ok((session_id, ciphertext_json)) => Ok(vec![ | ||
JsValue::from_str(&session_id), | ||
JsValue::from_str(&ciphertext_json), | ||
] | ||
.into_boxed_slice()), | ||
Err(e) => Err(JsValue::from_str(&e.to_string())), | ||
} | ||
} | ||
|
||
#[wasm_bindgen] | ||
pub fn encrypt_message( | ||
sending_handle: &str, | ||
session_id: &str, | ||
message: &str, | ||
) -> Result<String, JsValue> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These types look both to be Strings, only the error gets converted to a wasmbind:JsValue. Shouldn't they be the same type? |
||
let instances = INSTANCE_MAP.lock().unwrap(); | ||
let mut instance = instances | ||
.get(sending_handle) | ||
.ok_or("sending_handle not found")? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this conversion from an Option to Result. You could have used a match, but this is much cleaner |
||
.take(); | ||
|
||
let result = instance.encrypt_message_serialized(session_id, message); | ||
// Do we actually need to do this? | ||
instances.get(sending_handle).unwrap().set(instance); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't have to do this here. Instead of using This pattern comes from my own lack of Rust borrow-checker knowledge when implementing the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In fact, now that we're using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The type checker doesn't seem to like that, but it recommends that I replace the cell with a Does have some scary language about concurrent usage leading to a panic, but I think that's fine for now given that all our code is synchronous. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've committed that change here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good, anything works for now as long as borrow/type checker likes it. We can take a holistic look at the map/cell strategy in a subsequent PR. Actually, on second look: RefCell seems much better than the take/set strategy. Nice!
neekolas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
match result { | ||
Ok(ciphertext_json) => Ok(ciphertext_json), | ||
Err(e) => Err(JsValue::from_str(&e.to_string())), | ||
} | ||
Comment on lines
+108
to
+111
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems fine to me, however there are more idomatic ways to use a Match statement.
|
||
} | ||
|
||
#[wasm_bindgen] | ||
pub fn decrypt_message( | ||
handle: &str, | ||
session_id: &str, | ||
ciphertext: &str, | ||
) -> Result<String, JsValue> { | ||
let instances = INSTANCE_MAP.lock().unwrap(); | ||
let mut instance = instances.get(handle).ok_or("handle not found")?.take(); | ||
|
||
let result = instance.decrypt_message_serialized(session_id, ciphertext); | ||
// Do we actually need to do this? | ||
neekolas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
instances.get(handle).unwrap().set(instance); | ||
|
||
match result { | ||
Ok(plaintext) => Ok(plaintext), | ||
Err(e) => Err(JsValue::from_str(&e.to_string())), | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,19 +12,48 @@ it("can instantiate", async () => { | |
|
||
it("can run self test", async () => { | ||
const xmtp = await XMTPWasm.initialize(); | ||
const xmtpv3 = await xmtp.getXMTPv3(); | ||
const res = await xmtpv3.selfTest(); | ||
const xmtpv3 = xmtp.getXMTPv3(); | ||
const res = xmtpv3.selfTest(); | ||
expect(res).toBe(true); | ||
}); | ||
|
||
it("can do a simple conversation", async () => { | ||
const wasm = await XMTPWasm.initialize(); | ||
const alice = await wasm.newVoodooInstance(); | ||
const bob = await wasm.newVoodooInstance(); | ||
const alice = wasm.newVoodooInstance(); | ||
const bob = wasm.newVoodooInstance(); | ||
|
||
const outboundJson = await alice.createOutboundSession(bob, "hello there"); | ||
const { sessionId, payload } = await alice.createOutboundSession( | ||
bob, | ||
"hello there" | ||
); | ||
expect(typeof sessionId).toBe("string"); | ||
// Unused, but test JSON parseable | ||
const _ = JSON.parse(outboundJson); | ||
const inboundPlaintext = await bob.createInboundSession(alice, outboundJson); | ||
expect(inboundPlaintext).toBe("hello there"); | ||
const _ = JSON.parse(payload); | ||
const { payload: inboundPayload } = await bob.createInboundSession( | ||
alice, | ||
payload | ||
); | ||
expect(inboundPayload).toBe("hello there"); | ||
}); | ||
|
||
it("can send a message", async () => { | ||
const wasm = await XMTPWasm.initialize(); | ||
const alice = wasm.newVoodooInstance(); | ||
const bob = wasm.newVoodooInstance(); | ||
|
||
const { sessionId, payload } = await alice.createOutboundSession( | ||
bob, | ||
"hello there" | ||
); | ||
await bob.createInboundSession(alice, payload); | ||
expect(typeof sessionId).toBe("string"); | ||
|
||
const msg = "hello there"; | ||
const encrypted = await alice.encryptMessage(sessionId, msg); | ||
expect(typeof encrypted).toBe("string"); | ||
|
||
// Alice can't decrypt her own message. Does work for Bob though | ||
// const decrypted = await alice.decryptMessage(sessionId, encrypted); | ||
const decrypted = await bob.decryptMessage(sessionId, encrypted); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. beautiful |
||
expect(decrypted).toBe(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.
I'm not sure I follow the need for the Box here.
My understanding was that JsValues through WASM_BINDGEN get copied between the memory space?
Heap Allocating here seems unnecessary at a high level.