Introduce AccountFetcher to common-sdk #45

odcheung · 2023-06-20T08:38:26Z

Add AccountFetcher to allow easy fetching / caching of on-chain accounts
Class is adapted from AccountFetcherWithRetentionPolicy from the Orca2 UI
Class will be used as the caching layer for whirlpools-sdk, replacing AccountFetcher

Verification

Unit test cases written in this PR
Ran the Whirlpools unit test suite after adapting this class as the new AccountCache class in whirlpools-sdk

yugure-orca

How about not defining an interface for AccountFetchOpts, but rather AccountCache<T, AccountFetchOpts>?
Since it is an interface definition, it can be left to the implementer to decide whether or not to use TTL.

I think refreshAll was used quite infrequently even in AccountFetcher.
So I wonder It is not necessary to define it in the interface.

I would like to have a way to retrieve multiple types of accounts at once, as shown below, since a single RPC call can retrieve consistent data.
However, this may cause some accounts to be updated more recently later, so it would be better to define this as the same function as getMultipleAccountsInMap, etc., rather than a cache layer. WDYT ?

addresses = [[a1, a2, a3], [a4, a5, a6], [a7]]
parsers = [[ParsableWhirlpool, ParsableTickArray, ParsableTickArray], ...]

odcheung · 2023-06-22T23:38:47Z

Thanks @yugure-orca.

I've made the following amendments:

renamed cache back to fetcher (naming felt weird because a cache should not make network calls)
renamed ttl to maxAge
The options is now a generic type on the interface
removed refreshAll from the interface

I think the ability to fetch multiple types at once is a reasonable responsibility for this class. Of course, since this class depends on the account-request utilities layer, that layer would have to support it as well. Can you elaborate more on the "some accounts may be updated more recently"? I'd imagine if only accounts that exceeds the maxAge will be filtered, fetched and updated. We'll reconstruct the result purely based off of cached values just like we do for getAccounts / getAccountsAsArray today.

Also, I think given this PR is more for porting and no new feature depends on the get multiple types in 1 request just yet, I think that new feature should be left in a separate PR. what do you think?

yugure-orca

Thanks for the update.
Yeah "Fetcher" sounds good to me, too.

Also, I think given this PR is more for porting and no new feature depends on the get multiple types in 1 request just yet, I think that new feature should be left in a separate PR. what do you think?

Yep, agreed. Separate PR is comfortable.🙂

Can you elaborate more on the "some accounts may be updated more recently"?

The consistency aspect is more troublesome than the multiple types aspect.

assuming:

fetchSize = 10 (not 100)
addresses = [[A, B, C, D], [E, F], [G], [A, H, I, J], [B, K], [C]]

We need to fetch each sub-array consistently and it means we need to fetch them in a single RPC call.
So we will divide the addresses into 2 getMultipleAccountInfo (gMA) RPC call.

1st gMA ([[A, B, C, D], [E, F], [G]].flat())
2nd gMA ([[A, H, I, J], [B, K], [C]].flat())

If all of these calls are responded based on the same context slot, it is absolutely consistent.
But if 2nd gMA is responded based on a newer slot than 1st gMA, we need to handle duplicated addresses (A, B, C) correctly.

As for the cache hit decision, whether [A, B, C, D] is a cache hit or not is also a case where all [A, B, C, D] exist in the cache and the results are obtained from the same slot.

yugure-orca · 2023-06-23T00:56:24Z

packages/common-sdk/src/web3/network/fetcher/simple-fetcher-impl.ts

