1231 onboarding numeraire selection2 #28

Valentine1898 · 2024-06-13T11:28:25Z

close penumbra-zone/web#1231
close penumbra-zone/web#1063

the user may not select any numeraire, it won't break anything
all old prices are deleted, no matter how numeraires changed (this can be improved later)

changeset-bot · 2024-06-13T11:28:28Z

⚠️ No Changeset found

Latest commit: d2f7230

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Valentine1898 · 2024-06-13T11:55:00Z

apps/extension/src/shared/components/numeraires-form.tsx

+  const { chainId } = useChainIdQuery();
+  const { selectedNumeraires, selectNumeraire, saveNumeraires, networkChainId } =
+    useStore(useNumerairesSelector);
+  const numeraires = useMemo(() => getNumeraireFromRegistry(chainId ?? networkChainId), [chainId]);


For some reason we can't get chainId for onboarding using useChainIdQuery (even though the services are already started).

Hm, could you add a comment explaining this, for future devs' sake?

Valentine1898 · 2024-06-13T11:58:26Z

apps/extension/src/state/persist.ts

+  if (changes['params']) {
+    const stored = changes['params'].newValue as
+      | StorageItem<LocalStorageState['params']>
+      | undefined;
+    set(
+      produce((state: AllSlices) => {
+        state.network.chainId = stored?.value
+          ? AppParameters.fromJsonString(stored.value).chainId
+          : state.network.chainId;
+      }),
+    );
+  }


Just to point out that we never actually save 'params', seems like that should be a separate issue

Wait, where is params coming from? Unless I'm missing something, I don't think changes will ever have a params key 🤔

Also, we don't store the chain ID in local storage. We store the grpcEndpoint, from which we fetch the chain ID on app startup. So I think this if (changes['params']) block should be deleted.

Ah, I see now that the chain ID is fetched from local storage in startWalletServices. But to your point, it seems to never be saved to local storage. So yeah, maybe we should just remove all references to it in local storage? 🤔

