From 1d9f9a55775ab3e4be1970af83b68d97d3f4e258 Mon Sep 17 00:00:00 2001 From: Le-Caignec Date: Mon, 31 Aug 2026 18:37:31 +0200 Subject: [PATCH] feat: remove temporary migration shims and update error handling in contracts and tests --- contracts/facets/IexecEscrowTokenFacet.sol | 32 +++---------------- contracts/registries/Registry.sol | 16 +--------- test/byContract/IexecERC20/IexecERC20.test.ts | 8 ++--- .../IexecEscrow/IexecEscrowToken.test.ts | 8 ++--- test/byContract/registries/registries.test.ts | 31 ++++++++++-------- 5 files changed, 30 insertions(+), 65 deletions(-) diff --git a/contracts/facets/IexecEscrowTokenFacet.sol b/contracts/facets/IexecEscrowTokenFacet.sol index 802ceea5b..c49b907bf 100644 --- a/contracts/facets/IexecEscrowTokenFacet.sol +++ b/contracts/facets/IexecEscrowTokenFacet.sol @@ -238,13 +238,7 @@ contract IexecEscrowTokenFacet is IexecEscrowToken, IexecTokenSpender, IexecERC2 ) external override returns (bool) { PocoStorageLib.PocoStorage storage $ = PocoStorageLib.getPocoStorage(); _transfer(sender, recipient, amount); - // TEMPORARY MIGRATION FIX: Check allowance to prevent underflow and revert without reason for backward compatibility - // TODO: Remove this in the next major version - uint256 currentAllowance = $.m_allowances[sender][_msgSender()]; - if (currentAllowance < amount) { - revert(); - } - _approve(sender, _msgSender(), currentAllowance - amount); + _approve(sender, _msgSender(), $.m_allowances[sender][_msgSender()] - amount); return true; } @@ -262,13 +256,7 @@ contract IexecEscrowTokenFacet is IexecEscrowToken, IexecTokenSpender, IexecERC2 uint256 subtractedValue ) external override returns (bool) { PocoStorageLib.PocoStorage storage $ = PocoStorageLib.getPocoStorage(); - // TEMPORARY MIGRATION FIX: Check allowance to prevent underflow and revert without reason for backward compatibility - // TODO: Remove this in the next major version - uint256 currentAllowance = $.m_allowances[_msgSender()][spender]; - if (currentAllowance < subtractedValue) { - revert(); - } - _approve(_msgSender(), spender, currentAllowance - subtractedValue); + _approve(_msgSender(), spender, $.m_allowances[_msgSender()][spender] - subtractedValue); return true; } @@ -276,13 +264,7 @@ contract IexecEscrowTokenFacet is IexecEscrowToken, IexecTokenSpender, IexecERC2 require(sender != address(0), "ERC20: transfer from the zero address"); require(recipient != address(0), "ERC20: transfer to the zero address"); PocoStorageLib.PocoStorage storage $ = PocoStorageLib.getPocoStorage(); - uint256 senderBalance = $.m_balances[sender]; - // TEMPORARY MIGRATION FIX: Check balance to prevent underflow and revert without reason for backward compatibility - // TODO: Remove this in the next major version - if (senderBalance < amount) { - revert(); - } - $.m_balances[sender] = senderBalance - amount; + $.m_balances[sender] = $.m_balances[sender] - amount; $.m_balances[recipient] = $.m_balances[recipient] + amount; emit Transfer(sender, recipient, amount); } @@ -302,14 +284,8 @@ contract IexecEscrowTokenFacet is IexecEscrowToken, IexecTokenSpender, IexecERC2 function _burn(address account, uint256 amount) internal { require(account != address(0), "ERC20: burn from the zero address"); PocoStorageLib.PocoStorage storage $ = PocoStorageLib.getPocoStorage(); - uint256 accountBalance = $.m_balances[account]; - // TEMPORARY MIGRATION FIX: Check balance to prevent underflow and revert without reason for backward compatibility - // TODO: Remove this in the next major version - if (accountBalance < amount) { - revert(); - } $.m_totalSupply = $.m_totalSupply - amount; - $.m_balances[account] = accountBalance - amount; + $.m_balances[account] = $.m_balances[account] - amount; emit Transfer(account, address(0), amount); } diff --git a/contracts/registries/Registry.sol b/contracts/registries/Registry.sol index a5eea9fd9..92b504e27 100644 --- a/contracts/registries/Registry.sol +++ b/contracts/registries/Registry.sol @@ -33,14 +33,6 @@ abstract contract Registry is IRegistry, ERC721Enumerable, Ownable { proxyCodeHash = keccak256(proxyCode); } - // TEMPORARY MIGRATION FIX: Override _checkOwner to catch custom errors and throw string errors for backward compatibility - // TODO: Remove this override in the next major version - function _checkOwner() internal view override { - if (owner() != _msgSender()) { - revert("Ownable: caller is not the owner"); - } - } - function initialize(address _previous) external onlyOwner { require(!initialized); initialized = true; @@ -70,14 +62,8 @@ abstract contract Registry is IRegistry, ERC721Enumerable, Ownable { /* Factory */ function _mintCreate(address _owner, bytes memory _args) internal returns (address) { - // TEMPORARY MIGRATION FIX: Check if contract already exists to revert without custom error for backward compatibility - // TODO: Remove this in the next major version - address entry = _mintPredict(_owner, _args); - if (entry.code.length > 0) { - revert("Create2: Failed on deploy"); - } // Create entry (proxy) - entry = Create2.deploy(0, keccak256(abi.encodePacked(_args, _owner)), proxyCode); + address entry = Create2.deploy(0, keccak256(abi.encodePacked(_args, _owner)), proxyCode); InitializableUpgradeabilityProxy(payable(entry)).initialize(master, _args); // Mint corresponding token _mint(_owner, uint256(uint160(entry))); diff --git a/test/byContract/IexecERC20/IexecERC20.test.ts b/test/byContract/IexecERC20/IexecERC20.test.ts index 3c1fa0c09..85d30273e 100644 --- a/test/byContract/IexecERC20/IexecERC20.test.ts +++ b/test/byContract/IexecERC20/IexecERC20.test.ts @@ -97,7 +97,7 @@ describe('ERC20', async () => { it('Should not transfer when sender balance is too low', async () => { await expect( iexecPocoAsHolder.transfer(recipient.address, value + 1n), - ).to.be.revertedWithoutReason(); + ).to.be.revertedWithPanic(0x11); }); }); @@ -201,13 +201,13 @@ describe('ERC20', async () => { it('Should not transferFrom when owner balance is too low', async () => { await expect( iexecPocoAsSpender.transferFrom(holder.address, spender.address, value + 1n), - ).to.be.revertedWithoutReason(); + ).to.be.revertedWithPanic(0x11); }); it('Should not transferFrom when spender allowance is too low', async () => { await iexecPocoAsHolder.approve(spender.address, value - 1n).then((tx) => tx.wait()); await expect( iexecPocoAsSpender.transferFrom(holder.address, spender.address, value), - ).to.be.revertedWithoutReason(); + ).to.be.revertedWithPanic(0x11); }); }); @@ -260,7 +260,7 @@ describe('ERC20', async () => { it('Should not decrease allowance of a value greater than old allowance', async () => { await expect( iexecPocoAsHolder.decreaseAllowance(spender.address, 1), - ).to.be.revertedWithoutReason(); + ).to.be.revertedWithPanic(0x11); }); it('Should not decrease allowance from the zero address', async () => { await expect( diff --git a/test/byContract/IexecEscrow/IexecEscrowToken.test.ts b/test/byContract/IexecEscrow/IexecEscrowToken.test.ts index f7bf57223..10978d2ec 100644 --- a/test/byContract/IexecEscrow/IexecEscrowToken.test.ts +++ b/test/byContract/IexecEscrow/IexecEscrowToken.test.ts @@ -305,13 +305,13 @@ describe('IexecEscrowToken', () => { .withArgs(proxyAddress, accountA.address, 0); }); it('Should not withdraw tokens with empty balance', async () => { - await expect(iexecPocoAsAccountA.withdraw(amount)).to.be.revertedWithoutReason(); + await expect(iexecPocoAsAccountA.withdraw(amount)).to.be.revertedWithPanic(0x11); }); it('Should not withdraw tokens with insufficient balance', async () => { await rlcInstanceAsAccountA.approve(proxyAddress, amount).then((tx) => tx.wait()); await iexecPocoAsAccountA.deposit(amount).then((tx) => tx.wait()); - await expect(iexecPocoAsAccountA.withdraw(amount + 1n)).to.be.revertedWithoutReason(); + await expect(iexecPocoAsAccountA.withdraw(amount + 1n)).to.be.revertedWithPanic(0x11); }); }); @@ -366,7 +366,7 @@ describe('IexecEscrowToken', () => { it('Should not withdraw to another address with empty balance', async () => { await expect( iexecPocoAsAccountA.withdrawTo(amount, accountB.address), - ).to.be.revertedWithoutReason(); + ).to.be.revertedWithPanic(0x11); }); it('Should not withdraw to another address with insufficient balance', async () => { await rlcInstanceAsAccountA.approve(proxyAddress, amount).then((tx) => tx.wait()); @@ -374,7 +374,7 @@ describe('IexecEscrowToken', () => { await expect( iexecPocoAsAccountA.withdrawTo(amount + 1n, accountB.address), - ).to.be.revertedWithoutReason(); + ).to.be.revertedWithPanic(0x11); }); }); diff --git a/test/byContract/registries/registries.test.ts b/test/byContract/registries/registries.test.ts index 597684f99..98428c63b 100644 --- a/test/byContract/registries/registries.test.ts +++ b/test/byContract/registries/registries.test.ts @@ -97,14 +97,17 @@ describe('Registries', () => { }); it('Should not initialize when user is not the owner', async () => { - await expect(appRegistry.initialize(ZeroAddress)).to.be.revertedWith( - 'Ownable: caller is not the owner', + await expect(appRegistry.initialize(ZeroAddress)).to.be.revertedWithCustomError( + appRegistry, + 'OwnableUnauthorizedAccount', ); - await expect(datasetRegistry.initialize(ZeroAddress)).to.be.revertedWith( - 'Ownable: caller is not the owner', + await expect(datasetRegistry.initialize(ZeroAddress)).to.be.revertedWithCustomError( + datasetRegistry, + 'OwnableUnauthorizedAccount', ); - await expect(workerpoolRegistry.initialize(ZeroAddress)).to.be.revertedWith( - 'Ownable: caller is not the owner', + await expect(workerpoolRegistry.initialize(ZeroAddress)).to.be.revertedWithCustomError( + workerpoolRegistry, + 'OwnableUnauthorizedAccount', ); }); @@ -131,15 +134,15 @@ describe('Registries', () => { }); it('Should not set base URI when user is not the owner', async () => { - await expect(appRegistry.setBaseURI(`https://new.url.iex.ec/app/`)).to.be.revertedWith( - 'Ownable: caller is not the owner', - ); + await expect( + appRegistry.setBaseURI(`https://new.url.iex.ec/app/`), + ).to.be.revertedWithCustomError(appRegistry, 'OwnableUnauthorizedAccount'); await expect( datasetRegistry.setBaseURI(`https://new.url.iex.ec/dataset/`), - ).to.be.revertedWith('Ownable: caller is not the owner'); + ).to.be.revertedWithCustomError(datasetRegistry, 'OwnableUnauthorizedAccount'); await expect( workerpoolRegistry.setBaseURI(`https://new.url.iex.ec/workerpool/`), - ).to.be.revertedWith('Ownable: caller is not the owner'); + ).to.be.revertedWithCustomError(workerpoolRegistry, 'OwnableUnauthorizedAccount'); }); }); @@ -280,7 +283,7 @@ describe('Registries', () => { await expect( appRegistry.createApp(appProvider.address, ...createAppArgs), - ).to.be.revertedWith('Create2: Failed on deploy'); + ).to.be.revertedWithCustomError(appRegistry, 'Create2FailedDeployment'); }); it('Should check that a new app is well registered on new app registry', async () => { @@ -385,7 +388,7 @@ describe('Registries', () => { await expect( datasetRegistry.createDataset(datasetProvider.address, ...createDatasetArgs), - ).to.be.revertedWith('Create2: Failed on deploy'); + ).to.be.revertedWithCustomError(datasetRegistry, 'Create2FailedDeployment'); }); }); @@ -469,7 +472,7 @@ describe('Registries', () => { await expect( workerpoolRegistry.createWorkerpool(scheduler.address, ...createWorkerpoolArgs), - ).to.be.revertedWith('Create2: Failed on deploy'); + ).to.be.revertedWithCustomError(workerpoolRegistry, 'Create2FailedDeployment'); }); }); });