feat: Use namespaced storage #236
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the contract storage from a traditional diamond proxy pattern to use namespaced storage following EIP-7201. The changes replace direct access to storage variables with calls to a getPocoStorage() function that returns a storage struct pointer.
Key changes:
- Replace direct storage variable access (e.g.,
m_balances[account]) with namespaced storage access (e.g.,$.m_balances[account]) - Restructure the
Storecontract to use aPocoStoragestruct with namespaced storage location - Update all facet contracts to retrieve storage through
getPocoStorage()function
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Store.v8.sol | Converts storage from individual variables to namespaced PocoStorage struct |
| Store.sol | Converts storage from individual variables to namespaced PocoStorage struct |
| IexecAccessorsFacet.sol | Updates all storage access to use namespaced storage pattern |
| IexecERC20Core.sol | Updates balance and allowance operations to use namespaced storage |
| IexecEscrow.v8.sol | Updates escrow operations (lock/unlock/seize) to use namespaced storage |
| IexecPoco1Facet.sol | Updates deal matching and order management to use namespaced storage |
| IexecPoco2Facet.sol | Updates consensus and contribution logic to use namespaced storage |
| IexecPocoBoostFacet.sol | Updates boost workflow operations to use namespaced storage |
| IexecConfigurationFacet.sol | Updates configuration management to use namespaced storage |
| Multiple other facets | Updates various operations to use namespaced storage pattern |
|
Is this different due to solidity version ? |
Yes. v6 vs v8. |
There was a problem hiding this comment.
What about using this type of comment to split the storage and make it more readable ? => https://github.com/iExecBlockchainComputing/rlc-multichain/blob/09761d4bb9c252a6ca9a0020d2ad7979ec9ae75f/src/RLCLiquidityUnifier.sol#L62
There was a problem hiding this comment.
Not fan of these comments personally, we use it in other projects.
We can enhance later.
| mapping(bytes32 => address) m_presigned; | ||
| // Each order has a volume (>=1). This tracks how much is consumed from | ||
| // the volume of each order. Mapping an order hash to its consumed amount. | ||
| mapping(bytes32 => uint256) m_consumed; | ||
| // a mapping to store PoCo classic deals. | ||
| mapping(bytes32 => IexecLibCore_v5.Deal) m_deals; | ||
| mapping(bytes32 => IexecLibCore_v5.Task) m_tasks; // per task | ||
| mapping(bytes32 => IexecLibCore_v5.Consensus) m_consensus; // per task | ||
| mapping(bytes32 => mapping(address => IexecLibCore_v5.Contribution)) m_contributions; // per task-worker | ||
| mapping(address => uint256) m_workerScores; // per worker |
There was a problem hiding this comment.
what do you think using the same format of commentary before each mapping like :
https://github.com/iExecBlockchainComputing/dataprotector-sdk/blob/bab0e444f59932c88b546b02dda88be44d2edd6b/packages/sharing-smart-contract/contracts/DataProtectorSharing.sol#L50
There was a problem hiding this comment.
Yes done in this PR Add comments to store mappings
| // Use in registries. | ||
| interface IRegistry is IERC721Enumerable { | ||
| function isRegistered(address _entry) external view returns (bool); | ||
| } |
There was a problem hiding this comment.
should we move that into a dedicated file instead ?
There was a problem hiding this comment.
Not needed because it's only used here.
| // Seized funds of workerpools that do not honor their deals are sent | ||
| // out to this kitty address. | ||
| // It is determined with address(uint256(keccak256(bytes('iExecKitty'))) - 1). | ||
| address public constant KITTY_ADDRESS = 0x99c2268479b93fDe36232351229815DF80837e23; |
There was a problem hiding this comment.
Why not having a variable (uint256) directly into the PocoStorage struct ?
There was a problem hiding this comment.
Using constant reduces gas cost.
This PR is purely for renaming. Fixes are in the next PR to simplify diff.
Migration steps:
receiveandfallbackbehavior inDiamond.solModuletoFacetin contracts namesDiamond.solfeature/diamondindevelop