feat: migrate old IexecAccessorsFacet to IexecPocoAccessorsFacet #259

gfournieriExec · 2025-09-18T15:30:50Z

To test
on one terminal:
ARBITRUM_FORK=true npx hardhat node --no-deploy
on another terminal :

(export ARBITRUM_FORK=true; \
npx hardhat run scripts/upgrades/accessors/deploy-and-update-accessor-facet.ts --network external-hardhat)

… functionality and proxy updates

Copilot

Pull Request Overview

This PR migrates from the old IexecAccessorsFacet to a new IexecPocoAccessorsFacet as part of a diamond proxy upgrade process. The migration includes expanding the accessor functionality to include token, account, category, registry, and constants accessors.

Key changes:

Replaces the old accessor facet with an expanded IexecPocoAccessorsFacet containing comprehensive accessor methods
Adds scripts for deploying and updating the diamond proxy with the new facet
Updates configuration to support Arbitrum fork testing
Updates test expectations to reflect new error handling behavior

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`contracts/facets/IexecPocoAccessorsFacet.sol`	Expanded facet with comprehensive accessor methods for tokens, accounts, categories, registries, and constants
`contracts/interfaces/IexecPocoAccessors.sol`	Updated interface definition to include all new accessor methods
`scripts/updateProxy/update-diamond-proxy.ts`	Main orchestration script for the proxy update process
`scripts/updateProxy/1_update-proxy-with-new-facet.ts`	Detailed diamond cut implementation to replace old accessor facets
`scripts/updateProxy/0_deploy-updated-accessor-facet.ts`	Script to deploy the new accessor facet
`hardhat.config.ts`	Added Arbitrum fork configuration for testing
`deploy/0_deploy.ts`	Commented out old accessor facet deployment
`test/byContract/IexecCategoryManager/IexecCategoryManager.test.ts`	Updated test assertion for error handling

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

scripts/updateProxy/update-diamond-proxy.ts

scripts/updateProxy/1_update-proxy-with-new-facet.ts

codecov · 2025-09-18T15:37:24Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.29%. Comparing base (f7c634b) to head (999aac0).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #259      +/-   ##
==========================================
- Coverage   83.66%   78.29%   -5.37%     
==========================================
  Files          38       38              
  Lines        1218     1304      +86     
  Branches      227      229       +2     
==========================================
+ Hits         1019     1021       +2     
- Misses        199      283      +84

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

contracts/interfaces/IexecPocoAccessors.sol

hardhat.config.ts

scripts/updateProxy/0_deploy-updated-accessor-facet.ts

scripts/updateProxy/1_update-proxy-with-new-facet.ts

test/byContract/IexecCategoryManager/IexecCategoryManager.test.ts

contracts/facets/IexecPocoAccessorsFacet.sol

Le-Caignec · 2025-09-19T07:46:46Z

contracts/facets/IexecPocoAccessorsFacet.sol

            );
    }
+
+    // ========= Token and Account Accessors =========


Which token ? RLC or SRLC ?
Can we precise that ?

feat: change to pure instead of view

Le-Caignec · 2025-09-19T07:49:45Z

deploy/0_deploy.ts

    const facets = [
        new IexecAccessorsABILegacyFacet__factory(),
-        new IexecAccessorsFacet__factory(),
+        // new IexecAccessorsFacet__factory(), to test the updated IexecPocoAccessorsFacet


can you replace it directly with the new facet ?

already the case, will remove the line with the comment

feat: remove commented code for IexecAccessorsFacet in deploy script

Le-Caignec · 2025-09-19T07:53:14Z

test/byContract/IexecCategoryManager/IexecCategoryManager.test.ts

        await expect(
            iexecPocoAsAnyone.viewCategory(lastCategoryIndex + 1n),
-        ).to.be.revertedWithoutReason();
+        ).to.be.revertedWithPanic(0x32);


why it changed ?
Why we don't want to introduced custom error ?

Checking why it change
But at least we don't want to change the logic with intoducing a custom error now
Could be done later if needed

that's due to @8 solidity https://docs.soliditylang.org/en/v0.8.23/control-structures.html#panic-via-assert-and-error-via-require

Le-Caignec · 2025-09-19T07:59:05Z

