Skip to content

Optimize ArtifactsManager#artifactExists #8122

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.

Sign up for GitHub

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

Merged
alcuadrado merged 5 commits into from
Apr 13, 2026
Merged

Optimize ArtifactsManager#artifactExists #8122

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
14e4883
Optimize ArtifactsManager#artifactExists
alcuadrado Apr 10, 2026
edfa548
Add changeset
alcuadrado Apr 10, 2026
bd991f5
Add missing tests
alcuadrado Apr 10, 2026
b1e4fcb
Add comment explaining an edge-case
alcuadrado Apr 13, 2026
b432591
Fix typo and formatting
alcuadrado Apr 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 8 additions & 14 deletions packages/hardhat/src/internal/builtin-plugins/artifacts/artifact-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,22 +81,16 @@ export class ArtifactManagerImplementation implements ArtifactManager {
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,
);
}
Comment on lines 81 to 94

Copilot AI Apr 10, 2026

Copy link

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.

@alcuadrado alcuadrado Apr 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

good point, done


public async getBuildInfoId(
Expand Down
2 changes: 0 additions & 2 deletions packages/hardhat/src/types/artifacts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,6 @@ export interface ArtifactManager {
* This function doesn't throw if the name is not unique.
*
* @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.

@schaable schaable Apr 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@alcuadrado alcuadrado Apr 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

@alcuadrado alcuadrado Apr 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Addressed in b1e4fcb

*/
artifactExists(contractNameOrFullyQualifiedName: string): Promise<boolean>;

Expand Down
Loading