-
Notifications
You must be signed in to change notification settings - Fork 925
refactor(experimental): graphql: revise accounts schema for data inputs #2068
refactor(experimental): graphql: revise accounts schema for data inputs #2068
Conversation
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
1b32527
to
6fd5498
Compare
be0de10
to
4927097
Compare
6fd5498
to
153f535
Compare
911bcfb
to
6d20469
Compare
153f535
to
3df7020
Compare
6d20469
to
f05f15b
Compare
3df7020
to
e718da9
Compare
f05f15b
to
a4345a7
Compare
e718da9
to
112f2d3
Compare
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 think this is the time to write visitors. It'll become much more tractable to handle all of the many many edge cases:
{
coolAccount: account {
data(encoding: BASE58)
... on TokenAccount {
mint
}
owner {
dataYouDoNotWantYourVisitorToProcessYet: data(encoding: BASE64)
}
...SpecialAccountsFragment
...CompressedDataFragment
}
}
fragment SpecialAccountsFragment on Account {
... on VoteAccount {
authority
}
}
fragment CompressedDataFragment on Account {
compressedData: data(encoding: BASE64ZSTD)
}
Oooh. I think I found that ‘merge the type info into the visitor’ thing I was looking for before.
https://github.com/DxCx/graphql-js/blob/async/src/language/visitor.js#L321-L351
fbbdd5d
to
3037133
Compare
0643a09
to
6b596b0
Compare
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.
K, I'm suspicious that there might be a bug here somewhere because I would not expect the test with no fragment specific fields to fetch with jsonParsed
encoding. I think we need a few kinds of tests, multiplied by the three ways of making subselections (fields, inline fragments, and fragment spreads).
- Subselections that don't require a fetch at all (ie. just fetching
address
) - Subselections that require
jsonParsed
(ie. literally requesting__typename
, subtype-specific fields likeMintAccount#mint
) - Subselections that require baseX
data
…plus a combination of each.
packages/rpc-graphql/src/resolvers/__tests__/account-resolver-test.ts
Outdated
Show resolved
Hide resolved
packages/rpc-graphql/src/resolvers/__tests__/account-resolver-test.ts
Outdated
Show resolved
Hide resolved
packages/rpc-graphql/src/resolvers/__tests__/account-resolver-test.ts
Outdated
Show resolved
Hide resolved
packages/rpc-graphql/src/resolvers/__tests__/account-resolver-test.ts
Outdated
Show resolved
Hide resolved
if (argumentNode) { | ||
if (argumentNode.value.kind === 'EnumValue') { | ||
if (argumentNode.value.value === 'BASE_58') { | ||
return 'base58'; | ||
} | ||
if (argumentNode.value.value === 'BASE_64') { | ||
return 'base64'; | ||
} | ||
if (argumentNode.value.value === 'BASE_64_ZSTD') { | ||
return 'base64+zstd'; | ||
} | ||
} | ||
if (argumentNode.value.kind === 'Variable') { | ||
return variableValues[argumentNode.value.name.value] as AccountLoaderArgs['encoding']; | ||
} |
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.
Could always implement this with visit()
rather than spelunking in value.kind
. This probably applies anywhere you find yourself doing .kind
.
Or not. This is fine too!
const fragmentDefinition = info.fragments[node.name.value]; | ||
const fragmentType = schema.getType(fragmentDefinition.typeCondition.name.value)?.name; | ||
let rootType; | ||
if (root.kind === 'Field') { |
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.
God I wish this thing let you write AST selectors. https://medium.com/dailyjs/eslint-selectors-just-got-super-powers-402ab942def5
Then you could do things like:
visit(node, {
'FragmentSpread > Field': (node, key, parent, path, ancestors) => {
// Only finds `Field` nodes who are the children of `FragmentSpread` nodes.
},
});
If only there were like a TSQuery for GraphQL? https://medium.com/@phenomnominal/easier-typescript-tooling-with-tsquery-d74f04f2b29d
3037133
to
a1b1848
Compare
6b596b0
to
0790485
Compare
expect(rpc.getAccountInfo).not.toHaveBeenCalledWith('AyGCwnwxQMCqaU4ixReHt8h5W4dwmxU7eM3BEQBdWVca', { | ||
encoding: 'jsonParsed', | ||
}); |
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.
@steveluscher It seems to me that the resolver is working correctly. I'm wondering if, in regards to your comments, you may have overlooked the .not
here?
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.
🪦 OMG I missed that. Nice work!
8953f62
to
10aedf8
Compare
Merge activity
|
a1b1848
to
169b304
Compare
10aedf8
to
77e1783
Compare
🎉 This PR is included in version 1.90.1 🎉 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 commit re-works the schema to deal with
encoding
anddataSlice
directlyon the
data
field, rather than anaccount
query at the top-level.With this particular commit, the number of RPC calls to resolve a wide range of
queries is likely increased - such as multiple
data
fields onprogramAccounts
.However, the subsequent commits in this stack will seek to remedy this.
Specifically, the next commit introduces better account resolvers, which may
lessen the RPC calls for
account
queries by way of cache hits, but the commitafter that will introduce the revamped batch loader, which provides significant
optimizations to the number and timing of RPC calls.
Further up the stack are more changes to ensure the other queries, like
programAccounts
, get to share in the benefits of similar optimizations.Closes #1831