+    // Filter out all unexpired accounts to get the accounts to fetch
+    const undefinedAccounts = addressStrs.filter((addressStr) => {
+      const cached = this.cache.get(addressStr);
+      const elapsed = cached ? now - (cached?.fetchedAt ?? 0) : Number.NEGATIVE_INFINITY;


super nit: in getAccount, it is !!cached. (I think both works well)

odcheung · 2023-06-23T01:36:13Z

gotcha. Yeah that edge case would be an issue even for the current getAccounts calls. I'll leave a comment in the code.

An efficient solution is also not exactly clear if we must enforce all gMA calls to be returned on a consistent slot. But the good thing is this use-case isn't present (yet) for the UI, so we still have some time to figure it out.

terranmoccasin

Nice this will be great, thanks for doing this.

Just one main comment about forcing refetches - not sure if that's something the fetcher implementation is intended to support.

terranmoccasin · 2023-06-23T01:03:13Z

packages/common-sdk/src/web3/network/fetcher/simple-fetcher-impl.ts

+    opts?: SimpleAccountFetchOptions | undefined,
+    now: number = Date.now()
+  ) {
+    const addressStrs = AddressUtil.toStrings(addresses);


nit: since this is in a private method, we can probably use PublicKey[] as the type for addresses and skip this transformation and ensure we do the transform at the caller

The transformation is conditional and the transformation is pretty slow. If we force the transformation back to PublicKey, then the getAccounts methods will have to perform the transformation twice, which can be quite costly

Ah yes gotcha, makes sense since it's just transforming everything to strings

terranmoccasin · 2023-06-23T01:09:17Z

packages/common-sdk/src/web3/network/fetcher/simple-fetcher-impl.ts

+  maxAge?: number;
+};
+
+// SimpleAccountCache is a simple implementation of AccountCache that stores the fetched


nit: update comments

terranmoccasin · 2023-06-23T01:18:38Z

packages/common-sdk/src/web3/network/fetcher/simple-fetcher-impl.ts

+      result.set(addressStr, value);
+    });
+
+    // invariant(result.size === addresses.length, "not enough results fetched");


nit: should we keep this?

ah yes. remvoed

terranmoccasin · 2023-06-23T01:28:25Z

packages/common-sdk/src/web3/network/fetcher/simple-fetcher-impl.ts

+    const addressStr = AddressUtil.toString(address);
+
+    const cached = this.cache.get(addressStr);
+    const maxAge = opts?.maxAge ?? this.retentionPolicy.get(parser) ?? Number.POSITIVE_INFINITY;


To force refetch, I would be tempted to try and pass in maxAge = 0. I believe this line would end up using the retention policy, which could be confusing. Perhaps change to explicitly checking if maxAge is undefined here?

Also the case where retentionPolicy has a zero value. Should that make the fetcher always fetch latest data?

yeah good catch. upgraded the getMaxAge fn and added tests

odcheung added 5 commits June 20, 2023 16:26

add fetcher

cb31411

prettier

c0db2b2

remove line in package

5225bb4

add commnets

0a468cb

prettier for tests

67bb1c4

odcheung requested review from terranmoccasin and yugure-orca June 20, 2023 08:38

more comments

b429f00

odcheung mentioned this pull request Jun 20, 2023

Replace AccountFetcher with WhirlpoolAccountFetcher orca-so/whirlpools#107

Merged

yugure-orca reviewed Jun 22, 2023

View reviewed changes

odcheung added 2 commits June 23, 2023 07:17

address comments

689119f

rename cache to fetcher

3e7b1d6

odcheung changed the title ~~Introduce AccountCache to common-sdk~~ Introduce AccountFetcher to common-sdk Jun 22, 2023

odcheung added 2 commits June 23, 2023 08:18

fix fetcher name

615891d

fix package version name

a461acf

yugure-orca approved these changes Jun 23, 2023

View reviewed changes

address comments

a4536cd

terranmoccasin reviewed Jun 23, 2023

View reviewed changes

odcheung added 3 commits June 23, 2023 09:39

add a comment about consistency

21c1008

address tmoc comments

eef4843

fix maxAge calculation

224e919

odcheung merged commit 5a933ce into main Jun 23, 2023

odcheung deleted the meep/account-cache branch June 23, 2023 04:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce AccountFetcher to common-sdk #45

Introduce AccountFetcher to common-sdk #45

odcheung commented Jun 20, 2023 •

edited

Loading

yugure-orca left a comment

odcheung commented Jun 22, 2023

yugure-orca left a comment

yugure-orca Jun 23, 2023

odcheung commented Jun 23, 2023

terranmoccasin left a comment

terranmoccasin Jun 23, 2023

odcheung Jun 23, 2023

terranmoccasin Jun 23, 2023

terranmoccasin Jun 23, 2023

terranmoccasin Jun 23, 2023

odcheung Jun 23, 2023

terranmoccasin Jun 23, 2023

terranmoccasin Jun 23, 2023

odcheung Jun 23, 2023

Introduce AccountFetcher to common-sdk #45

Introduce AccountFetcher to common-sdk #45

Conversation

odcheung commented Jun 20, 2023 • edited Loading

Verification

yugure-orca left a comment

Choose a reason for hiding this comment

odcheung commented Jun 22, 2023

yugure-orca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odcheung commented Jun 23, 2023

terranmoccasin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odcheung commented Jun 20, 2023 •

edited

Loading