-
Notifications
You must be signed in to change notification settings - Fork 915
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add get program accounts RPC method #1522
Add get program accounts RPC method #1522
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm!! What a beast
packages/rpc-core/src/rpc-methods/__tests__/get-program-accounts-test.ts
Show resolved
Hide resolved
d3d77bd
to
2f844e8
Compare
@@ -127,6 +127,142 @@ export const ALLOWED_NUMERIC_KEYPATHS: Partial< | |||
['value', KEYPATH_WILDCARD, 'data', 'parsed', 'info', 'commission'], | |||
['value', KEYPATH_WILDCARD, 'data', 'parsed', 'info', 'votes', KEYPATH_WILDCARD, 'confirmationCount'], | |||
], | |||
getProgramAccounts: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof. Related comment: #1492 (comment)
expect(accountInfo[0]).toMatchObject({ | ||
account: expect.objectContaining({ | ||
data: expect.any(String), | ||
executable: expect.any(Boolean), | ||
lamports: expect.any(BigInt), | ||
owner: expect.any(String), | ||
rentEpoch: expect.any(BigInt), | ||
space: expect.any(BigInt), | ||
}), | ||
pubkey: expect.any(String), | ||
}); | ||
expect(accountInfo[1]).toMatchObject({ | ||
account: expect.objectContaining({ | ||
data: expect.any(String), | ||
executable: expect.any(Boolean), | ||
lamports: expect.any(BigInt), | ||
owner: expect.any(String), | ||
rentEpoch: expect.any(BigInt), | ||
space: expect.any(BigInt), | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we actually assert on the values of the accounts, since they come from fixtures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definitely could, we just have a lot of tests already that don't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I sort of think we should. It's an easy lay-up for completing some of these tests and asserting proper functionality. Happy to start this work
We just have to make sure we don't change fixtures! 😝
getProgramAccounts( | ||
program: Base58EncodedAddress, | ||
config: GetProgramAccountsApiCommonConfig & | ||
GetProgramAccountsApiSliceableCommonConfig & | ||
Readonly<{ | ||
encoding: 'base64'; | ||
withContext: true; | ||
}> | ||
): RpcResponse<AccountWithPubkey<AccountInfoBase & AccountInfoWithBase64EncodedData>[]>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect it won't - I did something similar here: https://solanalabs.slack.com/archives/C050QNCV6BU/p1691418848861819
This playground works correctly
But when I used the same pattern in this codebase it wouldn't work, and just treated the output as like number | RpcResponse<number>
for any input
I think it might be the same underlying issue as #1280 but I'm not sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fucked around and found out (that there's a TypeScript bug). #1531
🎉 This PR is included in version 1.78.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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. |
This PR adds the
getProgramAccounts
RPC method for experimental web3jsThis is very similar to
getMultipleAccounts
, with the fun extra quirk that it can return either an RPC response or not, depending on the value ofwithContext
. Annoyingly my attempts to use a conditional type (Typescript playground) didn't work, it just always returnedT | RpcResponse<T>
. I suspect this is the same underlying issue as #1280So instead this method has 10 overloads, (4 encodings + no encoding) * 2 options for
withContext
. The previous limit was 5, so that's why there's a change to json-rpc-types.tsI also decided against refactoring to try to share data structures between
getAccountInfo
,getMultipleAccounts
andgetProgramAccounts
. They have subtly different behaviours/structures and I think it's just clearer to keep them as-is for now.Ref #1449