Allow more transaction fields for RPC field selection#371

DZakh · 2024-11-29T15:44:00Z

In the next PRs:

Delete cache after finishing fetching the batch
Add field selection by event, otherwise, it will add too many unnecessary rpc calls for indexers with many event kinds

DZakh · 2024-12-02T13:25:07Z

+  loaderQueue: SDSL.Queue.t<'key>,
  // Keys for items that have been loaded already. Used to evict the oldest keys from cache.
-  loadedKeys: SDSL.PriorityQueue.t<int>,
+  loadedKeys: SDSL.Queue.t<'key>,


It was tricky to make PriorityQueue make with a string value, so I thought that a normal queue will work for us similarly good.

If I recall correctly the reason that priority queue was used is becaues somehow the block promises were resolving in reverse order. (ie later blocks first, earlier blocks last)

Maybe it's worth checking that we haven't broken that behaviour.

DZakh · 2024-12-02T13:25:57Z

-      Logging.error({
-        "err": err,
-        "msg": `EE1100: Top level promise timeout reached. Please review other errors or warnings in the code. This function will retry in ${(am._retryDelayMillis / 1000)
-            ->Belt.Int.toString} seconds. It is highly likely that your indexer isn't syncing on one or more chains currently. Also take a look at the "suggestedFix" in the metadata of this command`,
-        "metadata": am.metadata,
-      })


Had to move the dependency on Logging, to move the file to npm package and be able normally edit it with ReScript watch mode enabled.

DZakh · 2024-12-02T13:26:29Z

            initial_block_interval: 10_000,
            backoff_multiplicative: 0.8,
-            acceleration_additive: 2_000,
+            acceleration_additive: 500,


Changed after discussion

We must still monitor if this is low enough I think. If queries are maxing out below 2500 here then backoff will be less than the acceleration. Obviously it's configurable so it can be dealt with case by case.

DZakh · 2024-12-02T13:27:11Z

        TransactionIndex,
        #[subenum(RpcTransactionField)]
        Hash,
+        #[subenum(RpcTransactionField)]
        From,
+        #[subenum(RpcTransactionField)]
        To,
        Gas,
+        #[subenum(RpcTransactionField)]
        GasPrice,
+        #[subenum(RpcTransactionField)]
        MaxPriorityFeePerGas,
+        #[subenum(RpcTransactionField)]
        MaxFeePerGas,
        CumulativeGasUsed,
        EffectiveGasPrice,


We can allow more fields after moving to our own rpc client.

Why can't we do all fields? Are the others only available in a different rpc method or something?

Ethers simply doesn't expose them, or exposes them already in decoded way, which is difficult to handle with current schema set up.

JonoPrest · 2024-12-04T09:12:20Z

+  inProgress: Utils.Set.t<'key>,
  // Keys  for items that we have not started loading yet.
-  loaderQueue: SDSL.PriorityQueue.t<int>,
+  loaderQueue: SDSL.Queue.t<'key>,


I guess what makes me nervous of this polymorphic 'key param, is that really it's constrained to a number or a string else there could be unexpected behaviours. If it were an object for instance then there is more nuance to using it.

My type OCD is making me want to see this 'key type either statically typed as Int(int) | Str(string) or to pass a toString function rather than casting with Utils.magic

JonoPrest · 2024-12-04T09:16:27Z

+      switch am.onError {
+      | None => ()
+      | Some(onError) => onError(am, ~exn=err)
+      }


A thought I had with this side effect logging, is that we may want to be able clean up/cancel this onError (or even the actual task) from where the lazy loader is called. In case you want to implemented a higher level timeout etc. Then you don't have to worry about stale logs and retries after you've already handled it etc.

I scoped out all the timeout handling changes for now

JonoPrest · 2024-12-04T09:17:29Z

+  // A hot fix after we use the version where it's supported
+  // https://github.com/DZakh/rescript-schema/blob/v8.4.0/docs/rescript-usage.md#removetypevalidation
+  let removeTypeValidationInPlace = schema => {
+    // The variables input is guaranteed to be an object, so we reset the rescript-schema type filter here
+    (schema->Obj.magic)["f"] = ()
+  }
+


😅 I will trust you on this, I have no idea what's going on here

JonoPrest · 2024-12-04T09:19:15Z

+let nativeSchema: S.t<bigint> = S.custom("BigInt", s => {
+  {
+    parser: unknown => {
+      if Js.typeof(unknown) !== "bigint" {
+        s.fail("Expected bigint")
+      } else {
+        unknown->Obj.magic
+      }
+    },
+  }
+})


Is this not built into rescript built into rescript schema out of interest?

It's in v8.4.0

JonoPrest · 2024-12-04T09:36:11Z

    }

