feat: ADR-006 implementation #1652

VanishMax · 2024-08-06T18:41:42Z

Implements ADR-006. Based on previously-closed #1567 and uses newer features from #1648

This PR has its own, newer version of the ADR, that completely matches the implementation. Adds reconnect function and uses client.service to create PromiseClient.

To check the implementation, do this:

Check out the branch of this PR
Use meet adr 006 prax-wallet/web#134 PR in Prax while using this PR's packages
Use feat: use the latest client package nextjs-penumbra-client-example#5 PR in NextJS example to see how this ADR affects real apps

changeset-bot · 2024-08-06T18:41:46Z

🦋 Changeset detected

Latest commit: c681365

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@penumbra-zone/client	Major
minifront	Patch

Not sure what this means? Click here to learn what changesets are.

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

turbocrime

i think there were some steps backwards here. review is based on reading the changeset

turbocrime · 2024-08-06T20:31:48Z

docs/adrs/006-web-apis.md

+  /**
+   * Tries to reconnect to the injected provider (the first one with `connected` state)
+   * without asking for user approval
+   */
+  readonly reconnect: (providerUrl?: string) => Promise<string>;


i don't think this is appropriate.

any provider may identify itself as connected

multiple providers may identify themself as connected

a user likely wishes to use a specific provider

implementing any automatic selection behavior in this client api is likely to guide developers to create a negative user experience for a user that wishes to select a provider. so the api should avoid any automatic selection behavior.

requiring the developer to deliberately select a frontend will force developers to anticipate this ambiguity. at that point they can either

prompt users to select a specific provider

implement some technique for their dapp to remember

encode their own priority

currently, minifront will only connect to prax. but by implementing this reconnect api and then using it in minifront, minifront may connect to any provider that happens to appear in the record, and it's not clear if the selection is even deterministic.

you've removed the PenumbraClient.providers method, but with that method a developer could easily implement a similar behavior by filtering the returned record by testing isConnected on each provider.

edit: i see there is a similar standalone method for getting all providers

Fair enough. Somehow I thought checking for manifest validity would be enough but I see know it isn't. Especially at the 'multiple providers may identify themself as connected' point.

Probably will delete it for now

turbocrime · 2024-08-06T20:38:33Z

docs/adrs/006-web-apis.md

+export type PenumbraManifest = Partial<chrome.runtime.ManifestV3> &
+  Required<Pick<chrome.runtime.ManifestV3, 'name' | 'version' | 'description' | 'icons'>>;
+
+export type getInjectedProvider = (penumbraOrigin: string) => Promise<PenumbraProvider>;
+
+export type getAllInjectedProviders = () => string[];
+
+export type getPenumbraManifest = (
+  penumbraOrigin: string,
+  signal?: AbortSignal,
+) => Promise<PenumbraManifest>;
+
+export type getAllPenumbraManifests = () => Record<
+  keyof (typeof window)[typeof PenumbraSymbol],
+  Promise<PenumbraManifest>
+>;


It seemed to be required by previous discussion that a PenumbraClient object should collect methods a developer would be interested in for selecting and then initiating a provider connection

is there a reason these are broken out again?

moved useful utility methods into the client with get prefixes

turbocrime · 2024-08-06T20:40:34Z

docs/adrs/006-web-apis.md

+export type getInjectedProvider = (penumbraOrigin: string) => Promise<PenumbraProvider>;
+
+export type getAllInjectedProviders = () => string[];


i'd like to avoid the term 'injected' because in the future the api may not actually require injection, given possible externally_connectable features

at that point to avoid misleading terminology, either the api would require a breaking change to rename, or another set of methods identifying a distinct set of providers that don't actually require a practically distinct api

by avoiding the term 'injected' and referring solely to 'providers' then there's no potential terminology problem in the future.

removed these functions in favor of the get.ts from #1648

packages/client/src/client.ts

turbocrime · 2024-08-06T20:45:39Z

packages/client/src/client.ts

+  private assertPort() {
+    if (!this.port) {
+      throw new PenumbraProviderNotConnectedError(this.origin);
+    }
+    return this.port;
+  }
+
+  private assertProvider() {
+    if (!this.provider) {
+      throw new PenumbraProviderNotConnectedError(this.origin);
+    }
+    return this.provider;
+  }


given discussion requesting that the client avoid throwing, these methods were removed in #1648

notably, a PenumbraClient instance is useless without a selected provider. #1648 moved the origin parameter into the constructor - is there a good reason to revert that?

good point, removed these assertion methods in favor of non-throwing.

as for the question – i will continue pursuing the client without the necessary origin in the constructor. It is not useless – it allows the creation of client in the setup of the users' app and the future reuse, a good practice popularized by many libraries (listed in the ADR as the source of inspiration). This approach is proven to be effective in terms of DX

turbocrime · 2024-08-06T20:50:41Z

docs/adrs/006-web-apis.md

+  /**
+   * Connects to the injected provider, asks for user approval if wasn't connected before.
+   * If `providerUrl` argument is not provided, tries to connect to the first injected provider.
+   * Returns the manifest URL of the connected provider or throws an error otherwise.
+   */
+  readonly connect: (providerUrl?: string) => Promise<string>;