scripts/updateProxy/1_update-proxy-with-new-facet.ts

+        const hasSpecificFunction = facet.functionSelectors.includes(specificFunctionSignature);
+        if (hasSpecificFunction) {
+            oldAccessorFacets.add(facet.facetAddress);
+        }


I don't understand this part ? Can you explain it ?

explained on slack

Le-Caignec · 2025-09-22T08:42:58Z

hardhat.config.ts

+            ...(isArbitrumFork && {
+                forking: {
+                    url: process.env.ARBITRUM_RPC_URL || 'https://arbitrum.gateway.tenderly.co',
+                    blockNumber: process.env.ARBITRUM_BLOCK_NUMBER


Why specify the block number ? By default, I think it uses the latest block. Isn’t it correct to rely on the default value ?

feat: remove blockNumber configuration for Arbitrum forking

Le-Caignec · 2025-09-22T08:48:17Z

scripts/upgrades/accessors/1_update-proxy-with-new-facet.ts

+
+    const [account] = await ethers.getSigners();
+
+    const updatedFacetAddress = (await deployments.get('IexecPocoAccessorsFacet')).address; //C


Suggested change

const updatedFacetAddress = (await deployments.get('IexecPocoAccessorsFacet')).address; //C

const updatedFacetAddress = (await deployments.get('IexecPocoAccessorsFacet')).address;

fix: remove commented code in update proxy script for IexecPocoAccessorsFacet

Le-Caignec · 2025-09-22T08:59:34Z

scripts/upgrades/accessors/1_update-proxy-with-new-facet.ts

+    }
+
+    // Functions to add - ALL functions from the new facet, but exclude any that exist in other (non-accessor) facets
+    const newFacetFactory = new IexecPocoAccessorsFacet__factory(iexecLibOrders); //C


Suggested change

const newFacetFactory = new IexecPocoAccessorsFacet__factory(iexecLibOrders); //C

const newFacetFactory = new IexecPocoAccessorsFacet__factory(iexecLibOrders);

fix: remove commented code in update proxy script for IexecPocoAccessorsFacet

Le-Caignec · 2025-09-22T09:39:12Z

scripts/upgrades/accessors/1_update-proxy-with-new-facet.ts

+    const functionsToRemove: string[] = [];
+    const functionsToRemoveByFacet = new Map<string, string[]>();


Using functionsToRemoveByFacet is enough, right?

refactor: remove unused functionsToRemove array in update proxy script

scripts/upgrades/accessors/1_update-proxy-with-new-facet.ts

…orsFacet

… in proxy upgrade script

…et in new script

scripts/upgrades/accessors/deploy-and-update-accessor-facet.ts

…pt for IexecPocoAccessorsFacet

…y for new facet integration

…nd update test to expect revert without reason

scripts/upgrades/accessors/deploy-and-update-accessor-facet.ts

… for IexecPocoAccessorsFacet

… deploy script for IexecPocoAccessorsFacet

… for IexecPocoAccessorsFacet

…onated signer for fork testing

contracts/facets/IexecPocoAccessorsFacet.sol

…tegory function

scripts/upgrades/accessors/deploy-and-update-accessor-facet.ts

…old facets in deploy script

feat: Update IexecPocoAccessorsFacet and related scripts for enhanced…

f34d071

… functionality and proxy updates

gfournieriExec requested review from zguesmi, Le-Caignec and Copilot September 18, 2025 15:30

Copilot AI reviewed Sep 18, 2025

View reviewed changes

scripts/updateProxy/update-diamond-proxy.ts Outdated Show resolved Hide resolved

scripts/updateProxy/1_update-proxy-with-new-facet.ts Outdated Show resolved Hide resolved

scripts/updateProxy/1_update-proxy-with-new-facet.ts Outdated Show resolved Hide resolved

zguesmi reviewed Sep 19, 2025

View reviewed changes

test/byContract/IexecCategoryManager/IexecCategoryManager.test.ts Outdated Show resolved Hide resolved

zguesmi assigned gfournieriExec Sep 19, 2025

Le-Caignec reviewed Sep 19, 2025

View reviewed changes

gfournieriExec and others added 8 commits September 22, 2025 08:14

feat: Remove useless file

4a44957

feat: Refactor deal and task accessors in IexecPocoAccessorsFacet

99b1b47

feat: Update Arbitrum RPC URL in Hardhat configuration

be36a3a

feat: move new upgrades scripts in accessors subfolder

7c7fe07

feat: refactor deployment script for IexecPocoAccessorsFacet

8b261fd

feat: change to pure instead of view

b27b116

feat: remove commented code for IexecAccessorsFacet in deploy script

9e1d7eb

Merge branch 'main' into feature/new-viewer-fct

27f02ab

Le-Caignec reviewed Sep 22, 2025

View reviewed changes

Le-Caignec approved these changes Sep 22, 2025

View reviewed changes

gfournieriExec added 6 commits September 22, 2025 11:53

feat: remove blockNumber configuration for Arbitrum forking

2939683

fix: remove commented code in update proxy script for IexecPocoAccess…

99acad3

…orsFacet

refactor: remove unused functionsToRemove array in update proxy script

359d32c

refactor: remove deployment record update for IexecPocoAccessorsFacet…

aa5d0e1

… in proxy upgrade script

feat: implement deployment and update logic for IexecPocoAccessorsFac…

cc99fa1

…et in new script

refactor: simplify old accessor facets identification in deploy script

ce727e7

zguesmi reviewed Sep 22, 2025

View reviewed changes

scripts/upgrades/accessors/deploy-and-update-accessor-facet.ts Outdated Show resolved Hide resolved

scripts/upgrades/accessors/deploy-and-update-accessor-facet.ts Outdated Show resolved Hide resolved

gfournieriExec added 3 commits September 22, 2025 17:12

refactor: update variable names for clarity in deploy and update scri…

22b517e

…pt for IexecPocoAccessorsFacet

refactor: replace function selector retrieval with linkContractToProx…

78d8a97

…y for new facet integration

fix: add boundary check for category index in viewCategory function a…

706cbb0

…nd update test to expect revert without reason

zguesmi reviewed Sep 23, 2025

View reviewed changes

gfournieriExec added 4 commits September 23, 2025 10:56

refactor: remove unnecessary local fork block mining in deploy script…

5033eb9

… for IexecPocoAccessorsFacet

refactor: streamline diamond proxy interaction and enhance logging in…

f4204dc

… deploy script for IexecPocoAccessorsFacet

refactor: simplify old accessor facets removal logic in deploy script…

49a5e4b

… for IexecPocoAccessorsFacet

refactor: update proxy owner signer logic to conditionally use impers…

c46526a

…onated signer for fork testing

zguesmi reviewed Sep 23, 2025

View reviewed changes

contracts/facets/IexecPocoAccessorsFacet.sol Show resolved Hide resolved

fix: ensure retro-compatibility by reverting without reason in viewCa…

b1105fd

…tegory function

zguesmi approved these changes Sep 23, 2025

View reviewed changes

refactor: fix log message formatting and enhance removal logging for …

999aac0

…old facets in deploy script

gfournieriExec merged commit 786bee4 into main Sep 23, 2025
3 of 4 checks passed

gfournieriExec deleted the feature/new-viewer-fct branch September 23, 2025 11:32

This was referenced Sep 23, 2025

chore(main): release poco 6.1.0 #257

Closed

chore(main): release poco 6.1.0 #263

Closed

chore(main): release poco 6.1.0 #265

Draft


		const [account] = await ethers.getSigners();

		const updatedFacetAddress = (await deployments.get('IexecPocoAccessorsFacet')).address; //C

	const newFacetFactory = new IexecPocoAccessorsFacet__factory(iexecLibOrders); //C
	const newFacetFactory = new IexecPocoAccessorsFacet__factory(iexecLibOrders);

		const functionsToRemove: string[] = [];
		const functionsToRemoveByFacet = new Map<string, string[]>();

feat: migrate old IexecAccessorsFacet to IexecPocoAccessorsFacet #259

feat: migrate old IexecAccessorsFacet to IexecPocoAccessorsFacet #259

Uh oh!

Conversation

gfournieriExec commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gfournieriExec commented Sep 18, 2025 •

edited

Loading

codecov bot commented Sep 18, 2025 •

edited

Loading