Skip to content

Conversation

@zguesmi
Copy link
Member

@zguesmi zguesmi commented Oct 14, 2025

Notes

  • contracts/facets/IexecEscrowTokenSwapFacet.sol has been removed to minimize upgrade overhead. It shouldn't break any user interaction because the module was not deployed on Bellecour nor Arbitrum.

Breaking changes

All modified tests are to be considered as breaking changes.

Next steps:

  • Upgrade interfaces from >=0.6.0 to ^0.8.0
  • Remove package @openzeppelin/contracts (v3)
  • Rename @openzeppelin/contracts-v5 to @openzeppelin/contracts
  • Sort solidity imports
  • Sunset iexec-solidity
  • Sunset iexec-interfaces
  • Remove uniswap references
  • Update docs and diagrams

Comment on lines +41 to +47
/**
* @dev Added for retrocompatibility!
*
* @dev Returns the base URI set via {setBaseURI}. This will be
* automatically added as a prefix in {tokenURI} to each token's ID.
*/
function baseURI() public view returns (string memory) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@codecov
Copy link

codecov bot commented Oct 14, 2025

Codecov Report

❌ Patch coverage is 84.61538% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.84%. Comparing base (63d0017) to head (ecaf5fb).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
contracts/facets/IexecEscrowNativeFacet.sol 0.00% 6 Missing ⚠️
...racts/registries/proxy/BaseUpgradeabilityProxy.sol 70.00% 3 Missing ⚠️
contracts/registries/proxy/Proxy.sol 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #287      +/-   ##
==========================================
+ Coverage   84.85%   89.84%   +4.99%     
==========================================
  Files          37       38       +1     
  Lines        1241     1201      -40     
  Branches      235      236       +1     
==========================================
+ Hits         1053     1079      +26     
+ Misses        188      122      -66     

☔ 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.

import {PocoStorageLib} from "../libs/PocoStorageLib.sol";
import {FacetBase} from "./FacetBase.sol";

// TODO uncomment and fix file
Copy link
Member Author

@zguesmi zguesmi Oct 14, 2025

Choose a reason for hiding this comment

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

TODO

Comment on lines +112 to +114
// try address(0)(_identity).isValidSignature(_hash, _signature) returns (bytes4 value) {
// return value == address(0)(address(0)).isValidSignature.selector;
// } catch (bytes memory /*lowLevelData*/) {}
Copy link
Member Author

@zguesmi zguesmi Oct 14, 2025

Choose a reason for hiding this comment

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

TODO

Comment on lines +128 to +136
// try IERC1271(_identity).isValidSignature(_predicat, _signature) returns (bytes4 value) {
// return value == IERC1271(address(0)).isValidSignature.selector;
// } catch (bytes memory /*lowLevelData*/) {}

// try IERC1654(_identity).isValidSignature(keccak256(_predicat), _signature) returns (
// bytes4 value
// ) {
// return value == IERC1654(0).isValidSignature.selector;
// } catch (bytes memory /*lowLevelData*/) {}
Copy link
Member Author

@zguesmi zguesmi Oct 14, 2025

Choose a reason for hiding this comment

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

TODO

IexecConfigurationExtra,
IexecERC20,
IexecERC20Common,
IexecERC20,
Copy link
Member Author

Choose a reason for hiding this comment

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

To resolve compiler dependency linearisation issue.

@Copilot Copilot AI review requested due to automatic review settings October 24, 2025 08:11
Copy link
Contributor

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 upgrades the project's Solidity compiler version from 0.6.0 to 0.8.0, representing a significant modernization effort with breaking changes. The upgrade includes updating OpenZeppelin contracts to v5, removing the deprecated IexecEscrowTokenSwapFacet, and updating test assertions to match Solidity 0.8.0's new error handling patterns (custom errors and panic codes).

Key changes include:

  • Migration from Solidity 0.6.0 to 0.8.0 across all contracts
  • Replacement of revertedWith string assertions with revertedWithCustomError and revertedWithPanic in tests
  • Removal of IexecEscrowTokenSwapFacet and related Uniswap functionality
  • Updates to OpenZeppelin v5 imports and error handling patterns
  • Consolidation of storage libraries and interface references

Reviewed Changes

