Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

Allow extending numeric keypaths when extending Solana RPC API #3212

Conversation

mcintyre94
Copy link
Contributor

@mcintyre94 mcintyre94 commented Sep 6, 2024

This PR adds additional optional config to createSolanaRpcApi that allows defining numeric keypaths for additional methods.

This means that if you use createSolanaRpcApi to define an API that extends ours (as in our example), then you can add numeric keypaths to the default response transformer for the additional methods.

Eg

const heliusRpc = createSolanaRpcApi<HeliusRpcMethods>({
   ...DEFAULT_RPC_CONFIG,
   extendAllowedNumericKeypaths: {
     getAsset: [["token_info", "decimals"]]
  }
})

Without this you inherit our bigint upcasting (since this is on the default response transformer from createSolanaRpcApi), but have no way to exempt certain fields on your own methods.

Note that the semantics here are that you're extending the default allowed keypaths, so you can't override those included in the default, ie the Solana RPC methods. It's assumed that if you're extending our API using this opinionated method then you've implemented our API as-is. So the extended keypaths are typed to exclude the methods in SolanaRpcApi.

Copy link

changeset-bot bot commented Sep 6, 2024

🦋 Changeset detected

Latest commit: c83bba1

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

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

@lorisleiva
Copy link
Contributor

Do you want to rebase this on top of your latest PR so you're not constraint by RpcApiMethods?

@mcintyre94 mcintyre94 force-pushed the extend-allowed-numeric-keypaths branch from 9d03f3b to 02bbf77 Compare September 6, 2024 17:12
@mcintyre94 mcintyre94 changed the base branch from master to rpc-api-methods-cleanup September 6, 2024 17:12
Copy link
Contributor Author

mcintyre94 commented Sep 6, 2024

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @mcintyre94 and the rest of your teammates on Graphite Graphite

@mcintyre94
Copy link
Contributor Author

Yep, updated + added typetests :)

@mcintyre94 mcintyre94 force-pushed the extend-allowed-numeric-keypaths branch from 02bbf77 to 69e52d2 Compare September 6, 2024 17:16
@mcintyre94 mcintyre94 force-pushed the rpc-api-methods-cleanup branch from 740d46b to 1f94447 Compare September 6, 2024 17:22
@mcintyre94 mcintyre94 force-pushed the extend-allowed-numeric-keypaths branch from 69e52d2 to 18baa72 Compare September 6, 2024 17:23
Copy link
Contributor

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to make this argument extend the allowed numerics, instead of just overriding? What if I don't care about the loss of precision and I definitely want number for certain fields (ie. timestamp, lol)?

@mcintyre94
Copy link
Contributor Author

Any reason to make this argument extend the allowed numerics, instead of just overriding?

  • I didn't want to override in a way that forces you to provide the entire list for Solana methods, because the idea of this API (IMO) is to let you add methods to the RPC without worrying about ours. We know you're extending SolanaRpcApi so we should default to sensible behaviour for Solana methods.
  • But I am doubting the restriction that you can't override our methods, if you want to. I don't know if there are any examples that change responses, but I know that eg Triton have a custom getRecentPrioritizationFees that modifies input params of the existing method. It seems reasonable that such a method might want to modify response shape too. I think I'll tweak the typing so that you can override existing ones, but don't have to.

@buffalojoec
Copy link
Contributor

buffalojoec commented Sep 9, 2024

  • I think I'll tweak the typing so that you can override existing ones, but don't have to.

Cool! You could also just make it a true override, and give people a way to use the default constant that ships with the transformer library and modify certain keys. Whatever you prefer, I just wondered if my motivation made sense.

@mcintyre94 mcintyre94 force-pushed the rpc-api-methods-cleanup branch from 1f94447 to 728d517 Compare September 9, 2024 16:21
@mcintyre94 mcintyre94 force-pushed the extend-allowed-numeric-keypaths branch from 40d3a77 to 203c4da Compare September 9, 2024 16:21
@mcintyre94 mcintyre94 changed the base branch from rpc-api-methods-cleanup to rpc-subscriptions-methods-cleanup September 9, 2024 16:22
@mcintyre94 mcintyre94 force-pushed the extend-allowed-numeric-keypaths branch from 203c4da to c83bba1 Compare September 9, 2024 16:26
Copy link
Contributor

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love this on ‘configuration bad, composition good’ grounds.

The way I would imagine a third party doing this is to front the calls to getAsset.

type TestApi = SolanaRpcApi & {
    someNewFunction(): void;
    someOtherFunction(): void;
};

const TEST_API_RESPONSE_TRANSFORMER = getDefaultResponseTransformerForSolanaRpc({
    allowedNumericKeyPaths: {
        someNewFunction: [['someNewKeyPath']],
        someOtherFunction: [['someOtherKeyPath']],
    },
});

const solanaRpcApi = createSolanaRpcApi<TestApi>();

const testApi = new Proxy(solanaRpcApi, {
    defineProperty() {
        return false;
    },
    deleteProperty() {
        return false;
    },
    get(target, p, receiver) {
        const methodName = p.toString();
        const originalMethod = Reflect.get(target, p, receiver);
        if (methodName !== 'someNewFunction' && methodName !== 'someOtherFunction') {
            return originalMethod;
        }
        return (...args) => {
            const requestPlan = originalMethod(...args);
            return {
                ...requestPlan,
                responseTransformer: TEST_API_RESPONSE_TRANSFORMER,
            };
        };
    },
}) as RpcApi<TestApi>;

Copy link
Contributor

This also summarily settles the ‘extend vs. override’ debate because now the decision to extend or override lies in the hands of the person who designs the Proxy. You could even layer together our transformer with yours!

@mcintyre94
Copy link
Contributor Author

mcintyre94 commented Sep 9, 2024

Fair enough! I'm less convinced this will improve things (much) for my Helius use case too, because their fetch API (as opposed to JSON RPC API) methods can't use other aspects of our default response transformer either, so you still end up with some special casing similar to your example there.

I'll close this, but separately might fiddle with the response transformer exports if I end up trying to write a custom one and find something useful we're not exporting.

@mcintyre94 mcintyre94 closed this Sep 9, 2024
Copy link
Contributor

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants