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

accounts + native transfers #1210

Open
wants to merge 55 commits into
base: v0.8
Choose a base branch
from
Open

accounts + native transfers #1210

wants to merge 55 commits into from

Conversation

kyscott18
Copy link
Collaborator

@kyscott18 kyscott18 commented Nov 1, 2024

Closes #1157 #1198 #1205

Tasks

sync

  • debug rpc types
  • TransactionFilter type
  • TransferFilter type
  • TraceFilter type
  • _debug_traceBlockByNumber + _debug_traceBlockByHash
  • TransactionFilter fragments
  • TransferFilter fragments
  • TraceFilter fragments
  • isTransactionFilterMatched
  • isTransferFilterMatched
  • isTraceFilterMatched
  • create traces table, drop callTraces
  • decodeEvents
  • buildEvents
  • getEvents

sync-historical

  • syncTrace
  • syncTransaction
  • order traces checkpoint.eventIndex
  • determine failed traces
  • detect inconsistent traces
  • factory support
  • avoid refetching with traceCache

sync-realtime

  • fetchBlockEventData
  • handleBlock
  • factory support

API

  • AccountSource
  • ponder.config.ts
  • buildConfigAndIndexingFunctions

@khaidarkairbek khaidarkairbek self-assigned this Nov 3, 2024
Comment on lines 722 to 725
const blocks = Array.from(
{ length: interval[1] - interval[0] + 1 },
(_, i) => interval[0] + i,
);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would feel a little better about this if we just used an array and a for loop

Comment on lines 422 to 405
if (block.transactions.find((t) => t.hash === hash) === undefined) {
throw new Error(
`Detected inconsistent RPC responses. 'trace.blockHash' ${callTrace.blockHash} does not match 'block.hash' ${block.hash}`,
`Detected inconsistent RPC responses. 'trace.transactionHash' ${hash} not found in 'block.transactions' ${block.hash}`,
);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nice

Comment on lines 399 to 401
let traces = await _debug_traceBlockByNumber(args.requestQueue, {
blockNumber: toHex(blockNumber),
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When I was evaluating the debug rpc methods, I notice that traceBlockByHash was often way faster than traceBlockByNumber. It would require a different algorithm because block hash isn't immediately available, but something we should try before shipping

Comment on lines 304 to 306
const syncTraces = await _debug_traceBlockByNumber(args.requestQueue, {
blockNumber: toHex(blockNumber),
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should start to think about how we can avoid requesting the same block twice if there are multiple transfer and transaction filters

// });

// Remove traces that dont match transaction or transfer filter
traces = traces.filter((trace) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we need to do a depth first search over all the traces, either in this block or above where traces is declared.

It performs three functions:

  • processes child traces (each call can have several sub-calls)
  • orders traces for checkpoint.eventIndex
  • determine failed traces

See sync-historical

const dfs = (
syncTrace: SyncTrace["result"][],
transactionHash: Hex,
parentTrace: SyncTrace["result"] | undefined,
) => {
for (const trace of syncTrace) {
if (filter.type === "transfer") {
if (
isTransferFilterMatched({
filter,
block: { number: toHex(blockNumber) },
trace,
})
) {
// TODO(kyle) handle factory
traces.push({ trace, transactionHash, position });
}
} else {
if (
isTransactionFilterMatched({
filter,
block: { number: toHex(blockNumber) },
trace,
})
) {
// TODO(kyle) handle factory
traces.push({ trace, transactionHash, position });
}
}
if (trace.error !== undefined) {
failedTraces.set(trace, trace.error);
} else if (parentTrace && failedTraces.has(parentTrace)) {
failedTraces.set(trace, failedTraces.get(parentTrace)!);
}
position++;
if (trace.calls) {
dfs(trace.calls, transactionHash, trace);
}
}
};


return {
block,
logs,
factoryLogs,
callTraces,
traces,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should this type be the same as ??

const traces: {
trace: SyncTrace["result"];
transactionHash: Hex;
position: number;
}[] = [];

if (shouldRequestTraces) {
const traces = await _trace_block(args.requestQueue, {
traces = await _debug_traceBlockByNumber(args.requestQueue, {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should use _debug_traceBlockByHash. It's more robust when reorgs occur because it unambiguously refers to the exact block

Base automatically changed from v0.7 to main November 8, 2024 17:52
}
return isMatched;
});

transactions = transactions.filter((t) => transactionHashes.has(t.hash));
transactionReceipts = transactionReceipts.filter((t) =>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should revisit this once we implement the includeTransactionReceipt feature

Comment on lines 192 to 197
if (
filter.includeReverted === false &&
(trace.revertReason !== undefined || trace.error !== undefined)
) {
return false;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not super clear, this function returns true for the case "should this trace be saved in the database" and not "does this trace exactly match the filter such that it will cause an indexing function to run". The reason for the distinction is that we don't want the cache to be completely useless when the filter type slightly changes. For that reason this check should be removed.

In this example, if the user decides that they want to include reverted traces after they have already fully synced their app, every block will need to be resynced.

Maybe we could add some comments acknowledging the properties that are ignored.

packages/core/src/sync-realtime/filter.ts Outdated Show resolved Hide resolved
@kevinfaveri
Copy link

Hello guys; I have a pressing timeline on getting something for native transfers and would like to explore using this here... Do you think it will be ready within next 1/2 weeks? Any timeline in mind?

Thank you in advance.

@kyscott18
Copy link
Collaborator Author

Hello guys; I have a pressing timeline on getting something for native transfers and would like to explore using this here... Do you think it will be ready within next 1/2 weeks? Any timeline in mind?

We're hoping to get a prerelease out in the next few days, but don't want to make any promises on when we will release v0.8.

@kyscott18 kyscott18 marked this pull request as ready for review November 20, 2024 01:37
Comment on lines 336 to 344
isTransactionFilterMatched({
filter: {
...filter,
fromAddress: fromAddress,
toAddress: toAddress,
},
block,
transaction,
})
Copy link
Collaborator Author

@kyscott18 kyscott18 Nov 20, 2024

Choose a reason for hiding this comment

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

I like this, do you think we could get around overriding filter.fromAddress by adding an optional parameter fromChildAddresses: Set<Address>? Then we could reuse this in sync-realtime as well

@@ -173,6 +208,36 @@ export const createRealtimeSync = (
}
}

const isFactoryAddressMatched = ({
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could get rid of this if we update is[type]FilterMatched to handle factories

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.

feat: use debug namespace for inner transactions feat: filter types [Feature] Native transfers
3 participants