ethereum / sourcify

Decentralized Solidity contract source code verification service
https://sourcify.dev
MIT License
781 stars 395 forks source link

Research and plan: Refactoring the verification flow for inconsistencies #1318

Open kuzdogan opened 7 months ago

kuzdogan commented 7 months ago

Looking at the logs I noticed a contract returning multiple errors in all StorageServices: Error storing to RepositoryV1 ...RepositoryV2 ...AllianceDatabase ...SourcifyDatabase.

The contract in question is 0xbf1B6429214BcbDFF065697aFC6A4EC2ba392078 on chain 10. This was an Import from Etherscan request with id 8419c051fafabc40af058b9b8d34967a.

I wasn't able to reproduce the same logs initially with the session API locally. Upon trying the non-session API I was able to reproduce the same logs.

Turns out this is because the verification through the session and non-session take different flows. Namely the session flow goes to verifyContractsInSession function which checks the runtimeMatch and creationMatch and only stores the contracts if one of them exists: https://github.com/ethereum/sourcify/blob/5295fa3c1336a6901856a060a8122646fa5be055/services/server/src/server/controllers/verification/verification.common.ts#L457-L459

In contrast the stateless flow first calls services.verification.verifyDeployed and then directly calls storeMatch without checking the runtime and creationMatch https://github.com/ethereum/sourcify/blob/5295fa3c1336a6901856a060a8122646fa5be055/services/server/src/server/controllers/verification/etherscan/stateless/etherscan.stateless.handlers.ts#L42-L43

IMO the storing should only be done in the verification service and not dispersed throughout the codebase. In principal, all the verification related things should be done in the Verif. Service. A similar thing with all the extra-file-input-bug related code which as well spread across the codebase.

Example:

Other Issues:

marcocastignoli commented 7 months ago

IMO the storing should only be done in the verification service and not dispersed throughout the codebase. In principal, all the verification related things should be done in the Verif. Service. A similar thing with all the extra-file-input-bug related code which as well spread across the codebase.

When we refactored the code I remember that the initial idea was to have different services and use them one after the other in the API handler.

// e.g.
function legacyVerifyEndpoint() {
    // 1. check if the contract already exists with the storageService
    if(storageService.checkByChainAndAddress()){ ... }

    // 2. check uploaded files and create CheckedContracts (validationService that doesn't exist yet)
    checkedContracts = await checkFiles(solc, inputFiles);

    // 3. call verification service 
    const match = await services.verification.verifyDeployed

    // 4. call storage service
    await services.storage.storeMatch(contract, match);
}

But I see your point, I think it actually makes sense to call the storageService inside the verificationService in order to have a consistent flow across all the API handlers

marcocastignoli commented 3 weeks ago

Middleware Proposal

In a recent pull request, I refactored the logic for checking an existing perfect match into an Express middleware:

router
  .route("/verify")
  .post(checkPerfectMatch, safeHandler(legacyVerifyEndpoint));

This change led me to consider a more modular approach: creating dedicated middleware for each step of the verification process, such as checking if a contract exists, validating uploaded data, calling the verification service, and interacting with the storage service. Each middleware will modify the req object pushing the information for the next middleware.

E.g.

router
  .route("/verify")
  .post(checkPerfectMatch, validateFiles, verifyContract, storeContract);

This approach would enhance the clarity and consistency of the API flow, making it explicit and reusable across different endpoints. Each API would follow the same middleware pattern, allowing for shared logic and standardized processes.

In cases where an API receives a different set of inputs, we can add conversion middleware to ensure compatibility with the standard middleware pipeline.

E.g.

// `convertSolcJsonFiles` transforms the JSON file into metadata and files to be compatible with `validateFiles`
router
  .route("/verify/solc-json")
  .post(checkPerfectMatch, convertSolcJsonFiles, validateFiles, verifyContract, storeContract);