Port flatten task to v3 #6068

antico5 · 2024-12-19T15:32:35Z

This PR introduces an implementation for the flatten task. Both the implementation and the tests were ported and adapted from v2.

Details to point out:

The usage of the tsort package was replaced by toposort, which has 30x weekly download count and specially, it's typed.
On v2, there were multiple subtasks associated with flatten. The most important being (shortened): FLATTEN, GET_FLATTENED_SOURCE, GET_FLATTENED_SOURCE_AND_METADATA. I wasn't sure there was a need to define more than one task, so I went with this approach. Maybe I could move the printing (contract and warnings) to a separate one, in case just the flattened file and metadata need to be consumed by an external plugin.
For testing I used several fixture projects, which was the approach taken in v2. I noticed the engineering guidelines suggest to avoid this unless necessary, but I'd find it dificult to test this without using them. If we wanted that, we'd have to think about a mock filesystem/resolver.
For asserting stdio I hooked the console.log and console.warn implementations on the test cases. I tried to find an injection approach but it wasn't clear to me how, since tasks don't allow function arguments.
On v2 tests, flattened contracts were compiled. I didn't include that part on v3 tests, since I'm not sure it's in scope. As I understand it, the flatten task doesn't guarantee the the output is valid, compilable solidity code.

Closes #5647

vercel · 2024-12-19T15:32:40Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
hardhat	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 27, 2025 5:50pm

changeset-bot · 2024-12-19T15:32:40Z

⚠️ No Changeset found

Latest commit: 82323e1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions · 2024-12-19T15:34:09Z

hardhat

Total size of the bundle: 229M
Total number of dependencies (including transitive): 62

List of dependencies (sorted by size)

224M	total
30M	@ignored/edr-optimism-linux-x64-musl
30M	@ignored/edr-optimism-linux-x64-gnu
27M	@ignored/edr-optimism-linux-arm64-musl
27M	@ignored/edr-optimism-linux-arm64-gnu
22M	@ignored/edr-optimism-win32-x64-msvc
21M	@ignored/edr-optimism-darwin-x64
20M	esbuild
20M	@ignored/edr-optimism-darwin-arm64
8.9M	solc
2.8M	@sentry/tracing
2.4M	micro-eth-signer
1.9M	@noble/curves
1.7M	undici
1.2M	@sentry/types
1.2M	@noble/hashes
932K	@sentry/node
920K	@sentry/utils
856K	zod
816K	@ignored/hardhat-vnext-utils
616K	micro-packed
576K	tsx
548K	@sentry/core
504K	fast-equals
492K	@scure/bip39
460K	@ignored/edr
384K	@ignored/edr-optimism
368K	ethereum-cryptography
344K	@sentry/hub
320K	enquirer
320K	@ignored/hardhat-vnext-errors
284K	semver
192K	ws
188K	commander
168K	@scure/base
136K	adm-zip
128K	get-tsconfig
96K	@scure/bip32
92K	chalk
88K	tslib
88K	@sentry/minimal
84K	js-sha3
76K	agent-base
72K	@nomicfoundation/solidity-analyzer
68K	debug
64K	lru_map
64K	https-proxy-agent
60K	@ignored/hardhat-vnext-zod-utils
56K	rfdc
56K	follow-redirects
48K	memorystream
48K	command-exists
48K	ansi-colors
44K	tmp
40K	toposort
40K	resolve-pkg-maps
36K	p-map
32K	cookie
24K	strip-ansi
24K	env-paths
24K	ansi-regex
20K	os-tmpdir
20K	ms

alcuadrado · 2025-01-12T19:52:53Z

v-next/hardhat/package.json

@@ -68,6 +68,7 @@
    "@types/debug": "^4.1.4",
    "@types/node": "^20.14.9",
    "@types/semver": "^7.5.8",
+    "@types/toposort": "^2.0.7",


We have an implementation of topological sort, which we should reuse

FYI, it's here -

hardhat/v-next/hardhat/src/internal/core/plugins/resolve-plugin-list.ts

Line 26 in 00119c3

