-
Notifications
You must be signed in to change notification settings - Fork 105
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
Conversation
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}`, | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
let traces = await _debug_traceBlockByNumber(args.requestQueue, { | ||
blockNumber: toHex(blockNumber), | ||
}); |
There was a problem hiding this comment.
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
const syncTraces = await _debug_traceBlockByNumber(args.requestQueue, { | ||
blockNumber: toHex(blockNumber), | ||
}); |
There was a problem hiding this comment.
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) => { |
There was a problem hiding this comment.
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
ponder/packages/core/src/sync-historical/index.ts
Lines 324 to 368 in 2117cec
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, |
There was a problem hiding this comment.
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 ??
ponder/packages/core/src/sync-historical/index.ts
Lines 313 to 317 in 2117cec
const traces: { | |
trace: SyncTrace["result"]; | |
transactionHash: Hex; | |
position: number; | |
}[] = []; |
if (shouldRequestTraces) { | ||
const traces = await _trace_block(args.requestQueue, { | ||
traces = await _debug_traceBlockByNumber(args.requestQueue, { |
There was a problem hiding this comment.
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
isTransactionFilterMatched({ | ||
filter: { | ||
...filter, | ||
fromAddress: fromAddress, | ||
toAddress: toAddress, | ||
}, | ||
block, | ||
transaction, | ||
}) |
There was a problem hiding this comment.
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 = ({ |
There was a problem hiding this comment.
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
@@ -27,6 +32,36 @@ export type IndexingFunctions = { | |||
[eventName: string]: (...args: any) => any; | |||
}; | |||
|
|||
const flattenSource = < |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flattenSources
?
|
||
if (!sourceEventName) { | ||
throw new Error( | ||
`Validation failed: Invalid event '${eventName}', expected format '{sourceName}:{eventName}' or '{sourceName}.{eventName}'.`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or '{sourceName}.{functionName}'
?
...(config.blocks ?? {}), | ||
}).find((_sourceName) => _sourceName === sourceName); | ||
|
||
if (!matchedSourceName) { | ||
// Multi-network has N sources, but the hint here should not have duplicates. | ||
const uniqueSourceNames = dedupe( | ||
Object.keys({ ...(config.contracts ?? {}), ...(config.blocks ?? {}) }), | ||
Object.keys({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you already have a sourceNames
set in scope here from above, could use that
const result = await executeLog(indexingService, { event }); | ||
if (result.status !== "success") { | ||
return result; | ||
const toErrorMeta = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this should be a static function that doesn't rely on the event
object being in scope (accepts it as an arg). I think as-is this function will get reinitialized inside the hot loop.
@@ -455,9 +428,9 @@ const executeSetup = async ( | |||
return { status: "success" }; | |||
}; | |||
|
|||
const executeLog = async ( | |||
const executeEvent = async ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice simplifications to this file
.addColumn("to", "varchar(42)") | ||
.addColumn("gas", "numeric(78, 0)", (col) => col.notNull()) | ||
.addColumn("gasUsed", "numeric(78, 0)", (col) => col.notNull()) | ||
.addColumn("input", "text", (col) => col.notNull()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this is not null? I've definitely seen traces with an "empty" input, but maybe it's an empty string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.addColumn("subcalls", "integer", (col) => col.notNull()) | ||
.execute(); | ||
|
||
// `getEvents` benefits from an index on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this definitely feels over-indexed, but I'm happy to leave it as-is until we tackle the sync tables overhaul.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it definitely felt like overkill but not sure which ones should be left out
packages/core/src/sync/events.ts
Outdated
case "account": { | ||
switch (source.filter.type) { | ||
case "transaction": { | ||
// TODO(kyle) what if toAddress and fromAddress are both undefined? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we know where do display {sourceName}:transaction:from
or ${sourceName}:transaction:to
if the user creates an account with { address: undefined }
. I think we should actually validate against this
/** Number of subcalls. */ | ||
subcalls: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this still count non-CALL
traces? eg
/** Number of child traces produced during the execution of this trace. */
subtraces: number;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the naming is not super clear. Trace
has a propery calls
, but the Trace
s inside calls
don't have to have type === "CALL"
let index = 0; | ||
// all traces that weren't included because the trace has an error | ||
// or the trace's parent has an error, mapped to the error string | ||
const failedTraces = new Map< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine but I think this Map will contain frames across multiple transactions within the block
@kyscott18 Hey, curious on the technical implementation here. I see you use debug RPC APIs (trace by block number). Why a simple block by number wouldn't suffice (like, by filtering msg.value > 0 as a transfer of native and using tx.origin as the "from")? I suppose it is because you want the internal calls, but what information are you missing on a native transfer that you would need the debug/trace RPC vs the normal get block by number? I can think of one transfer that Alice > Bob > Charlie would be interpreted as Alice > Charlie; but too practical effects, on that transaction, the money went from Alice to Charlie at the end of the block confirmation, right? What is the imporstance of knowing bob in the middle of the internal call? Is this for example some custom contract that would send part of a deposit from user to 2 users, which could create the Alice > Bob > Charlie situation? And then if you only consider in / out of block you wouldnt see that Bob got a cut too? |
Closes #1157 #1198 #1205
Tasks
sync
TransactionFilter
typeTransferFilter
typeTraceFilter
type_debug_traceBlockByNumber
+_debug_traceBlockByHash
TransactionFilter
fragmentsTransferFilter
fragmentsTraceFilter
fragmentsisTransactionFilterMatched
isTransferFilterMatched
isTraceFilterMatched
traces
table, dropcallTraces
decodeEvents
buildEvents
getEvents
sync-historical
syncTrace
syncTransaction
checkpoint.eventIndex
traceCache
sync-realtime
fetchBlockEventData
handleBlock
API
AccountSource
ponder.config.ts
buildConfigAndIndexingFunctions