-
Notifications
You must be signed in to change notification settings - Fork 493
Add logic for writing signatures to db inside SourcifyDatabaseService
#2357
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
Changes from all commits
4b327c1
fd4bd64
9be3c13
3702f3b
6bfbf5f
c4e7116
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ import { | |
| Field, | ||
| FIELDS_TO_STORED_PROPERTIES, | ||
| StoredProperties, | ||
| Tables, | ||
| } from "../utils/database-util"; | ||
| import { | ||
| ContractData, | ||
|
|
@@ -28,6 +29,7 @@ import { | |
| VerificationJob, | ||
| Match, | ||
| VerificationJobId, | ||
| BytesKeccak, | ||
| } from "../../types"; | ||
| import Path from "path"; | ||
| import { | ||
|
|
@@ -37,6 +39,7 @@ import { | |
| toMatchLevel, | ||
| } from "../utils/util"; | ||
| import { getAddress, id as keccak256Str } from "ethers"; | ||
| import { extractSignaturesFromAbi } from "../utils/signature-util"; | ||
| import { BadRequestError } from "../../../common/errors"; | ||
| import { RWStorageIdentifiers } from "./identifiers"; | ||
| import semver from "semver"; | ||
|
|
@@ -884,6 +887,57 @@ export class SourcifyDatabaseService | |
| }); | ||
| } | ||
|
|
||
| private async storeSignatures( | ||
| poolClient: PoolClient, | ||
| verifiedContractId: Tables.VerifiedContract["id"], | ||
| verification: VerificationExport, | ||
| ): Promise<void> { | ||
| try { | ||
| const compiledContractResult = | ||
| await this.database.getCompilationIdForVerifiedContract( | ||
| verifiedContractId, | ||
| poolClient, | ||
| ); | ||
| if (compiledContractResult.rowCount === 0) { | ||
| throw new Error( | ||
| `No compilation found for verifiedContractId ${verifiedContractId}`, | ||
| ); | ||
| } | ||
| const compilationId = compiledContractResult.rows[0].compilation_id; | ||
|
|
||
| const abi = verification.compilation.contractCompilerOutput.abi; | ||
| if (!abi) { | ||
| throw new Error("No ABI found in compilation output"); | ||
| } | ||
|
|
||
| const signatureData = extractSignaturesFromAbi(abi); | ||
| const signatureColumns = signatureData.map((sig) => ({ | ||
| signature_hash_32: bytesFromString<BytesKeccak>(sig.signatureHash32), | ||
| signature: sig.signature, | ||
| signature_type: sig.signatureType, | ||
| })); | ||
|
|
||
| await this.database.insertSignatures(signatureColumns, poolClient); | ||
| await this.database.insertCompiledContractSignatures( | ||
| compilationId, | ||
|
Comment on lines
+914
to
+922
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Small thing but we are passing the same I don't have a strong opinion here but just wanted to point out.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I did this on purpose to keep the code shorter. IMO, when you accept an object typed via an interface, you should also expect the object to possibly have more properties, because TypeScript doesn't guarantee the absence of other properties ("duck typing"). |
||
| signatureColumns, | ||
| poolClient, | ||
| ); | ||
|
|
||
| logger.info("Stored signatures to SourcifyDatabase", { | ||
| verifiedContractId, | ||
| compilationId, | ||
| signatureCount: signatureData.length, | ||
| }); | ||
| } catch (error) { | ||
| // Don't throw on errors, the job should not fail | ||
| logger.error("Error storing signatures", { | ||
| verifiedContractId, | ||
| error: error, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // Override this method to include the SourcifyMatch | ||
| async storeVerificationWithPoolClient( | ||
| poolClient: PoolClient, | ||
|
|
@@ -892,7 +946,7 @@ export class SourcifyDatabaseService | |
| verificationId: VerificationJobId; | ||
| finishTime: Date; | ||
| }, | ||
| ): Promise<void> { | ||
| ): Promise<{ verifiedContractId: Tables.VerifiedContract["id"] }> { | ||
| try { | ||
| const { type, verifiedContractId, oldVerifiedContractId } = | ||
| await super.insertOrUpdateVerification(verification, poolClient); | ||
|
|
@@ -962,6 +1016,8 @@ export class SourcifyDatabaseService | |
| poolClient, | ||
| ); | ||
| } | ||
|
|
||
| return { verifiedContractId: verifiedContractId }; | ||
| } catch (error: any) { | ||
| logger.error("Error storing verification", { | ||
| error: error, | ||
|
|
@@ -970,19 +1026,30 @@ export class SourcifyDatabaseService | |
| } | ||
| } | ||
|
|
||
| // Override this method to include the SourcifyMatch | ||
| async storeVerification( | ||
| verification: VerificationExport, | ||
| jobData?: { | ||
| verificationId: VerificationJobId; | ||
| finishTime: Date; | ||
| }, | ||
| ): Promise<void> { | ||
| const { verifiedContractId } = await this.withTransaction( | ||
| async (transactionPoolClient) => { | ||
| return await this.storeVerificationWithPoolClient( | ||
| transactionPoolClient, | ||
| verification, | ||
| jobData, | ||
| ); | ||
| }, | ||
| ); | ||
|
|
||
| // Separate transaction because storing the verification should not fail | ||
| // if signatures cannot be stored | ||
| await this.withTransaction(async (transactionPoolClient) => { | ||
| await this.storeVerificationWithPoolClient( | ||
| await this.storeSignatures( | ||
| transactionPoolClient, | ||
| verifiedContractId, | ||
| verification, | ||
| jobData, | ||
| ); | ||
| }); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| import { id as keccak256str, Fragment, JsonFragment } from "ethers"; | ||
|
|
||
| export interface SignatureData { | ||
| signature: string; | ||
| signatureHash32: string; | ||
| signatureType: "function" | "event" | "error"; | ||
| } | ||
|
|
||
| export function extractSignaturesFromAbi(abi: JsonFragment[]): SignatureData[] { | ||
| const signatures: SignatureData[] = []; | ||
|
|
||
| for (const item of abi) { | ||
| let fragment: Fragment; | ||
| try { | ||
| fragment = Fragment.from(item); | ||
| } catch (error) { | ||
| // Ignore invalid fragments | ||
| // e.g. with custom type as they can appear in library ABIs | ||
| continue; | ||
| } | ||
| switch (fragment.type) { | ||
| case "function": | ||
| case "event": | ||
| case "error": | ||
| signatures.push(getSignatureData(fragment)); | ||
| } | ||
|
Comment on lines
+25
to
+26
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should wrapt this in a try/catch because fragment.format() will fail for some library contracts that have an invalid "type" field. Library ABI items are allowed to have custom types because their ABIs are not intended to be publicly callable. However this breaks with ethers when assembling the signature. See the latest messages in the Solidity private chat.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's also add a test case for the above issue
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for pointing this out. I made the function throwing in this case now, so we would not store any of the signatures if only one custom type was found. Or was your intention rather to ignore the fragments with custom types? |
||
| } | ||
|
|
||
| return signatures; | ||
| } | ||
|
|
||
| function getSignatureData(fragment: Fragment): SignatureData { | ||
| const signature = fragment.format("sighash"); | ||
| return { | ||
| signature, | ||
| signatureHash32: keccak256str(signature), | ||
| signatureType: fragment.type as "function" | "event" | "error", | ||
| }; | ||
| } | ||
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.
Good catch, how did you recognize this?
Actually this makes me realize now this function is difficult to read with .then()s. It's ok though.
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 just read over the function and wondered why the function is async.
We mainly have the
then()style here because we need to keep a reference to the promise object. We could move the inner logic to another function to useawait, but I think it's fine.