Skip to content

Conversation

zguesmi
Copy link
Member

@zguesmi zguesmi commented Jul 24, 2025

Migration steps:

  • Migrate src files (minimal changes)
  • Modify receive and fallback behavior in Diamond.sol
  • Rename all 1538 to 2535 in contracts and rename Module to Facet in contracts names
  • Update all facets to use namespaces app storage.
  • Remove ENSIntegration modules/functions
  • Update docs (soldoc, diagrams, ...)
  • Add unit tests for Diamond.sol

@zguesmi zguesmi marked this pull request as ready for review July 24, 2025 15:51
@zguesmi zguesmi self-assigned this Jul 24, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds unit tests for the Diamond contract as part of the migration to the Diamond pattern (ERC-2535). The tests verify Diamond contract deployment behavior, ownership setting, and facet cut functionality.

  • Adds comprehensive unit tests for Diamond contract deployment scenarios
  • Updates license headers to use SPDX-FileCopyrightText format
  • Removes legacy Migrations.sol contract that's no longer needed

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
test/byContract/Diamond.test.ts New test file covering Diamond deployment, ownership, and facet cut functionality
contracts/tools/Migrations.sol Removed legacy migrations contract
contracts/Diamond.sol Updated license header and comment formatting
CHANGELOG.md Added entry documenting the new Diamond unit tests
Comments suppressed due to low confidence (2)

test/byContract/Diamond.test.ts:66

  • The fallback redirection functionality mentioned in the PR description is not tested. Consider implementing this test to ensure the Diamond proxy correctly redirects fallback calls to facets.
        it.skip('[TODO] Should redirect fallback', async () => {});

test/byContract/Diamond.test.ts:67

  • The receive redirection functionality mentioned in the PR description is not tested. Consider implementing this test to ensure the Diamond proxy correctly redirects receive calls to facets.
        it.skip('[TODO] Should redirect receive', async () => {});

Copy link

codecov bot commented Jul 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.44%. Comparing base (3ccfc47) to head (6d07510).
Report is 9 commits behind head on feature/diamond.

Additional details and impacted files
@@                 Coverage Diff                 @@
##           feature/diamond     #224      +/-   ##
===================================================
+ Coverage            84.35%   84.44%   +0.08%     
===================================================
  Files                   37       37              
  Lines                 1112     1112              
  Branches               225      225              
===================================================
+ Hits                   938      939       +1     
+ Misses                 174      173       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

* @param deployer Signer to deploy the library.
* @returns The library configuration or an empty object.
*/
export async function getLibDiamondConfigOrEmpty(deployer: SignerWithAddress): Promise<any> {
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to type the promise output ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It generates a typing error so I use the generic type any.

Copy link
Contributor

@Le-Caignec Le-Caignec left a comment

Choose a reason for hiding this comment

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

Good Job 💪🏻

@zguesmi zguesmi merged commit f82d8cf into feature/diamond Jul 25, 2025
4 checks passed
@zguesmi zguesmi deleted the feature/diamond-migration-part3-diamond-unit-tests branch July 25, 2025 07:48
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.

3 participants