feat: add ADR-006 for web APIs and general description #1471
Conversation
|
|
looked over this, will post some detailed comments today |
|
okay, this reply is a little excessive and kind of disorganized, but i'm trying to be this begins with historical reasons and choices just to cover the existing at the end i'll have a list of proposed actions, later i might suggest some we definitely need clear documentation and direction so thank you for starting please read all of this as concept+notes+commentary+discussion in the interest the existing designimplementations announce by injectiondue to ui behavior specifications, manifest v3 and chrome limitations, and these scripts are injected not just to pages that the user approves, but to proto spec firstusing protofbuf to specify communication between dapp and extension was a use of the same protocol for both pd rpc and browser rpc ensures that browser support use of generated toolsso a dapp independenlty imports packages and service types to construct useful the client package exports very simple utilities that can validate providers as reactreact is eager to destroy and recreate state. sync rendering requirements make so an additional react-focused wrapper was requested. the react wrapper focuses internally it uses in the future it could export query functions for common activities that too many words about 'wallet'the term 'wallet' may be familiar to developers with experience with other The term 'wallet' is overloaded with several meanings. There are more than this,
penumbra walletsPenumbra includes the concepts of many accounts per wallet-keys, and many Outside the proto spec, penumbra wallet-apps may also contain multiple So penumbra wallet-apps will probably just expose one wallet at a time. It's So the term wallet may be 'familiar' but the familiarity might be more 'Prax wallet' does use the term, but critically, Prax only provides custody and the existing penumbra api is more of a chain apiClients to penumbra services are more literally clients to the chain, than the api is the same specified api used to query pd, the penumbra daemon many of these services are entirely public, and don't actually involve loosely, the 'view service' and 'custody service' together may be considered the penumbra api termsDapp developers won't be able to escape penumbra-specific concepts. The concepts In terms of api i think it's important to clearly speak in terms of
Reserve the term 'wallet' to refer to
avoid applying 'wallet' to any concept not directly involving wallet-keys.
action itemswhat the page api should not addthese are not action items: for privacy and security reasons, most
in general: if it's not in the protobuf spec, there shouldn't be a page api for it. what the page api could addthese suggestions need discussion and review before action.
This is probably it. These suggestions are candidates specifically because they Pretty much any other feature, involving an active connection to the provider, what the proto spec could add
errors
connectrpc tooling can be improvedwith some effort applied to a simple wrapper to maybe move interface definitions to a dedicated packageInterface definitions could be moved to a dedicated package, leaving focus on integrationsThe primary objective right now is not dapp development but integration, To this end, good demos, and a well-featured react package with helpful hooks The react package could begin adding modules that export queriers. Some of the |
|
@turbocrime I highly appreciate the context and the ideas you shared. Special thanks for explaining different wallet-related terms – I never thought about Prax as just wallet-api implementation. Interestingly, not so many things actually contradict with the ADR. Some needs to be rewritten, some apis updated and added. Strongly agree on integrations focus: making examples, writing docs and collecting feedback is extremely important too. It could open our eyes even wider to see the needs of the community. Agree that wallet-app settings should not be exposed to the API, at least not in the injected connection and not in this ADR. If wallet developers want it, they could create their own logic exposing this feature. Going to remove this section from the proposal. While I'm rewriting the ADR, one more question: can we actually use 'public services' publicly? I mean, right now we require users to have an injected connection, and it's not public to me. It might not require the wallet keys but the service users are querying is kinda auth-protected by the connection. So maybe we shouldn't expose the 'public services' term to users? |
|
Visit the preview URL for this PR (updated for commit d414238): https://penumbra-ui-preview--pr1471-feat-adr-006-client-llv69aur.web.app (expires Wed, 24 Jul 2024 15:47:16 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 709d729610ef7a6369b23f1cb2b820a60cc685b1 |
|
@turbocrime I updated the ADR considering your feedback. Would appreciate if you elaborate on the "what the page api could add" section of your comment with examples and maybe addition to this ADR |
docs/adrs/006-web-apis.md
Outdated
| /** Should synchronously return the present connection state. | ||
| * - `true` indicates active connection. | ||
| * - `false` indicates connection is closed or rejected. | ||
| * - `undefined` no attempt has resolved. Connection may be attempted | ||
| */ | ||
| readonly isConnected: () => boolean | undefined; |
There was a problem hiding this comment.
A boolean | undefined is not a good API design. It should either be boolean or ConnectedState representing many possible states.
There was a problem hiding this comment.
Changed to boolean. The ConnectedState can be accessed via state function as a more descriptive one
There was a problem hiding this comment.
i want to keep this aspect of the api.
it is intuitive that 'truthy value means yes, falsy value means no'.
the additional information of 'maybe' and 'never' is available by inspection of the falsy value.
- 'undefined' is used to represent a connection that is unknown - it should become 'true' or 'false' later
- 'false' is used to represent a connection that is rejected - it will definitely remain 'false'
i think this is simple to explain and simple to understand. i find it very useful to have a simple truthy/falsy check.
detailed state is available from the state method.
There was a problem hiding this comment.
Sorry, this is not a good API design. We should not assign meaning to undefined and cause our consumers to try to guess what it means and hope they infer like we have. We should replace this with an enum if there are more possible states than two.
There was a problem hiding this comment.
'false' is used to represent a connection that is rejected - it will definitely remain 'false'
@turbocrime why can't users re-establish the connection when isConnected = false?
docs/adrs/006-web-apis.md
Outdated
| /** Call to gain approval. Returns `MessagePort` to this provider. | ||
| * Might throw descriptive errors if user denies the connection */ | ||
| readonly connect: () => Promise<MessagePort>; |
There was a problem hiding this comment.
This combines connect() with request() right? Think that is a good thing.
There was a problem hiding this comment.
yeah, it combines both functions together
There was a problem hiding this comment.
previously these were combined, then separated because it was asked that they become separate. we've had recent discussions where it was suggested that helper functions exported from the client package should not helpfully perform this combination. so it seems like opinions are contradictory.
these are currently separate and i think should remain separate.
if they are combined, then a long 'connection init is happening' wait is indistinguishable from a long 'user approval is happening' wait. it is useful for these to be distinct time periods.
There was a problem hiding this comment.
In what cases would a consumer want to initiate a connection, but not trigger an approval? This behavior does not feel standard amongst wallet APIs in the industry.
There was a problem hiding this comment.
can't think of the cases when the request shouldn't be followed by connect. Don't know about the previous decisions but reverting them doesn't seem to be really breaking
docs/adrs/006-web-apis.md
Outdated
| /** | ||
| * A connection listener function. Fires a callback with | ||
| * the `origin`, `state`, and `connected` fields each time the state changes | ||
| */ | ||
| readonly onConnectionChange: (cb: (connection: { origin: string, connected: boolean, state: PenumbraState }) => void) => void; | ||
| } | ||
| ``` |
There was a problem hiding this comment.
It's not clear to me how this compares to the recently added pattern of add/remove event listeners. Is this a better pattern that should replace that?
There was a problem hiding this comment.
It's basically a simplified version of the listeners with added connected boolean to the callback. I feel like the event in addEventListener is redundant since users will only access the detail property of event. But if we don't expose CustomEvent, then probably it shouldn't be called addEventListener.
We can vote on this design or come up with another version
There was a problem hiding this comment.
the event interface is standard, and extensible. flexibility may help avoid breaking changes to this api. it may eventually be useful for the provider to emit additional detail with the events, or emit other event types we haven't thought of yet.
for example we could add a specific 'penumbraconnected' event without changing any interface signature in a way that breaks consumers.
use the standard functionality also prevents any implementation errors on our part - all of the callback logic is handled very well by browser developers. we only write the message types.
There was a problem hiding this comment.
Ok, I agree to compromise. Put back the addEventListener and removeEventListener but simplified the types definition. In the current implementation (probably why I was so hesitant to accept this API before), CustomEvents aren't really typed, so in Nextjs example I wrote the following:
const typedEvent = event as CustomEvent<{ origin: string, state: PenumbraState }>;If we fix the types, it'll already be better. Even though I was really tempted to change the addEventListener to simply addListener: (type: 'penumbrastate', cb: (value: { origin: string; connected: boolean; state: PenumbraState }) => void)
docs/adrs/006-web-apis.md
Outdated
| /** Synchronously creates a connectrpc `PromiseClient` instance to a given Penumbra service */ | ||
| readonly getService: <SERVICE extends PenumbraService>(service: SERVICE) => PromiseClient<SERVICE>; |
There was a problem hiding this comment.
I like that the client is something with methods on it. I've suggested this previously before. Think it's more clear versus having random functions that get imported.
There was a problem hiding this comment.
clarifying: this client interface should not be part of the injected script
i avoided adding a client construction method to the page interface intentionally.
- a content-script injection must meet the page interface
- all packages and types required to meet this interface will be injected
- due to requested functionality, the content script is injected to every page the user visits
- due to requested functionality, the content script is injected at
document_startwhich blocks page execution - a developer writing a dapp for this interface is going to import a package to interact with it anyway, so heavy things can go in there, and avoid injection.
without any effort at size reduction, the current injection imports no dependencies and compiles to 4775 bytes. this is acceptable for a script that we are causing the user to execute on every single page they visit.
if we add client creation to the page interface, the injection increases to 217022 bytes, including dependencies such as all service type definitions and a complete library for instantiating a promiseclient, and connectrpc's entire dependency tree. this is injected to every page the user visits, and will block page execution.
https://developer.chrome.com/docs/extensions/develop/concepts/content-scripts#run_time
so, this must remain part of a different module, like the existing client helper functions, and should not be returned from any page interface function
There was a problem hiding this comment.
@turbocrime thanks for measuring the bundle! You point is absolutely valid – we don't want users to load too much JS on EVERY page. For this reason, I updated the ADR and moved the function from the client instance to @penumbra-zone/client/service. Though, this function can and should modify the client instance by inserting initiated services, so the service clients won't be duplicated
docs/adrs/006-web-apis.md
Outdated
| ```ts | ||
| 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> | ||
| >; |
There was a problem hiding this comment.
It would be nice if this was encapsulated so users don't have to guess imports.
There was a problem hiding this comment.
How do you imagine the encapsulation in this case? Adding the independent public actions inside the client feels weird
There was a problem hiding this comment.
I previously suggested something like:
interface PenumbraClient {
requestPraxConnection(): Promise<void>;
isPraxConnected(): boolean;
isPraxInstalled(): boolean;
throwIfPraxNotInstalled(): void;
throwIfPraxNotConnected(): void;
createPraxClient<T extends ServiceType>(serviceType: T): PromiseClient<T>;
}But open if folks don't find that useful
docs/adrs/006-web-apis.md
Outdated
| ```ts | ||
| const res = await client.getAddressByIndex({ account: 0 }); | ||
| if (res) { | ||
| console.log(res.data); | ||
| } else if (res.error instanceof NotConnectedError) { | ||
| client.reconnect(); | ||
| } else { | ||
| console.error('Unknown error:', res.error); | ||
| } | ||
| ``` |
There was a problem hiding this comment.
This example exhibits some patterns that don't feel so great. The caller of service functions shouldn't have to keep track of and troubleshoot the client connection state. It feels like the wrong place to do so. Think erroring is fine because before they attempt to make this call, their view should first determine whether the client is connected or not---and if not, show a connect button (and not make this fetch at all).
While pairing with Finch on the governance site, this was a common pattern:
export default function useAppParameters() {
const { viewClient, connected } = useStore((state) => state.prax);
return useQuery({
queryKey: ["appParameters"],
queryFn: async () => {
return (await viewClient())?.appParameters({});
},
enabled: connected,
});
}
|
Some additional related issues: |
|
@turbocrime Let's finish this up. Wanted to mention why stick to this design if we have almost the same api right now. This ADR, as you mentioned before, renames a bunch of methods with the main idea to unify the notion of 'client'. It was mostly referred to the service client but is changing in this ADR to the Penumbra client as a set of encapsulated methods to work with the injected connection. It's a common pattern for libraries that users are used to: no need to guess the imports, create the 'client' once and share everywhere, while it stores the useful state and simplifies the work. The main point disagreement for now is this Gabe's comment about the connection listener. We can vote on this, I don't mind different versions. At tell me if there's more to add to this document, I might have forgotten something. |
|
still going through this will add some more review later. please push the latest version it doesn't seem up-to-date wrt the comments |
|
have some work done for implementing these issues and some ADR, will push tonight or in the morning |
turbocrime
force-pushed
the
feat/adr-006-client-apis
branch
from
7f4d398
to
a19b911
Compare
Co-Authored-By: the letter L <[email protected]>
Specs for the
clientpackage API.Slogan of this PR is: better developer experience, greater penumbra adoption