-
Notifications
You must be signed in to change notification settings - Fork 933
refactor(experimental): provide account encoding on the data field #2017
Conversation
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. Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
450582d
to
ca5cf6d
Compare
ca5cf6d
to
d8547c8
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 what this code is doing is:
- unconditionally doing one
account.load
with noencoding
- doing
account.loads
with whateverencoding
is on zero or moredata
fields in the subselection
What I hope we can make happen is stuff like this:
- If there are no
data
fields, fetch the account with the fastest-to-fetch encoding - If there is one
data
field set to Y encoding, then both fetches inresolveAccount
and inresolveAccountData
should use Y encoding
Back to your queue for comments!
@@ -13,3 +13,19 @@ export const resolveAccount = (fieldName: string) => { | |||
) => | |||
parent[fieldName] === null ? null : context.loaders.account.load({ ...args, address: parent[fieldName] }, info); |
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.
Maybe this is what you mean by ‘This is remedied in the next commit in the stack,’ but what should happen here is:
- The account resolver introspects the query itself, and looks into its own subselection for direct child
data
fields.
a. If there are no data fields, fetch the account with whatever encoding is lightest on the RPC.
b. If there are data fields, fetch the account with one of those encodings (to ensure a cache hit between the fetch for the account and the fetch for at least one of the data fields).
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.
Yeah, I was going to tackle this in the next commit, but I guess now that I think of it there's no reason for this one then. 😅
I think this is probably a good approach. I can latch into this to help with |
Closing to pull this off the current stack and introduce a new stack for schema revisions. |
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 refactors the accounts schema to allow for providing specific account
data encoding only when requesting the
data
field, which is always going tobe a string of encoded data.
Accounts are queried from the RPC as
jsonParsed
by default, so when this fieldis provided with an explicit
encoding
, it will return the account's data inthe requested encoding in the
data
field.Note this means it's now possible to query multiple data encodings on accounts,
as depicted in the tests.
For more context, here's a review comment by @steveluscher on a previous PR:
#1837 (comment)
Important Note: This change will cause
programAccounts
queries to spamthe RPC for each account, as the resolver is called from each account's context.
This is remedied in the next commit in the stack.