Add /files/contracts/partial/{chain} endpoint and explicit ordering…

…, resolve SQL NULL comparison bug (#1416) This PR was intended to add the `/files/contracts/partial/{chain}` endpoint that is missing to list all contracts under a certain chain. However during the process, specifically when adding proper tests, I've noticed the `partial` and `any` endpoints not working as expected, and some other potential issues explained below for further investigation. TLDR; - Adding the `/files/contracts/partial/{chain}` endpoint as specified in #1415 with an option to sort `asc` and `desc` - Adding tests - [SQL NULL comparison bug](#sql-null-comparison-bug) - Potential issues: - [Deduplication not working in `compiled_contracts`](#deduplication-not-working-in-compiled_contracts) - [`matchType` not used in `database-utils.ts`](#matchtype-not-used-in-database-utils-ts) ## SQL NULL comparison bug This was due to the queries being used in `countSourcifyMatchAddresses` and `getSourcifyMatchAddressesByChainAndMatch`. Both SQL queries count the number of perfect matches with `creation_match = 'perfect' OR runtime_match = 'perfect'` and the partial matches with the logical inverse `creation_match != 'perfect' AND runtime_match != 'perfect'`. However this does not work as expected in SQL because the `creation_match` field in our case can be `NULL` and any comparison with `NULL` resolves to `NULL`, and not to `true` as expected. This can also be seen by comparing the contract numbers in `stats.json` vs the API output: `/stats.json` for Mainnet: ```js "1": { ... "full_match": 131482, "partial_match": 529877, }, // total: 661,359 ``` `/files/contracts/full/1/`: ``` "pagination": { ... "totalResults": 132056 } ``` `/files/contracts/any/1/`: ``` "pagination": { ... "totalResults": 273320 } ``` The difference (i.e. partial count) gives 141,264 whereas from stats.json we've got 529,877. The bug can be resolved by adding a `COALESCE(creation_match, '')` to make the `NULL` values `''` for comparisons. # Potential issues ## Deduplication not working in `compiled_contracts` When debugging the tests I've noticed the identical test contracts are getting added to the `compiled_contracts` table multiple times. I'd have expected them to be deduplicated because the table has a unique key property in the schema definition: https://github.com/ethereum/sourcify/blob/653952de199123406cea9c3ebda4eda88cc0652a/services/database/migrations/20231109160022-create-alliance-schema.js#L204 However during testing the contracts get added multiple times: ```SQL SELECT compiler, "language", creation_code_hash, runtime_code_hash FROM public.compiled_contracts x ``` <img width="757" alt="image" src="https://github.com/ethereum/sourcify/assets/13069972/ce877a1f-5e6a-48de-a1e4-2127b67fb8f2"> Am I missing something or is there something wrong? ### To reproduce 1) add a breakpoint at https://github.com/ethereum/sourcify/blob/653952de199123406cea9c3ebda4eda88cc0652a/services/server/test/integration/repository-handlers/files.spec.ts#L56 2) Add an `.only` modifier to describe: https://github.com/ethereum/sourcify/blob/653952de199123406cea9c3ebda4eda88cc0652a/services/server/test/integration/repository-handlers/files.spec.ts#L43 3) run the `Mocha - Server Integration`, deploy and verify multiple contracts, and connect to the test database at `localhost:5431`. ### `matchType` in `database_utils.ts` While looking at the `database_utils.ts` code I've noticed the `matchType` variable is not used in functions: https://github.com/ethereum/sourcify/blob/eeec8244c25db361f2f94981268118b188c1f53a/services/server/src/server/services/utils/database-util.ts#L429 https://github.com/ethereum/sourcify/blob/eeec8244c25db361f2f94981268118b188c1f53a/services/server/src/server/services/utils/database-util.ts#L452 But it seems these functions themselves are not used at all. Can they be removed?
ethereum · Jun 4, 2024 · f4ea8ee · f4ea8ee
2 parents 5d03be2 + e7b0237
commit f4ea8ee
Show file tree

Hide file tree

Showing 15 changed files with 285 additions and 132 deletions.
diff --git a/...c/server/controllers/repository/get-contract-addresses-paginated-all.stateless.paths.yaml b/...c/server/controllers/repository/get-contract-addresses-paginated-all.stateless.paths.yaml
@@ -26,6 +26,13 @@ paths:
             type: number
             minimum: 1
             maximum: 200
+        - name: order
+          in: query
+          required: false
+          schema:
+            type: string
+            enum: [asc, desc]
+          description: Order of the results. Default is "asc" (earliest verified contract first)
       responses:
         "200":
           description: Chain is available as a full match or partial match in the repository

diff --git a/.../server/controllers/repository/get-contract-addresses-paginated-full.stateless.paths.yaml b/.../server/controllers/repository/get-contract-addresses-paginated-full.stateless.paths.yaml
@@ -26,6 +26,14 @@ paths:
             type: number
             minimum: 1
             maximum: 200
+        - name: order
+          in: query
+          required: false
+          schema:
+            type: string
+            enum: [asc, desc]
+          description: Order of the results. Default is "asc" (earliest verified contract first)
+
       responses:
         "200":
           description: Chain is available as a full match in the repository

diff --git a/...rver/controllers/repository/get-contract-addresses-paginated-partial.stateless.paths.yaml b/...rver/controllers/repository/get-contract-addresses-paginated-partial.stateless.paths.yaml
@@ -0,0 +1,69 @@
+openapi: "3.0.0"
+
+paths:
+  /files/contracts/partial/{chain}:
+    get:
+      summary: Get the contract addresses partially verified on a chain
+      description: Returns the partially verified contracts from the repository for the desired chain. API is paginated. Limit must be a number between 1 and 200.
+      tags:
+        - Repository
+      parameters:
+        - name: chain
+          in: path
+          required: true
+          schema:
+            type: string
+            format: sourcify-chainId
+        - name: page
+          in: query
+          required: false
+          schema:
+            type: number
+        - name: limit
+          in: query
+          required: false
+          schema:
+            type: number
+            minimum: 1
+            maximum: 200
+        - name: order
+          in: query
+          required: false
+          schema:
+            type: string
+            enum: [asc, desc]
+          description: Order of the results. Default is "asc" (earliest verified contract first)
+      responses:
+        "200":
+          description: Chain is available as a full match in the repository
+          content:
+            application/json:
+              schema:
+                type: object
+                properties:
+                  results:
+                    type: array
+                    items:
+                      type: string
+                    example:
+                      [
+                        "0x1fE5d745beABA808AAdF52057Dd7AAA47b42cFD0",
+                        "0xE9c31091868d68598Ac881738D159A63532d12f9",
+                      ]
+                  pagination:
+                    type: object
+                    properties:
+                      currentPage:
+                        type: number
+                      totalPages:
+                        type: number
+                      resultsPerPage:
+                        type: number
+                      resultsCurrentPage:
+                        type: number
+                      totalResults:
+                        type: number
+                      hasNextPage:
+                        type: boolean
+                      hasPreviousPage:
+                        type: boolean
diff --git a/services/server/src/server/controllers/repository/repository.handlers.ts b/services/server/src/server/controllers/repository/repository.handlers.ts
@@ -21,7 +21,8 @@ type PaginatedConractRetrieveMethod = (
   chain: string,
   match: MatchLevel,
   page: number,
-  limit: number
+  limit: number,
+  descending: boolean
 ) => Promise<PaginatedContractData>;
 
 export function createEndpoint(
@@ -75,7 +76,8 @@ export function createPaginatedContractEndpoint(
         req.params.chain,
         match,
         parseInt((req.query.page as string) || "0"),
-        parseInt((req.query.limit as string) || "200")
+        parseInt((req.query.limit as string) || "200"),
+        req.query.order === "desc" // default is asc
       );
     } catch (err: any) {
       return next(new NotFoundError(err.message));

diff --git a/services/server/src/server/controllers/repository/repository.routes.ts b/services/server/src/server/controllers/repository/repository.routes.ts
@@ -52,23 +52,52 @@ const router: Router = Router();
   {
     prefix: "/contracts/full",
     method: createPaginatedContractEndpoint(
-      (chain, match, page, limit) =>
-        services.storage.getPaginatedContracts(chain, match, page, limit),
+      (chain, match, page, limit, descending) =>
+        services.storage.getPaginatedContracts(
+          chain,
+          match,
+          page,
+          limit,
+          descending
+        ),
       "full_match"
     ),
   },
+  {
+    prefix: "/contracts/partial",
+    method: createPaginatedContractEndpoint(
+      (chain, match, page, limit, descending) =>
+        services.storage.getPaginatedContracts(
+          chain,
+          match,
+          page,
+          limit,
+          descending
+        ),
+      "partial_match"
+    ),
+  },
   {
     prefix: "/contracts/any",
     method: createPaginatedContractEndpoint(
-      (chain, match, page, limit) =>
-        services.storage.getPaginatedContracts(chain, match, page, limit),
+      (chain, match, page, limit, descending) =>
+        services.storage.getPaginatedContracts(
+          chain,
+          match,
+          page,
+          limit,
+          descending
+        ),
       "any_match"
     ),
   },
   {
     prefix: "",
     method: createEndpoint(
-      (chain, address, match) => services.storage.getContent(chain, address, match), "full_match"),
+      (chain, address, match) =>
+        services.storage.getContent(chain, address, match),
+      "full_match"
+    ),
   },
 ].forEach((pair) => {
   router

diff --git a/services/server/src/server/services/StorageService.ts b/services/server/src/server/services/StorageService.ts
@@ -277,21 +277,24 @@ export class StorageService {
     chainId: string,
     match: MatchLevel,
     page: number,
-    limit: number
+    limit: number,
+    descending: boolean = false
   ): Promise<PaginatedContractData> => {
     try {
       return this.sourcifyDatabase!.getPaginatedContracts(
         chainId,
         match,
         page,
-        limit
+        limit,
+        descending
       );
     } catch (error) {
       logger.error("Error while getting paginated contracts from database", {
         chainId,
         match,
         page,
         limit,
+        descending,
         error,
       });
       throw new Error("Error while getting paginated contracts from database");

diff --git a/services/server/src/server/services/storageServices/SourcifyDatabaseService.ts b/services/server/src/server/services/storageServices/SourcifyDatabaseService.ts
@@ -153,7 +153,7 @@ export class SourcifyDatabaseService
         }
       );
       throw new BadRequestError(
-        `Cannot fetch more than ${MAX_RETURNED_CONTRACTS_BY_GETCONTRACTS} contracts (${fullTotal} full matches, ${partialTotal} partial matches), please use /contracts/{full|any}/${chainId} with pagination`
+        `Cannot fetch more than ${MAX_RETURNED_CONTRACTS_BY_GETCONTRACTS} contracts (${fullTotal} full matches, ${partialTotal} partial matches), please use /contracts/{full|any|partial}/${chainId} with pagination`
       );
     }
 
@@ -198,7 +198,8 @@ export class SourcifyDatabaseService
     chainId: string,
     match: MatchLevel,
     page: number,
-    limit: number
+    limit: number,
+    descending: boolean = false
   ): Promise<PaginatedContractData> => {
     await this.init();
 
@@ -233,29 +234,31 @@ export class SourcifyDatabaseService
       matchAddressesCountResult.rows[0].partial_total
     );
     const anyTotal = fullTotal + partialTotal;
