feat: add internalTransactions table / api#543
Conversation
API E2E Test Results185 tests 185 ✅ 18s ⏱️ Results for commit bf45a17. ♻️ This comment has been updated with latest results. |
API Prividium E2E Test Results4 tests 4 ✅ 6s ⏱️ Results for commit bf45a17. ♻️ This comment has been updated with latest results. |
Unit Test Results 4 files 261 suites 12m 17s ⏱️ Results for commit bf45a17. ♻️ This comment has been updated with latest results. |
|
Visit the preview URL for this PR: |
dutterbutter
changed the title
|
Do you plan to add a BFF endpoint for the front-end? |
I havent thought about it yet. If needed I will do so subsequently when I open a PR for UI internal tx tables. Unless you suggest otherwise? |
Ok, fine |
| public readonly number: number; | ||
|
|
||
| @ManyToOne(() => InternalTransaction, { onDelete: "CASCADE" }) | ||
| @JoinColumn([ |
There was a problem hiding this comment.
Why? Please use number from InternalTransaction, it's faster as it is PK.
| @PrimaryColumn({ generated: true, type: "bigint" }) | ||
| public readonly number: number; | ||
|
|
||
| @ManyToOne(() => InternalTransaction, { onDelete: "CASCADE" }) |
There was a problem hiding this comment.
Please use onDelete: "CASCADE only on block for consistency, we intentionally allow cascade deletion only on block as logically you can only revert an entire block, never a single transaction.
| ]) | ||
| public readonly internalTransaction: InternalTransaction; | ||
|
|
||
| @Index() |
There was a problem hiding this comment.
@Index(["transactionHash", "traceAddress"]) makes this index redundant
| @Index(["blockNumber", "traceIndex"]) | ||
| @Index(["transactionHash", "traceAddress"], { unique: true }) | ||
| @Index(["type", "blockNumber", "traceIndex"]) | ||
| @Index(["from"]) |
There was a problem hiding this comment.
I don't think we need from and to indexes
| @PrimaryColumn({ generated: true, type: "bigint" }) | ||
| public readonly number: number; | ||
|
|
||
| @ManyToOne(() => Transaction, { onDelete: "CASCADE" }) |
There was a problem hiding this comment.
| @@ -1,12 +1,15 @@ | |||
| export * from "./base.repository"; | |||
There was a problem hiding this comment.
Why exporting an abstract repository from here?
|
|
||
| private async addAddressInternalTransactions(records: Partial<InternalTransaction>[]): Promise<void> { | ||
| const addressInternalTransactions = records.flatMap((record) => { | ||
| const baseRecord = { |
There was a problem hiding this comment.
Not critical, but I'd rather exclude number and include all the other fields, typeorm just ignores the fields not in the schema, but we won't need to edit this function if we decide to add some more fields.
|
|
||
| const denormalizedRecords = []; | ||
|
|
||
| if (record.from) { |
There was a problem hiding this comment.
Why checking if from is not nullable?
| return denormalizedRecords; | ||
| }); | ||
|
|
||
| if (addressInternalTransactions.length === 0) { |
| value: BigInt(transactionTrace.value || "0x0").toString(), | ||
| gas: transactionTrace.gas ? BigInt(transactionTrace.gas).toString() : undefined, | ||
| gasUsed: transactionTrace.gasUsed ? BigInt(transactionTrace.gasUsed).toString() : undefined, | ||
| input: transactionTrace.input || "0x", |
There was a problem hiding this comment.
Why not just leave these as undefined as in the DB these fields are nullable?
| { | ||
| ...pagingOptions, | ||
| limit: pagingOptions.offset, | ||
| route: "account/txlistinternal", |
There was a problem hiding this comment.
route is not needed for this endpoint
| from: internalTx.from, | ||
| to: internalTx.to || "", | ||
| value: internalTx.value, | ||
| gas: internalTx.gas?.toString() || "", |
There was a problem hiding this comment.
Why .toString()? Isn't it already a string?
| value: internalTx.value, | ||
| gas: internalTx.gas?.toString() || "", | ||
| input: internalTx.input || "", | ||
| type: internalTx.callType || internalTx.type.toLowerCase(), |
There was a problem hiding this comment.
Isn't type always present? Also why type.toLowerCase()? Let's just store it in lowercase?
| public readonly timestamp: Date; | ||
|
|
||
| @Column({ type: "varchar", nullable: true }) | ||
| public readonly callType?: string; |
| input: transactionTrace.input || "0x", | ||
| output: transactionTrace.output || "0x", | ||
| type: traceType.toUpperCase(), | ||
| callType: traceType, |
There was a problem hiding this comment.
Isn't it supposed to be something different? Also do we really need it?
| input: internalTx.input || "", | ||
| type: internalTx.callType || internalTx.type.toLowerCase(), | ||
| contractAddress: | ||
| internalTx.type.toUpperCase() === "CREATE" || internalTx.type.toUpperCase() === "CREATE2" |
There was a problem hiding this comment.
Why is this check needed? internalTx.to is anyway undefined if type is CREATE or CREATE2
| paginationOptions: IPaginationOptions | ||
| ): Promise<Pagination<InternalTransaction>> { | ||
| if (filterOptions.address) { | ||
| const normalizedAddress = normalizeAddressTransformer.to(filterOptions.address); |
There was a problem hiding this comment.
No need for normalizing, as address is bytea
| queryBuilder.where({ address: normalizedAddress }); | ||
|
|
||
| if (filterOptions.transactionHash) { | ||
| const normalizedHash = normalizeAddressTransformer.to(filterOptions.transactionHash); |
| const isContract = addressRecord?.bytecode && addressRecord.bytecode !== "0x"; | ||
|
|
||
| if (!isContract) { | ||
| queryBuilder.andWhere("internalTransaction.value > :zero", { zero: "0" }); |
There was a problem hiding this comment.
Why not just internalTransaction.value > 0 ?
| const isContract = addressRecord?.bytecode && addressRecord.bytecode !== "0x"; | ||
|
|
||
| if (!isContract) { | ||
| queryBuilder.andWhere("internalTransaction.value > :zero", { zero: "0" }); |
There was a problem hiding this comment.
Why skipping 0 value if address is not a contract?
| } | ||
|
|
||
| const queryBuilder = this.createBaseQuery(); | ||
| // filter by transaction hash |
There was a problem hiding this comment.
Please remove these comments
| // filter by transaction hash | ||
| // filter by transaction hash | ||
| if (filterOptions.transactionHash) { | ||
| const normalizedHash = normalizeAddressTransformer.to(filterOptions.transactionHash); |
| transactionHash: normalizedHash, | ||
| }); | ||
| } | ||
| // block range filtering |
There was a problem hiding this comment.
I'd suggest to remove these type of comments. Your code is easy to read.
| return await paginate<InternalTransaction>(queryBuilder, paginationOptions); | ||
| } | ||
|
|
||
| /** |
| /** | ||
| * Find internal transactions by transaction hash | ||
| */ | ||
| public async findByTransactionHash(transactionHash: string): Promise<InternalTransaction[]> { |
There was a problem hiding this comment.
Why is this function needed if findAll covers it, especially with hardcoded page and limit?
| import { CountableEntity } from "./countable.entity"; | ||
|
|
||
| @Entity({ name: "internalTransactions" }) | ||
| @Index(["transactionHash", "traceIndex"]) |
There was a problem hiding this comment.
Let's discuss how each of these indexes is used
2 participants
What ❔
Why ❔
To view:
By address:
curl "http://localhost:3020/api?module=account&action=txlistinternal&address=0x785c0219d5e23950f88452d23113b3b680006e6a"By hash:
curl "http://localhost:3020/api?module=account&action=txlistinternal&txhash=0xfecccf9afeea8f502950fc5c697b6827161df699347604666050a8ed2a93fe68"By block range:
curl "http://localhost:3020/api?/account/txlistinternal?startblock=1000&endblock=2000&page=1&offset=100Checklist