async function reverseTopologicalSort(

alcuadrado · 2025-01-12T20:34:01Z

v-next/hardhat/test/internal/builtin-plugins/flatten/task-action.ts

+  beforeEach(() => {
+    // Replace console.log and console.warn so we can assert on their output
+    consoleLogBuffer.length = 0;
+    consoleWarnBuffer.length = 0;
+
+    console.log = (...args: unknown[]) => {
+      consoleLogBuffer.push(args.join(" "));
+    };
+    console.warn = (...args: unknown[]) => {
+      consoleWarnBuffer.push(args.join(" "));
+    };
+  });
+
+  afterEach(() => {
+    console.log = originalConsoleLog;
+    console.warn = originalConsoleWarn;
+  });


Instead of doing this, the task action can accept these params

print = console.log, warn = console.warn

which are not exposed to the CLI, nor hooked into it in any way.

Then, to test it, we just need to call the task action directly.

If we wanted to do that, here's an example -

hardhat/v-next/hardhat/src/internal/builtin-plugins/console/task-action.ts

Line 16 in 00119c3

options?: repl.ReplOptions;

We do, however, assert on the returned values already so it might not be as necessary to inspect the entire stdio. It might still be useful to suppress it though.

alcuadrado · 2025-01-12T20:35:20Z

The usage of the tsort package was replaced by toposort, which has 30x weekly download count and specially, it's typed.

Also added a comment about this, but we already have an implementation of top sort.

On v2, there were multiple subtasks associated with flatten. The most important being (shortened): FLATTEN, GET_FLATTENED_SOURCE, GET_FLATTENED_SOURCE_AND_METADATA. I wasn't sure there was a need to define more than one task, so I went with this approach. Maybe I could move the printing (contract and warnings) to a separate one, in case just the flattened file and metadata need to be consumed by an external plugin.

Single task is the way to go. If we need to make something extensible, we can add hooks.

For testing I used several fixture projects, which was the approach taken in v2. I noticed the engineering guidelines suggest to avoid this unless necessary, but I'd find it dificult to test this without using them. If we wanted that, we'd have to think about a mock filesystem/resolver.

I guess it's fine for this case.

For asserting stdio I hooked the console.log and console.warn implementations on the test cases. I tried to find an injection approach but it wasn't clear to me how, since tasks don't allow function arguments.

See the comments.

On v2 tests, flattened contracts were compiled. I didn't include that part on v3 tests, since I'm not sure it's in scope. As I understand it, the flatten task doesn't guarantee the the output is valid, compilable solidity code.

I think this is ok.

galargh

I think it's a good idea to use an internal version of topological sort implementation if we have most of it already in place.

Other than that most of my comments are minor and, as I suspect, mostly directed at the v2 implementation of the task. Feel free to take into account only where it makes sense to you.

As for the testing side of this, I think there are some good candidates for lower level testing - like helper functions, regexes, etc. We don't necesarilly have to add this range of tests in this iteration though. The higher level tests, the E2E ones do seem reasonable to me and do cover the core of the task so I'd be happy to proceed with them.

galargh · 2025-01-27T09:12:47Z

v-next/hardhat/package.json

@@ -68,6 +68,7 @@
    "@types/debug": "^4.1.4",
    "@types/node": "^20.14.9",
    "@types/semver": "^7.5.8",
+    "@types/toposort": "^2.0.7",


FYI, it's here -

hardhat/v-next/hardhat/src/internal/core/plugins/resolve-plugin-list.ts

Line 26 in 00119c3

async function reverseTopologicalSort(

v-next/hardhat/src/internal/builtin-plugins/flatten/index.ts

galargh · 2025-01-27T09:19:32Z

v-next/hardhat/src/internal/builtin-plugins/flatten/task-action.ts

+export interface FlattenActionArguments {
+  files: string[];
+}


If you want to expose some arguments for testing only, for example stdio streams, you can do it as follows

hardhat/v-next/hardhat/src/internal/builtin-plugins/console/task-action.ts

Line 16 in 00119c3

options?: repl.ReplOptions;

This is very useful for testing the entire tasks.

galargh · 2025-01-27T09:21:12Z

v-next/hardhat/src/internal/builtin-plugins/flatten/task-action.ts

+
+export interface FlattenActionResult {
+  flattened: string;
+  metadata: FlattenMetadata | null;


Making metadata optional seems more natural to me.

Suggested change

metadata: FlattenMetadata | null;

metadata?: FlattenMetadata;

galargh · 2025-01-27T09:28:52Z

v-next/hardhat/src/internal/builtin-plugins/flatten/task-action.ts

+
+  // Return empty string when no files are resolved
+  if (Array.from(dependencyGraph.getAllFiles()).length === 0) {
+    return { flattened, metadata: null };


Another option would be to return undefined if there are no files to flatten or even throw an error. I don't have strong preference on one of these three options over the other. Do we know how we expect this task to be consumed programatically? Is it expected to be consumed at all - other than in tests?

galargh · 2025-01-27T10:10:34Z

v-next/hardhat/test/internal/builtin-plugins/flatten/task-action.ts

+async function createHRE() {
+  return createHardhatRuntimeEnvironment({}, {}, process.cwd());
+}


Can we just do createHardhatRuntimeEnvironment({}) inline?

galargh · 2025-01-27T10:13:16Z

v-next/hardhat/test/internal/builtin-plugins/flatten/task-action.ts

+  beforeEach(() => {
+    // Replace console.log and console.warn so we can assert on their output
+    consoleLogBuffer.length = 0;
+    consoleWarnBuffer.length = 0;
+
+    console.log = (...args: unknown[]) => {
+      consoleLogBuffer.push(args.join(" "));
+    };
+    console.warn = (...args: unknown[]) => {
+      consoleWarnBuffer.push(args.join(" "));
+    };
+  });
+
+  afterEach(() => {
+    console.log = originalConsoleLog;
+    console.warn = originalConsoleWarn;
+  });


If we wanted to do that, here's an example -

hardhat/v-next/hardhat/src/internal/builtin-plugins/console/task-action.ts

Line 16 in 00119c3

options?: repl.ReplOptions;

We do, however, assert on the returned values already so it might not be as necessary to inspect the entire stdio. It might still be useful to suppress it though.

galargh · 2025-01-27T10:15:20Z

v-next/hardhat/test/internal/builtin-plugins/flatten/task-action.ts

+  });
+
+  describe("SPDX licenses and pragma abicoder directives", () => {
+    describe("Flatten files that dont contain SPDX licenses or pragma directives", () => {


Suggested change

describe("Flatten files that dont contain SPDX licenses or pragma directives", () => {

describe("Flatten files that don't contain SPDX licenses or pragma directives", () => {

galargh · 2025-01-27T10:23:35Z

v-next/hardhat/test/fixture-projects/flatten-task/consistent-build-info-names/.gitignore

We don't really need this file in this fixture project I think.

galargh · 2025-01-27T10:24:59Z

v-next/hardhat/test/fixture-projects/flatten-task/consistent-build-info-names/hardhat.config.js

+// This project is compiled from scratch multiple times in the same test, which
+// produces a lot of logs. We override this task to omit those logs.


In the v3 implementation of the task, we don't do the compilation so I don't think we need this either. In fact, I think we should be able to remove all the hardhat.config.js/hardhat.config.ts from these projects and still get the tests to work as expected. To be verified though.

Co-authored-by: Piotr Galar <[email protected]>

Port flatten task to v3

eca4985

antico5 requested a review from alcuadrado December 19, 2024 15:32

github-actions bot added the status:triaging label Dec 19, 2024

github-actions bot assigned kanej Dec 19, 2024

schaable added status:ready This issue is ready to be worked on v-next A Hardhat v3 development task and removed status:triaging labels Dec 30, 2024

alcuadrado reviewed Jan 12, 2025

View reviewed changes

alcuadrado requested a review from galargh January 12, 2025 19:54

alcuadrado reviewed Jan 12, 2025

View reviewed changes

kanej linked an issue Jan 17, 2025 that may be closed by this pull request

Implement npx hardhat flatten task in v3 #5647

Open

galargh reviewed Jan 27, 2025

View reviewed changes

Update v-next/hardhat/src/internal/builtin-plugins/flatten/index.ts

82323e1

Co-authored-by: Piotr Galar <[email protected]>

vercel bot deployed to Preview January 27, 2025 17:50 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port flatten task to v3 #6068

Port flatten task to v3 #6068

antico5 commented Dec 19, 2024

vercel bot commented Dec 19, 2024 •

edited

Loading

changeset-bot bot commented Dec 19, 2024 •

edited

Loading

github-actions bot commented Dec 19, 2024

alcuadrado Jan 12, 2025

galargh Jan 27, 2025

alcuadrado Jan 12, 2025

galargh Jan 27, 2025

alcuadrado commented Jan 12, 2025

galargh left a comment

galargh Jan 27, 2025

galargh Jan 27, 2025

galargh Jan 27, 2025

galargh Jan 27, 2025

galargh Jan 27, 2025

galargh Jan 27, 2025

galargh Jan 27, 2025

galargh Jan 27, 2025

galargh Jan 27, 2025

	metadata: FlattenMetadata \| null;
	metadata?: FlattenMetadata;

	describe("Flatten files that dont contain SPDX licenses or pragma directives", () => {
	describe("Flatten files that don't contain SPDX licenses or pragma directives", () => {

		// This project is compiled from scratch multiple times in the same test, which
		// produces a lot of logs. We override this task to omit those logs.

Port flatten task to v3 #6068

Are you sure you want to change the base?

Port flatten task to v3 #6068

Conversation

antico5 commented Dec 19, 2024

vercel bot commented Dec 19, 2024 • edited Loading

changeset-bot bot commented Dec 19, 2024 • edited Loading

⚠️ No Changeset found

github-actions bot commented Dec 19, 2024

hardhat

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcuadrado commented Jan 12, 2025

galargh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Dec 19, 2024 •

edited

Loading

changeset-bot bot commented Dec 19, 2024 •

edited

Loading