-    if (match === "full_match") {
-      if (fullTotal === 0) {
-        return res;
-      }
-      res.pagination.totalResults = fullTotal;
-    } else if (match === "any_match") {
-      if (anyTotal === 0) {
-        return res;
-      }
-      res.pagination.totalResults = anyTotal;
+    const matchTotals: Record<MatchLevel, number> = {
+      full_match: fullTotal,
+      partial_match: partialTotal,
+      any_match: anyTotal,
+    };
+
+    // return empty res if requested `match` total is zero
+    if (matchTotals[match] === 0) {
+      return res;
     }
+    res.pagination.totalResults = matchTotals[match];
 
     res.pagination.totalPages = Math.ceil(
       res.pagination.totalResults / res.pagination.resultsPerPage
     );
 
+    // Now make the real query for addresses
     const matchAddressesResult =
       await Database.getSourcifyMatchAddressesByChainAndMatch(
         this.databasePool,
         parseInt(chainId),
         match,
         page,
-        limit
+        limit,
+        descending
       );
 
     if (matchAddressesResult.rowCount > 0) {

diff --git a/services/server/src/server/services/utils/database-util.ts b/services/server/src/server/services/utils/database-util.ts
@@ -430,52 +430,6 @@ export async function insertSourcifyMatch(
   );
 }
 
-export async function insertSourcifySync(
-  pool: Pool,
-  { chain_id, address, match_type }: Tables.SourcifySync
-) {
-  // I'm doing this because before passing the match_type I'm calling getMatchStatus(match)
-  // but then I need to convert the status to full_match | partial_match
-  let matchType;
-  switch (match_type) {
-    case "perfect":
-      matchType = "full_match";
-      break;
-    case "partial":
-      matchType = "partial_match";
-      break;
-  }
-  await pool.query(
-    `INSERT INTO sourcify_sync 
-      (chain_id, address, match_type, synced)
-      VALUES ($1, $2, $3, true)`,
-    [chain_id, address, match_type]
-  );
-}
-
-export async function updateSourcifySync(
-  pool: Pool,
-  { chain_id, address, match_type }: Tables.SourcifySync
-) {
-  // I'm doing this because before passing the match_type I'm calling getMatchStatus(match)
-  // but then I need to convert the status to full_match | partial_match
-  let matchType;
-  switch (match_type) {
-    case "perfect":
-      matchType = "full_match";
-      break;
-    case "partial":
-      matchType = "partial_match";
-      break;
-  }
-  await pool.query(
-    `UPDATE sourcify_sync SET
-      match_type=$3
-    WHERE chain_id=$1 AND address=$2;`,
-    [chain_id, address, match_type]
-  );
-}
-
 // Update sourcify_matches to the latest (and better) match in verified_contracts,
 // you need to pass the old verified_contract_id to be updated.
 // The old verified_contracts are not deleted from the verified_contracts table.
@@ -579,17 +533,17 @@ export function normalizeRecompiledBytecodes(
 export async function countSourcifyMatchAddresses(pool: Pool, chain: number) {
   return await pool.query(
     `
-    SELECT
-        contract_deployments.chain_id,
-        SUM(CASE WHEN sourcify_matches.creation_match = 'perfect' OR sourcify_matches.runtime_match = 'perfect' THEN 1 ELSE 0 END) AS full_total,
-        SUM(CASE WHEN sourcify_matches.creation_match != 'perfect' AND sourcify_matches.runtime_match != 'perfect' THEN 1 ELSE 0 END) AS partial_total
-    FROM sourcify_matches
-    JOIN verified_contracts ON verified_contracts.id = sourcify_matches.verified_contract_id
-    JOIN contract_deployments ON 
-        contract_deployments.id = verified_contracts.deployment_id
-        AND contract_deployments.chain_id = $1
-    GROUP BY contract_deployments.chain_id;
-    `,
+  SELECT
+  contract_deployments.chain_id,
+  SUM(CASE 
+    WHEN COALESCE(sourcify_matches.creation_match, '') = 'perfect' OR sourcify_matches.runtime_match = 'perfect' THEN 1 ELSE 0 END) AS full_total,
+  SUM(CASE 
+    WHEN COALESCE(sourcify_matches.creation_match, '') != 'perfect' AND sourcify_matches.runtime_match != 'perfect' THEN 1 ELSE 0 END) AS partial_total
+  FROM sourcify_matches
+  JOIN verified_contracts ON verified_contracts.id = sourcify_matches.verified_contract_id
+  JOIN contract_deployments ON contract_deployments.id = verified_contracts.deployment_id
+  WHERE contract_deployments.chain_id = $1
+  GROUP BY contract_deployments.chain_id;`,
     [chain]
   );
 }
@@ -599,18 +553,19 @@ export async function getSourcifyMatchAddressesByChainAndMatch(
   chain: number,
   match: "full_match" | "partial_match" | "any_match",
   page: number,
-  paginationSize: number
+  paginationSize: number,
+  descending: boolean = false
 ) {
   let queryWhere = "";
   switch (match) {
     case "full_match": {
       queryWhere =
-        "WHERE sourcify_matches.creation_match = 'perfect' OR sourcify_matches.runtime_match = 'perfect'";
+        "WHERE COALESCE(sourcify_matches.creation_match, '') = 'perfect' OR sourcify_matches.runtime_match = 'perfect'";
       break;
     }
     case "partial_match": {
       queryWhere =
-        "WHERE sourcify_matches.creation_match != 'perfect' AND sourcify_matches.runtime_match != 'perfect'";
+        "WHERE COALESCE(sourcify_matches.creation_match, '') != 'perfect' AND sourcify_matches.runtime_match != 'perfect'";
       break;
     }
     case "any_match": {
@@ -621,6 +576,11 @@ export async function getSourcifyMatchAddressesByChainAndMatch(
       throw new Error("Match type not supported");
     }
   }
+
+  const orderBy = descending
+    ? "ORDER BY verified_contracts.id DESC"
+    : "ORDER BY verified_contracts.id ASC";
+
   return await pool.query(
     `
     SELECT
@@ -631,6 +591,7 @@ export async function getSourcifyMatchAddressesByChainAndMatch(
         contract_deployments.id = verified_contracts.deployment_id
         AND contract_deployments.chain_id = $1
     ${queryWhere}
+    ${orderBy}
     OFFSET $2 LIMIT $3
     `,
     [chain, page * paginationSize, paginationSize]