I assume the params were added to local storage as part of a PR to solve the problem of running service work without a network (probably #998)
I accept that it was intended that the params should be stored, but it was just missed by mistake

@turbocrime perhaps you can give more context regarding params

I've created a separate issue for that #32

Valentine1898 · 2024-06-13T11:59:27Z

apps/extension/src/storage/types.ts

@@ -22,5 +23,6 @@ export interface LocalStorageState {
  passwordKeyPrint: KeyPrintJson | undefined;
  fullSyncHeight: number | undefined;
  knownSites: OriginRecord[];
-  params: Jsonified<AppParameters> | undefined;
+  params: Stringified<AppParameters> | undefined;


We can't seem to save Jsonified to local storage

Valentine1898 · 2024-06-13T12:00:50Z

apps/extension/src/wallet-services.ts

+      case ServicesMessage.ChangeNumeraires:
+        void (async () => {
+          const newNumeraires = await localExtStorage.get('numeraires');
+          blockProcessor.setNumeraires(newNumeraires.map(n => AssetId.fromJsonString(n)));
+          await indexedDb.clearSwapBasedPrices();
+        })().then(() => respond());
+        return true;


I figured we might not have to stop sync to change the numeraires

Valentine1898 · 2024-06-13T12:01:18Z

packages/query/src/block-processor.ts

-  numeraires: Metadata[];
+  numeraires: AssetId[];


We didn't actually need Metadata here

Valentine1898 · 2024-06-13T12:04:08Z

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

+  async clearSwapBasedPrices(): Promise<void> {
+    const tx = this.db.transaction('PRICES', 'readwrite');
+    const store = tx.objectStore('PRICES');
+
+    let cursor = await store.openCursor();
+    while (cursor) {
+      const price = EstimatedPrice.fromJson(cursor.value);
+      if (!price.numeraire?.equals(this.stakingTokenAssetId)) {
+        await cursor.delete();
+      }
+      cursor = await cursor.continue();
+    }
+    await tx.done;
+  }


It would be much easier to delete all price records, but we should actually keep the prices for delegation assets because they don't depend on the numeraires setting

jessepinho

Great work. I have a few smaller comments, but the biggest issue is whether there's a way to repopulate the price data after wiping it out because the numeraires changed. If that's already addressed and I'm missing something, feel free to merge after addressing my comments. Otherwise, I think that needs addressing before merge.

apps/extension/src/routes/page/onboarding/set-numeraire.tsx

apps/extension/src/routes/popup/settings/settings.tsx

apps/extension/src/shared/components/numeraires-form.tsx

jessepinho · 2024-06-14T17:27:51Z

apps/extension/src/shared/components/numeraires-form.tsx

+  const { chainId } = useChainIdQuery();
+  const { selectedNumeraires, selectNumeraire, saveNumeraires, networkChainId } =
+    useStore(useNumerairesSelector);
+  const numeraires = useMemo(() => getNumeraireFromRegistry(chainId ?? networkChainId), [chainId]);


Hm, could you add a comment explaining this, for future devs' sake?

apps/extension/src/shared/components/numeraires-form.tsx

jessepinho · 2024-06-14T17:35:13Z

apps/extension/src/state/persist.ts

+  if (changes['params']) {
+    const stored = changes['params'].newValue as
+      | StorageItem<LocalStorageState['params']>
+      | undefined;
+    set(
+      produce((state: AllSlices) => {
+        state.network.chainId = stored?.value
+          ? AppParameters.fromJsonString(stored.value).chainId
+          : state.network.chainId;
+      }),
+    );
+  }


Wait, where is params coming from? Unless I'm missing something, I don't think changes will ever have a params key 🤔

Also, we don't store the chain ID in local storage. We store the grpcEndpoint, from which we fetch the chain ID on app startup. So I think this if (changes['params']) block should be deleted.

jessepinho · 2024-06-14T17:39:16Z

apps/extension/src/state/persist.ts

+  if (changes['params']) {
+    const stored = changes['params'].newValue as
+      | StorageItem<LocalStorageState['params']>
+      | undefined;
+    set(
+      produce((state: AllSlices) => {
+        state.network.chainId = stored?.value
+          ? AppParameters.fromJsonString(stored.value).chainId
+          : state.network.chainId;
+      }),
+    );
+  }


Ah, I see now that the chain ID is fetched from local storage in startWalletServices. But to your point, it seems to never be saved to local storage. So yeah, maybe we should just remove all references to it in local storage? 🤔

jessepinho · 2024-06-14T17:54:30Z

packages/query/src/block-processor.ts

+  setNumeraires(numeraires: AssetId[]): void {
+    this.numeraires = numeraires;
+  }


Something I'm a little unclear on: when wallet services calls this method, it will wipe out existing prices. But how do those prices get repopulated? Does the user have to wait for the next swap to appear on chain to trigger re-saving of prices? Or is there a way to ensure the prices get updated as soon as the numeraire gets updated? (Sorry if I'm missing something that's already addressed in your code.)

I guess if swaps are frequent, it would be a minor inconvenience to the user that they wouldn't see equivalent prices for a while after changing numeraires.

Other solutions are to:

scan through the whole list of numeraires regardless of the user's choice (then the user's choice will only affect the service layer that returns prices) - but this is an additional cost in scanning and storage.

after changing numeraires, scan some number of blocks into the past - theoretically, we can add this improvement in the future

Hm, yeah — if you and Henry already discussed it and he's fine with it, then it's fine. This is probably more of a product decision than an engineering one.

Valentine1898 · 2024-06-14T18:13:22Z

but the biggest issue is whether there's a way to repopulate the price data after wiping it out because the numeraires changed

@jessepinho
That's a good point, I'll add some comments to the code explaining this.

It's a bit of a controversial decision as far as I'm concerned, but the user will just have to wait a bit for the prices to populate "naturally" by scanning for new blocks and BSODs in them.

Also the comment where Henry and I discussed this

penumbra-zone/web#1063 (comment)

Valentine1898 · 2024-06-14T18:18:31Z

Since this PR contains both packages and extension changes, I will also make a duplicate PR in the penumbra-zone/web repo that will only contain the packages changes

Co-authored-by: Jesse Pinho <jesse@jessepinho.com>

Valentine1898 added 10 commits June 12, 2024 01:36

use numeraire from local storage

710a1cf

save multiple numeraires in local storage

25bac21

added NumerairesSlice

339e636

add ChangeNumeraires service message

10be038

numeraires persist

9701986

add icons

c427ddf

remove onboardNumeraires

88be240

delete prices when numeraire is changed

992394d

refactor & fix numeraires update bug

bcd6c0b

add idb tests

b60e273

fix chain-id

109c34d

Valentine1898 commented Jun 13, 2024

View reviewed changes

Valentine1898 requested a review from grod220 June 13, 2024 12:07

grod220 requested review from a team and removed request for grod220 June 13, 2024 15:23

jessepinho approved these changes Jun 14, 2024

View reviewed changes

Valentine1898 and others added 8 commits June 14, 2024 21:30

Update apps/extension/src/shared/components/numeraires-form.tsx

c92c9e0

Co-authored-by: Jesse Pinho <jesse@jessepinho.com>

Update apps/extension/src/shared/components/numeraires-form.tsx

b83c2aa

Co-authored-by: Jesse Pinho <jesse@jessepinho.com>

Update apps/extension/src/routes/popup/settings/settings.tsx

ad64679

Co-authored-by: Jesse Pinho <jesse@jessepinho.com>

Update apps/extension/src/routes/page/onboarding/set-numeraire.tsx

c17991e

Co-authored-by: Jesse Pinho <jesse@jessepinho.com>

Update apps/extension/src/routes/page/onboarding/set-numeraire.tsx

40306ac

Co-authored-by: Jesse Pinho <jesse@jessepinho.com>

fix lint

fabf6ae

add docs for clearSwapBasedPrices

b1af2bf

add chainId comment

d2f7230

This was referenced Jun 14, 2024

[duplicate] 1231 onboarding numeraire selection2 penumbra-zone/web#1282

Merged

'params' are never stored in local storage #32

Closed

Valentine1898 self-assigned this Jun 14, 2024

Valentine1898 merged commit 27d8552 into main Jun 14, 2024
6 checks passed

Valentine1898 deleted the 1231-onboarding-numeraire-selection2 branch June 14, 2024 20:22

grod220 added a commit that referenced this pull request Dec 3, 2024

Sync block clients (#28)

675b574

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1231 onboarding numeraire selection2 #28

1231 onboarding numeraire selection2 #28

Valentine1898 commented Jun 13, 2024 •

edited

Loading

changeset-bot bot commented Jun 13, 2024 •

edited

Loading

Valentine1898 Jun 13, 2024

jessepinho Jun 14, 2024

Valentine1898 Jun 13, 2024

jessepinho Jun 14, 2024

jessepinho Jun 14, 2024

Valentine1898 Jun 14, 2024 •

edited

Loading

Valentine1898 Jun 14, 2024

Valentine1898 Jun 14, 2024

Valentine1898 Jun 13, 2024

Valentine1898 Jun 13, 2024

Valentine1898 Jun 13, 2024

Valentine1898 Jun 13, 2024

jessepinho left a comment

jessepinho Jun 14, 2024

jessepinho Jun 14, 2024

jessepinho Jun 14, 2024

jessepinho Jun 14, 2024

Valentine1898 Jun 14, 2024

jessepinho Jun 14, 2024

Valentine1898 commented Jun 14, 2024

Valentine1898 commented Jun 14, 2024

1231 onboarding numeraire selection2 #28

1231 onboarding numeraire selection2 #28

Conversation

Valentine1898 commented Jun 13, 2024 • edited Loading

changeset-bot bot commented Jun 13, 2024 • edited Loading

⚠️ No Changeset found

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 Jun 14, 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

jessepinho left a 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

Choose a reason for hiding this comment

Valentine1898 commented Jun 14, 2024

Valentine1898 commented Jun 14, 2024

Valentine1898 commented Jun 13, 2024 •

edited

Loading

changeset-bot bot commented Jun 13, 2024 •

edited

Loading

Valentine1898 Jun 14, 2024 •

edited

Loading