Optimize ArtifactsManager#artifactExists#8122
Conversation
🦋 Changeset detectedLatest commit: b432591 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
alcuadrado
added
no changeset needed
There was a problem hiding this comment.
Pull request overview
This PR optimizes ArtifactManagerImplementation#artifactExists by avoiding exception-based control flow and instead checking cached filesystem metadata (FsData) to determine whether an artifact is present.
Changes:
- Reworked
artifactExiststo use cached sets/maps from#getFsData()rather than callinggetArtifactPath()and catching errors. - Updated the
ArtifactManagertype documentation to remove the (now-incorrect)@throwsnote for non-unique names.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/hardhat/src/types/artifacts.ts | Aligns artifactExists docs with the intended behavior (no throw on non-unique names). |
| packages/hardhat/src/internal/builtin-plugins/artifacts/artifact-manager.ts | Implements the optimized artifactExists using cached filesystem-derived indices. |
| public async artifactExists( | ||
| contractNameOrFullyQualifiedName: string, | ||
| ): Promise<boolean> { | ||
| try { | ||
| // This throw if the artifact doesn't exist | ||
| await this.getArtifactPath(contractNameOrFullyQualifiedName); | ||
|
|
||
| return true; | ||
| } catch (error) { | ||
| if (HardhatError.isHardhatError(error)) { | ||
| if ( | ||
| error.number === HardhatError.ERRORS.CORE.ARTIFACTS.NOT_FOUND.number | ||
| ) { | ||
| return false; | ||
| } | ||
| } | ||
| const { bareNameToFullyQualifiedNameMap, allFullyQualifiedNames } = | ||
| await this.#getFsData(); | ||
|
|
||
| throw error; | ||
| if (this.#isFullyQualifiedName(contractNameOrFullyQualifiedName)) { | ||
| return allFullyQualifiedNames.has(contractNameOrFullyQualifiedName); | ||
| } | ||
|
|
||
| return bareNameToFullyQualifiedNameMap.has( | ||
| contractNameOrFullyQualifiedName, | ||
| ); | ||
| } |
There was a problem hiding this comment.
artifactExists behavior changed to return a boolean based on cached fs data (and no longer relies on getArtifactPath throwing). There are existing tests for ArtifactManagerImplementation in packages/hardhat/test/internal/builtin-plugins/artifacts/artifact-manager.ts, but none cover artifactExists—especially the non-unique bare-name case that this change is meant to handle. Add tests that assert: (1) returns true for an existing bare name, (2) returns true for an existing fully-qualified name, (3) returns false for a missing name, and (4) returns true (and does not throw) when multiple artifacts share the same bare name.
alcuadrado
requested review from
ChristopherDedominici
and removed request for
schaable
| * | ||
| * @param contractNameOrFullyQualifiedName Contract or fully qualified name. | ||
| * @throws Throws an error if a non-unique contract name is used, | ||
| * indicating which fully qualified names can be used instead. |
There was a problem hiding this comment.
Given that this function’s semantics have changed, it would be good to call it out here. When there are multiple contracts with the same name, artifactExists("Foo") returning true no longer guarantees that readArtifact("Foo") will succeed:
if (await hre.artifacts.artifactExists("Foo")) {
const a = await hre.artifacts.readArtifact("Foo"); // MULTIPLE_FOUND error
}This could be confusing for users who expect readArtifact not to throw if artifactExists returned true.
There was a problem hiding this comment.
I'll add a comment. That's a good point.
Note that I don't think the intentional semantics changed. I did a bit of and this method didn't used to have the @throws. I made a mistake in this commit by adding every @throws possible, despite contradicting the description.
schaable
left a comment
schaable
left a comment
There was a problem hiding this comment.
Looks good, left a comment regarding documentation but I'm pre-approving it.
ArtifactsManager#artifactExistsuse to try to read the artifact, and returnfalsewhen the read threw. That was slow, because the error thatreadArtifactgenerates includes all the similar contract names, which is a heavy operation. This implementation checks for the existence directly, making it much faster.