Dynamic Contract Preregistration improvements#259

JonoPrest · 2024-10-11T14:44:53Z

Before this change. If the indexer had pre-indexed dynamic contracts, then begun the indexing process, then restarted during this process. It would only get the relevant dynamic contracts registered up until the block processed.

Now, it will restart with all dynamic contract registrations related to the events that are configured for pre registration.

Changes:

Changed data structure of dynamic contract registrations table to include needed fields
Added a function generator for tables to avoid the need to hardcode fields on general queries. In time I will apply this to each table so that we move away from hardcoded sql queries and lean more on the table framework to make changes to tables/schemas less daunting.
Adds a way to startup with dynamic contract pre registrations

…n restart

JonoPrest · 2024-10-11T14:45:38Z

    "dynamic_contract_registry",
    ~fields=[
      mkField("chain_id", Integer, ~isPrimaryKey),
-      mkField("event_id", Numeric),


I removed event_id and rather use explicit log index and block number

Cool, it makes sense for these to be explicit.

JonoPrest · 2024-10-11T14:49:22Z

+module PostgresInterop = {
+  type pgFn<'payload, 'return> = (Postgres.sql, 'payload) => promise<'return>
+  type batchSetFn<'a> = (Postgres.sql, array<'a>) => promise<unit>
+  external eval: string => 'a = "eval"
+
+  let makeBatchSetFnString = (table: table) => {
+    let fieldNamesInQuotes =
+      table->getNonDefaultFieldNames->Array.map(fieldName => `"${fieldName}"`)
+    `(sql, rows) => {
+      return sql\`
+        INSERT INTO "public"."${table.tableName}"
+        \${sql(rows, ${fieldNamesInQuotes->Js.Array2.joinWith(", ")})}
+        ON CONFLICT(${table->getPrimaryKeyFieldNames->Js.Array2.joinWith(", ")}) DO UPDATE
+        SET
+        ${fieldNamesInQuotes
+      ->Array.map(fieldNameInQuotes => `${fieldNameInQuotes} = EXCLUDED.${fieldNameInQuotes}`)
+      ->Js.Array2.joinWith(", ")};\`
+    }`
+  }
+
+  let chunkBatchQuery = async (
+    sql,
+    entityDataArray: array<'entity>,
+    queryToExecute: pgFn<array<'entity>, 'return>,
+    ~maxItemsPerQuery=500,
+  ) => {
+    let responses = []
+    let i = ref(0)
+    let shouldContinue = () => i.contents < entityDataArray->Array.length
+    // Split entityDataArray into chunks of maxItemsPerQuery
+    while shouldContinue() {
+      let chunk =
+        entityDataArray->Js.Array2.slice(~start=i.contents, ~end_=i.contents + maxItemsPerQuery)
+      let response = await queryToExecute(sql, chunk)
+      responses->Js.Array2.push(response)->ignore
+      i := i.contents + maxItemsPerQuery
+    }
+    responses
+  }
+
+  let makeBatchSetFn = (~table, ~rowsSchema: S.t<array<'a>>): batchSetFn<'a> => {
+    let batchSetFn: pgFn<array<Js.Json.t>, unit> = table->makeBatchSetFnString->eval
+    async (sql, rows) => {
+      let rowsJson =
+        rows->S.serializeOrRaiseWith(rowsSchema)->(Utils.magic: Js.Json.t => array<Js.Json.t>)
+      let _res = await chunkBatchQuery(sql, rowsJson, batchSetFn)
+    }
+  }
+}


This makeBatchSetFn, uses javascript eval (like rescript schema) for compiling a batch set sql query for the given table at startup time.

You can see the tests I've written for it. I've only applied it to dynamic contract registry table so far. But it's really helpful to be able to just change the table in the rescript file and the queries become derived from that. Minimizes chance of bugs creeping in when you add/remove/change fields here and don't update the raw js file with the sql query correctly.

Interesting. It definitely feels hacky using eval, but I definitely get you that since it is unified with the structure here in rescript and the types are generated dynamically, it's not as risky as the break in interface between rescripts and js that previously there was.

Is there any movement in the Rescript community of binding to these kinds of template literals like `sql`` directly?

Hmm, I'm thinking it might be better still to generate these functions at codegen time rather than with eval in this case. Is the primary motivation that you want everything to just be a library and you just set some parameters once on this JavaScript library and everything gets generated in code rather than codegen step?

Is there any movement in the Rescript community of binding to these kinds of template literals like `sql`` directly?

There already is an implementation but it only works for if you can bind directly to the the template literal function. In this case we instantiate sql first and only then it can be used as an object literal.

There's a few more problems that eval solves here rather than just templating a query though.

If you like in the DbFunctionsImplementation.js, I have already found a way to generalize queries using just the library and this is currently working for all codegen entities. These problem with it is, it's super fragile code, really difficult to read or reason about and all they dynamicness is offloaded to the runtime of the function so there is some runtime overhead. With eval, we essentially JIT compile a static function with no extra runtime overhead. The resulting query to postgres is much simpler and the js function becomes more testable.

Hmm, I'm thinking it might be better still to generate these functions at codegen time rather than with eval in this case.

I would disagree that it is better to codegen these things. This is exactly what we used to do with all the entity insert functions etc at codegen time. One of the problems is that there is no relationship between hardcoded codegen values and the frameworks being built. So for eg. say I make an edit to the Table.t type and how it gets used. Those changes don't trickle down to the db interop without changing hardcoded fields. Say for instance I change the getFields function on the table to filter out defaulted fields or something. Now I need to rewrite that logic in the codegenerator and there's no relationship to the framework.

Is the primary motivation that you want everything to just be a library and you just set some parameters once on this JavaScript library and everything gets generated in code rather than codegen step?

I would say yes, the main motivation is that we are trying to move away from leaning on codegen and make the whole indexer more lib like. But the codegen still has the ultimate responsibility of generating types and apis while everything in the lib should tend towards static reusable code.

Cool sounds good. I was just poking a little bit to understand better. 👍

JonoPrest · 2024-10-11T14:50:00Z

+  let chunkBatchQuery = async (
+    sql,
+    entityDataArray: array<'entity>,
+    queryToExecute: pgFn<array<'entity>, 'return>,
+    ~maxItemsPerQuery=500,
+  ) => {
+    let responses = []
+    let i = ref(0)
+    let shouldContinue = () => i.contents < entityDataArray->Array.length
+    // Split entityDataArray into chunks of maxItemsPerQuery
+    while shouldContinue() {
+      let chunk =
+        entityDataArray->Js.Array2.slice(~start=i.contents, ~end_=i.contents + maxItemsPerQuery)
+      let response = await queryToExecute(sql, chunk)
+      responses->Js.Array2.push(response)->ignore
+      i := i.contents + maxItemsPerQuery
+    }
+    responses
+  }


This function is just a rescript reimplementation of the chunk function used in the js file.

JonoPrest · 2024-10-11T14:51:24Z

+      {
+        //Test the preRegistration restart function getting all the dynamic contracts
+        let setRegisterPreRegistration: (
+          Types.HandlerTypes.Register.t<'a>,
+          bool,
+        ) => unit = %raw(`(register, bool)=> {
+          if (!register.eventOptions) {
+            register.eventOptions = {};
+          } 
+          register.eventOptions.shouldPreRegisterDynamicContracts=bool;
+        }`)
+
+        Types.ERC20Factory.TokenCreated.handlerRegister->setRegisterPreRegistration(true)
+
+        let restartedChainFetcher = await ChainFetcher.makeFromDbState(
+          chainConfig,
+          ~maxAddrInPartition=Env.maxAddrInPartition,
+        )
+
+        let restartedFetchState =
+          restartedChainFetcher.fetchState.partitions->List.head->Option.getExn
+
+        let dynamicContracts =
+          restartedFetchState.dynamicContracts
+          ->Belt.Map.valuesToArray
+          ->Array.flatMap(set => set->Belt.Set.String.toArray)
+
+        Assert.deepEqual(
+          [Mock.mockDyamicToken1->Address.toString, Mock.mockDyamicToken2->Address.toString],
+          restartedChainFetcher.dynamicContractPreRegistration->Option.getExn->Js.Dict.keys,
+          ~message="Should return all the dynamic contracts related to handler that uses preRegistration",
+        )
+
+        Assert.deepEqual(
+          [],
+          dynamicContracts,
+          ~message="Should have no dynamic contracts yet since this tests the case starting in preregistration",
+        )
+
+        Types.ERC20Factory.TokenCreated.handlerRegister->setRegisterPreRegistration(false)
+      }
+


Added a test in the middle of this one that sets the preRegister config to true and back to false in the middle to test the restart state in the case of an indexer using pre registration

JasoonS

I didn't really find anything wrong logic-wise, but tried to review as in-depth as I could.

I still think we should use the same variable name for the Boolean of pre-registering.

JasoonS · 2024-10-15T07:09:58Z

    "dynamic_contract_registry",
    ~fields=[
      mkField("chain_id", Integer, ~isPrimaryKey),
-      mkField("event_id", Numeric),


Cool, it makes sense for these to be explicit.

JasoonS · 2024-10-15T07:22:36Z

+module PostgresInterop = {
+  type pgFn<'payload, 'return> = (Postgres.sql, 'payload) => promise<'return>
+  type batchSetFn<'a> = (Postgres.sql, array<'a>) => promise<unit>
+  external eval: string => 'a = "eval"
+
+  let makeBatchSetFnString = (table: table) => {
+    let fieldNamesInQuotes =
+      table->getNonDefaultFieldNames->Array.map(fieldName => `"${fieldName}"`)
+    `(sql, rows) => {
+      return sql\`
+        INSERT INTO "public"."${table.tableName}"
+        \${sql(rows, ${fieldNamesInQuotes->Js.Array2.joinWith(", ")})}
+        ON CONFLICT(${table->getPrimaryKeyFieldNames->Js.Array2.joinWith(", ")}) DO UPDATE
+        SET
+        ${fieldNamesInQuotes
+      ->Array.map(fieldNameInQuotes => `${fieldNameInQuotes} = EXCLUDED.${fieldNameInQuotes}`)
+      ->Js.Array2.joinWith(", ")};\`
+    }`
+  }
+
+  let chunkBatchQuery = async (
+    sql,
+    entityDataArray: array<'entity>,
+    queryToExecute: pgFn<array<'entity>, 'return>,
+    ~maxItemsPerQuery=500,
+  ) => {
+    let responses = []
+    let i = ref(0)
+    let shouldContinue = () => i.contents < entityDataArray->Array.length
+    // Split entityDataArray into chunks of maxItemsPerQuery
+    while shouldContinue() {
+      let chunk =
+        entityDataArray->Js.Array2.slice(~start=i.contents, ~end_=i.contents + maxItemsPerQuery)
+      let response = await queryToExecute(sql, chunk)
+      responses->Js.Array2.push(response)->ignore
+      i := i.contents + maxItemsPerQuery
+    }
+    responses
+  }
+
+  let makeBatchSetFn = (~table, ~rowsSchema: S.t<array<'a>>): batchSetFn<'a> => {
+    let batchSetFn: pgFn<array<Js.Json.t>, unit> = table->makeBatchSetFnString->eval
+    async (sql, rows) => {
+      let rowsJson =
+        rows->S.serializeOrRaiseWith(rowsSchema)->(Utils.magic: Js.Json.t => array<Js.Json.t>)
+      let _res = await chunkBatchQuery(sql, rowsJson, batchSetFn)
+    }
+  }
+}


Interesting. It definitely feels hacky using eval, but I definitely get you that since it is unified with the structure here in rescript and the types are generated dynamically, it's not as risky as the break in interface between rescripts and js that previously there was.

JasoonS · 2024-10-15T07:25:24Z

+module PostgresInterop = {
+  type pgFn<'payload, 'return> = (Postgres.sql, 'payload) => promise<'return>
+  type batchSetFn<'a> = (Postgres.sql, array<'a>) => promise<unit>
+  external eval: string => 'a = "eval"
+
+  let makeBatchSetFnString = (table: table) => {
+    let fieldNamesInQuotes =
+      table->getNonDefaultFieldNames->Array.map(fieldName => `"${fieldName}"`)
+    `(sql, rows) => {
+      return sql\`
+        INSERT INTO "public"."${table.tableName}"
+        \${sql(rows, ${fieldNamesInQuotes->Js.Array2.joinWith(", ")})}
+        ON CONFLICT(${table->getPrimaryKeyFieldNames->Js.Array2.joinWith(", ")}) DO UPDATE
+        SET
+        ${fieldNamesInQuotes
+      ->Array.map(fieldNameInQuotes => `${fieldNameInQuotes} = EXCLUDED.${fieldNameInQuotes}`)
+      ->Js.Array2.joinWith(", ")};\`
+    }`
+  }
+
+  let chunkBatchQuery = async (
+    sql,
+    entityDataArray: array<'entity>,
+    queryToExecute: pgFn<array<'entity>, 'return>,
+    ~maxItemsPerQuery=500,
+  ) => {
+    let responses = []
+    let i = ref(0)
+    let shouldContinue = () => i.contents < entityDataArray->Array.length
+    // Split entityDataArray into chunks of maxItemsPerQuery
+    while shouldContinue() {
+      let chunk =
+        entityDataArray->Js.Array2.slice(~start=i.contents, ~end_=i.contents + maxItemsPerQuery)
+      let response = await queryToExecute(sql, chunk)
+      responses->Js.Array2.push(response)->ignore
+      i := i.contents + maxItemsPerQuery
+    }
+    responses
+  }
+
+  let makeBatchSetFn = (~table, ~rowsSchema: S.t<array<'a>>): batchSetFn<'a> => {
+    let batchSetFn: pgFn<array<Js.Json.t>, unit> = table->makeBatchSetFnString->eval
+    async (sql, rows) => {
+      let rowsJson =
+        rows->S.serializeOrRaiseWith(rowsSchema)->(Utils.magic: Js.Json.t => array<Js.Json.t>)
+      let _res = await chunkBatchQuery(sql, rowsJson, batchSetFn)
+    }
+  }
+}


Is there any movement in the Rescript community of binding to these kinds of template literals like `sql`` directly?

JasoonS · 2024-10-15T07:31:27Z

+module PostgresInterop = {
+  type pgFn<'payload, 'return> = (Postgres.sql, 'payload) => promise<'return>
+  type batchSetFn<'a> = (Postgres.sql, array<'a>) => promise<unit>
+  external eval: string => 'a = "eval"
+
+  let makeBatchSetFnString = (table: table) => {
+    let fieldNamesInQuotes =
+      table->getNonDefaultFieldNames->Array.map(fieldName => `"${fieldName}"`)
+    `(sql, rows) => {
+      return sql\`
+        INSERT INTO "public"."${table.tableName}"
+        \${sql(rows, ${fieldNamesInQuotes->Js.Array2.joinWith(", ")})}
+        ON CONFLICT(${table->getPrimaryKeyFieldNames->Js.Array2.joinWith(", ")}) DO UPDATE
+        SET
+        ${fieldNamesInQuotes
+      ->Array.map(fieldNameInQuotes => `${fieldNameInQuotes} = EXCLUDED.${fieldNameInQuotes}`)
+      ->Js.Array2.joinWith(", ")};\`
+    }`
+  }
+
+  let chunkBatchQuery = async (
+    sql,
+    entityDataArray: array<'entity>,
+    queryToExecute: pgFn<array<'entity>, 'return>,
+    ~maxItemsPerQuery=500,
+  ) => {
+    let responses = []
+    let i = ref(0)
+    let shouldContinue = () => i.contents < entityDataArray->Array.length
+    // Split entityDataArray into chunks of maxItemsPerQuery
+    while shouldContinue() {
+      let chunk =
+        entityDataArray->Js.Array2.slice(~start=i.contents, ~end_=i.contents + maxItemsPerQuery)
+      let response = await queryToExecute(sql, chunk)
+      responses->Js.Array2.push(response)->ignore
+      i := i.contents + maxItemsPerQuery
+    }
+    responses
+  }
+
+  let makeBatchSetFn = (~table, ~rowsSchema: S.t<array<'a>>): batchSetFn<'a> => {
+    let batchSetFn: pgFn<array<Js.Json.t>, unit> = table->makeBatchSetFnString->eval
+    async (sql, rows) => {
+      let rowsJson =
+        rows->S.serializeOrRaiseWith(rowsSchema)->(Utils.magic: Js.Json.t => array<Js.Json.t>)
+      let _res = await chunkBatchQuery(sql, rowsJson, batchSetFn)
+    }
+  }
+}


Hmm, I'm thinking it might be better still to generate these functions at codegen time rather than with eval in this case. Is the primary motivation that you want everything to just be a library and you just set some parameters once on this JavaScript library and everything gets generated in code rather than codegen step?

JasoonS · 2024-10-15T07:39:19Z


  let chainMetadata = await DbFunctions.ChainMetadata.getLatestChainMetadataState(~chainId)

+  let shouldPreRegisterDynamicContracts = chainConfig->Config.shouldPreRegisterDynamicContracts


Suggested change

let shouldPreRegisterDynamicContracts = chainConfig->Config.shouldPreRegisterDynamicContracts

let preRegisterDynamicContracts = chainConfig->Config.shouldPreRegisterDynamicContracts

I still think the name should be consistent codebase wide (for users and inside codegen).

The function in config can keep the 'should'

I just don't see how removing the prefix is an improvement. The type and the variables purpose is plain as day as is.

The improvement is that it is clear that it is consistent from the user's handler file into our code and if anyone who looks at the generated code sees this variable, they know it's the same as the boolean they defined in their contract register.

What is the purpose of having 'should'? My argument is if there's value in having the word 'should', then users should also have that same variable name.

JasoonS · 2024-10-15T07:44:30Z

+  @module("./DbFunctionsImplementation.js")
+  external readDynamicContractsOnChainIdMatchingEventsRaw: (
+    Postgres.sql,
+    ~chainId: int,
+    ~preRegisteringEvents: array<preRegisteringEvent>,
+  ) => promise<Js.Json.t> = "readDynamicContractsOnChainIdMatchingEvents"


I guess the plan is to eventually move even read queries to what you did with the batchSet of the dynamic contract registry?

Maybe, but probably not for dedicated functions that aren't shared between tables. It would be nice to extend the Table module to have query builders that can do a bit more startup validation etc. But first it will be to try and trim down the hardcoded sql functions to a small subset that's easier to manage.

JasoonS · 2024-10-15T07:46:54Z

+      mkField("registering_event_block_timestamp", Integer),
+      mkField("registering_event_contract_name", Text),
+      mkField("registering_event_name", Text),
+      mkField("registering_event_src_address", Text),


btw, Has there been any investigation to use bytes instead of strings for addresses (in general - not here specifically)?

No I haven't yet but reckon that would be a good improvement 👍🏼, just not sure how to make hasura send "0x" prefix with string hexidecimal when returning the values.

JasoonS · 2024-10-15T07:51:35Z

+    ) {
+    | exception exn =>
+      exn->ErrorHandling.mkLogAndRaise(
+        ~msg="Failed to read dynamic contracts on chain id matching events",


This could be more detailed:

Suggested change

~msg="Failed to read dynamic contracts on chain id matching events",

~msg=`Failed to read dynamic contracts on chain id ${chainId} matching events`

Or even (not exact code):

Suggested change

~msg="Failed to read dynamic contracts on chain id matching events",

~msg=`Failed to read dynamic contracts on chain id ${chainId} matching the following events: ${preRegisteringEvents->map(event=>event.name /* or give full details */)}`

Actually can't we just pass a ~logger with an object of the additional context to the logger?

Can do 👍🏼

JasoonS · 2024-10-15T08:03:21Z

+          register.eventOptions.shouldPreRegisterDynamicContracts=bool;
+        }`)
+
+        Types.ERC20Factory.TokenCreated.handlerRegister->setRegisterPreRegistration(true)


Would calling this here not possibly affect the other tests below this? Would it not be better to isolate this into a separate small test so logic affect other test by mistake?

It doesn't affect the test below because the handlers aren't using pre registrations. It was the fastest way I could think of writing a test. I can try improve it but since it's a global value mutating it from any test could cause problems. Maybe the best thing todo is have a reset callback that sets it back to whatever it was before.

You can see I reset it back to false after the assertions

…ts internally

* Update dynamic contract registry table and operations * Add tests for eval function * Handle default fields on tables in set fn * Add registering_event_contract_name field to registry * Add alternative query for fetching pre registered dynamic contracts on restart * Add chain id context to error log * Rename shouldPreRegisterDynamicContracts to preRegisterDynamicContracts internally * Fix test and make sure to reset eventOptions

JonoPrest added 5 commits October 11, 2024 14:02

Update dynamic contract registry table and operations

ff47802

Add tests for eval function

82cd7e2

Handle default fields on tables in set fn

29c7c21

Add registering_event_contract_name field to registry

e4b6d64

Add alternative query for fetching pre registered dynamic contracts o…

87c38b5

…n restart

JonoPrest requested a review from JasoonS October 11, 2024 14:44

JonoPrest commented Oct 11, 2024

View reviewed changes

JasoonS approved these changes Oct 15, 2024

View reviewed changes

JonoPrest added 3 commits October 15, 2024 11:53

Add chain id context to error log

d9e0e08

Rename shouldPreRegisterDynamicContracts to preRegisterDynamicContrac…

aa6e499

…ts internally

Fix test and make sure to reset eventOptions

7fe9970

JonoPrest merged commit 3ff4f9e into main Oct 15, 2024

JonoPrest deleted the jp/dyn-contract-improvements branch October 15, 2024 11:10


		let chainMetadata = await DbFunctions.ChainMetadata.getLatestChainMetadataState(~chainId)

		let shouldPreRegisterDynamicContracts = chainConfig->Config.shouldPreRegisterDynamicContracts

	let shouldPreRegisterDynamicContracts = chainConfig->Config.shouldPreRegisterDynamicContracts
	let preRegisterDynamicContracts = chainConfig->Config.shouldPreRegisterDynamicContracts

	~msg="Failed to read dynamic contracts on chain id matching events",
	~msg=`Failed to read dynamic contracts on chain id ${chainId} matching events`

Uh oh!

Conversation

JonoPrest commented Oct 11, 2024

Uh oh!

JonoPrest Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

JasoonS 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JonoPrest Oct 11, 2024 •

edited

Loading