Make rpc non-thenable #508
Conversation
🦋 Changeset detectedLatest commit: 6b7e878 The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 packages
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 |
BundleMonFiles updated (4)
Unchanged files (123)
Total files change +101B +0.03% Final result: ✅ View report in BundleMon website ➡️ |
There was a problem hiding this comment.
Thanks, this makes a lot of sense!
Would you be able to make the same change to the subscription RPC which uses the same pattern?
Separately to this PR I'll also open an issue to check if there are any other properties we should be intercepting to stop JS mis-interpreting our proxy objects like this.
|
That sounds good. As an additional safeguard, we might consider restricting the proxy to only expose known Solana RPC method names. That way, any unexpected property access (like then or others) would default to undefined, avoiding misinterpretation by JavaScript and keeping the surface of the proxy more predictable. I can look into implementing that if it seems worthwhile, but it could also be part of the task you just created. |
|
For now let's push anything else to that task. We should definitely do |
| ? ( | ||
| ...args: Parameters<TSubscriptionMethodImplementation> | ||
| ) => PendingRpcSubscriptionsRequest<ReturnType<TSubscriptionMethodImplementation>> | ||
| : never; |
There was a problem hiding this comment.
It looks like something has tweaked the formatting here. If you run pnpm style:fix at the root of the repo it'll apply our formatting/prettier and should make the CI happy!
We don't do this on purpose, because to do so would require that we enumerate every method name, which would require that we ship all of those strings in the JavaScript bundle. |
| @@ -72,6 +72,9 @@ function makeProxy<TRpcMethods, TRpcTransport extends RpcTransport>( | |||
| return false; | |||
| }, | |||
| get(target, p, receiver) { | |||
| if (p === 'then') { | |||
There was a problem hiding this comment.
Note for myself: this doesn't have to be p.toString() because there's no way of calling the then method of a Promise using a Symbol.
packages/rpc-subscriptions-spec/src/__tests__/rpc-subscriptions-test.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Steven Luscher <steveluscher@users.noreply.github.com>
…s-test.ts Co-authored-by: Steven Luscher <steveluscher@users.noreply.github.com>
|
Thanks for the review! I learned something new, but i wish this wasn't something that had to be learned 😆 |
Co-authored-by: Callum McIntyre <mcintyre1994@gmail.com>
|
One last question: are we convinced that we don't also need to blocklist |
|
@steveluscher I don't think so - the MDN docs call out |
|
Hmm, I didn't think about that when I encountered this issue. After looking a bit online, it seems that both
However, catch would be called on the rpc object and go through the it('treats catch as an RPC method name and fails appropriately', () => {
const getPropertySpy = jest.spyOn(Reflect, 'get');
// @ts-expect-error We're intentionally testing runtime behavior that TypeScript prevents
const catchFunction = rpcSubscriptions.catch;
// The function should throw when called
expect(() => {
catchFunction(() => { });
}).toThrow(new SolanaError(SOLANA_ERROR__RPC_SUBSCRIPTIONS__CANNOT_CREATE_SUBSCRIPTION_PLAN, {
notificationName: 'catch',
}));
// Verify it tried to look up 'catch' as an RPC method name
const spyCalls = getPropertySpy.mock.calls;
expect(spyCalls.some(call => call[1] === 'catch')).toBe(true);
getPropertySpy.mockRestore();
});Given this behavior, I can make the following changes:
if (p === 'then' || p === 'catch' || p === 'finally') {
return undefined;
}
|
|
Oh @mcintyre94, good find. So JavaScript only treats an object as Promise-like if it has a .then() method, that's what makes it "thenable". My test was actually just confirming that catch is treated like any other invalid RPC method name, not testing any Promise-related behavior. |
|
Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up. |
Problem
RPC objects created by Solana Kit use a proxy that converts all property access into methods. When JavaScript's Promise machinery checks for a
.thenproperty (which happens when youasync awaitan RPC object), the proxy intercepts this and returns a method{ send: [AsyncFunction: send] }instead ofundefinedor the proper Promise interface, making the RPC object appear thenable when it isn't. This causes JavaScript/TypeScript to silently terminate the program when you try toawaitthe RPC object as if it were a Promise.Summary of Changes
makeProxyfunction to specifically interceptthenproperty access and returnundefined.thenlike any other RPC method