Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add event logs to stf and db #409

Merged
merged 27 commits into from
Aug 23, 2024
Merged

Conversation

ecioppettini
Copy link
Contributor

@ecioppettini ecioppettini commented Aug 6, 2024

Implements parts of #390

There some missing parts still though:

  • Emitting the mqtt events.
  • Maybe expose some helper to use at the client side (although we may have this, I need to check)
  • I'm not sure the current indexing make sense. We may want a composite index instead and always ask for all the indexed fields? But it's less versatile.
  • The current rest interface has the issue that only 10 fields can be filtered. That said, I'm not sure how to this while leveraging pgtyped and checked arguments.
  • Also I imagine we want some block range size limitation.

@ecioppettini ecioppettini self-assigned this Aug 6, 2024
@ecioppettini ecioppettini force-pushed the enzo/add-event-logs-to-stf-and-db branch 2 times, most recently from 9f913ac to b569c27 Compare August 14, 2024 08:59
@ecioppettini ecioppettini force-pushed the enzo/add-event-logs-to-stf-and-db branch from 635f2ba to 6d815f5 Compare August 15, 2024 09:18
@ecioppettini ecioppettini marked this pull request as ready for review August 15, 2024 09:20
packages/engine/paima-runtime/tsconfig.json Show resolved Hide resolved
packages/engine/paima-sm/src/index.ts Outdated Show resolved Hide resolved
packages/engine/paima-standalone/src/utils/import.ts Outdated Show resolved Hide resolved
packages/node-sdk/paima-db/src/sql/events.sql Outdated Show resolved Hide resolved
packages/paima-sdk/paima-events/src/app-events.ts Outdated Show resolved Hide resolved
packages/paima-sdk/paima-utils/src/config.ts Show resolved Hide resolved
eventDescriptions: { topic: string; fieldName: string; indexed: boolean }[]
): Promise<boolean> {
for (const event of eventDescriptions) {
const indexName = `index_${event.topic.slice(0, 20)}_${event.fieldName}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: document this in the docs (one index per field)

@@ -753,6 +753,61 @@ const TABLE_DATA_CDE_DYNAMIC_PRIMITIVE_CONFIG: TableData = {
creationQuery: QUERY_CREATE_TABLE_CDE_DYNAMIC_PRIMITIVE_CONFIG,
};

const QUERY_CREATE_TABLE_EVENT = `
CREATE TABLE event (
id SERIAL PRIMARY KEY,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't use serial fields here. The STF will eventually process inputs in parallel, making the order non-deterministic

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can probably change the pk to block height + index inside the block instead, but I'm not sure it would make any difference. Can you explain what does it mean to process inputs in parallel? Because even then you could call the stf multiple times in parallel and then serialize the results.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having the ID be deterministic even in the case where transactions are executed in parallel is equivalent to the other open discussion on this PR: #409 (comment)

// eslint-disable-next-line @typescript-eslint/no-explicit-any
data: { [fieldName: string]: any };
tx: number;
idx: number;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably not clear from the name that this is meant to represent the order in which this log was executed within the transaction

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed it to logIndex. Only caveat is that if we wrap this to implement eth_getLogs, we need to remember that this is the index of the log in the tx, and the other one is the index of the log in the block. I guess we may be able to compute the index of the log out of the tx index and the log index though?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe we may want to change this field to be the log index in the block to be consistent with that? I just used the index in the tx because it's what was described in #373

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah good catch. I feel it should probably by the index in the block instead of in the transaction then

Copy link
Contributor

@SebastienGllmt SebastienGllmt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving modulo the logIndex comment

@SebastienGllmt SebastienGllmt merged commit 72bed85 into master Aug 23, 2024
@SebastienGllmt SebastienGllmt deleted the enzo/add-event-logs-to-stf-and-db branch August 23, 2024 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants