842 use bufbuild type to storetransmit fvk instead of bech32 string #846

Valentine1898 · 2024-03-28T00:48:40Z

close #842

packages/types/src/wallet.ts

Valentine1898 · 2024-03-28T10:45:10Z

apps/extension/src/state/connected-sites.test.ts

@@ -19,6 +19,7 @@ describe('Connected Sites Slice', () => {

  beforeEach(() => {
    localStorage = mockLocalExtStorage();
+    //
    useStore = create<AllSlices>()(initializeStore(mockSessionExtStorage(), localStorage));


Here and in a number of other tests I get an error

TS2322: Type 'UseBoundStore<WithImmer<StoreApi<AllSlices>>>' is not assignable to type 'UseBoundStore<StoreApi<AllSlices>>'.

As I understand it, this is due to the addition of bufbuild types to WalletsSlice, but I'm not sure if we should rewrite the tests or if we shouldn't use bufbuild types in the slice

So this is a downside of zustand+immer 😞 . It's not allowed to store bufbuild classes directly. It seems to be a limitation because immer cannot properly do a safe copy of it. So we likely need to be using WalletJson here. But does this invalidate this whole task? Maybe not, we still should be storing FullViewingKey.toJson() in storage instead of a string.

apps/extension/src/routes/popup/settings/settings-full-viewing-key.tsx

apps/extension/src/state/wallets.ts

grod220 · 2024-03-28T11:11:37Z

apps/extension/src/state/connected-sites.test.ts

@@ -19,6 +19,7 @@ describe('Connected Sites Slice', () => {

  beforeEach(() => {
    localStorage = mockLocalExtStorage();
+    //
    useStore = create<AllSlices>()(initializeStore(mockSessionExtStorage(), localStorage));


So this is a downside of zustand+immer 😞 . It's not allowed to store bufbuild classes directly. It seems to be a limitation because immer cannot properly do a safe copy of it. So we likely need to be using WalletJson here. But does this invalidate this whole task? Maybe not, we still should be storing FullViewingKey.toJson() in storage instead of a string.

packages/bech32/src/full-viewing-key.ts

packages/storage/src/chrome/local.ts

grod220 · 2024-03-28T11:22:41Z

packages/wasm/crate/src/build.rs

 ///     witness_data: `WitnessData``
 /// Returns: `Action`
 #[wasm_bindgen]
 pub fn build_action(
    transaction_plan: JsValue,
    action_plan: JsValue,
-    full_viewing_key: &str,
+    full_viewing_key: JsValue,


Somewhat related to #844, but we can do something like this so we get types:

pub fn build_action( transaction_plan: JsValue, action_plan: JsValue, full_viewing_key: &[u8], witness_data: JsValue, ) -> WasmResult<JsValue> { let full_viewing_key: FullViewingKey = FullViewingKey::decode(full_viewing_key)?;

and then in typescript we pass fullViewingKey.inner

Why is it better? Do we want to get rid of toJson() serialization and serde_wasm_bindgen?
Should we do this in all cases when handling {inner: Uint8Array} types?

I'm working through these cases in #834. I go back and forth honestly. Perhaps it's not better than JsValue. We still pass an opaque type that needs deserializing on the other side. Maybe ignore this for now.

Ok, here's the idea with this:

wasm: https://github.com/penumbra-zone/web/pull/834/files#diff-8379276b019dd49da4c1b561332a55996d838efd25a8e02609f6ed40625e94d1R103-R108

typescript: https://github.com/penumbra-zone/web/pull/834/files#diff-be70ea133ec02a72fd2fc4cd0b3c2782d9ce8d074a426f5490e9abf78e49c29bR14-R17

JsValue can be anything. And it gets typed to an any. However, in some cases, we can encode/decode through binary. If we pass an Uint8Array straight to wasm and receive one back, the typescript signature will be strongly enforced. It increases typesafety. It won't work in every case, but for pb types, it often can.

Valentine1898 · 2024-03-28T13:50:53Z

The V1 -> V2 migration was successful, but now that we have V2 in local storage state we get an error

Cannot read properties of undefined (reading 'V1')

grod220 · 2024-03-28T13:56:25Z

Wherever that bug is, sounds like a good candidate for testing 😅

Valentine1898 · 2024-03-28T18:53:18Z

packages/wasm/crate/src/keys.rs

+    let fvk: FullViewingKey = FullViewingKey::decode(full_viewing_key)?;
+    let result = serde_wasm_bindgen::to_value(&fvk.wallet_id())?;
+    Ok(result)


@grod220
WalletId has no method .encode_to_vec()

Looks like WalletId is missing a trait in the core repo. I'll open a PR to fix there. In the meantime, let's do:

pub fn get_wallet_id(full_viewing_key: JsValue) -> WasmResult<Vec<u8>> { utils::set_panic_hook(); let fvk: FullViewingKey = serde_wasm_bindgen::from_value(full_viewing_key)?; // Can do `fvk.wallet_id().encode_to_vec()` when Domain impl added to WalletId in core let wallet_id_proto = WalletId::from(fvk.wallet_id()); Ok(wallet_id_proto.encode_to_vec()) }

Valentine1898 · 2024-04-01T19:00:35Z

packages/storage/src/chrome/local.ts

-export enum LocalStorageVersion {
-  V1 = 'V1',
-}
-
-export interface OriginRecord {
-  origin: string;
-  choice: UserChoice;
-  date: number;
-}
-
-export interface LocalStorageState {
-  wallets: WalletJson[];
-  grpcEndpoint: string;
-  passwordKeyPrint?: KeyPrintJson;
-  fullSyncHeight: number;
-  knownSites: OriginRecord[];
-}


I moved the types that were defined @penumbra-zone/storage/src/chrome/local to @penumbra-zone/types/src/local-storage.
This was to avoid a cyclic import which created a bug with the error Cannot read properties of undefined (reading 'V1') .
This also created a lot of import changes in this PR.

Valentine1898 · 2024-04-01T19:11:40Z

packages/storage/src/indexed-db/indexed-db.test.ts

+        walletId: new WalletId({
+          inner: Uint8Array.from({ length: 8 }, () => Math.floor(Math.random() * 256)),
+        }),


This test is actually incorrect, if we want to test idb version update, we should not generate a random dbName, only increase the version.
But the thing is, if we fix this test, it causes all storage tests to hang and timeout error. I will open a separate PR for this fix (since this PR is big without it)

Valentine1898 · 2024-04-01T19:17:15Z

packages/wasm/crate/src/keys.rs

-#[wasm_bindgen]
-pub fn get_short_address_by_index(full_viewing_key: &str, index: u32) -> WasmResult<JsValue> {
-    utils::set_panic_hook();
-
-    let fvk = FullViewingKey::from_str(full_viewing_key)?;
-    let (address, _dtk) = fvk.incoming().payment_address(index.into());
-    let short_address = address.display_short_form();
-    Ok(JsValue::from_str(&short_address))
-}


grod220

Really well done! This wasn't easy 😅 . Before you merge, please do some extensive testing with all user flows (migrating from v1/swapping/staking/send/etc) to ensure there aren't any bugs we missed.

Domain types implement the trait `DomainType` which adds proto encoding/decoding to the struct. This PR adds it for `WalletId`. This is helpful in the wasm crate when we pass over the wasm-javascript boundary: penumbra-zone/web#846 (comment)

Valentine1898 · 2024-04-02T13:16:00Z

I'll just point out
Migration works only in case of version increase (V1-->V2).
If you try to rollback extension with V2 version of local storage to V1 version you will just break everything.

I don't think users will have a case when a version is decreased, and developers should just consider that this can happen when switching between branches.

grod220 · 2024-04-02T13:23:04Z

If you try to rollback extension with V2 version of local storage to V1 version you will just break everything.

Ah, good point. After you merge, please make a note in #browser-extension in discord.

Valentine1898 linked an issue Mar 28, 2024 that may be closed by this pull request

Use bufbuild type to store/transmit FVK instead of bech32 string #842

Closed

Valentine1898 commented Mar 28, 2024

View reviewed changes

grod220 reviewed Mar 28, 2024

View reviewed changes

grod220 mentioned this pull request Mar 28, 2024

replaceable use of JsValue in wasm crate #845

Closed

9 tasks

Valentine1898 commented Mar 28, 2024

View reviewed changes

Valentine1898 force-pushed the 842-use-bufbuild-type-to-storetransmit-fvk-instead-of-bech32-string branch from 828bb2f to 108d4dc Compare March 29, 2024 11:08

Valentine1898 added 14 commits April 1, 2024 21:26

try migration & refactor wasm

8b41bc6

refactor fvk using

b99eef3

bump local storage version

1c2ca41

fix

dfda466

bypass zustand+immer limitation

d4af46d

fix tests

a0cd32b

move migration to separate file

0312e89

avoid circular imports

9d0bf1c

add test for v1-migration

b797ced

lint

1f60350

improve wasm signatures

34aa946

fix tests

9d89970

fmt

769a2ef

fix rust lint and idb test

888283b

Valentine1898 force-pushed the 842-use-bufbuild-type-to-storetransmit-fvk-instead-of-bech32-string branch from ed06732 to 888283b Compare April 1, 2024 18:26

fix idb tests

d3d40ef

Valentine1898 marked this pull request as ready for review April 1, 2024 18:53

Valentine1898 commented Apr 1, 2024

View reviewed changes

Valentine1898 requested review from grod220, jessepinho, TalDerei and turbocrime April 1, 2024 19:12

Valentine1898 changed the title ~~WIP: 842 use bufbuild type to storetransmit fvk instead of bech32 string~~ 842 use bufbuild type to storetransmit fvk instead of bech32 string Apr 1, 2024

remove empty comments

93ecdcd

Valentine1898 commented Apr 1, 2024

View reviewed changes

move back types in storage package

b70d7b0

grod220 mentioned this pull request Apr 2, 2024

Add WalletId domain impl penumbra-zone/penumbra#4146

Merged

temp fix encode_to_vec for get_wallet_id

f137396

grod220 approved these changes Apr 2, 2024

View reviewed changes

fix wasm tests

c380115

fix build tx

c6bdb40

Valentine1898 merged commit 4530faf into main Apr 2, 2024
6 checks passed

Valentine1898 deleted the 842-use-bufbuild-type-to-storetransmit-fvk-instead-of-bech32-string branch April 2, 2024 14:17

turbocrime mentioned this pull request Apr 4, 2024

bech32 package does not validate properly #778

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

842 use bufbuild type to storetransmit fvk instead of bech32 string #846

842 use bufbuild type to storetransmit fvk instead of bech32 string #846

Valentine1898 commented Mar 28, 2024

Valentine1898 Mar 28, 2024 •

edited

Loading

grod220 Mar 28, 2024

grod220 Mar 28, 2024

grod220 Mar 28, 2024

Valentine1898 Mar 28, 2024

grod220 Mar 28, 2024

grod220 Mar 28, 2024

Valentine1898 commented Mar 28, 2024

grod220 commented Mar 28, 2024

Valentine1898 Mar 28, 2024

grod220 Apr 2, 2024

Valentine1898 Apr 1, 2024

Valentine1898 Apr 1, 2024

Valentine1898 Apr 1, 2024

grod220 left a comment

Valentine1898 commented Apr 2, 2024

grod220 commented Apr 2, 2024

842 use bufbuild type to storetransmit fvk instead of bech32 string #846

842 use bufbuild type to storetransmit fvk instead of bech32 string #846

Conversation

Valentine1898 commented Mar 28, 2024

Valentine1898 Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Valentine1898 commented Mar 28, 2024

grod220 commented Mar 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grod220 left a comment

Choose a reason for hiding this comment

Valentine1898 commented Apr 2, 2024

grod220 commented Apr 2, 2024

Valentine1898 Mar 28, 2024 •

edited

Loading