returning manifest url from the connect method seems like the least useful choice.

it seems more better to return the MessagePort or a created Transport, allowing the caller to handle the Transport directly if they wish, as designed in #1648

if the manifest is really desired, just go ahead and provide a parsed manifest - the connection init process must fetch it anyway to confirm it exists.

That's true – the provider origin URL is not that useful. The same can be said about MessagePort or Transport – they are of no value for the library users. It might be better to return Promise<void> for now until we find out the really useful return, to not produce breaking changes in the future

changed to return Promise<void>

turbocrime · 2024-08-06T20:54:57Z

packages/client/src/client.ts

+  public onConnectionChange(callback: (detail: PenumbraStateEventDetail) => void) {
+    this.callbacks.add(callback);
+  }


it seems like there's now no way to remove a callback

copied the AbortController usage here

turbocrime · 2024-08-06T21:03:49Z

packages/client/src/client.ts

+  public async reconnect(requireOrigin?: string) {
+    const providers = assertPenumbra();
+
+    let provider: PenumbraProvider | undefined;
+    let origin = requireOrigin;
+    if (requireOrigin) {
+      provider = assertProviderRecord(requireOrigin);
+    } else {
+      const connectedEntry = Object.entries(providers).find(([, provider]) =>
+        provider.isConnected(),
+      );
+      origin = connectedEntry?.[0];
+      provider = connectedEntry?.[1];
+    }
+
+    if (!origin || !provider?.isConnected()) {
+      throw new PenumbraProviderNotConnectedError(origin);
+    }
+
+    await assertProviderManifest(origin);
+
+    this.origin = origin;
+    this.provider = provider;
+    this.port = await provider.connect();
+    this.callbacks.forEach(cb => cb(new PenumbraStateEvent(origin, provider.state(), true).detail));
+
+    return origin;
+  }


given multiple connected providers, which provider is preferred by this method?

removed the reconnect

turbocrime · 2024-08-06T21:58:15Z

if some of these changes were intended to address state management concerns discussed in DM, i don't think they achieve that objective

…tation # Conflicts: # packages/react/CHANGELOG.md # packages/react/package.json

VanishMax · 2024-08-08T16:44:27Z

closed in favor of #1648

VanishMax added 6 commits August 6, 2024 19:47

feat(client): implement ADR-006 with reconnect function

4bd5a6c

fix: remove react package for now

272079e

chore: format the code, remove logs

597cc1e

feat: add ADR-006 to this PR

4e7bf55

feat: use client.service instead of createServiceClient

0471160

chore: changeset

a8f25d6

VanishMax requested review from grod220 and turbocrime August 6, 2024 18:41

VanishMax self-assigned this Aug 6, 2024

turbocrime requested changes Aug 6, 2024

View reviewed changes

turbocrime reviewed Aug 6, 2024

View reviewed changes

VanishMax added 3 commits August 7, 2024 12:01

feat(client): update the client package implementation

7bf19b2

feat: update the ADR to match client package

0d272a6

fix: format

e95a601

VanishMax requested a review from turbocrime August 7, 2024 10:22

Merge remote-tracking branch 'origin/main' into feat/adr-006-implemen…

c681365

…tation # Conflicts: # packages/react/CHANGELOG.md # packages/react/package.json

VanishMax closed this Aug 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ADR-006 implementation #1652

feat: ADR-006 implementation #1652

VanishMax commented Aug 6, 2024 •

edited

Loading

changeset-bot bot commented Aug 6, 2024 •

edited

Loading

turbocrime left a comment

turbocrime Aug 6, 2024 •

edited

Loading

VanishMax Aug 6, 2024

turbocrime Aug 6, 2024

VanishMax Aug 7, 2024

turbocrime Aug 6, 2024

VanishMax Aug 7, 2024

turbocrime Aug 6, 2024

VanishMax Aug 7, 2024

turbocrime Aug 6, 2024

VanishMax Aug 7, 2024

VanishMax Aug 7, 2024

turbocrime Aug 6, 2024

VanishMax Aug 7, 2024

turbocrime Aug 6, 2024

VanishMax Aug 7, 2024

turbocrime commented Aug 6, 2024

VanishMax commented Aug 8, 2024

		export type getInjectedProvider = (penumbraOrigin: string) => Promise<PenumbraProvider>;

		export type getAllInjectedProviders = () => string[];

feat: ADR-006 implementation #1652

feat: ADR-006 implementation #1652

Conversation

VanishMax commented Aug 6, 2024 • edited Loading

changeset-bot bot commented Aug 6, 2024 • edited Loading

🦋 Changeset detected

turbocrime left a comment

Choose a reason for hiding this comment

turbocrime Aug 6, 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

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

Choose a reason for hiding this comment

turbocrime commented Aug 6, 2024

VanishMax commented Aug 8, 2024

VanishMax commented Aug 6, 2024 •

edited

Loading

changeset-bot bot commented Aug 6, 2024 •

edited

Loading

turbocrime Aug 6, 2024 •

edited

Loading