-    pub fn to_rescript_schema(&self) -> String {
+    pub fn to_rescript_schema(&self, with_db_coerce: bool) -> String {


It seems like horrible param tunnelling to just handle the one bigint case...

Can't we find a way to improve this? Maybe having explicit functions that don't take the boolean flag. Or just have a dedicated rescript type for the native version?

Also I don't like seeing boolean flags called in functions because you have to go to the definition to understand what they are for. Better to use an enum for the flag or wrap the bool in a new type like struct WithDbCoerce(bool)

JonoPrest · 2024-12-04T09:43:22Z

+  | [{location: "hash"}]
+  | [{location: "hash"}, {location: "transactionIndex"}]
+  | [{location: "transactionIndex"}, {location: "hash"}] =>
+    (log: Ethers.log) =>
+      {
+        "hash": log.transactionHash,
+        "transactionIndex": log.transactionIndex,
+      }
+      ->parseOrThrowReadableError
+      ->Promise.resolve


Looks like it will add transactionIndex regardless of whether it was selected when hash is selected

Just realised you are still calling parseOrThrowReadableError! No need to respond to the above

It will be stripped by the parse function

JonoPrest

Hey @DZakh, just a couple of comments/nitpicks but nothing blocking. Thanks so much looks awesome!

DZakh

@JonoPrest Could you review the changes after your previous review?

DZakh · 2024-12-04T11:34:31Z

+  // The function used to load the result.
+  loaderFn: 'key => promise<'value>,


Having key as an unboxed variant would make the loaderFn annoying for usage, so I've decided to use Map to support literally any 'key type.

Cool, that works 🙂

I still think just passing toString dependency is the best because it can avoid any confusion about passing reference vs a new object etc. And I'm sure Js.Dict is more performant than Map etc

Makes sense. But I assume even thoug Js.Dict.t more performant than Map, but after adding toString there shouldn't be a difference.

DZakh · 2024-12-04T11:34:55Z

+#[derive(Debug, PartialEq, Clone)]
+pub enum RescriptSchemaMode {
+    ForDb,
+    ForFieldSelection,
+}


Like how it looks like with the explicit mode

Yeah definitely much easier to understand

DZakh · 2024-12-04T11:36:02Z

+    am.resolvers
+    ->Utils.Map.get(k)
+    ->Belt.Option.forEach(r => {
+      let _ = am.resolvers->Utils.Map.delete(k)


Added the line here. Previously there was a memory leak, since we never cleaned up the promise resolvers.

JonoPrest · 2024-12-04T11:46:32Z

    }

-    pub fn to_rescript_schema(&self, with_db_coerce: bool) -> String {
+    pub fn to_rescript_schema(&self, mode: &RescriptSchemaMode) -> String {


Why does it need to be a reference out of interest? Can't the fn just take ownership of the flag?

It would require to do .clone in a few places, so I thought it's simpler to work with a reference.

JonoPrest · 2024-12-04T11:56:34Z

@JonoPrest Could you review the changes after your previous review?

Nice @DZakh looks good, made some last comments but happy for you to merge whenever

DZakh added 5 commits November 29, 2024 18:43

Move Ethers to npm package

a6dfd63

Decrease default acceleration_additive

af15f9d

Prepare for transaction field selection

3c454be

Test with getTransaction request

e727712

Clean up

12ff1a1

DZakh marked this pull request as draft November 29, 2024 15:44

DZakh added 4 commits December 2, 2024 13:44

Query the transaction via the Ethers client

fef71bf

Move LazyLoader to npm package

a04cf28

Use LazyLoader to get transactions

54b07ef

Allow more fields for RPC feild selection

a710934

DZakh changed the title ~~Add field selection for RPC~~ Allow more transaction fields for RPC field selection Dec 2, 2024

DZakh requested a review from JonoPrest December 2, 2024 13:14

DZakh marked this pull request as ready for review December 2, 2024 13:14

Fix tests

c8669ee

DZakh commented Dec 2, 2024

View reviewed changes

JonoPrest reviewed Dec 4, 2024

View reviewed changes

JonoPrest approved these changes Dec 4, 2024

View reviewed changes

DZakh added 2 commits December 4, 2024 15:29

Fixes after review

0b6ccab

Fix memory leak

144c9a5

DZakh commented Dec 4, 2024

View reviewed changes

JonoPrest reviewed Dec 4, 2024

View reviewed changes

DZakh merged commit 415d1fa into main Dec 4, 2024

DZakh deleted the dz/rpc-field-selection branch December 4, 2024 12:36

		// The function used to load the result.
		loaderFn: 'key => promise<'value>,

Uh oh!

Conversation

DZakh commented Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonoPrest left a comment

Choose a reason for hiding this comment

Uh oh!

DZakh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonoPrest commented Dec 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

DZakh commented Nov 29, 2024 •

edited

Loading