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

AES256GCM decrypt: replace Observable with Promise #778

Merged
merged 11 commits into from
Jan 10, 2023
Merged

AES256GCM decrypt: replace Observable with Promise #778

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c961275
AES256GCM decrypt: replace Observable with Promise
bangtoven Jan 9, 2023
6107ab1
WalletSDKRelay
bangtoven Jan 9, 2023
acebeca
fix linter
bangtoven Jan 9, 2023
349616e
prettier
bangtoven Jan 9, 2023
d242915
hmmmmm
bangtoven Jan 9, 2023
bdf1b16
rejects.toThrowError
bangtoven Jan 9, 2023
4190870
rejects.toThrow
bangtoven Jan 9, 2023
0d0ceae
update comment
bangtoven Jan 9, 2023
c41d68c
return promise
bangtoven Jan 9, 2023
23ed8a5
use async / await instead
bangtoven Jan 10, 2023
72ee522
prettier
bangtoven Jan 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions packages/wallet-sdk/src/relay/WalletSDKRelay.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

import bind from "bind-decorator";
import { ethErrors } from "eth-rpc-errors";
import { BehaviorSubject, Observable, of, Subscription, zip } from "rxjs";
import { BehaviorSubject, from, Observable, of, Subscription, zip } from "rxjs";
import {
catchError,
distinctUntilChanged,
Expand Down Expand Up @@ -777,8 +777,7 @@ export class WalletSDKRelay extends WalletSDKRelayAbstract {
private handleIncomingEvent(event: ServerMessageEvent): void {
try {
this.subscriptions.add(
aes256gcm
.decrypt(event.data, this.session.secret)
from(aes256gcm.decrypt(event.data, this.session.secret))
.pipe(map(c => JSON.parse(c)))
.subscribe({
next: json => {
Expand Down
27 changes: 10 additions & 17 deletions packages/wallet-sdk/src/relay/aes256gcm.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,12 @@ describe("aes256gcm", () => {
expect(encrypted.length).toEqual(90);

// decrypted output matches original input
decrypt(encrypted, randSecret).subscribe({
next: decrypted => {
decrypt(encrypted, randSecret).then(
decrypted => {
expect(decrypted).toBe("plain text string");
},
});
() => {},
);
});

test("decrypt", () => {
Expand All @@ -31,11 +32,12 @@ describe("aes256gcm", () => {
},
};

decrypt(cipherText, secret).subscribe({
next: value => {
expect(sampleDataResult).toEqual(value);
decrypt(cipherText, secret).then(
value => {
expect(sampleDataResult).toEqual(JSON.parse(value));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wonder how this has been working
decrypt returns Promise (or used to be Observable) of string, so value (string) and sampleDataResult (object) couldn't be equal, right?
image

Copy link
Contributor

Choose a reason for hiding this comment

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

did the observable resolve to the string or was it an object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was string as well. i haven't changed the internal data type.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

I just logged out what the old test returns when run and it seems like it actually returns an object. Incorrect typing on the decrypt function?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vishnumad i am getting a string with old code
image
image

Copy link
Contributor

Choose a reason for hiding this comment

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

@bangtoven Yup, you're correct it is a string. This test is just broken and will pass no matter what you do in the subscribe function because it's an observable. You can test this by replacing the current expect with expect(true).toEqual(false);. We need to actually block until the observable emits the value.

 test("decrypt", done => {
    const cipherText =
      "06593325a922a928913b5c6ea26f848c4545bcea4e26c4f5ee745316ff22b2780aeccc565730514b2820a94b03f5f89fe7542a35bbdd87a1d52a4352f49482781113db09266c668696778e0a94bc9f866f1e92e7262fd0bb811838284cc64cbc4552b33e9c6fb2582cea4f49471d6d46a16a5c8ac83ee8483ed4dc01f1fde3bfd7a2f173715e0a8d09dd4907483f096a845bff698831ea277c1ca4223d3f6073174cb35119d0a795c1a9cb4f32ee1dcc254d8931";
    const sampleDataResult = {
      type: "WEB3_RESPONSE",
      id: "791fe0ec3dc3de49",
      response: {
        method: "requestEthereumAccounts",
        result: ["0xdf0635793e91d4f8e7426dbd9ed08471186f428d"],
      },
    };

    decrypt(cipherText, secret).subscribe({
      next: value => {
        expect(sampleDataResult).toEqual(value);
        done();
      },
    });
  });

Copy link
Contributor

Choose a reason for hiding this comment

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

@bangtoven You'll run into a similar issue with the promises in the current test. The test won't actually get marked as failed currently since you're not awaiting the result of the promise.
image

test("decrypt", async () => {
    // ...
    const value = await decrypt(cipherText, secret);
    expect(value).toEqual(sampleDataResult);
  });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vishnumad hmm. actually i was able to find type error since the test failed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bangtoven It does print the error to console, but doesn't mark the test as failed. See the orange RUNS next to the last line in the screenshot above vs red FAIL below when using await
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, gotcha. i just made change to the test code to return the promise.then block.

},
});
() => {},
);
});

test("errors", async () => {
Expand All @@ -47,15 +49,6 @@ describe("aes256gcm", () => {
"secret must be 256 bits",
);

await expect(
decrypt("text", secret).subscribe(
() => {
fail("expected error");
},
error => {
expect(error).toBe("expected error");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure how this worked before as well.
error's message is not equal to expected error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @erin-at-work

},
),
);
await expect(decrypt("text", secret)).rejects.toThrow();
});
});
14 changes: 4 additions & 10 deletions packages/wallet-sdk/src/relay/aes256gcm.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
// Copyright (c) 2018-2022 Coinbase, Inc. <https://www.coinbase.com/>
// Licensed under the Apache License, version 2.0

import { Observable } from "rxjs";

import { hexStringToUint8Array, uint8ArrayToHex } from "../util";

/**
Expand Down Expand Up @@ -66,12 +64,9 @@ export async function encrypt(
*
* TODO: Update rxjs for promises
*/
export function decrypt(
cipherText: string,
secret: string,
): Observable<string> {
export function decrypt(cipherText: string, secret: string): Promise<string> {
if (secret.length !== 64) throw Error(`secret must be 256 bits`);
return new Observable<string>(subscriber => {
return new Promise<string>((resolve, reject) => {
void (async function () {
const secretKey: CryptoKey = await crypto.subtle.importKey(
"raw",
Expand Down Expand Up @@ -101,10 +96,9 @@ export function decrypt(
concattedBytes,
);
const decoder = new TextDecoder();
subscriber.next(decoder.decode(decrypted));
subscriber.complete();
resolve(decoder.decode(decrypted));
} catch (err) {
subscriber.error(err);
reject(err);
}
})();
});
Expand Down