feat(core): let the backend decide whether a request is served - #321
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Thanks @mariano-aguero for taking care of this. The overall changes made lgtm, the shape is right and the test coverage on the keyless paths reads well. Two paths need covering before this is safe to turn on, both about where x-dapp-origin actually gets attached. Since the header is the only thing that identifies a caller from the keys origin, any request that goes out without it is refused for a keyless integrator, and today the send and the fee estimate are two of those. Comments inline on both. The rest of what I found is minor or pre-existing, left inline as non-blocking so nothing gets lost. Happy to approve once the two are in |
Note — ENS reverse resolution has no keyless story yet (open question, not a blocker on this PR)Filing this as a note rather than an inline comment because the code involved ( What it is. The api key was never a header here — it rode inside the Why registering an origin won't fix it. The refused request is made by JustaName's backend, not by the browser. On that hop there is no Blast radius if left as-is. Options, roughly in order of how much I like them:
Happy to prototype (1) if that's the direction — it's contained to One unrelated flag while I was testing: my probe of the analytics guard used a deliberately malformed address expecting a validation error, but the keyed call was accepted ( |
|
@Ghadi8, on your note about ENS reverse resolution:
You are right on all of it, and this one I had to check before answering because it is the only finding today that an origin cannot fix. Confirmed: Filed as #345 with your measurements and the four options. Option 4 is what ships here, deliberately: the rest of the keyless path is closed and this needs a decision about which hop carries the credential rather than a change in this PR. I would take option 1 as the destination, though I would not size it as contained to On the junk analytics row: worth knowing that nothing validates that address on our side either. Thanks for testing it against production rather than reading it off the diff. The six inline ones are answered and resolved in their threads. |
What
apiKeybecomes optional on the SDK's entry types, an RPC refusal from the proxy now throws instead of resolving toundefined, and core names the dApp it is acting for on calls that go to a backend of ours.Why
Whether a request is served is the backend's decision, not the SDK's. The types said otherwise.
apiKeywas required onCreateJAWSDKOptionsandConstructorOptions, and@jaw.id/wagmiinherits it throughJawParameters, so a caller could not even build a request the server would get to answer.Three things fell out of relaxing it.
The chain list was built inside
if (params.apiKey), so a config without one produced an SDK with no chains and no error saying why. The condition is gone andcreateInitialChainshandles the absence itself.Five handlers refused locally before reaching the network: the four
wallet_get*inJAWSignerandwallet_revokePermissionsinCrossPlatformSigner. They now forward what they have and let the server answer.wallet_getCapabilitiesmatters most there, since it is what declares EIP-5792 atomic support.The URL builders drop the query parameter rather than sending
api-key=empty, which reaches the proxy as a malformed key and is rejected before anything else is considered.App-specific mode is unchanged and still requires a key. It hands one to the dApp's own
UIHandler, which has nowhere to get it, socreate()refuses such a config while the app is still being wired up.Naming the calling dApp
A request that carries no key has to be identifiable some other way, and the browser's
Originis that way from a dApp's own page. From the keys popup it is not: everything there runs on the keys origin, which is the same whichever dApp opened it.setDappOriginis how keys says which dApp a call acts for, taken from the origin the browser put on the message being served rather than from an earlier one, and core sends it asx-dapp-origin. A dApp's own page must never call it, since a value set there would be a claim the browser did not make.The value stays out of the persisted config. One origin serves every dApp in popup mode, so a stored one would greet the next dApp holding the previous one's.
Which hosts are told follows one rule in two places. The RPC and bundler transports are ours whatever host they point at, staging and local included, so they always carry the header. The urls that may belong to somebody else are a paymaster's, which is told only when it is on our proxy, and a
serverUrlan app-specific dApp points at its own server, which is never told.Analytics reports without a key rather than not at all.
logSignatureandlogAccountIssuanceused to return early when the config had no key, which would have left a caller with no metric of its own recorded anywhere. They send what they have now, and the dApp origin is what names the caller when the key is missing.The bug this uncovered
fetchRPCRequestnever checkedres.ok. A refusal is not a JSON-RPC envelope, so destructuring it gave{ result: undefined, error: undefined }and the call resolved toundefinedinstead of failing.handleGetCapabilitiesRequestthen cached that silence for 60 seconds, against a comment saying only a fulfilled response is cached.The path already existed for an invalid key, so this is a fix in its own right. It changes the error behaviour of every RPC call the SDK makes, not only the four reads: a caller treating
undefinedas "empty" now sees an exception. Nothing in this repo does, andgetCapabilitiesreaches wagmi through TanStack Query, where a rejection is what the query expects andundefinedwas the worse outcome.Before releasing
nx releaseversions off conventional commits, so this publishes a minor of@jaw.id/coreon its own. It pairs with a server-side change that is not deployed yet, and publishing first puts the client ahead of the server. That deploy lands first, or this publish waits.How to test
bun install bunx nx run-many -t typecheck lint test --projects=@jaw.id/core,@jaw.id/ui bunx nx api-check @jaw.id/core35 new tests, plus two in
JAWSigner.test.tsthat changed sides: they used to assert that a missing key silences the analytics report, and now assert that the report goes out without one. ThefetchRPCRequestblock inutils/provider.test.tsis the one worth reading, since its 401 case is what used to resolve toundefinedand now throws.api/rest.test.tsandstore/chain-clients/utils.test.tspin which hosts are told which dApp is calling.The docs changes are in
configuration/apiKey.mdxand the two pages that link to it: the key is optional in cross-platform mode, and a keyless call is served against the origin registry instead.Checklist
nx releasetakes the version bump from its typebunx prettier --check .andbunx nx affected -t lint test typecheck build api-checkpass, which is what CI runs@jaw.id/core's public API is unchanged, orbunx nx api-update @jaw.id/coreran and the report diff is part of this PR