Copilot reviewed 77 out of 77 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
test/byContract/registries/registries.test.ts Updated test assertions to use custom errors instead of string-based error messages
test/byContract/IexecEscrow/*.test.ts Replaced revertedWithoutReason with revertedWithPanic(0x11) for arithmetic underflow tests
test/byContract/IexecERC20/IexecERC20.test.ts Updated panic codes for underflow/overflow conditions
test/000_fullchain-boost.test.ts Renamed WorkerpoolInterface__factory to IWorkerpool__factory
contracts/registries/*.sol Upgraded pragma to 0.8.0, updated OpenZeppelin v5 imports, removed public constructor visibility
contracts/registries/Registry.sol Migrated to ERC721Enumerable, updated internal storage and casting patterns
contracts/registries/proxy/*.sol Added new proxy contracts adapted for Solidity 0.8.0
contracts/libs/PocoStorageLib.sol Consolidated storage library, updated interface types, replaced assembly syntax
contracts/facets/*.sol Updated pragma, removed SafeMath usage, updated imports to use explicit curly brace syntax
contracts/interfaces/*.sol Created new V3 interface, removed TokenSwap interface, updated ERC20 event definitions
deploy/0_deploy.ts Updated import paths for Ownable factory
scripts/tools/sol-to-uml.mjs Removed TokenSwap facet from diagram generation
hardhat.config.ts Removed references to deprecated contracts from documentation exclusions
abis/*.json Regenerated ABIs reflecting OpenZeppelin v5 custom errors and updated signatures

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

await expect(
datasetRegistry.createDataset(datasetProvider.address, ...createDatasetArgs),
).to.be.revertedWith('Create2: Failed on deploy');
).to.be.revertedWithCustomError(appRegistry, 'Create2FailedDeployment');
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

Incorrect registry used in error assertion. The test is calling datasetRegistry.createDataset() but checking for a custom error on appRegistry. Should use datasetRegistry for consistency.

Suggested change
).to.be.revertedWithCustomError(appRegistry, 'Create2FailedDeployment');
).to.be.revertedWithCustomError(datasetRegistry, 'Create2FailedDeployment');

Copilot uses AI. Check for mistakes.
await expect(
workerpoolRegistry.createWorkerpool(scheduler.address, ...createWorkerpoolArgs),
).to.be.revertedWith('Create2: Failed on deploy');
).to.be.revertedWithCustomError(appRegistry, 'Create2FailedDeployment');
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

Incorrect registry used in error assertion. The test is calling workerpoolRegistry.createWorkerpool() but checking for a custom error on appRegistry. Should use workerpoolRegistry for consistency.

Suggested change
).to.be.revertedWithCustomError(appRegistry, 'Create2FailedDeployment');
).to.be.revertedWithCustomError(workerpoolRegistry, 'Create2FailedDeployment');

Copilot uses AI. Check for mistakes.

import "./FacetBase.sol";
import "../interfaces/IexecConfigurationExtra.sol";
import {FacetBase } from "./FacetBase.sol";
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

Extra space before closing brace in import statement. Should be {FacetBase} without the trailing space.

Suggested change
import {FacetBase } from "./FacetBase.sol";
import {FacetBase} from "./FacetBase.sol";

Copilot uses AI. Check for mistakes.
import "../interfaces/IexecConfigurationExtra.sol";
import {FacetBase } from "./FacetBase.sol";
import {IexecConfigurationExtra} from "../interfaces/IexecConfigurationExtra.sol";
import {IRegistry} from "../registries/Registry.sol";
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

Incorrect import path for IRegistry. Should import from '../registries/IRegistry.sol' instead of '../registries/Registry.sol' since IRegistry is an interface, not the Registry contract itself.

Suggested change
import {IRegistry} from "../registries/Registry.sol";
import {IRegistry} from "../registries/IRegistry.sol";

Copilot uses AI. Check for mistakes.
Comment on lines +21 to 23
return registry.ownerOf(uint256(uint160(address(this))));
}

Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

[nitpick] Double type casting uint256(uint160(address(this))) is used here and in other locations. Consider extracting this to a helper function like _addressToTokenId() to improve code maintainability and reduce duplication.

Suggested change
return registry.ownerOf(uint256(uint160(address(this))));
}
return registry.ownerOf(_addressToTokenId());
}
function _addressToTokenId() internal view returns (uint256) {
return uint256(uint160(address(this)));
}

Copilot uses AI. Check for mistakes.
import {IApp} from "../registries/apps/IApp.v8.sol";
import {IWorkerpool} from "../registries/workerpools/IWorkerpool.v8.sol";
import {IexecPocoAccessors} from "../interfaces/IexecPocoAccessors.sol";
import {IRegistry} from "../registries/Registry.sol";
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

Incorrect import path for IRegistry. Should import from '../registries/IRegistry.sol' instead of '../registries/Registry.sol' to import the interface, not the implementation.

Suggested change
import {IRegistry} from "../registries/Registry.sol";
import {IRegistry} from "../registries/IRegistry.sol";

Copilot uses AI. Check for mistakes.
@Copilot Copilot AI review requested due to automatic review settings October 24, 2025 09:20
Copy link
Contributor

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

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


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

_;
}

function owner() public view returns (address) {
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

Complex nested type casting for address to uint256 conversion. Consider adding a comment explaining why the double cast through uint160 is necessary for Solidity 0.8.0 compatibility.

Suggested change
function owner() public view returns (address) {
function owner() public view returns (address) {
// Double cast through uint160 is necessary for Solidity 0.8.0+ compatibility.
// Direct casting from address to uint256 is not allowed in Solidity 0.8.0 and later.

Copilot uses AI. Check for mistakes.
@zguesmi zguesmi self-assigned this Oct 24, 2025
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.

1 participant