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

Add /files/contracts/partial/{chain} endpoint and explicit ordering, resolve SQL NULL comparison bug #1416

Merged
merged 5 commits into from
Jun 4, 2024

Conversation

kuzdogan
Copy link
Member

@kuzdogan kuzdogan commented Jun 3, 2024

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;

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:

  "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:

CONSTRAINT compiled_contracts_pseudo_pkey UNIQUE (compiler, language, creation_code_hash, runtime_code_hash)

However during testing the contracts get added multiple times:

SELECT compiler, "language", creation_code_hash, runtime_code_hash FROM public.compiled_contracts x
image

Am I missing something or is there something wrong?

To reproduce

  1. add a breakpoint at

    const address = await deployAndVerifyContract(

  2. Add an .only modifier to describe:

    describe.only(`Pagination in /files/contracts/{full|any|partial}/${chainFixture.chainId}`, async function () {

  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:

export async function insertSourcifySync(

export async function updateSourcifySync(

But it seems these functions themselves are not used at all. Can they be removed?

@kuzdogan kuzdogan linked an issue Jun 3, 2024 that may be closed by this pull request
2 tasks
@kuzdogan kuzdogan changed the title Add /files/contracts/partial endpoint and explicit ordering Add /files/contracts/partial endpoint and explicit ordering, resolve SQL NULL comparison bug Jun 4, 2024
@kuzdogan kuzdogan changed the title Add /files/contracts/partial endpoint and explicit ordering, resolve SQL NULL comparison bug Add /files/contracts/partial/{chain} endpoint and explicit ordering, resolve SQL NULL comparison bug Jun 4, 2024
@kuzdogan kuzdogan marked this pull request as ready for review June 4, 2024 07:35
kuzdogan added 4 commits June 4, 2024 09:38
Since creation_match could get a `null` value in the database the comparisons if it's **not**
equal to `perfect` were failing because any comparison against a NULL value retuns NULL in SQL.

Fix these cases by using COALSCE to set NULL values to empty string on comparison
@kuzdogan kuzdogan force-pushed the add-files-partial-endpoint branch from eeec824 to 28c6827 Compare June 4, 2024 07:38
@marcocastignoli
Copy link
Member

Deduplication not working in compiled_contracts

I cannot understand why this happens. Maybe the unique constraint doesn't work with a NULL column?

But it seems these functions themselves are not used at all. Can they be removed?

Right now they are not used. If we need them for the rebuilding the database we can implement them again, you can remove them

@marcocastignoli
Copy link
Member

The bug can be resolved by adding a COALESCE(creation_match, '') to make the NULL values '' for comparisons.

I didn't know about it, good catch!

Copy link
Member

@marcocastignoli marcocastignoli left a comment

Choose a reason for hiding this comment

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

Just a doubt on why random is used, but other than that LGTM 👍

@kuzdogan
Copy link
Member Author

kuzdogan commented Jun 4, 2024

Deduplication not working in compiled_contracts

I cannot understand why this happens. Maybe the unique constraint doesn't work with a NULL column?

Ok I'll spin out another issue for the deduplication issue in `compiled_contracts

But it seems these functions themselves are not used at all. Can they be removed?

Right now they are not used. If we need them for the rebuilding the database we can implement them again, you can remove them

Ok but I don't really understand the use of the matchType variable there. I can keep the functions by noting they are unused for now, but the matchType is not used inside that function at all.

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]
);
}

@marcocastignoli
Copy link
Member

Ok but I don't really understand the use of the matchType variable there. I can keep the functions by noting they are unused for now, but the matchType is not used inside that function at all.

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]
);
}

Do you remember that inside sourcify_sync we had some values with full_match and some with perfect. The reason is that this function wasn't doing what it supposed to be doing, instead of inserting match_type, it should have inserted matchType. It's a mistake. As I told you that function is not used anymore, so we can remove it.

@kuzdogan kuzdogan merged commit f4ea8ee into staging Jun 4, 2024
6 checks passed
@kuzdogan kuzdogan deleted the add-files-partial-endpoint branch July 29, 2024 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: COMPLETED
Development

Successfully merging this pull request may close these issues.

Implementing /files/contracts/partial endpoint
2 participants