Skip to content

Conversation

@andrew-fleming
Copy link
Contributor

@andrew-fleming andrew-fleming commented Aug 29, 2025

Types of changes

What types of changes does your code introduce to OpenZeppelin Midnight Contracts?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

Fixes #200.

PR Checklist

  • I have read the Contributing Guide
  • I have added tests that prove my fix is effective or that my feature works
  • I have added documentation of new methods and any new behavior or changes to existing behavior
  • CI Workflows Are Passing

Overview

This PR proposes to:

  • Integrate Compact dev tools.
  • Refactor Compiler.ts
  • Refactor runCompiler.ts
  • Add tests for both^
  • Update the CI with the dev tool

This PR includes a breaking change: To develop on the repo, devs must install the Compact develop tool. The CI also installs the Compact CLI like this which simplifies installation

Due to the additional features of the dev tool, this PR proposes to refactor this repo's compact package. While writing tests for the current CLI, its fragility became evident and this refactor is long overdue. The proposed refactor aims to make the package much more maintainable by separating concerns. This PR also includes tests which is also long overdue

@andrew-fleming andrew-fleming requested a review from a team as a code owner August 29, 2025 01:26
@andrew-fleming andrew-fleming marked this pull request as draft August 29, 2025 04:26
@andrew-fleming andrew-fleming marked this pull request as ready for review September 3, 2025 04:27
Copy link
Contributor

Choose a reason for hiding this comment

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

We should make sure the corresponding doc file in the OpenZeppelin docs repo is updated.

andrew-fleming and others added 3 commits September 3, 2025 13:27
Co-authored-by: ⟣ €₥ℵ∪ℓ ⟢ <34749913+emnul@users.noreply.github.com>
Signed-off-by: Andrew Fleming <fleming.andrew@protonmail.com>
* @class CompactCliNotFoundError
* @extends Error
*/
export class CompactCliNotFoundError extends Error {
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to add a VersionNotFound error type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, it's getting caught like this:

✖ [COMPILE] Environment validation failed: Command failed: compact compile +0.25.0 --version

Error: Failed to run compactc

Caused by:
    0: Couldn't find compiler for aarch64-darwin (0.25.0)
    1: Directory does not exist: `"Users/totallyNotAFed/.compact/versions/0.25.0/aarch64-darwin"'


Troubleshooting:
  • Check that Compact CLI is installed and in PATH
  • Verify the specified Compact version exists
  • Ensure you have proper permissions

I think that's enough for users (just us), no?

Co-authored-by: ⟣ €₥ℵ∪ℓ ⟢ <34749913+emnul@users.noreply.github.com>
Signed-off-by: Andrew Fleming <fleming.andrew@protonmail.com>
});
});

describe('validateEnvironment', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add a test that validates an Error is thrown when an invalid Compact version is passed

Co-authored-by: ⟣ €₥ℵ∪ℓ ⟢ <34749913+emnul@users.noreply.github.com>
Signed-off-by: Andrew Fleming <fleming.andrew@protonmail.com>
Comment on lines 347 to 373
describe('displayEnvInfo', () => {
it('should display all environment information', () => {
UIService.displayEnvInfo(
'compact 0.1.0',
'Compactc 0.24.0',
'security',
'0.24.0',
);

// Test passes if no errors are thrown
expect(true).toBe(true);
});
});

describe('showCompilationStart', () => {
it('should show file count without target directory', () => {
UIService.showCompilationStart(5);

expect(true).toBe(true);
});

it('should show file count with target directory', () => {
UIService.showCompilationStart(3, 'security');

expect(true).toBe(true);
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we refactor these to match the not.toThrow pattern below?

@emnul
Copy link
Contributor

emnul commented Sep 4, 2025

Amazing work as always! Thank you for your contributions sir 🫡

@andrew-fleming
Copy link
Contributor Author

@emnul Small refactor. I moved all the error handling to runCompiler which separates concerns better. There's documentation on the proposed design and flow. Now, when we make changes to the compact package, we have a north star ⭐ lmk what you think

This also made testing easier, more organized, and improved. No more expect(true).toBe(true) :)

Copy link
Contributor

@emnul emnul left a comment

Choose a reason for hiding this comment

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

It gets even better 😲

@andrew-fleming andrew-fleming merged commit 4b72e67 into OpenZeppelin:main Sep 4, 2025
6 checks passed
@andrew-fleming andrew-fleming deleted the integrate-compact-toolchain branch September 4, 2025 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Integrate compact toolchain

2 participants