feat: add fallible methods to artifact manager#8095
Conversation
|
There was a problem hiding this comment.
Pull request overview
Adds fallible ArtifactManager APIs to avoid expensive error construction (e.g., editDistance-based suggestions) on common “missing artifact” paths.
Changes:
- Introduces
tryToReadArtifactandtryToGetArtifactPathreturningundefinedwhen the artifact doesn’t exist. - Refactors
artifactExiststo usetryToGetArtifactPathinstead of relying on exceptions. - Updates LazyArtifactManager forwarding and test mocks to satisfy the extended interface.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| v-next/hardhat/test/test-helpers/mock-artifact-manager.ts | Adds fallible methods to the Hardhat test mock. |
| v-next/hardhat/src/types/artifacts.ts | Extends the public ArtifactManager interface with fallible APIs and docs. |
| v-next/hardhat/src/internal/builtin-plugins/artifacts/hook-handlers/hre.ts | Forwards new fallible methods through the lazy HRE artifact manager. |
| v-next/hardhat/src/internal/builtin-plugins/artifacts/artifact-manager.ts | Implements fallible path/artifact lookup and optimizes artifactExists. |
| v-next/hardhat-ethers/test/helpers/artifact-manager-mock.ts | Adds fallible tryToReadArtifact and stubs tryToGetArtifactPath in ethers tests. |
| * @param contractNameOrFullyQualifiedName The name of the contract. | ||
| * It can be a contract bare contract name (e.g. "Token") if it's |
There was a problem hiding this comment.
The phrase 'a contract bare contract name' is grammatically incorrect and a bit confusing. Consider changing it to 'a bare contract name' to match typical terminology and improve readability.
| _contractNameOrFullyQualifiedName: string, | ||
| ): Promise<string | undefined> { | ||
| throw new HardhatError( | ||
| HardhatError.ERRORS.CORE.INTERNAL.NOT_IMPLEMENTED_ERROR, | ||
| { | ||
| message: "Not implemented in MockArtifactManager", | ||
| }, | ||
| ); |
There was a problem hiding this comment.
tryToGetArtifactPath is intended to be a fallible, non-throwing API for the common 'missing artifact' case. Having the mock always throw defeats the purpose and can break tests that switch to these new methods for performance. Consider implementing minimal behavior (e.g., return undefined when missing and a deterministic fake path when present) or at least returning undefined unconditionally if path resolution is irrelevant in these tests.
| _contractNameOrFullyQualifiedName: string, | |
| ): Promise<string | undefined> { | |
| throw new HardhatError( | |
| HardhatError.ERRORS.CORE.INTERNAL.NOT_IMPLEMENTED_ERROR, | |
| { | |
| message: "Not implemented in MockArtifactManager", | |
| }, | |
| ); | |
| contractNameOrFullyQualifiedName: string, | |
| ): Promise<string | undefined> { | |
| const artifact = this.#artifacts.get(contractNameOrFullyQualifiedName); | |
| if (artifact === undefined) { | |
| return undefined; | |
| } | |
| // Return a deterministic fake path for tests when the artifact exists. | |
| return `${contractNameOrFullyQualifiedName}.json`; |
| throw new HardhatError( | ||
| HardhatError.ERRORS.CORE.INTERNAL.NOT_IMPLEMENTED_ERROR, | ||
| { | ||
| message: "Not implemented in MockArtifactManager", | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Same issue as the Hardhat test mock: a tryTo* API should not throw for the normal 'not found' flow. In this mock, consider mapping contractNameOrFullyQualifiedName to the existing #artifactsPaths entries and returning undefined when absent, to better reflect the production contract and avoid test brittleness.
| throw new HardhatError( | |
| HardhatError.ERRORS.CORE.INTERNAL.NOT_IMPLEMENTED_ERROR, | |
| { | |
| message: "Not implemented in MockArtifactManager", | |
| }, | |
| ); | |
| const artifactFileName = this.#artifactsPaths.get( | |
| contractNameOrFullyQualifiedName, | |
| ); | |
| if (artifactFileName === undefined) { | |
| return undefined; | |
| } | |
| return artifactFileName; |
| tryToReadArtifact< | ||
| ContractNameT extends StringWithArtifactContractNamesAutocompletion, | ||
| >( | ||
| contractNameOrFullyQualifiedName: ContractNameT, | ||
| ): Promise<GetArtifactByName<ContractNameT> | undefined>; |
There was a problem hiding this comment.
Adding methods to the exported ArtifactManager interface is a breaking change for downstream/custom implementations (they must implement the new methods to typecheck). If external implementers are expected, consider providing a backwards-compatible path (e.g., a separate extended interface, or making these optional with clear fallback behavior) to reduce upgrade friction.
| tryToGetArtifactPath( | ||
| contractNameOrFullyQualifiedName: string, | ||
| ): Promise<string | undefined>; |
There was a problem hiding this comment.
Adding methods to the exported ArtifactManager interface is a breaking change for downstream/custom implementations (they must implement the new methods to typecheck). If external implementers are expected, consider providing a backwards-compatible path (e.g., a separate extended interface, or making these optional with clear fallback behavior) to reduce upgrade friction.
The existing methods always throw, which can be expensive when failures are common, especially if the thrown error is not even used. The new methods avoid throwing and instead return `undefined`.
Wodann
force-pushed
the
feat/fallible-artifact-methods
branch
from
d3f371a to
edf131d
Compare
|
@alcuadrado does #8122 make this PR obsolete? |
|
Oops, sorry I forgot to mention that. Yes. We decided to go for the option that doesn't modify the API. Once again, amazing finding! |
The existing
ArtifactManagermethods always throw, which can be expensive when failures are common, especially if the thrown error is not even used. The new methods avoid throwing and instead returnundefined.The Openzeppelin Contracts suffer from this problem, as they have a test suite that auto-generates test vectors for as many artifacts as possible (for more info, see 1 & 2.
As seen in this profiling run, Hardhat's
editDistancefunction takes the 4th largest amount of time.In total, the
#getFullyQualifiedNamefunction is consuming about 6.3% of the OZ test time:There are several reasons for this becoming a hot path despite being error handling code:
#getSimilarStringsdue to missing artifacts#getSimilarStringsis called with 764+ names to compare againsteditDistanceis called with names ranging in length from 3 to 39.editDistanceuses Levenshtein algorithm to determine the edit distance. The standard implementation has a time complexity ofO(m×n)After optimisation, we're almost spending no time (i.e. <0.1%) on this code path anymore:
The runtime of
npx hardhat test mocha --no-compilewent from 1:31.84 to 1:22.23, a reduction of 9.61s (-10.5%).The accompanying change in OpenZeppelin Contracts can be found here.
This PR also speeds up the
artifactExistsimplementation shipped with Hardhat by not requiring the generation of the (later discarded) error for non-existent artifacts.Considerations
O(m+n)time complexity; but given that OpenZeppelin never actually uses the generated error, I thought it would be a better alternative to introduce APIs that signal the possibility of the file not existing.#throwNotFoundErrorto return a cheap intermediate error type that needs to be converted to a full, expensive-to-calculate Hardhat error (with suggestion) at a later time. That intermediate error type would contain the following information:ArtifactManageras a "library API" would then have control over whether to use the error or not. End-user application like Hardhat's CLI would then invoke the error conversion on the intermediate error type.