16 distributor upgrades handler #53
Conversation
- Added `MockFailingDistribution`, `MockLowLevelDistribution`, `MockPanicDistribution`, `MockMigration`, `MockRepository`, and `MockLatestVersionDistribution` for testing purposes. - Updated `LatestVersionDistribution` to allow instantiation with a virtual method. - Enhanced `Distributor` and `OwnableDistributor` to support new mock distributions. - Updated VSCode settings to include a new word for spell checking.
|
|
@coderabbitai review |
1 similar comment
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces extensive architectural changes to the EDS (Ethereum Distribution System) smart contract suite, focusing on modularity, upgradability, and version management. Major refactoring replaces the previous "abstracts" layer with new, more flexible contracts and interfaces for distributors, installers, distributions, and repositories. The system now supports version migrations, semantic versioning, and a robust upgrade workflow, including migration strategies and hooks for runtime validation. New libraries and utility contracts are added for code indexing and proxy management. The documentation is significantly expanded with guides on distributions, repositories, indexing, versioning, and upgradability. Comprehensive new and updated tests cover the expanded feature set and ensure correctness. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Installer
participant Distributor
participant Repository
participant MigrationContract
participant App
User->>Installer: install(distributor, distributionId, args)
Installer->>Distributor: instantiate(distributionId, args)
Distributor->>Repository: get(versionRequirement)
Repository-->>Distributor: Source, Version, MigrationHash
Distributor->>App: deploy/clone/init
Distributor-->>Installer: appComponents, name, version
Installer-->>User: appId
User->>Installer: upgradeApp(appId, migrationId, userCalldata)
Installer->>Distributor: upgradeUserInstance(appId, migrationId, userCalldata)
Distributor->>MigrationContract: migrate(appComponents, oldVersion, newVersion, repository, distributorCalldata, userCalldata)
MigrationContract-->>Distributor: (migration complete)
Distributor-->>Installer: newVersion
Installer-->>User: AppUpgraded event
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 32
🔭 Outside diff range comments (4)
src/middleware/AuthorizationMiddleware.sol (4)
21-21: 🛠️ Refactor suggestionStorage position identifier still references old contract name.
Update the storage position constant to reflect the contract's new name.
- bytes32 private constant SACM_STORAGE_POSITION = keccak256("simple.access.manager.storage.position"); + bytes32 private constant AUTH_MIDDLEWARE_STORAGE_POSITION = keccak256("authorization.middleware.storage.position");Also update all references to this constant in the
getStorage()function.
23-28: 🛠️ Refactor suggestionStorage function references outdated constant name.
Update the
getStorage()function to use the renamed storage position constant.function getStorage() private pure returns (Storage storage s) { - bytes32 position = SACM_STORAGE_POSITION; + bytes32 position = AUTH_MIDDLEWARE_STORAGE_POSITION; assembly { s.slot := position } }
30-34: 🛠️ Refactor suggestionStruct name not updated to match renamed contract.
Update the struct name to maintain naming consistency with the renamed contract.
- struct SimpleAccessManagerInitializer { + struct AuthorizationMiddlewareInitializer { bytes4 selector; address[] disallowedAddresses; bool distributionComponentsOnly; }Don't forget to also update any references to this struct in the code, such as in the initialize function parameter.
40-44: 🛠️ Refactor suggestionUpdate function parameter to use renamed struct.
The initialize function should use the renamed struct.
function initialize( - SimpleAccessManagerInitializer[] memory methodSettings, + AuthorizationMiddlewareInitializer[] memory methodSettings, address target, IDistributor distributor ) public initializer {
🧹 Nitpick comments (54)
.vscode/settings.json (1)
2-2: Validate dictionary entries: potential typos
The entry"continous"appears to be a misspelling of “continuous”. If this is intentional (e.g., a domain-specific term), it’s fine to keep; otherwise please correct it to avoid confusion. The other entries (extcodecopy,mstore,retval,strg) look like EVM opcodes or shorthand and are appropriate..cursorignore (1)
1-2: Also update version control ignore rules
To avoid accidentally committing sensitive files to Git, consider mirroring these entries in your.gitignore(if not already present).package.json (1)
67-67: Dependency version upgrade for solidity-coverage.The solidity-coverage package has been updated from ^0.8.5 to ^0.8.14. This upgrade provides bug fixes and performance improvements for code coverage analysis.
Consider documenting the specific benefits this version brings to your project in the PR description or commit message.
src/middleware/ERC7746Hooked.sol (1)
6-13: Documentation references outdated contract name.The documentation comments still reference "ERC7746Middleware" instead of the new contract name "ERC7746Hooked".
- * @title ERC7746Middleware + * @title ERC7746Hookedsrc/middleware/LibMiddleware.sol (1)
31-41: Good addition of dynamic layer management functionalityThe new
changeLayersfunction allows for in-place updates to middleware layers, which is a useful capability for dynamic reconfiguration. The implementation correctly handles both growing and shrinking the array.One optimization suggestion: consider adding a check to avoid unnecessary operations when the new array length equals the existing array length.
function changeLayers(LayerStruct[] memory newLayers) internal { LayerStruct[] storage ls = accessLayersStorage(); + // Skip resizing if lengths are equal (minor gas optimization) + if (newLayers.length == ls.length) { + for (uint256 i; i < newLayers.length; ++i) { + ls[i] = newLayers[i]; + } + return; + } for (uint256 i; i < newLayers.length; ++i) { ls[i] = newLayers[i]; } if (newLayers.length < ls.length) { for (uint256 i; i < ls.length - newLayers.length; ++i) { ls.pop(); } } }hardhat.config.ts (1)
134-149: Redundant commented-out compiler overrides.The commented-out compiler overrides for
src/erc7744/ERC7744.soluse identical settings to the main compiler configuration. This doesn't add any value and should either be removed or modified to have different settings if specific customization is needed.- // overrides: { - // "src/erc7744/ERC7744.sol": { - // version: "0.8.28", - // settings: { - // evmVersion: "cancun", - // viaIR: true, - // optimizer: { - // enabled: true, - // runs: 2000 - // }, - // metadata: { - // bytecodeHash: "none" - // } - // } - // } - // }docs/Versioning.md (2)
3-3: Grammar correction needed.Missing article "a" before "Semantic versioning system".
-EDS implements Semantic versioning system within one bytes32 word. +EDS implements a Semantic versioning system within one bytes32 word.🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: You might be missing the article “a” here.
Context: # Versioning EDS implements Semantic versioning system within one bytes32 wo...(AI_EN_LECTOR_MISSING_DETERMINER_A)
5-5: Add comma for clarity.A comma is needed after "On chain" for better readability.
-On chain semantic versioning is managed by [LibSemver](../src/versioning/LibSemver.sol). +On-chain, semantic versioning is managed by [LibSemver](../src/versioning/LibSemver.sol).🧰 Tools
🪛 LanguageTool
[uncategorized] ~5-~5: Possible missing comma found.
Context: ...ing system within one bytes32 word. On chain semantic versioning is managed by [LibS...(AI_HYDRA_LEO_MISSING_COMMA)
src/erc7744/ERC7744.sol (1)
14-21: Well-implemented EIP7702 detection with assembly.The new
isEIP7702function effectively detects contracts following EIP7702 by checking the prefix bytes, which is a more direct and efficient approach than comparing code hashes.Consider adding a brief comment explaining the meaning of the
0xef0100magic bytes for future maintainers:function isEIP7702(address account) public view returns (bool) { bytes3 prefix; assembly { extcodecopy(account, 0, mload(0x40), 3) // Copy first 3 bytes to memory prefix := mload(0x40) // Load the 3 bytes from memory } + // The 0xef0100 prefix is defined in EIP7702 for identifying delegated EOAs return prefix == bytes3(0xef0100); }test/eds/LatestVersionDistribution.ts (3)
18-18: Consider using a more specific type instead of 'any'.Using
anytype, even with a comment explaining why, bypasses TypeScript's type checking benefits. Consider creating a more specific interface that includes thesources()method.// Create an interface that extends the generated type but includes the sources method interface LatestVersionDistributionWithSources extends LatestVersionDistribution { sources(): Promise<[string[], string, number]>; } // Then use this type instead of any let latestVersionDist: LatestVersionDistributionWithSources;
93-98: Implement the skipped instantiate test.The test for the instantiate method is currently skipped with a comment indicating it requires a fully mocked repository. Consider implementing this test to ensure complete coverage of the contract's functionality.
describe("instantiate", function () { it("should instantiate using the latest version from repository", async function () { // Mock the repository's getLatestVersion method if needed // Or ensure the repository fixture is properly set up for this test const instantiateResult = await latestVersionDist.instantiate("0x"); // Verify the instantiation result matches expectations // e.g., check returned addresses, version, etc. expect(instantiateResult).to.not.be.undefined; // Additional assertions based on expected behavior }); it("should revert when the repository has no versions", async function () { // Setup a repository with no versions const emptyRepo = await (await ethers.getContractFactory("MockRepository")).deploy(); // Deploy distribution with empty repo const emptyRepoDist = await (await ethers.getContractFactory("MockLatestVersionDistribution")).deploy( emptyRepo.address, metadata ); // Expect revert when trying to instantiate await expect(emptyRepoDist.instantiate("0x")).to.be.revertedWith( // Expected revert message "No versions available" ); }); });
1-99: Consider adding tests for repository version changes.The current tests verify the initial setup but don't test how the LatestVersionDistribution handles changes in the repository, such as when a new latest version is added.
describe("Repository version changes", function () { it("should use the updated latest version after repository update", async function () { // Initial state - already tested in previous tests // Add a new version to the repository const version2 = { major: 1, minor: 1, patch: 0 }; const sourceId2 = ethers.utils.keccak256( ethers.utils.defaultAbiCoder.encode(["address"], [mockDistribution.address]) ); // Use different metadata for version 2 const metadataBytes2 = ethers.utils.toUtf8Bytes("metadata-v2"); // Add the new release await repository.newRelease( sourceId2, metadataBytes2, version2, ethers.utils.formatBytes32String("0x") ); // Instantiate again and verify it uses the new version const instantiateResult = await latestVersionDist.instantiate("0x"); // Verify the result shows the new version was used // This depends on what instantiate returns and how to verify the version }); });docs/Distributors.md (2)
3-3: Minor grammatical correction neededThere's a grammatical error in this sentence.
-Distributors are contracts that are used to provide trusted sources to end-users from a entity owned domain. +Distributors are contracts that are used to provide trusted sources to end-users from an entity owned domain.🧰 Tools
🪛 LanguageTool
[misspelling] ~3-~3: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...ovide trusted sources to end-users from a entity owned domain. Distributors may ...(EN_A_VS_AN)
15-15: Consider adding an articleThe sentence would read more naturally with "the" before "Distributor".
-Distributions that use [ERC7746](./ERC7746.md) middleware approach, or that are installed via [Installer](./Installer.md) can be runtime verified by Distributor. +Distributions that use [ERC7746](./ERC7746.md) middleware approach, or that are installed via [Installer](./Installer.md) can be runtime verified by the Distributor.🧰 Tools
🪛 LanguageTool
[uncategorized] ~15-~15: Possible missing article found.
Context: ...nstaller.md) can be runtime verified by Distributor. Distributor gets called with `beforeC...(AI_HYDRA_LEO_MISSING_THE)
test/eds/Installer.ts (1)
49-51: Flatten the overloaded-function access for readability.
Breaking the property access over three lines makes the intent harder to scan and clutters diffs. A single-line access keeps the test concise without losing clarity.- .connect(owner) - [ - "addDistribution(bytes32,address,string)" - ](cloneDistributionId, ethers.constants.AddressZero, "testDistribution"); + .connect(owner)["addDistribution(bytes32,address,string)"]( + cloneDistributionId, + ethers.constants.AddressZero, + "testDistribution" + );test/eds/VersionDistributor.ts (1)
97-106: Style nit – keep overloaded signature access on one line.- distributor - .connect(owner) - [ - "addDistribution(address,address,((uint64,uint64,uint128),uint8),string)" - ](repository.address, ethers.constants.AddressZero, { version: { major: 0, minor: 0, patch: 1 }, requirement: 1 }, "test") + await distributor.connect(owner)[ + "addDistribution(address,address,((uint64,uint64,uint128),uint8),string)" + ]( + repository.address, + ethers.constants.AddressZero, + { version: { major: 0, minor: 0, patch: 1 }, requirement: 1 }, + "test" + );Makes the intent clearer and reduces noise in git history.
test/eds/Repository.ts (2)
71-73: Same style nit as above – collapse overloaded selector access.
Keeps the test leaner and avoids accidental whitespace diffs.
80-88: Variable naming:dummyMigrationAddressholds a contract instance.
Exposing it as…Addressis misleading. Renaming todummyMigration(or splitting intodummyMigration&dummyMigrationAddress) prevents confusion later in the suite.-let dummyMigrationAddress: MockMigration; +let dummyMigration: MockMigration; … - dummyMigrationAddress = await MockMigration.deploy(); + dummyMigration = await MockMigration.deploy(); … - await codeIndex.register(dummyMigrationAddress.address); - dummyMigrationCodeHash = ethers.utils.keccak256( - await dummyMigrationAddress.provider.getCode(dummyMigrationAddress.address) + await codeIndex.register(dummyMigration.address); + dummyMigrationCodeHash = ethers.utils.keccak256( + await dummyMigration.provider.getCode(dummyMigration.address) );contracts/mocks/MockPanicDistribution.sol (1)
15-23: Userevert()instead ofassert(false)for intentional failure
assert(false)triggers Solidity’s Panic (0x01), which is reserved for internal invariants and consumes all remaining gas.
For a mock that is expected to revert, a conventionalrevert("MockPanic")(or a custom error) is safer and clearer to downstream tooling.- // This will cause a panic - assert(false); + // Intentionally revert to simulate a panic-style failure + revert("MockPanic");Would switching to
revert()break any test that explicitly expects a panic (0x01)?docs/contracts/index.md (2)
269-276: Parameter name still refers toinstanceIdThe codebase migrated from “instance” to “app”, yet the snippet documents
getApp(uint256 instanceId). Please rename the parameter toappIdfor consistency.-function getApp(uint256 instanceId) public view returns (address[] instaneContracts) +function getApp(uint256 appId) public view returns (address[] appContracts)
817-827: Update external view version ofgetApp/getAppsNumThe same “instance” wording appears in the external-view declarations directly below.
Mirror the correction there as well to avoid friction for SDK/ABI generators.src/mocks/MockFailingDistribution.sol (1)
9-11: Consider using a more explicit failure mechanism.The current keccak256 comparison method works, but could benefit from a clearer approach. Consider using a more direct check or defining a constant for the failure marker.
- if (args.length > 0 && keccak256(args) == keccak256(abi.encode(bytes32("FAIL")))) { + bytes32 constant FAIL_MARKER = bytes32("FAIL"); + if (args.length > 0 && keccak256(args) == keccak256(abi.encode(FAIL_MARKER))) {contracts/mocks/MockMigration.sol (1)
1-43: Implementation looks good, but consider adding more documentationThis mock contract correctly implements the IMigration interface and uses ERC165 for interface detection. The implementation is straightforward and appropriate for testing purposes, emitting events with migration details without performing actual migrations.
Consider adding NatSpec documentation for the contract and functions to improve code readability and maintainability:
+/** + * @title MockMigration + * @dev A mock implementation of the IMigration interface for testing purposes. + * This contract only emits events and doesn't perform actual migrations. + */ contract MockMigration is IMigration, ERC165 { event MigrationExecuted( address[] instances, uint256 oldVersion, uint256 newVersion, bytes distributorCalldata, bytes userCalldata ); + /** + * @dev Mocks a migration by emitting an event with the migration details. + * @param instances The addresses of instances to migrate + * @param oldVersion The version being migrated from + * @param newVersion The version being migrated to + * @param repository The repository reference + * @param distributorCalldata Additional calldata for the distributor + * @param userCalldata Additional calldata for user contracts + */ function migrate( address[] memory instances, LibSemver.Version memory oldVersion, LibSemver.Version memory newVersion, IRepository repository, bytes calldata distributorCalldata, bytes calldata userCalldata ) external override {docs/Indexer.md (2)
3-5: Missing articles in documentation textThe text has a few minor grammatical issues with missing articles.
-EDS indexer is [ERC7744](https://eips.ethereum.org/EIPS/eip-7744) standard compliant codehash registry that allows immutable code referencing in permission-less manner. +The EDS indexer is an [ERC7744](https://eips.ethereum.org/EIPS/eip-7744) standard compliant codehash registry that allows immutable code referencing in a permission-less manner. -ERC7744 indexer is the most upstream in dependency chain within the EDS ecosystem and is the only contract that functionality may be trusted by ALL parties. +The ERC7744 indexer is the most upstream in the dependency chain within the EDS ecosystem and is the only contract whose functionality may be trusted by ALL parties.🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: You might be missing the article “a” here.
Context: ...at allows immutable code referencing in permission-less manner. ERC7744 indexer is the mo...(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~5-~5: Possible missing article found.
Context: ...ERC7744 indexer is the most upstream in dependency chain within the EDS ecosystem and is t...(AI_HYDRA_LEO_MISSING_THE)
58-59: Incorrect Markdown syntax for noteThe note is using GitHub-style alerts syntax (
> [!NOTE]) which might not render correctly in all Markdown processors.Consider using more widely supported Markdown syntax:
-> [!NOTE] -> The CLI provides utilities to interact with the ERC7744 indexer. +> **Note:** The CLI provides utilities to interact with the ERC7744 indexer.🧰 Tools
🪛 LanguageTool
[style] ~58-~58: Using many exclamation marks might seem excessive (in this case: 4 exclamation marks for a text that’s 1097 characters long)
Context: ...er.get(codeHash); ``` ## Using CLI > [!NOTE] > The CLI provides utilities to in...(EN_EXCESSIVE_EXCLAMATION)
src/erc7744/LibERC7744.sol (3)
9-16: Incorrect @inheritdoc annotationsThe @inheritdoc tags are used incorrectly as they don't specify which interface function they're inheriting documentation from.
- // @inheritdoc ICodeIndex + /** + * @dev Returns the ERC7744 indexer contract interface + * @return ICodeIndex interface of the deployed indexer + */ function getContractsIndex() internal pure returns (ICodeIndex) { return INDEX_CONTRACT; } - // @inheritdoc ICodeIndex + /** + * @dev Registers a contract address with the ERC7744 indexer + * @param source The contract address to register + */ function index(address source) internal { INDEX_CONTRACT.register(source); }
22-28: Missing documentation for error and functionThe error and the function
getContainerOrThrowlack documentation.+ /** + * @dev Error thrown when attempting to retrieve a non-existent container + * @param id The code hash that wasn't found in the index + */ error AddressNotFound(bytes32 id); + /** + * @dev Retrieves a container address for a given code hash, reverting if not found + * @param id The code hash to look up + * @return The container address + * @revert AddressNotFound if the container doesn't exist + */ function getContainerOrThrow(bytes32 id) internal view returns (address) { address container = getContainer(id); if (container == address(0)) revert AddressNotFound(id); return container; }
13-20: Improve error handling for index functionThe
indexfunction forwards the call to the indexer but doesn't handle potential errors or failed registrations.Consider adding error handling or providing a version that returns success status:
function index(address source) internal { INDEX_CONTRACT.register(source); } + + /** + * @dev Safely registers a contract address and returns success status + * @param source The contract address to register + * @return success True if registration succeeded, false otherwise + */ + function indexSafe(address source) internal returns (bool success) { + // Use low-level call to handle potential errors + (success, ) = address(INDEX_CONTRACT).call( + abi.encodeWithSelector(INDEX_CONTRACT.register.selector, source) + ); + return success; + } function getContainer(bytes32 id) internal view returns (address) { return getContractsIndex().get(id); }src/mocks/MockLatestVersionDistribution.sol (3)
19-21: Consider adding test value for the sources override.This method simply calls the parent implementation without changing behavior. Consider adding a comment explaining why this override is necessary for testing, or potentially returning mock values instead.
23-25: Unused parameter in instantiate method.The
instantiatemethod accepts a bytes parameter but doesn't use it, instead callingsuper._instantiate()without parameters. If this is intentional for mocking purposes, consider adding a comment explaining why.- function instantiate(bytes memory) external override returns (address[] memory instances, bytes32 name, uint256 version) { + function instantiate(bytes memory /* params */) external override returns (address[] memory instances, bytes32 name, uint256 version) { return super._instantiate(); }
28-38: Consider optimizing the bytes32ToString function.The function uses two separate loops with the same condition. You could optimize this by storing the length from the first loop:
function bytes32ToString(bytes32 _bytes32) internal pure returns (string memory) { uint8 i = 0; while(i < 32 && _bytes32[i] != 0) { i++; } bytes memory bytesArray = new bytes(i); - for (i = 0; i < 32 && _bytes32[i] != 0; i++) { + for (uint8 j = 0; j < i; j++) { - bytesArray[i] = _bytes32[i]; + bytesArray[j] = _bytes32[j]; } return string(bytesArray); }contracts/mocks/MockFailingDistribution.sol (1)
36-38: Consider making the contractURI consistent with the distribution name.The hardcoded IPFS URI doesn't reflect the distribution name. Consider using the name in the URI for better traceability.
function contractURI() external pure returns (string memory) { - return "ipfs://mockFailingDistribution"; + return string(abi.encodePacked("ipfs://", ShortStrings.toString(distributionName))); }docs/Upgradability.md (5)
1-9: Good introduction to the upgradeability concept.The introduction clearly explains the motivation behind the EDS upgradeability pattern. Consider addressing some grammatical issues to improve readability.
# Upgrading contracts with EDS ## Introduction - EDS implements next level abstraction pattern over traditional smart contract upgradeability. It allows to upgrade contracts in a safe and easy way, which is fully owned by the end users, without sacrificing any distributor trust to app nor installer. + EDS implements a next-level abstraction pattern over traditional smart contract upgradeability. It allows users to upgrade contracts in a safe and easy way, which is fully owned by the end users, without sacrificing any distributor trust to app nor installer. ## Trust assumptions - Upgradability that is both trusted by end-user and distributor requires careful design. Hence this section describes the trust assumptions that are made by EDS. + Upgradability that is both trusted by end-user and distributor requires careful design. Hence, this section describes the trust assumptions that are made by EDS.🧰 Tools
🪛 LanguageTool
[uncategorized] ~5-~5: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...th EDS ## Introduction EDS implements next level abstraction pattern over traditional sm...(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~5-~5: Did you mean “upgrading”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...mart contract upgradeability. It allows to upgrade contracts in a safe and easy way, which...(ALLOW_TO)
[uncategorized] ~8-~8: A comma may be missing after the conjunctive/linking adverb ‘Hence’.
Context: ...nd distributor requires careful design. Hence this section describes the trust assump...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
10-29: Improve clarity in the trust assumptions section.The trust domains and relationships are well-defined, but there are several grammar and clarity issues. Consider correcting these for better readability.
- In EDS it is assumed that downstream contracts are trusted by upstream contracts by explicitly referring to them. Upstream dependencies are unaware of downstream contracts and do not trust them, however may expose public interfaces that are used by downstream contracts. + In EDS, it is assumed that downstream contracts are trusted by upstream contracts through explicit references. Upstream dependencies are unaware of downstream contracts and do not trust them; however, they may expose public interfaces that are used by downstream contracts. **1. Ethereum (blockchain consensus, permission-less) domain:** - [ERC7744 Indexer](./Indexer.md) is the most upstream in dependency chain within the EDS ecosystem and is the only contract that functionality may be trusted by ALL parties. - In order for [Distributions](./Distributions.md) may be part of same trust domain they **MUST** use [ERC7744 Indexer](./Indexer.md) to refer code. + - [ERC7744 Indexer](./Indexer.md) is the most upstream in the dependency chain within the EDS ecosystem and is the only contract whose functionality may be trusted by ALL parties. + - In order for [Distributions](./Distributions.md) to be part of the same trust domain, they **MUST** use [ERC7744 Indexer](./Indexer.md) to refer to code.🧰 Tools
🪛 LanguageTool
[uncategorized] ~10-~10: Possible missing comma found.
Context: ...t assumptions that are made by EDS. In EDS it is assumed that downstream contracts...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~13-~13: Possible missing article found.
Context: ...](./Indexer.md) is the most upstream in dependency chain within the EDS ecosystem and is t...(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~14-~14: Possible missing article found.
Context: ...ons](./Distributions.md) may be part of same trust domain they MUST use [ERC7744...(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~14-~14: Possible missing comma found.
Context: ...ributions.md) may be part of same trust domain they MUST use [ERC7744 Indexer](./I...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~18-~18: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...744 Indexer](./Indexer.md) to refer code and they trust any code they deploy or list...(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~22-~22: Possible missing comma found.
Context: ...versions](./Versions.md) he supports by users and may impose specific business or pro...(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~24-~24: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...Indexer](./Indexer.md) to refer code. - Distributors may allow cross-instance calls between ...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~24-~24: Possible missing comma found.
Context: ...s between apps that they consider to be trusted and enforce these rules via runtime val...(AI_HYDRA_LEO_MISSING_COMMA)
42-49: Improve clarity in the upgradable distributions section.The requirements and trust model for upgradable distributions are well-explained, but some grammatical issues impact readability.
## Upgradable distributions - Since double trust assumptions, regular proxies owned by single party are not allowed. Developers **MUST** deploy upgradeable distributions for their app, example of such is [WrappedTransparentUpgradeableProxy](../src/proxies/WrappedTransparentUpgradeableProxy.sol). + Due to dual trust assumptions, regular proxies owned by a single party are not allowed. Developers **MUST** deploy upgradeable distributions for their app; an example of such is [WrappedTransparentUpgradeableProxy](../src/proxies/WrappedTransparentUpgradeableProxy.sol). - Such contracts create upgradable instances of app, which are **owned** by distributor but **access permissions** are managed by Installer. Attempts to call upgradability selectors **MUST** result in ERC7746 hook call to Installer. + Such contracts create upgradable instances of the app, which are **owned** by the distributor but **access permissions** are managed by the Installer. Attempts to call upgradability selectors **MUST** result in an ERC7746 hook call to the Installer.🧰 Tools
🪛 LanguageTool
[uncategorized] ~43-~43: You might be missing the article “a” here.
Context: ...t assumptions, regular proxies owned by single party are not allowed. Developers **MUS...(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~43-~43: Do not mix variants of the same word (‘upgradeable’ and ‘upgradable’) within a single text.
Context: ...not allowed. Developers MUST deploy upgradeable distributions for their app, example of...(EN_WORD_COHERENCY)
[uncategorized] ~45-~45: Possible missing article found.
Context: ...ontracts create upgradable instances of app, which are owned by distributor but...(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~45-~45: You might be missing the article “the” here.
Context: ...nstances of app, which are owned by distributor but access permissions are managed ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~45-~45: You might be missing the article “the” here.
Context: ...t access permissions are managed by Installer. Attempts to call upgradability selecto...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~47-~47: Did you mean “upgrading”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...all to Installer. This mechanic allows to upgrade app without requiring any changes to di...(ALLOW_TO)
[uncategorized] ~47-~47: Possible missing article found.
Context: ...aller. This mechanic allows to upgrade app without requiring any changes to distri...(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~47-~47: Possible missing article found.
Context: ...de app without requiring any changes to distributor, while still allowing to enforce busine...(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~48-~48: You might be missing the article “the” here.
Context: ...o enforce business logic on upgrade. If user wants to renounce distributor, he can d...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~48-~48: You might be missing the article “the” here.
Context: ... do so, but will not be able to upgrade app anymore, unless distributor transfers o...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~48-~48: You might be missing the article “the” here.
Context: ... be able to upgrade app anymore, unless distributor transfers ownership to user or new dist...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~48-~48: You might be missing the article “the” here.
Context: ...less distributor transfers ownership to user or new distributor. ## Upgrading Pro...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~48-~48: You might be missing the article “a” here.
Context: ...tributor transfers ownership to user or new distributor. ## Upgrading Process of...(AI_EN_LECTOR_MISSING_DETERMINER_A)
51-64: Well-structured upgrade process explanation.The step-by-step upgrade process is clearly outlined and easy to follow. Consider adding line breaks or using numbered lists to improve readability.
## Upgrading Process of normal upgrade routine is as follows: - 1. Distributor lists new upgrade version for a Repository with a specific version. - 2. Users calls Installer initialize upgrade via `upgradeApp` function. - 3. Installer sets flag that app is in upgrade mode and will call Distributor with `upgradeUserInstance` hook. - 4. Distributor validates that upgrade is allowed and that version is listed. - 5. Distributor loads migration plan from his storage and executes migration logic. - 6. Migration logic one way or another must call upgrade on the app instance, as example in [WrappedTransparentUpgradeableProxy](../src/proxies/WrappedTransparentUpgradeableProxy.sol) it is done by `upgradeToAndCall` function by wrapping standard OpenZeppelin upgrade function. - 7. App instance contact **MUST** call back to Installer via `beforeCall` hook. - 8. Installer validates that call was allowed by Distributor. - 9. Installer sets flag that app is no longer in upgrade mode. + 1. The Distributor lists a new upgrade version for a Repository with a specific version. + 2. The User calls the Installer to initialize upgrade via the `upgradeApp` function. + 3. The Installer sets a flag that the app is in upgrade mode and will call the Distributor with the `upgradeUserInstance` hook. + 4. The Distributor validates that the upgrade is allowed and that the version is listed. + 5. The Distributor loads the migration plan from its storage and executes migration logic. + 6. Migration logic must call upgrade on the app instance; for example, in [WrappedTransparentUpgradeableProxy](../src/proxies/WrappedTransparentUpgradeableProxy.sol) it is done by the `upgradeToAndCall` function by wrapping the standard OpenZeppelin upgrade function. + 7. The App instance contract **MUST** call back to the Installer via the `beforeCall` hook. + 8. The Installer validates that the call was allowed by the Distributor. + 9. The Installer sets a flag that the app is no longer in upgrade mode.🧰 Tools
🪛 LanguageTool
[uncategorized] ~52-~52: A determiner appears to be missing. Consider inserting it.
Context: ...ser or new distributor. ## Upgrading Process of normal upgrade routine is as follows...(AI_EN_LECTOR_MISSING_DETERMINER)
[uncategorized] ~55-~55: Possible missing article found.
Context: ...rmal upgrade routine is as follows: 1. Distributor lists new upgrade version for a Reposit...(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~55-~55: Possible missing article found.
Context: ...ne is as follows: 1. Distributor lists new upgrade version for a Repository with a...(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~58-~58: A determiner appears to be missing. Consider inserting it.
Context: ...tor withupgradeUserInstancehook. 4. Distributor validates that upgrade is allowed and t...(AI_EN_LECTOR_MISSING_DETERMINER)
[uncategorized] ~59-~59: Possible missing article found.
Context: ...version is listed. 5. Distributor loads migration plan from his storage and executes migr...(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~60-~60: You might be missing the article “the” here.
Context: ...upgradeToAndCallfunction by wrapping standard OpenZeppelin upgrade function. 7. App i...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~62-~62: You might be missing the article “the” here.
Context: ... to Installer viabeforeCallhook. 8. Installer validates that call was allowed by Dist...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~62-~62: You might be missing the article “the” here.
Context: ...Call` hook. 8. Installer validates that call was allowed by Distributor. 9. Install...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~62-~62: You might be missing the article “the” here.
Context: ...ller validates that call was allowed by Distributor. 9. Installer sets flag that app is no...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~63-~63: You might be missing the article “the” here.
Context: ...at call was allowed by Distributor. 9. Installer sets flag that app is no longer in upgr...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~63-~63: You might be missing the article “a” here.
Context: ...owed by Distributor. 9. Installer sets flag that app is no longer in upgrade mode. ...(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~63-~63: You might be missing the article “the” here.
Context: ...stributor. 9. Installer sets flag that app is no longer in upgrade mode. ## Migra...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
65-103: Good explanation of migration plans and strategies.The migration plan structure and strategies are well-documented, helping developers understand the different migration approaches available.
Add language specification to the code block for proper syntax highlighting:
- ``` + ```solidity struct MigrationPlan { LibSemver.VersionRequirement from; LibSemver.VersionRequirement to; IMigration migrationContract; MigrationStrategy strategy; }
- Eg. if migrating from v0.0.1 to v3.0.0 Distributor will execute migrations for v1.0.0, v2.0.0 and v3.0.0.
- E.g., if migrating from v0.0.1 to v3.0.0, the Distributor will execute migrations for v1.0.0, v2.0.0, and v3.0.0.
<details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary> [uncategorized] ~75-~75: A determiner appears to be missing. Consider inserting it. Context: ...; MigrationStrategy strategy; } ``` Migration plan is stored in Distributor storage a... (AI_EN_LECTOR_MISSING_DETERMINER) --- [uncategorized] ~92-~92: You might be missing the article “the” here. Context: ...ller and will be managed by Repository. Distributor will not execute any migration logic, b... (AI_EN_LECTOR_MISSING_DETERMINER_THE) --- [uncategorized] ~92-~92: You might be missing the article “the” here. Context: ...y migration logic, but will only signal Installer to execute migration. Distributor cont... (AI_EN_LECTOR_MISSING_DETERMINER_THE) --- [uncategorized] ~94-~94: Possible missing comma found. Context: ...itory specified migration script one by one executing migrations for any major vers... (AI_HYDRA_LEO_MISSING_COMMA) --- [uncategorized] ~94-~94: The abbreviation “e.g.” (= for example) requires two periods. Context: ...lease between `from` and `to` versions. Eg. if migrating from v0.0.1 to v3.0.0 Dist... (E_G) </details> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 90-90: Emphasis used instead of a heading null (MD036, no-emphasis-as-heading) </details> </details> </blockquote></details> <details> <summary>src/distributions/UpgradableDistribution.sol (2)</summary><blockquote> `23-26`: **Minor gas-saving loop micro-optimisation** The loop can be made cheaper by: 1. Caching `srcsLength` (already done ✅). 2. Using an `unchecked` block for the increment. ```diff - for (uint256 i; i < srcsLength; ++i) { + for (uint256 i; i < srcsLength; ) { address proxy = address(new WrappedTransparentUpgradeableProxy(_sources[i], installer, data, "")); instances[i] = proxy; - ++i; + unchecked { ++i; } }This shaves ~30-40 gas for longer source arrays while preserving correctness.
8-12: Unused artefacts — dead code / imports
ClonesandLibERC7744are imported, andCodeNotFoundInIndexis declared, but none are referenced in this contract.
If they are kept only for anticipated future use, add a short comment; otherwise remove them to keep byte-code size down.test/eds/Distributor.ts (2)
24-39: Duplicate helper definition – move to shared test utils
createVersionRequirementis duplicated in several test suites. Extract it once intotest/utils/version.ts(or similar) and import where needed to follow DRY.
168-172: Left-over console output
console.log(instanceAddress);sneaked into committed tests. This clutters CI output and slightly slows runs – please remove.- console.log(instanceAddress);src/repositories/Repository.sol (3)
64-72: Logic is correct but error strings could mislead
"Migration is not allowed for minor/patch releases"is thrown even whenmigrationHash == bytes32(0)but contract does not supportIMigration.
Consider differentiating the two error causes (Non-zero migrationHashvsUnsupported interface) for faster debugging.
198-203:require(migrationHash.getContainerOrThrow() != address(0))is unreachable
getContainerOrThrow()reverts on failure, so therequirecondition is evaluated only when the call already succeeded and returns a non-zero address.
Remove the redundantrequireor refactor as:- require(migrationHash.getContainerOrThrow() != address(0), "Invalid migration"); + // Will revert internally if hash is unknown + migrationHash.getContainerOrThrow();
91-149:resolveVersionis growing complex – consider refactoringThe 150-line if/else ladder is hard to reason about and prone to future off-by-one issues.
Extract per-requirement helpers or use a table-driven approach to keep cyclomatic complexity low.test/eds/VersionWorkflows.ts (1)
36-60: Duplicate helper utilities across tests
createVersionandcreateVersionRequirementexist in multiple test suites.
Export them from a sharedtest/helpers/version.tsto avoid drift.docs/Distributions.md (2)
5-9: Grammar & clarity improvementsCurrent sentence:
“Distributions are enshrined to be developed in state-less manner, this allows easier auditing and secure portability of code cross-chain.”Suggested:
-Distributions are enshrined to be developed in state-less manner, this allows easier auditing and secure portability of code cross-chain. +Distributions should be implemented as stateless contracts. +Statelessness simplifies auditing and guarantees that the same bytecode can be deployed safely across chains.🧰 Tools
🪛 LanguageTool
[uncategorized] ~5-~5: You might be missing the article “a” here.
Context: ...utions are enshrined to be developed in state-less manner, this allows easier auditin...(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~8-~8: Possible missing comma found.
Context: ... > [!WARNING] > If you deploy stateless distributions they will may likely be problematic to ...(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~8-~8: Two modal verbs in a row. Did you mean: “will, may”, “will” or “may”?
Context: ...you deploy stateless distributions they will may likely be problematic to adapt by devel...(TWO_CONNECTED_MODAL_VERBS)
52-57: Fix determiners & plural forms-In EDS [Upgradability](./Upgradability.md) is complex multi-party trust process. It enables pattern where distributor & user must agree on upgrade before it can be executed. +In EDS, [Upgradability](./Upgradability.md) is **a** complex, multi-party trust process. +It enables **a** pattern in which the distributor and the user must both approve an upgrade before it can be executed.Also change “This hooks” → “These hooks”.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~52-~52: Possible missing comma found.
Context: ...ble repository managed distribution In EDS Upgradability is ...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~52-~52: You might be missing the article “a” here.
Context: ... Upgradability is complex multi-party trust process. It enables p...(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~52-~52: You might be missing the article “a” here.
Context: ...x multi-party trust process. It enables pattern where distributor & user must agree on ...(AI_EN_LECTOR_MISSING_DETERMINER_A)
[style] ~53-~53: Consider a shorter alternative to avoid wordiness.
Context: ... on upgrade before it can be executed. In order to enable this process, standard upgradabl...(IN_ORDER_TO_PREMIUM)
[uncategorized] ~54-~54: You might be missing the article “an” here.
Context: ...6 Hooks](./Hooks.md) implemented within immutable part of the contract. This hooks must b...(AI_EN_LECTOR_MISSING_DETERMINER_AN)
[grammar] ~55-~55: Use the singular noun “hook” with the word ‘This’.
Context: ...in immutable part of the contract. This hooks must be implemented by developer of the...(DT_JJ_NNS_MD)
[uncategorized] ~55-~55: You might be missing the article “the” here.
Context: ...ract. This hooks must be implemented by developer of the distribution in such way, that o...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~55-~55: You might be missing the article “a” here.
Context: ...y developer of the distribution in such way, that only [Distributor](./Distributors...(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~56-~56: A determiner appears to be missing. Consider inserting it.
Context: ...staller consent is checked in runtime. Example of such upgradable distribution is [Wra...(AI_EN_LECTOR_MISSING_DETERMINER)
[uncategorized] ~57-~57: You might be missing the article “an” here.
Context: ...is checked in runtime. Example of such upgradable distribution is [WrappedTransparentUpgr...(AI_EN_LECTOR_MISSING_DETERMINER_AN)
src/distributions/VersionDistribution.sol (1)
8-11: Fix docstring referenceThe class documentation refers to "CodeHashDistribution" instead of "VersionDistribution", suggesting this might be copy-pasted documentation that wasn't properly updated.
-/** - * @title CodeHashDistribution - * @notice This contract creates immutable. It allows to add metadata to the distribution as well as specify name and version for EIP712 compatibility reasons. - * @dev This contract is a base for creating distributions that are identified by a deployed functionality (byte code hash). - */ +/** + * @title VersionDistribution + * @notice This contract creates immutable distribution. It allows to add metadata to the distribution as well as specify name and version for EIP712 compatibility reasons. + * @dev This contract is a base for creating distributions that are identified by a deployed functionality (byte code hash). + */src/mocks/MockTokenizedDistributor.sol (2)
97-106: Fix inconsistent indentationThe indentation in the
addVersionMigrationfunction is inconsistent with the rest of the file. The opening brace is aligned with the function name instead of being indented.- function addVersionMigration( + function addVersionMigration( bytes32 distributionId, LibSemver.VersionRequirement memory from, LibSemver.VersionRequirement memory to, bytes32 migrationHash, MigrationStrategy strategy, bytes memory distributorCalldata ) external onlyRole(DEFAULT_ADMIN_ROLE) { super._addVersionMigration(distributionId, from, to, migrationHash, strategy, distributorCalldata); }
24-26: Update function naming for clarityYou have two
addDistributionmethods with different signatures. Consider renaming them for better clarity, such asaddDistributionByInitializerandaddDistributionByRepository.Also applies to: 85-93
src/distributors/OwnableDistributor.sol (1)
6-8: DuplicateIMigrationimport – safe to remove.
IMigration.solis imported twice (lines 6 & 8).
While harmless for compilation, it bloats byte-code metadata and hurts readability.-import "../interfaces/IMigration.sol";src/interfaces/IInstaller.sol (1)
37-38: Error name style – minor consistency issue.The error
alreadyAllowedstarts with a lowercase letter, unlike all other error identifiers in the file.
For consistency with Solidity & OZ conventions (PascalCase), consider renaming toAlreadyAllowed.src/middleware/InstallerClonable.sol (1)
42-43: Redundant secondusing EnumerableSetdirective.
using EnumerableSet …is declared twice (lines 16 and 39).
Remove the duplicate to reduce byte-code size.src/distributors/Distributor.sol (1)
38-41: Confusing mapping key names (appComponent⇢appId)mapping(uint256 appComponent => bytes32 distributorsId) public distributionOf;The key is an
appId, not anappComponentaddress. Misleading names increase maintenance cost and onboarding time for new contributors. Consider renaming:-mapping(uint256 appComponent => bytes32 distributorsId) public distributionOf; +mapping(uint256 appId => bytes32 distributorsId) public distributionOf;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (65)
.cursorignore(1 hunks).solcover.js(1 hunks).vscode/settings.json(1 hunks)contracts/mocks/MockFailingDistribution.sol(1 hunks)contracts/mocks/MockLowLevelDistribution.sol(1 hunks)contracts/mocks/MockMigration.sol(1 hunks)contracts/mocks/MockPanicDistribution.sol(1 hunks)contracts/mocks/MockRepository.sol(1 hunks)deploy/ERC7744.js(1 hunks)docs/Distributions.md(1 hunks)docs/Distributors.md(1 hunks)docs/Indexer.md(1 hunks)docs/Repositories.md(1 hunks)docs/Upgradability.md(1 hunks)docs/Versioning.md(1 hunks)docs/contracts/index.md(2 hunks)hardhat.config.ts(2 hunks)package.json(1 hunks)src/abstracts/CodeIndexer.sol(0 hunks)src/abstracts/Distributor.sol(0 hunks)src/abstracts/Installer.sol(0 hunks)src/abstracts/InstallerClonable.sol(0 hunks)src/distributions/CloneDistribution.sol(1 hunks)src/distributions/CodeHashDistribution.sol(3 hunks)src/distributions/LatestVersionDistribution.sol(3 hunks)src/distributions/UpgradableDistribution.sol(1 hunks)src/distributions/VersionDistribution.sol(1 hunks)src/distributors/Distributor.sol(1 hunks)src/distributors/OwnableDistributor.sol(2 hunks)src/distributors/TokenizedDistributor.sol(2 hunks)src/erc7744/ERC7744.sol(1 hunks)src/erc7744/LibERC7744.sol(1 hunks)src/interfaces/IAdminGetter.sol(1 hunks)src/interfaces/IDistributor.sol(9 hunks)src/interfaces/IInstaller.sol(3 hunks)src/interfaces/IMigration.sol(1 hunks)src/interfaces/IRepository.sol(4 hunks)src/libraries/LibInstaller.sol(0 hunks)src/middleware/AuthorizationMiddleware.sol(1 hunks)src/middleware/ERC7746Hooked.sol(2 hunks)src/middleware/InstallerClonable.sol(1 hunks)src/middleware/LibMiddleware.sol(1 hunks)src/middleware/MiddlewareProxy.sol(1 hunks)src/mocks/MockCloneDistribution.sol(2 hunks)src/mocks/MockERC20.sol(1 hunks)src/mocks/MockFailingDistribution.sol(1 hunks)src/mocks/MockInstaller.sol(2 hunks)src/mocks/MockLatestVersionDistribution.sol(1 hunks)src/mocks/MockLowLevelDistribution.sol(1 hunks)src/mocks/MockMigration.sol(1 hunks)src/mocks/MockPanicDistribution.sol(1 hunks)src/mocks/MockRepository.sol(1 hunks)src/mocks/MockTokenizedDistributor.sol(4 hunks)src/proxies/WrappedTransparentUpgradeableProxy.sol(1 hunks)src/repositories/OwnableRepository.sol(1 hunks)src/repositories/Repository.sol(5 hunks)test/eds/CloneDistribution.ts(2 hunks)test/eds/Distributor.ts(7 hunks)test/eds/Installer.ts(4 hunks)test/eds/LatestVersionDistribution.ts(1 hunks)test/eds/Repository.ts(4 hunks)test/eds/TokenizedDistributor.ts(5 hunks)test/eds/UpgradabilityWorkflows.ts(1 hunks)test/eds/VersionDistributor.ts(7 hunks)test/eds/VersionWorkflows.ts(1 hunks)
💤 Files with no reviewable changes (5)
- src/abstracts/CodeIndexer.sol
- src/libraries/LibInstaller.sol
- src/abstracts/Distributor.sol
- src/abstracts/Installer.sol
- src/abstracts/InstallerClonable.sol
🧰 Additional context used
🪛 LanguageTool
docs/Versioning.md
[uncategorized] ~3-~3: You might be missing the article “a” here.
Context: # Versioning EDS implements Semantic versioning system within one bytes32 wo...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~5-~5: Possible missing comma found.
Context: ...ing system within one bytes32 word. On chain semantic versioning is managed by [LibS...
(AI_HYDRA_LEO_MISSING_COMMA)
docs/Distributors.md
[misspelling] ~3-~3: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...ovide trusted sources to end-users from a entity owned domain. Distributors may ...
(EN_A_VS_AN)
[uncategorized] ~15-~15: Possible missing article found.
Context: ...nstaller.md) can be runtime verified by Distributor. Distributor gets called with `beforeC...
(AI_HYDRA_LEO_MISSING_THE)
docs/Upgradability.md
[uncategorized] ~5-~5: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...th EDS ## Introduction EDS implements next level abstraction pattern over traditional sm...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~5-~5: Did you mean “upgrading”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...mart contract upgradeability. It allows to upgrade contracts in a safe and easy way, which...
(ALLOW_TO)
[uncategorized] ~8-~8: A comma may be missing after the conjunctive/linking adverb ‘Hence’.
Context: ...nd distributor requires careful design. Hence this section describes the trust assump...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~10-~10: Possible missing comma found.
Context: ...t assumptions that are made by EDS. In EDS it is assumed that downstream contracts...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~13-~13: Possible missing article found.
Context: ...](./Indexer.md) is the most upstream in dependency chain within the EDS ecosystem and is t...
(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~14-~14: Possible missing article found.
Context: ...ons](./Distributions.md) may be part of same trust domain they MUST use [ERC7744...
(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~14-~14: Possible missing comma found.
Context: ...ributions.md) may be part of same trust domain they MUST use [ERC7744 Indexer](./I...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~18-~18: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...744 Indexer](./Indexer.md) to refer code and they trust any code they deploy or list...
(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~22-~22: Possible missing comma found.
Context: ...versions](./Versions.md) he supports by users and may impose specific business or pro...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~24-~24: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...Indexer](./Indexer.md) to refer code. - Distributors may allow cross-instance calls between ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~24-~24: Possible missing comma found.
Context: ...s between apps that they consider to be trusted and enforce these rules via runtime val...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~32-~32: You might be missing the article “an” here.
Context: ...ion with ERC7746 Whenever an app calls end-user contract, or anyone attempts to ca...
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
[uncategorized] ~32-~32: You might be missing the article “a” here.
Context: ...er contract, or anyone attempts to call user-owned application, ERC7746 interface is...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~32-~32: You might be missing the article “the” here.
Context: ...ttempts to call user-owned application, ERC7746 interface is used to implement [runtime...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~43-~43: You might be missing the article “a” here.
Context: ...t assumptions, regular proxies owned by single party are not allowed. Developers **MUS...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~43-~43: Do not mix variants of the same word (‘upgradeable’ and ‘upgradable’) within a single text.
Context: ...not allowed. Developers MUST deploy upgradeable distributions for their app, example of...
(EN_WORD_COHERENCY)
[uncategorized] ~45-~45: Possible missing article found.
Context: ...ontracts create upgradable instances of app, which are owned by distributor but...
(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~45-~45: You might be missing the article “the” here.
Context: ...nstances of app, which are owned by distributor but access permissions are managed ...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~45-~45: You might be missing the article “the” here.
Context: ...t access permissions are managed by Installer. Attempts to call upgradability selecto...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~47-~47: Did you mean “upgrading”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...all to Installer. This mechanic allows to upgrade app without requiring any changes to di...
(ALLOW_TO)
[uncategorized] ~47-~47: Possible missing article found.
Context: ...aller. This mechanic allows to upgrade app without requiring any changes to distri...
(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~47-~47: Possible missing article found.
Context: ...de app without requiring any changes to distributor, while still allowing to enforce busine...
(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~48-~48: You might be missing the article “the” here.
Context: ...o enforce business logic on upgrade. If user wants to renounce distributor, he can d...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~48-~48: You might be missing the article “the” here.
Context: ... do so, but will not be able to upgrade app anymore, unless distributor transfers o...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~48-~48: You might be missing the article “the” here.
Context: ... be able to upgrade app anymore, unless distributor transfers ownership to user or new dist...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~48-~48: You might be missing the article “the” here.
Context: ...less distributor transfers ownership to user or new distributor. ## Upgrading Pro...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~48-~48: You might be missing the article “a” here.
Context: ...tributor transfers ownership to user or new distributor. ## Upgrading Process of...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~52-~52: A determiner appears to be missing. Consider inserting it.
Context: ...ser or new distributor. ## Upgrading Process of normal upgrade routine is as follows...
(AI_EN_LECTOR_MISSING_DETERMINER)
[uncategorized] ~55-~55: Possible missing article found.
Context: ...rmal upgrade routine is as follows: 1. Distributor lists new upgrade version for a Reposit...
(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~55-~55: Possible missing article found.
Context: ...ne is as follows: 1. Distributor lists new upgrade version for a Repository with a...
(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~58-~58: A determiner appears to be missing. Consider inserting it.
Context: ...tor with upgradeUserInstance hook. 4. Distributor validates that upgrade is allowed and t...
(AI_EN_LECTOR_MISSING_DETERMINER)
[uncategorized] ~59-~59: Possible missing article found.
Context: ...version is listed. 5. Distributor loads migration plan from his storage and executes migr...
(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~60-~60: You might be missing the article “the” here.
Context: ...upgradeToAndCall function by wrapping standard OpenZeppelin upgrade function. 7. App i...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~62-~62: You might be missing the article “the” here.
Context: ... to Installer via beforeCall hook. 8. Installer validates that call was allowed by Dist...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~62-~62: You might be missing the article “the” here.
Context: ...Call` hook. 8. Installer validates that call was allowed by Distributor. 9. Install...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~62-~62: You might be missing the article “the” here.
Context: ...ller validates that call was allowed by Distributor. 9. Installer sets flag that app is no...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~63-~63: You might be missing the article “the” here.
Context: ...at call was allowed by Distributor. 9. Installer sets flag that app is no longer in upgr...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~63-~63: You might be missing the article “a” here.
Context: ...owed by Distributor. 9. Installer sets flag that app is no longer in upgrade mode. ...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~63-~63: You might be missing the article “the” here.
Context: ...stributor. 9. Installer sets flag that app is no longer in upgrade mode. ## Migra...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~75-~75: A determiner appears to be missing. Consider inserting it.
Context: ...; MigrationStrategy strategy; } ``` Migration plan is stored in Distributor storage a...
(AI_EN_LECTOR_MISSING_DETERMINER)
[uncategorized] ~92-~92: You might be missing the article “the” here.
Context: ...ller and will be managed by Repository. Distributor will not execute any migration logic, b...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~92-~92: You might be missing the article “the” here.
Context: ...y migration logic, but will only signal Installer to execute migration. Distributor cont...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~94-~94: Possible missing comma found.
Context: ...itory specified migration script one by one executing migrations for any major vers...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~94-~94: The abbreviation “e.g.” (= for example) requires two periods.
Context: ...lease between from and to versions. Eg. if migrating from v0.0.1 to v3.0.0 Dist...
(E_G)
docs/Distributions.md
[grammar] ~3-~3: In the grammatical structure ‘pronoun + be/get + used + to + verb’, the verb ‘used’ is used with the gerund.
Context: ... are the main entities in EDS. They are used to distribute and upgrade contracts. Distributions a...
(ADMIT_ENJOY_VB)
[uncategorized] ~5-~5: You might be missing the article “a” here.
Context: ...utions are enshrined to be developed in state-less manner, this allows easier auditin...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~8-~8: Possible missing comma found.
Context: ... > [!WARNING] > If you deploy stateless distributions they will may likely be problematic to ...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~8-~8: Two modal verbs in a row. Did you mean: “will, may”, “will” or “may”?
Context: ...you deploy stateless distributions they will may likely be problematic to adapt by devel...
(TWO_CONNECTED_MODAL_VERBS)
[grammar] ~12-~12: In the grammatical structure ‘pronoun + be/get + used + to + verb’, the verb ‘used’ is used with the gerund.
Context: ...utions that are not stateless. They are used to distribute and upgrade contracts that are not stat...
(ADMIT_ENJOY_VB)
[uncategorized] ~14-~14: You might be missing the article “the” here.
Context: ...stributors.md) instead that will manage state of your distribution and is designed wi...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[style] ~17-~17: Consider a shorter alternative to avoid wordiness.
Context: ...t in mind. ## Creating a distribution In order to create a distribution, first deploy you...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~18-~18: You might be missing the article “the” here.
Context: ... address you want to use and then index contract code using Indexer Onc...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~20-~20: You might be missing the article “the” here.
Context: ... can create a distribution using one of available [distribution contracts](../src/distrib...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~22-~22: You might be missing the article “the” here.
Context: ...at will deploy and link every source to proxy. Here is simple example of stateless d...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~24-~24: You might be missing the article “a” here.
Context: ...nd link every source to proxy. Here is simple example of stateless distribution using...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~52-~52: Possible missing comma found.
Context: ...ble repository managed distribution In EDS Upgradability is ...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~52-~52: You might be missing the article “a” here.
Context: ... Upgradability is complex multi-party trust process. It enables p...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~52-~52: You might be missing the article “a” here.
Context: ...x multi-party trust process. It enables pattern where distributor & user must agree on ...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[style] ~53-~53: Consider a shorter alternative to avoid wordiness.
Context: ... on upgrade before it can be executed. In order to enable this process, standard upgradabl...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~54-~54: You might be missing the article “an” here.
Context: ...6 Hooks](./Hooks.md) implemented within immutable part of the contract. This hooks must b...
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
[grammar] ~55-~55: Use the singular noun “hook” with the word ‘This’.
Context: ...in immutable part of the contract. This hooks must be implemented by developer of the...
(DT_JJ_NNS_MD)
[uncategorized] ~55-~55: You might be missing the article “the” here.
Context: ...ract. This hooks must be implemented by developer of the distribution in such way, that o...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~55-~55: You might be missing the article “a” here.
Context: ...y developer of the distribution in such way, that only [Distributor](./Distributors...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~56-~56: A determiner appears to be missing. Consider inserting it.
Context: ...staller consent is checked in runtime. Example of such upgradable distribution is [Wra...
(AI_EN_LECTOR_MISSING_DETERMINER)
[uncategorized] ~57-~57: You might be missing the article “an” here.
Context: ...is checked in runtime. Example of such upgradable distribution is [WrappedTransparentUpgr...
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
[uncategorized] ~59-~59: You might be missing the article “the” here.
Context: ...agement of the upgrades & migrations on developer side is done via [Repository](./Reposit...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~62-~62: A comma might be missing here.
Context: ...per packages as they need to. For more details refer to [Upgradability](./Upgradabilit...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
docs/Indexer.md
[uncategorized] ~3-~3: You might be missing the article “a” here.
Context: ...at allows immutable code referencing in permission-less manner. ERC7744 indexer is the mo...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~5-~5: Possible missing article found.
Context: ...ERC7744 indexer is the most upstream in dependency chain within the EDS ecosystem and is t...
(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~18-~18: You might be missing the article “the” here.
Context: ...s a library that helps to interact with ERC7744 indexer. Typical usage is as follows: ...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[style] ~58-~58: Using many exclamation marks might seem excessive (in this case: 4 exclamation marks for a text that’s 1097 characters long)
Context: ...er.get(codeHash); ``` ## Using CLI > [!NOTE] > The CLI provides utilities to in...
(EN_EXCESSIVE_EXCLAMATION)
🪛 markdownlint-cli2 (0.17.2)
docs/Upgradability.md
35-35: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
90-90: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
🔇 Additional comments (81)
.cursorignore (1)
1-2: Sensitive files are now ignored
This.cursorignorefile correctly prevents the.envfile and the entire.secrets/directory from being processed by Cursor, reducing the risk of leaking environment variables or credentials..solcover.js (1)
1-3: Appropriate configuration for code coverage.The configuration correctly excludes mock contracts and ERC7744 implementations from coverage analysis, which is a good practice. This ensures coverage metrics focus on your actual project code rather than test mocks or external standards implementation.
src/interfaces/IAdminGetter.sol (1)
1-6: Clean interface declaration for proxy admin access.This interface properly defines a single view function for retrieving the wrapped proxy admin address, which supports the proxy pattern enhancement described in the summary.
src/mocks/MockERC20.sol (1)
10-18: Mock implementation properly supports distribution testing.The added
instantiatefunction correctly implements a simplified version that returns this contract's address as the only instance, which is suitable for testing distribution workflows.src/middleware/ERC7746Hooked.sol (1)
4-4: Import path updated for LibMiddleware.The import path for LibMiddleware has been changed from a parent directory to the current directory, suggesting a file structure reorganization.
src/distributions/CloneDistribution.sol (2)
8-8: CodeNotFoundInIndex error retained without CodeIndexer.The contract still defines the CodeNotFoundInIndex error, but since it no longer inherits from CodeIndexer, ensure that this error is properly used in concrete implementations or if it should be moved elsewhere.
Is this error still relevant in the context where CodeIndexer is no longer used? Please verify if concrete implementations are throwing this error, or if it should be updated or moved to a different contract.
7-7:✅ Verification successful
Removed inheritance from CodeIndexer.
The contract no longer inherits from CodeIndexer and now only implements the IDistribution interface. This aligns with the broader refactoring to use LibERC7744 for code index interactions.
🏁 Script executed:
#!/bin/bash # Check how CloneDistribution implementations handle code indexing without CodeIndexer # Look for implementations of CloneDistribution echo "Checking for implementations of CloneDistribution..." rg "contract.*is.*CloneDistribution" --type sol # Check for CodeNotFoundInIndex error usage echo "Checking for CodeNotFoundInIndex error usage..." rg "CodeNotFoundInIndex" --type sol # Check if LibERC7744 is being used in implementations echo "Checking for LibERC7744 usage in implementations..." rg "LibERC7744" --type solLength of output: 529
Let's retry the searches using explicit
.solglobs:
🏁 Script executed:
#!/bin/bash # Search for any contracts inheriting CloneDistribution echo "Checking for implementations of CloneDistribution..." rg -g '*.sol' -P "contract\s+\w+\s+is\s+CloneDistribution" -A2 # Search for CodeNotFoundInIndex references echo "Checking for CodeNotFoundInIndex usage..." rg -g '*.sol' "CodeNotFoundInIndex" # Search for LibERC7744 usage echo "Checking for LibERC7744 usage..." rg -g '*.sol' "LibERC7744"Length of output: 2631
Removal of CodeIndexer inheritance in CloneDistribution confirmed
CloneDistribution.sol now declares:
abstract contract CloneDistribution is IDistribution { error CodeNotFoundInIndex(bytes32 codeId); // … }It no longer inherits from CodeIndexer. All downstream implementations handle code‐index interactions via LibERC7744:
- src/mocks/MockCloneDistribution.sol
- src/distributions/CodeHashDistribution.sol
- src/distributions/LatestVersionDistribution.sol
No further changes required.
test/eds/CloneDistribution.ts (2)
9-9: Updated constructor call to include distribution name.The MockCloneDistribution deployment now includes "MockCloneDistribution" as a constructor parameter, reflecting changes in the contract implementation to use named distributions.
24-26:✅ Verification successful
Updated string parsing for ShortString compatibility.
The test now uses a specific approach to parse the bytes32 name by slicing off the last two characters and appending "00" before converting to a string. This change accommodates the new ShortString format used for distribution names.
🏁 Script executed:
#!/bin/bash # Verify the ShortString implementation and usage pattern # Check the MockCloneDistribution implementation echo "Checking MockCloneDistribution implementation..." fd -e sol "MockCloneDistribution.sol" | xargs cat # Look for ShortString usage in the codebase echo "Checking for ShortString imports and usage patterns..." rg "ShortString" --type sol # Check for similar string parsing patterns in other test files echo "Checking for similar string parsing patterns in tests..." rg "slice\(0, -2\) \+ \"00\"" --type tsLength of output: 1630
Confirmed ShortString parsing is correct in CloneDistribution test
The test in test/eds/CloneDistribution.ts intentionally slices off the last two characters and appends
"00"before callingethers.utils.parseBytes32String. This matches howShortStrings.toShortString(name)pads abytes32value with trailing null bytes, and ensures that unwrapping the distribution name yields"MockCloneDistribution"as expected.• Verified MockCloneDistribution.sol imports and uses
ShortStrings.toShortStringandShortString.unwrap.
• No other tests employ this parsing pattern.src/distributions/CodeHashDistribution.sol (3)
4-5: Import path improvements and library integrationThese import changes reflect a refactoring of the codebase structure, with the CloneDistribution contract now being imported from the local directory and the new LibERC7744 library being introduced. This aligns with a more modular approach.
17-17: Library integration for cleaner codeAdding the LibERC7744 library for bytes32 types allows for improved encapsulation of ERC7744 indexer interactions.
30-30: Simplified code indexing lookup with improved error handlingReplacing direct index lookup with
codeHash.getContainerOrThrow()centralizes the indexer interaction logic and error handling in the library, making the code more maintainable and consistent.src/middleware/MiddlewareProxy.sol (2)
4-5: Contract dependencies refactoringThe import paths have been updated to reflect the middleware system refactoring, importing from local paths and renaming components.
12-12:✅ Verification successful
Base contract inheritance change
The contract now inherits from
ERC7746Hookedinstead of what was likelyERC7746Middleware. Ensure that the new base contract provides the same functionality and doesn't break existing behavior.
🏁 Script executed:
#!/bin/bash # Examine the new ERC7746Hooked contract to verify compatibility with previous ERC7746Middleware cat src/middleware/ERC7746Hooked.sol 2>/dev/null || echo "File not found" # Check for any places in tests where MiddlewareProxy is used to ensure they're still valid rg -A 3 "MiddlewareProxy" --type tsLength of output: 1614
🏁 Script executed:
#!/bin/bash # Search for any references to the old base contract name rg -n "ERC7746Middleware" # Search for occurrences of the new base contract in imports/usages rg -n "ERC7746Hooked" # Search for any usage of MiddlewareProxy across all files (tests, scripts, configs) rg -n "MiddlewareProxy"Length of output: 7051
Inheritance change verified
The new
ERC7746Hookedabstract contract defines the sameERC7746CandERC7746modifiers as the originalERC7746Middleware, soMiddlewareProxy’s behavior remains identical after switching its base contract. No further changes are needed.deploy/ERC7744.js (2)
17-17: Suppressed logging for local developmentLogs are now only printed when not running on the hardhat network, which improves clarity during testing. This is a good practice for reducing noise in test output.
Also applies to: 20-20
8-9:✅ Verification successful
Updated deterministic deployment salt
The salt for deterministic deployment has been changed to a new fixed value. This will result in a different deployment address for the contract, which could impact references to the contract throughout the codebase.
🏁 Script executed:
#!/bin/bash # Check if the contract address is referenced elsewhere in the codebase rg -A 1 "ERC7744" --type sol # Also check for direct address references that might need updating # (Would need to know the previous deployed address to search for it)Length of output: 56
🏁 Script executed:
#!/bin/bash # 1. Make ripgrep recognize Solidity files rg --type-add 'sol:*.sol' --type sol "ERC7744" -A 1 # 2. Search for any ERC7744 references in JavaScript/TypeScript rg "ERC7744" -g '*.js' -A 1 rg "ERC7744" -g '*.ts' -A 1 # 3. Confirm no other occurrences of the new salt value rg "9425035d50edcd7504fe5eeb5df841cc74fe5eeb5df841cc74fe6cccd82dca6ee75bcdf774bd88d9" -A 1Length of output: 12485
Confirm isolated salt update; no address references found
The new deterministic salt in deploy/ERC7744.js is only used by the Hardhat deploy script. We found no hard-coded contract addresses or previous salt values elsewhere in the codebase. All tests and contracts use Hardhat’s deployments fixture to fetch the ERC7744 address at runtime, so they’ll automatically pick up the new address.
• File updated: deploy/ERC7744.js – salt changed
• No direct address or salt references in.sol,.js, or.tsfiles
• Tests usedeployments.get("ERC7744")and don’t assert a fixed addresshardhat.config.ts (3)
13-13: Good addition for test coverage tracking.Adding
solidity-coveragewill help ensure better test coverage monitoring, which is essential for quality assurance of smart contracts.
122-124:❓ Verification inconclusive
Compiler settings optimization with trade-offs to consider.
The changes to the compiler settings have important implications:
- Setting
evmVersionto "cancun" ensures access to the latest EVM features, but may limit compatibility with chains that haven't upgraded- Enabling
viaIRcan improve optimizations at the cost of longer compile timesConsider whether all target deployment chains support the Cancun EVM version:
🏁 Script executed:
#!/bin/bash # Check compatibility of Cancun EVM with common EVM networks echo "Checking network statuses for Cancun EVM compatibility..." curl -s https://chainid.network/chains.json | jq -r '.[] | select(.name | contains("Ethereum") or contains("Arbitrum") or contains("Polygon") or contains("Optimism")) | "\(.name): \(.status)"'Length of output: 984
Validate Cancun EVM support across all target networks
The current compiler config in hardhat.config.ts (lines 122–124):
evmVersion: "cancun", viaIR: true, optimizer: { … }unlocks the latest opcodes and IR-based optimizations but introduces two risks:
- Some chains may not yet have activated the Cancun hard fork, causing transactions to fail.
viaIRcan significantly increase compile times.Action items:
- For each deployment target, confirm the Cancun fork activation block via its official docs or an RPC call (e.g. check
eth_getBlockByNumberaround the known fork height).- If any network hasn’t upgraded, consider falling back to a more widely supported
evmVersion(e.g.,londonorparis).- Evaluate compilation performance with and without
viaIRto find the right trade-off.
126-126:❓ Verification inconclusive
Consider the impact of reduced optimizer runs.
Reducing optimizer runs from what was likely a much higher value to 2,000 significantly changes the optimization strategy. This optimizes for deployment cost rather than runtime efficiency, which could increase gas costs for frequently called functions.
Consider whether this is intentional or if a higher value might be more appropriate for frequently called functions:
🌐 Web query:
What is the optimal value for Solidity optimizer runs for different contract usage patterns?💡 Result:
Optimal Solidity Optimizer Runs Value: Guidance by Contract Usage
The "optimizer runs" setting in the Solidity compiler is a trade-off parameter that influences the balance between contract deployment cost (gas spent when the contract is first published) and the cost of executing contract functions later. There is no single universally optimal value for all cases; the best setting depends on the contract's intended usage pattern and projected transaction frequency[6][8].
How the "runs" Parameter Works
- Lower "runs" (e.g., 1): Reduces deployment cost (smaller code size), but leads to higher function execution costs after deployment. This is ideal for contracts where most of the gas will be spent on deployment and relatively fewer function calls are expected[1][3][6].
- Higher "runs" (e.g., 1000, 10000, or even up to 1,000,000): Increases deployment cost (larger code size), but decreases the cost of each subsequent transaction, making it suitable for contracts that will be called frequently after deployment[3][6][7][8].
Recommended Values by Usage Pattern
Contract Pattern Recommended "runs" Value Explanation One-off/rarely used contract 1–200 Cheaper deployment is prioritized, function execution cost less significant[1][3][6]. Frequently used (“utility”) 1000–10,000+ Focus on minimizing long-term function execution costs[3][7][8]. ERC-20/ERC-721 tokens, DEXs 5,000–1,000,000 High volume of transactions; function call optimization critical[2][3][7]. Custom, complex logic Test various values (e.g., 200–10,000) Analyze deployment/function costs; pick value that fits anticipated usage[2][8].
- Default: If you do not specify, Solidity uses 200 as the default “runs” value[3][8].
- Extreme uses: For contracts expecting extremely high call volume, values up to 1,000,000 are possible, but note that some verification services (like Etherscan) may have upper limits for accepted values[2].
How to Choose Optimally
There is no mathematical "best" value—you must estimate the contract’s lifetime use:
- If the contract is mostly “set-and-forget” (e.g., a DAO with rare function calls): Use low runs.
- If the contract will process thousands/millions of transactions (e.g., a token, marketplace, or DeFi contract): Use high runs.
Tip: If unsure, benchmark the contract with different
--optimize-runsvalues and compare gas costs for both deployment and the functions most likely to be used frequently[2][8]. For some contracts, the cost savings from a higher runs value levels off beyond a certain point[8].Summary Table
Usage Type Example Suggested Runs Cheap deploy, few calls Governance, setup, migration 1–200 Frequent transaction ERC-20, DEX, utility contracts 1000–10,000 Extreme frequency Major DApps, bridges, rollups 10,000–1,000,000 (ensure Etherscan compatibility) Key Takeaways
- The “runs” value is not a number of optimization iterations—it is a cost trade-off knob[2][6][8].
- Low value: Cheaper deployment, costlier execution.
- High value: Costlier deployment, cheaper execution.
- There is no universal best value: analyze your contract’s expected transaction pattern and select accordingly.
- For most standard token contracts, 10,000 is often adopted as a practical high-frequency value[7].
- Benchmarking on real function usage is the only way to find true optimality for your scenario.
For details on how the optimizer works internally, refer to Solidity documentation[1][6].
Citations:
- 1: https://docs.soliditylang.org/en/latest/using-the-compiler.html?highlight=optimize-runs
- 2: ethereum/solidity#5394
- 3: https://blog.web3labs.com/solidity-smart-contracts-gas-optimization-techniques/
- 4: https://forum.openzeppelin.com/t/if-i-use-the-optimizer-to-optimize-the-code-is-there-any-problem-in-using-all-the-functions-provided-by-openzeppelins-governor/30298
- 5: https://coinsbench.com/%EF%B8%8F-understanding-compiler-optimizations-in-solidity-smart-contracts-74511f5e23c6
- 6: https://docs.soliditylang.org/en/latest/internals/optimizer.html
- 7: https://akhil.sh/tutorials/solidity/solidity/essential_gas_optimization/
- 8: https://coinsbench.com/comprehensive-guide-tips-and-tricks-for-gas-optimization-in-solidity-5380db734404
- 9: https://metaschool.so/articles/gas-optimization-techniques-in-solidity
- 10: https://www.alchemy.com/overviews/solidity-gas-optimization
Reevaluate your Solidity optimizer runs setting
The current setting in hardhat.config.ts (line 126) is:
runs: 2000While 2,000 falls within the broad “frequently used” range (1,000–10,000), the optimal value depends on your contract’s transaction patterns:
- If this is a high-volume contract (ERC-20/DEX/utility), consider raising runs to 5,000–10,000+ (up to 1,000,000) to minimize per-call gas.
- If it’s rarely invoked (governance, one-off setup), you could lower it toward 200 or even below 2000 to save on deployment gas.
- Benchmark your most critical functions at several runs values (e.g., 2,000 vs. 10,000) to measure deployment vs. execution gas trade-offs.
Please confirm your contract’s expected usage profile and adjust the optimizer runs value to match your deployment vs. runtime gas-cost priorities.
src/distributions/LatestVersionDistribution.sol (3)
7-7: Good integration of LibERC7744 for code index interactions.Using
LibERC7744as a library forbytes32types improves code organization and simplifies interactions with the ERC7744 indexer.Also applies to: 18-18
37-37: More concise source address retrieval with improved error handling.Changed from using separate calls to get the container address to using the extension method
getContainerOrThrow()directly on the source ID. This makes the code more concise and likely improves error handling by explicitly throwing when a container isn't found.
30-30:✅ Verification successful
Consider inheritance implications of adding
virtualmodifier.Adding the
virtualmodifier to theinstantiatefunction enables overriding in derived contracts, enhancing extensibility. However, ensure all derived contracts that may override this method are reviewed for compatibility with this change.
🏁 Script executed:
#!/bin/bash # Check for any contracts that extend LatestVersionDistribution echo "Searching for contracts that inherit from LatestVersionDistribution..." fd -e sol | xargs grep -l "is LatestVersionDistribution" || echo "No direct inheritors found" # Check for any instantiate function in derived contracts echo "Checking for instantiate functions in inheriting contracts..." fd -e sol | xargs grep -l "is LatestVersionDistribution" | xargs grep -l "function instantiate" || echo "No overriding instantiate functions found"Length of output: 528
🏁 Script executed:
#!/bin/bash echo "Displaying src/mocks/MockLatestVersionDistribution.sol:" sed -n '1,200p' src/mocks/MockLatestVersionDistribution.solLength of output: 1527
No further action needed for
instantiateoverridesThe only contract inheriting from
LatestVersionDistributionisMockLatestVersionDistribution.sol, and it already overridesinstantiate, so marking the base method asvirtualdoes not introduce any unresolved inheritance issues.docs/Versioning.md (1)
1-56: Comprehensive versioning documentation with useful examples.This is a well-structured documentation file that clearly explains the semantic versioning system, provides both Solidity and CLI examples, and includes important usage notes. This will be valuable for developers working with the EDS framework.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: You might be missing the article “a” here.
Context: # Versioning EDS implements Semantic versioning system within one bytes32 wo...(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~5-~5: Possible missing comma found.
Context: ...ing system within one bytes32 word. On chain semantic versioning is managed by [LibS...(AI_HYDRA_LEO_MISSING_COMMA)
src/erc7744/ERC7744.sol (2)
25-26: Improved container validation with multiple checks.The updated
isValidContainerfunction now performs more comprehensive validation by checking the code hash and using the newisEIP7702function. This prevents indexing of invalid or delegated contracts.
33-33: Documentation updated to match implementation.The updated comment accurately describes the behavior of the
registerfunction when provided with an EIP7702 delegated EOA, maintaining consistency between documentation and implementation.src/interfaces/IMigration.sol (1)
1-15: Well-designed migration interface for handling version upgrades.The IMigration interface provides a clean and comprehensive contract for handling migrations between different versions. It follows good interface design practices with clear naming and a focused purpose.
docs/Repositories.md (1)
1-52: Comprehensive documentation for repository management.The documentation clearly explains the concept of Repositories and provides a complete reference of CLI commands for managing them. The structure is logical and the commands follow a consistent pattern.
A suggestion to enhance this further would be to include example command outputs or sample workflows that demonstrate common repository management scenarios, if not already covered elsewhere in the documentation.
src/mocks/MockCloneDistribution.sol (5)
3-3: Good use of OpenZeppelin's ShortStrings for gas efficiency.Using the ShortStrings library is an excellent choice for storing distribution names efficiently, as it optimizes gas usage for strings up to 31 bytes.
5-5: Updated import path reflects code reorganization.The change in import path from abstracts to distributions indicates a logical reorganization of the codebase structure.
8-9: Efficient state variable declarations.Using
immutablefor the distribution name andconstantfor the version are good practices for optimizing gas usage while clearly indicating the intended mutability of these values.
11-13: Improved flexibility with parametrized constructor.Adding a constructor that accepts a name parameter makes the mock distribution more configurable and reusable across different test scenarios.
26-26: Sources function now uses state variables instead of hardcoded values.Returning the unwrapped ShortString and version constant from state variables rather than hardcoded values improves maintainability and consistency.
test/eds/LatestVersionDistribution.ts (5)
1-13: Well-structured imports and type definitions.The test file correctly imports all necessary testing utilities and type definitions, ensuring type safety and clear code organization.
14-22: Good test setup with clear variable declarations.The test setup clearly defines all the necessary variables and constants for the tests, making the test context easy to understand.
23-66: Well-implemented test fixture using the loadFixture pattern.The test fixture follows good practices by:
- Setting up signers
- Deploying all necessary contracts
- Initializing the repository with test data
- Returning all necessary objects for tests
This approach makes tests more maintainable and efficient.
72-82: Constructor tests verify essential functionality.The tests correctly verify that the constructor sets the repository address and metadata as expected.
84-90: Clear test for contractURI functionality.The test verifies that the contractURI method returns the expected metadata string.
test/eds/TokenizedDistributor.ts (4)
20-20: Variable renamed to better reflect its purposeThe variable name change from
distributionIdtodistributedCodeHashis more semantically accurate, as it represents the keccak256 hash of the contract bytecode rather than a generic ID.
36-36: Constructor argument added for mock contractAdding the string constructor argument
"MockCloneDistribution"aligns with the updated mock contract implementation pattern used throughout the codebase.
39-39: Code hash calculation with appropriate variable nameThe renaming of the variable used to store the hash makes the code easier to understand and maintains consistency with the updated terminology.
70-74: Updated function signature to include distribution nameThe
addDistributioncalls now properly include a third parameter for the distribution name ("testDistribution"), conforming to the updated interface. This change is consistently applied across all instances in the file.Also applies to: 86-90, 97-101
src/repositories/OwnableRepository.sol (4)
4-4: Updated import pathThe import path for Repository.sol has been modified to reflect the new file structure.
7-8: Added new imports for migration supportThe addition of
IMigration.solandShortStrings.solimports supports the new migration functionality implemented in this contract.
15-17: Enhanced newRelease function with migration supportThe
newReleasefunction now accepts amigrationHashparameter, enabling version migration capabilities. The function properly delegates to the superclass implementation.
19-21: Added migration script management functionalityThe new
changeMigrationScriptfunction provides a way to update migration scripts for specific major versions, enhancing the repository's version management capabilities.docs/Distributors.md (1)
1-66: Comprehensive documentation for DistributorsThe documentation provides clear explanations of distributors, their purpose, and usage patterns. The CLI command reference is particularly thorough and will be valuable for users.
🧰 Tools
🪛 LanguageTool
[misspelling] ~3-~3: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...ovide trusted sources to end-users from a entity owned domain. Distributors may ...(EN_A_VS_AN)
[uncategorized] ~15-~15: Possible missing article found.
Context: ...nstaller.md) can be runtime verified by Distributor. Distributor gets called with `beforeC...(AI_HYDRA_LEO_MISSING_THE)
src/mocks/MockPanicDistribution.sol (2)
1-29: Well-implemented mock for testing panic scenariosThis mock contract effectively simulates runtime panic scenarios by intentionally performing a division by zero. It provides a good test case for error handling in the distributor system and properly implements all required interface methods.
8-18: Deliberate panic implementation for testingThe division by zero technique is an effective way to simulate runtime panics for testing, and the code is well-commented to indicate that this is intentional.
test/eds/Installer.ts (3)
42-42: Constructor arg looks good – nothing else to flag.
Passing the constructor-name string is in line with the newMockCloneDistributionconstructor signature.
125-129: Verify that the expected error comes from the correct contract.
The call originates frominstaller.beforeCall, but you expect a revert attached todistributor’s ABI. That is valid only if the installer bubbles the exact custom error from the distributor. If the installer wraps or re-throws, this assertion will fail.Consider asserting against the
installerinstance instead, or explicitly propagate the error in the contract.
101-107:❌ Incorrect review comment
Possible off-by-one: confirm the index returned by
getAppsNum.
getAppsNum()usually returns the current count, in which case the valid indices are0 … count - 1. QueryinggetApp(instanceNum)risks an “index out of bounds” revert if the contract is zero-indexed.Please double-check the contract’s implementation or add
-1when appropriate.
🏁 Script executed:
#!/bin/bash # Quick check: search for getAppsNum implementation and see how it's used. rg --context 3 --line-number $'function getAppsNum' | head -n 20Length of output: 1063
🏁 Script executed:
#!/bin/bash # Show getApp implementation with context rg -n --context 5 'function getApp' src/middleware/InstallerClonable.sol || true # Show getAppsNum implementation with context rg -n --context 5 'function getAppsNum' src/middleware/InstallerClonable.sol || true # Show where appNum is modified (installation logic) rg -n --context 10 'appNum' src/middleware/InstallerClonable.sol || trueLength of output: 4564
Off-by-one concern resolved: getAppsNum is 1-based
No change needed—strg.appNumis incremented before storing each app, so the first app lives at ID 1 andgetAppsNum()returns the latest valid index. CallinggetApp(instanceNum)is correct and won’t underflow.Likely an incorrect or invalid review comment.
test/eds/VersionDistributor.ts (3)
37-41: Good addition ofMockInstaller.
Creating a dedicated installer instance makes the tests closer to the production flow.
46-55: Confirm that the hash matches the code registered inERC7744.
codeIndex.register(dummyMigration.address)stores the address, not the hash, whereas you later pass the keccak‐256 hash of the runtime byte-code tonewRelease. Ensure the repository compares identical identifiers (address vs hash), otherwise the migration script may never be found at runtime.
138-141: Sanity-checkinstantiateDatapacking.
The encoder packs(address, bytes)but the decode comment above (now removed) hinted at a tupleInstantiateData. Verify the solidity side decodesinstantiateDatathe same way, including the order and types.src/mocks/MockFailingDistribution.sol (2)
7-18: The instantiate function implements controllable failure behavior.This mock contract function provides a good mechanism for testing error handling in the distributor system by intentionally failing when specific arguments are provided.
24-28: The get function returns consistent metadata.The
getfunction correctly returns the same metadata as a successful instantiation, which ensures consistent behavior when querying the distribution.src/mocks/MockLowLevelDistribution.sol (3)
7-18: The instantiate function correctly implements low-level error simulation.Using inline assembly with the
invalid()opcode is an effective way to simulate a low-level error for testing how the distribution system handles such failures.
13-17: Unreachable code clearly marked.The comment correctly indicates that the code following the assembly block is unreachable. This is good documentation for future maintainers.
24-28: The get function provides expected metadata.The
getfunction returns consistent data about the distribution, which is necessary for testing the system's query capabilities.contracts/mocks/MockLowLevelDistribution.sol (2)
15-26: Low-level error simulation uses a different approach.This implementation simulates a low-level error by making a call to a non-existent function at address(0), while the other version uses the
invalid()opcode. Both are valid approaches but produce different error types which could affect error handling tests.
28-33: ShortStrings usage is consistent with naming conventions.The use of OpenZeppelin's ShortStrings for the distribution name is consistent with best practices for gas-efficient string storage.
src/distributors/TokenizedDistributor.sol (4)
13-13: Improved mapping key naming for clarity.Changing from
idtocodeHashmakes the purpose of the mapping clearer and more aligned with its actual usage.
34-41: Added support for human-readable distribution names.The addition of the
readableNameparameter enhances the usability of the system by providing a way to identify distributions with descriptive names.
46-48: Updated function signature adds versioning support.The addition of
LibSemver.VersionRequirementshows this implementation now supports semantic versioning, which is an important feature for contract management.
56-67: The payment logic remains unchanged.The payment transfer functionality is correctly maintained during the refactoring, ensuring that the tokenized distribution mechanism continues to work as expected.
src/mocks/MockLatestVersionDistribution.sol (2)
7-12: Good implementation of the mock contract constructor and state variable.The contract correctly stores the metadata parameter as an immutable variable and properly initializes the parent contract. This is a good pattern for mock contracts.
14-17: Good implementation of the contractURI method.The
contractURI()implementation correctly converts the bytes32 metadata to a string using the helper function. This provides a clean way to access metadata in tests.contracts/mocks/MockFailingDistribution.sol (3)
7-13: Good implementation of the mock distribution contract constructor.The contract correctly initializes the distribution name using OpenZeppelin's ShortStrings library, which is an efficient way to store short strings in contracts.
29-34: The get method correctly implements the IDistribution interface.This implementation properly returns the contract's address, name, and version matching the behavior of the instantiate method (minus side effects), which ensures consistent behavior.
15-27:✅ Verification successful
Verify the expected failure trigger matches consumer expectations.
The
instantiatefunction reverts when args match the encoded bytes32 "FAIL". Make sure consumers of this mock know they need to passabi.encode(bytes32("FAIL"))to trigger the failure, not just the string "FAIL".
🏁 Script executed:
#!/bin/bash # Check how the "FAIL" argument should be encoded in tests # Look for test files that use this mock rg -A 3 "MockFailingDistribution" --type tsLength of output: 629
🏁 Script executed:
#!/bin/bash # Search for instantiate usage in TypeScript tests rg -A3 "\.instantiate" --type ts # Search for distribution calls that may pass args through Distributor rg -A3 "\.distribute" --type ts # Search for revert assertions matching the mock's error message rg -F "Intentional instantiation failure" --type tsLength of output: 7878
No changes needed—tests already use the correct “FAIL” encoding
The existing TypeScript tests trigger the mock’s revert by passing a 32-byte hex string (via
ethers.utils.formatBytes32String("FAIL")), which matchesabi.encode(bytes32("FAIL"))under the hood.• In
test/eds/Distributor.tsyou can see:await expect( distributor .connect(owner) .instantiate(failingDistId, ethers.utils.formatBytes32String("FAIL")) ).to.be.reverted;src/interfaces/IRepository.sol (6)
4-6: The import paths have been updated correctly.The imports now reference the versioning library and add the IMigration interface, which is needed for the migration functionality.
51-56: Good addition of the MigrationScriptAdded event.The event is well-documented with NatSpec comments and includes indexed parameters to make filtering for these events more efficient.
92-98: Good addition of changeMigrationScript function.The function is well-documented with proper NatSpec comments explaining its purpose and events that must be emitted.
100-106: Good addition of getMigrationScript function.The function includes helpful explanatory comments about how the migration script should behave, clarifying that it must handle migration from the previous major version to the new one.
120-125: Good addition of resolveVersion function.This new function provides an important capability to resolve version requirements to specific versions, which is essential for dependency management in versioned systems.
90-90:✅ Verification successful
Breaking change to the newRelease function - verify implementations are updated.
The
newReleasefunction signature has been updated to include a migration hash parameter. This is a breaking change that requires updates to all implementations of this interface.
🏁 Script executed:
#!/bin/bash # Find all implementation of the newRelease function # to verify they've been updated to include the migrationHash parameter # Find repository implementations rg "function newRelease\(" --include="*.sol" -A 2 # Check for implementations using Repository rg -A 3 "is IRepository" --include="*.sol"Length of output: 980
I’ll re-run searches with the correct ripgrep flags to locate all
newReleaseimplementations and any contracts implementingIRepository.
🏁 Script executed:
#!/bin/bash # Find all occurrences of newRelease in Solidity files rg --glob '*.sol' -n "function newRelease\(" # Find all contracts that implement the IRepository interface rg --glob '*.sol' -n "is IRepository"Length of output: 1020
Implementations updated to include
migrationHashinnewReleaseAll contracts implementing
IRepositoryalready match the updated signature:
- contracts/mocks/MockRepository.sol
- src/mocks/MockRepository.sol
- src/repositories/OwnableRepository.sol
No missing overrides remain.
docs/Upgradability.md (1)
30-40: Good explanation of runtime validation flow.The explanation of the runtime validation with ERC7746 clearly shows the call flow and middleware hooks. The ASCII diagram is particularly helpful for understanding the execution flow.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~32-~32: You might be missing the article “an” here.
Context: ...ion with ERC7746 Whenever an app calls end-user contract, or anyone attempts to ca...(AI_EN_LECTOR_MISSING_DETERMINER_AN)
[uncategorized] ~32-~32: You might be missing the article “a” here.
Context: ...er contract, or anyone attempts to call user-owned application, ERC7746 interface is...(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~32-~32: You might be missing the article “the” here.
Context: ...ttempts to call user-owned application, ERC7746 interface is used to implement [runtime...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🪛 markdownlint-cli2 (0.17.2)
35-35: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
test/eds/UpgradabilityWorkflows.ts (1)
212-220: Potential runtime error when reading first component
appComponents.contracts[0]is accessed without verifying thatcontracts.length > 0.
After the change above, add an explicit expect:expect(appComponents.contracts.length).to.be.gt(0);to fail fast if installer returns an empty array.
contracts/mocks/MockRepository.sol (1)
1-86: Clean implementation of the IRepository interfaceThe MockRepository contract provides a thorough implementation of the IRepository interface with proper version management, metadata handling, and migration script support. The code is well-structured and follows good practices.
src/mocks/MockRepository.sol (1)
1-86: Well-implemented mock for IRepository interfaceThis mock implementation provides all necessary functionality for the IRepository interface, with clear function implementations and appropriate state management. The contract properly emits events when state changes and handles version resolution correctly.
src/interfaces/IInstaller.sol (1)
130-138: Event/documentation mismatch – missingpermissionsdescription.
event Installedintroduces a third indexed parameterpermissions, yet the NatSpec block does not explain its semantics and the param list omits it.
Please document the meaning and expected values forpermissions(or remove the param if unused) to keep the public ABI self-describing.
docs/Versioning.md
Outdated
| contract MockRepository is IRepository { | ||
| using LibERC7744 for bytes32; | ||
| using LibSemver for LibSemver.Version; | ||
| using LibSemver for uint256 |
There was a problem hiding this comment.
Missing semicolon in code example.
The semicolon is missing after the using statement, which would cause a compilation error.
- using LibSemver for uint256
+ using LibSemver for uint256;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| using LibSemver for uint256 | |
| using LibSemver for uint256; |
| import {IERC7746} from "../interfaces/IERC7746.sol"; | ||
| import "@openzeppelin/contracts/utils/introspection/IERC165.sol"; | ||
| import "../interfaces/IRepository.sol"; | ||
| import "../libraries/LibSemver.sol"; | ||
| import "../versioning/LibSemver.sol"; | ||
| import {IERC7746} from "../interfaces/IERC7746.sol"; | ||
| import "../interfaces/IMigration.sol"; |
There was a problem hiding this comment.
Remove duplicate import of IERC7746
IERC7746 is imported twice (lines 3 and 7). The second import is redundant and will break Solidity compilation (Identifier already declared).
-import {IERC7746} from "../interfaces/IERC7746.sol";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import {IERC7746} from "../interfaces/IERC7746.sol"; | |
| import "@openzeppelin/contracts/utils/introspection/IERC165.sol"; | |
| import "../interfaces/IRepository.sol"; | |
| import "../libraries/LibSemver.sol"; | |
| import "../versioning/LibSemver.sol"; | |
| import {IERC7746} from "../interfaces/IERC7746.sol"; | |
| import "../interfaces/IMigration.sol"; | |
| import {IERC7746} from "../interfaces/IERC7746.sol"; | |
| import "@openzeppelin/contracts/utils/introspection/IERC165.sol"; | |
| import "../interfaces/IRepository.sol"; | |
| import "../versioning/LibSemver.sol"; | |
| import "../interfaces/IMigration.sol"; |
- Updated `hardhat.config.ts` to include new tasks for generating super interfaces from ABI files. - Modified `CloneDistribution`, `LatestVersionDistribution`, and `UpgradableDistribution` to implement `supportsInterface` for ERC165 compliance. - Introduced `SelfInstaller` contract for owner-based installation and management of distributions. - Enhanced `Distributor` to manage app states and migrations more effectively, including error handling for migration failures. - Added mock contracts for testing various distribution scenarios, including migration failures and ownership transfers. - Updated tests to cover new functionalities and ensure proper behavior of distribution and migration workflows.
…utors, Indexer, Repositories, Upgradability, and Versioning - Introduced comprehensive guides for Distributions and Distributors, detailing their creation, management, and operational commands. - Added documentation for the EDS Indexing system, explaining its role and usage. - Created guides for Repositories, outlining their purpose and CLI commands for version management. - Documented the Upgradability process, including trust assumptions and migration strategies. - Implemented a section on Versioning, detailing the semantic versioning system and CLI utilities for version management.
- Introduced a `mock` function to simulate migration script retrieval. - Added state variables to store mock addresses and calldata. - Implemented a new function `getMigrationScriptCalldata` to retrieve the mock calldata for testing purposes.
- Deleted `MockFailingDistribution`, `MockLowLevelDistribution`, `MockPanicDistribution`, `MockMigration`, and `MockRepository` as they are no longer needed. - Updated `Distributor` and related contracts to improve handling of app states and migration processes. - Introduced new mock contracts for testing, including `MockOwnableDistributor`, `MockInitializer`, and `MockNoReasonInitializer` to facilitate better testing scenarios.
- Refactored `UpgradableDistribution` to enhance its constructor and source management. - Removed `VersionDistribution` as it is no longer needed, consolidating functionality into `UpgradableDistribution`. - Updated the `instantiate` function to improve argument handling and ensure compatibility with the new structure. - Introduced `WrappedProxyInitializer` for better initialization handling in the distribution process. - Adjusted related tests to reflect changes in distribution handling and ensure proper functionality.
- Simplified error handling in the `Distributor` contract by removing redundant revert messages. - Updated tests to reflect the new behavior of reverting with a generic message when external initializers fail without a reason. - Cleaned up unnecessary debug logging in `ERC7746Hooked` tests to streamline output and focus on validation.
- Corrected the spelling of `REPOSITORY_MANAGED` in the `Distributor` and `IDistributor` contracts. - Updated the `AuthorizationMiddleware` to reflect the new naming convention. - Adjusted tests to ensure they reference the corrected migration strategy name, enhancing clarity and consistency.
…ions - Adjusted the expected final layer count from 2 to 1 after modifying the layers array. - Removed outdated comments regarding the verification of the second layer, as it is no longer applicable.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (1)
hardhat.config.ts (1)
24-34:⚠️ Potential issueWrong variable referenced – leads to empty ABI export
Inside the custom
emit_artifactssub-task the compile output is awaited in
const output = await next();but the subsequent loop still reads
args.output.contracts.
args.outputisundefined, thus no ABI files are written.-const promises = Object.entries(args.output.contracts).map( +const promises = Object.entries(output.contracts).map(Without this fix the
abi/*/*.tsfiles will be missing afternpx hardhat compile.@@ -const promises = Object.entries(args.output.contracts).map(async ([sourceName, contract]) => { +const promises = Object.entries(output.contracts).map(async ([sourceName, contract]) => {
♻️ Duplicate comments (12)
src/proxies/WrappedTransparentUpgradeableProxy.sol (2)
1-1:⚠️ Potential issueUnsupported compiler version
0.8.28The latest official Solidity release is
0.8.24.
Using an unreleased version blocks compilation for every consumer.-pragma solidity ^0.8.28; +pragma solidity ^0.8.24;
18-29:⚠️ Potential issueAdmin mismatch –
_adminparameter ignored for proxy administrationThe proxy's admin is set to
_owner, while the_adminparameter is only stored as a middleware layer.
If the caller passes aProxyAdmincontract expecting to control upgrades, it will have no authority.- ) TransparentUpgradeableProxy(_logic, _owner, _data) { + ) TransparentUpgradeableProxy(_logic, _admin, _data) {Additionally, the constructor parameter order is confusing. Consider reorganizing and improving the comments to clarify the purpose of each parameter.
src/repositories/Repository.sol (2)
52-64: Enhancements for migration hash validationThe migration hash validation could be improved by adding an early check for zero hash and providing clearer error messages.
function _newRelease(bytes32 sourceId, bytes memory metadata, LibSemver.Version memory version, bytes32 migrationHash) internal { uint256 versionFlat = version.toUint256(); if (versionFlat == 0 && version.major == 0) revert ReleaseZeroNotAllowed(); if (version.major > majorReleases) { + require(migrationHash != bytes32(0), "Migration hash required for major releases"); address migration = migrationHash.getContainerOrThrow(); - require(ERC165Checker.supportsInterface(migration, type(IMigration).interfaceId), "Invalid migration"); + require( + ERC165Checker.supportsInterface(migration, type(IMigration).interfaceId), + "Migration contract must implement IMigration" + ); if (version.major != majorReleases + 1) revert VersionIncrementInvalid(versionFlat); majorReleases = version.major; minorReleases[version.major] = 0; patchReleases[(uint128(version.major) << 64)] = 0; releaseMetadata[version.major] = metadata; releaseMigrationHash[version.major] = migrationHash;
4-7:⚠️ Potential issueMissing import for
IMigrationinterfaceThe code references
type(IMigration).interfaceIdon line 57, but theIMigrationinterface is never imported.import "../versioning/LibSemver.sol"; import "../interfaces/IRepository.sol"; import "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol"; import "../erc7744/LibERC7744.sol"; +import "../interfaces/IMigration.sol";test/eds/UpgradabilityWorkflows.ts (2)
139-139: Update length check to access contracts arrayThe current code checks
appComponents.lengthwhich verifies the number of tuple elements in the struct, not the number of deployed app components.Apply this fix:
-expect(appComponents.length).to.be.gt(0, "Should have at least one component"); +expect(appComponents.contracts.length).to.be.gt(0, "Should have at least one component");
177-180: Same tuple/array confusion after uninstallThe condition is using
appComponents.lengthinstead of checking the length of the contracts array.Apply this fix:
-if (appComponents && appComponents.length > 0) { +if (appComponents && appComponents.contracts.length > 0) { const isApp = await installer.isApp(appComponents.contracts[0]); expect(isApp).to.be.false; }test/eds/Distributor.ts (1)
361-380: Shadowing top-level fixtures complicates readabilityInside the "Distributor API" describe block, new
codeIndex/distributorvariables shadow the ones created in the outerbeforeEach.
Although it compiles, it makes the setup harder to follow and increases the risk of subtle mis-configuration.Prefer either:
- Re-using the outer fixtures, or
- Giving the inner ones distinct names (e.g.
apiDistributor).src/middleware/InstallerClonable.sol (2)
136-139: Event logs the first component for every iteration
This was highlighted in a previous review and is still present.- emit Installed(installation[0], distributionId, "0x", args); + emit Installed(installation[i], distributionId, bytes32(0), args);
143-155:appNum--breaks the monotonicity contract
getAppsNum()is documented to never decrease; decrementingappNum
opens the door for id collisions and breaks on-chain audits.Keep
appNummonotonic and, if necessary, track active apps with a
separate counter or mapping.src/interfaces/IDistributor.sol (3)
3-8: DuplicateIERC7746import
IERC7746is imported on lines 3 and 7. The second import shadows the
first and will not compile.
9-13: Typo in enum constant –REPOSITORY_MANGEDThe constant is still misspelled. Updating it now prevents the typo
from propagating to every consumer contract.
195-209: Invalid typeLibSemver.requirementsin eventsThe struct is named
VersionRequirement; usingrequirementswill not
compile.
🧹 Nitpick comments (22)
src/mocks/TestFacet.sol (1)
8-10: Initialize function implementation is appropriate for mock purposes.The
initialize()function correctly emits theInitializedevent. This implementation supports the testing of initialization patterns commonly used in upgradeable contract systems.Consider whether this function should include access control (like
onlyOwneror an initializer modifier) to better mimic real-world implementations, though this may not be necessary for testing purposes.src/mocks/MockLatestVersionDistribution.sol (2)
15-17: Consider removing unnecessary override.The
sources()function simply calls the parent implementation without adding any functionality. Unless this override is specifically needed for testing to intercept or monitor calls, consider removing it to reduce code clutter.
10-13: Consider adding documentation comments.As a mock contract used for testing, adding NatSpec comments would help other developers understand the purpose and behavior of this contract. Including information about which test scenarios this mock is designed for would be particularly useful.
src/mocks/MockOwnableDistributor.sol (4)
6-6: Unused import ShortStringsThe ShortString and ShortStrings imports from OpenZeppelin are not used in this contract. Consider removing them to keep the code clean and reduce gas costs during deployment.
-import {ShortString, ShortStrings} from "@openzeppelin/contracts/utils/ShortStrings.sol";
14-16: Missing access control on test helper functionsWhile this is acceptable for a mock contract used in testing, it's worth noting that these functions allow anyone to modify critical state variables. Consider adding a comment indicating this is intentional for testing purposes.
- function renounceApp(uint256 appId) public { + /** + * @dev Test helper function - intentionally has no access control for testing flexibility + */ + function renounceApp(uint256 appId) public {
18-20: Missing access control on setMigration test helperSimilar to the previous comment, consider adding a clarifying comment about the intentional lack of access control for testing purposes.
- function setMigration(uint256 appId, address migration) public { + /** + * @dev Test helper function - intentionally has no access control for testing flexibility + */ + function setMigration(uint256 appId, address migration) public {
1-21: Add missing NatSpec documentationThe contract is missing proper NatSpec documentation to explain its purpose and intended use in tests. Consider adding a complete NatSpec header to improve code maintainability.
// SPDX-License-Identifier: MIT pragma solidity ^0.8.19; import "../distributors/OwnableDistributor.sol"; -import {ShortString, ShortStrings} from "@openzeppelin/contracts/utils/ShortStrings.sol"; +/** + * @title MockOwnableDistributor + * @dev A mock implementation of OwnableDistributor that exposes internal state + * variables for testing purposes. This contract allows direct manipulation of + * app renouncement and migration states. + */ contract MockOwnableDistributor is OwnableDistributor {src/mocks/MockNoReasonInitializer.sol (2)
7-15: Consider implementing IInitializer interface explicitlyWhile the contract matches the function signatures needed for testing, it would be more explicit to implement the
IInitializerinterface that's already imported. This would make the contract's intended purpose clearer and ensure compatibility if the interface changes.-contract MockNoReasonInitializer { +contract MockNoReasonInitializer is IInitializer {
1-31: Add missing NatSpec documentationThe contract lacks proper NatSpec documentation to explain its purpose for silent initialization failures. Adding comprehensive documentation would help other developers understand its role in testing.
// SPDX-License-Identifier: MIT pragma solidity ^0.8.0; // We need to make sure the function selector matches what Distributor expects import {IInitializer} from "../interfaces/IInitializer.sol"; +/** + * @title MockNoReasonInitializer + * @dev A mock initializer that always reverts without providing a reason. + * This contract is used to test how the Distributor handles silent failures + * during initialization via delegatecall. + */ contract MockNoReasonInitializer {src/mocks/MockInitializer.sol (4)
7-20: Consider implementing IInitializer interface explicitlyThe contract imports IInitializer but doesn't explicitly implement it. To maintain contract integrity and make its purpose clearer, consider explicitly implementing the interface.
-contract MockInitializer { +contract MockInitializer is IInitializer {
31-38: Consider reusing code between overloaded methodsBoth initialize methods perform the same check and revert with the same error message. Consider refactoring to avoid code duplication by introducing a private helper function to handle the common logic.
+ /** + * @dev Internal helper to check for failure condition and revert if needed + * @param args The args to check for failure condition + */ + function _checkAndRevertIfNeeded(bytes memory args) private pure { + if (keccak256(args) == keccak256(bytes("FAIL"))) { + bytes memory errorMsg = bytes("Initializer failed as requested"); + assembly { + revert(add(32, errorMsg), mload(errorMsg)) + } + } + } function initialize(address[] memory, bytes memory args) external pure { - // Revert with a specific reason if args contain "FAIL" - if (keccak256(args) == keccak256(bytes("FAIL"))) { - // Use bytes for the error message to ensure it's properly passed back - bytes memory errorMsg = bytes("Initializer failed as requested"); - assembly { - revert(add(32, errorMsg), mload(errorMsg)) - } - } + _checkAndRevertIfNeeded(args); // Otherwise, succeed } // Solidity gets the selector from the first function that matches the name and parameter count // So we need to account for both possible calls function initialize( bytes32, address[] memory, bytes32, uint256, bytes calldata args ) external pure { // In case this gets called directly, delegate to the 2-parameter version - if (keccak256(args) == keccak256(bytes("FAIL"))) { - bytes memory errorMsg = bytes("Initializer failed as requested"); - assembly { - revert(add(32, errorMsg), mload(errorMsg)) - } - } + _checkAndRevertIfNeeded(bytes(args)); }
1-39: Add missing NatSpec documentationSimilar to other mock contracts, this one would benefit from comprehensive NatSpec documentation to explain its purpose and behavior to other developers.
// SPDX-License-Identifier: MIT pragma solidity ^0.8.0; // We need to make sure the function selector matches what Distributor expects import {IInitializer} from "../interfaces/IInitializer.sol"; +/** + * @title MockInitializer + * @dev A mock initializer that can be configured to succeed or fail based on input. + * It will revert with a specific error message when the argument equals "FAIL". + * Used to test initialization success and failure scenarios for the Distributor. + */ contract MockInitializer {
24-38: Comment could be more precise about function purposeThe comment "In case this gets called directly, delegate to the 2-parameter version" is misleading as the function doesn't actually delegate to the 2-parameter version. It performs the same check independently.
function initialize( bytes32, address[] memory, bytes32, uint256, bytes calldata args ) external pure { - // In case this gets called directly, delegate to the 2-parameter version + // Perform the same failure check as the 2-parameter version if (keccak256(args) == keccak256(bytes("FAIL"))) { bytes memory errorMsg = bytes("Initializer failed as requested"); assembly { revert(add(32, errorMsg), mload(errorMsg)) } } }hardhat.config.ts (1)
25-27: Path splitting is not OS-safe
sourceName.split("/")breaks on Windows, where back-slashes are used.
Usepath.basenameinstead:-const contractName = sourceName.split("/").pop()?.replace(".sol", "") || ""; +const contractName = path.basename(sourceName, ".sol");test/eds/UpgradabilityWorkflows.ts (2)
216-220: Add null check before accessing appComponents.contractsThe code could be more defensive with null/undefined checks when accessing nested arrays.
-if (appComponents && appComponents.length > 0) { +if (appComponents && appComponents.contracts && appComponents.contracts.length > 0) { const linkedDistributor = await installer.distributorOf(appComponents.contracts[0]); expect(linkedDistributor).to.equal(newDistributor.address); expect(await distributor.getAppId(appComponents.contracts[0])).to.equal(0); }
282-283: Match error expectation with implementationConsider specifying the exact custom error to expect, rather than relying on the generic
be.revertedWithCustomError.await expect( installer.connect(user).install(distributor.address, distributionId, installData) -).to.be.revertedWithCustomError(distributor, "DistributionNotFound"); +).to.be.revertedWithCustomError(distributor, "DistributionNotFound").or.to.be.reverted;docs/guides/Upgradability.md (2)
42-49: Consider adding a diagram for the upgradeable distributionsThe explanation of ownership and access permission boundaries is critical but can be difficult to visualize.
Consider adding a diagram showing the relationships between distributors, installers, and applications to help users understand how ownership and permissions are separated.
52-64: Consider adding an error handling section for upgrade failuresThe upgrade workflow explains the happy path, but doesn't cover how the system handles failures during the upgrade process.
Add a section covering error handling during upgrades, including how the system recovers from failed migrations and what state the contracts remain in after a failure.
test/eds/Distributor.ts (2)
279-305: Test appears incomplete - missing assertions after expected revertThis test expects a revert but doesn't assert any specific error or post-condition after the revert. It would be more useful to verify exactly which error is thrown and why.
Consider adding specific error assertions:
await expect( distributor .connect(owner) .instantiate(failingDistId, ethers.utils.formatBytes32String("FAIL")) -).to.be.reverted; +).to.be.revertedWithCustomError(distributor, "DistributionInstantiationFailed") + .or.to.be.revertedWithCustomError(distributor, "DistributionInstantiationPanic");
489-522: Consider using structured event parsing instead of manual accessThe code extracts event arguments using direct array access which is brittle and depends on the event parameter order. If the event parameters change order, this test will break.
Consider using structured event parsing with explicit property names:
const instantiatedEvent = receipt.events?.find((e) => e.event === "Instantiated"); expect(instantiatedEvent, "Instantiated event should exist").to.not.be.undefined; // Use the typesafe interface instead of raw array access const appId = instantiatedEvent.args.newAppId; const appComponents = instantiatedEvent.args.appComponents;This approach requires proper event typing in the test files but provides better long-term maintainability.
src/interfaces/IDistributor.sol (1)
109-115: Event parameter name is confusing
readableNameHashZappears to be a typo. Consider renaming to
readableNameHash(or drop the suffix entirely) to improve readability.src/distributors/Distributor.sol (1)
213-224: Redundant writes inside thefor-loop
distributionOf[appId] = distributorsIdis executed once per component
but stores the same value every time. Move it outside the loop to save
gas.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (35)
docs/guides/Distributions.md(1 hunks)docs/guides/Distributors.md(1 hunks)docs/guides/Indexer.md(1 hunks)docs/guides/Repositories.md(1 hunks)docs/guides/Upgradability.md(1 hunks)docs/guides/Versioning.md(1 hunks)hardhat.config.ts(4 hunks)src/distributions/CloneDistribution.sol(2 hunks)src/distributions/LatestVersionDistribution.sol(1 hunks)src/distributions/UpgradableDistribution.sol(1 hunks)src/distributors/Distributor.sol(1 hunks)src/interfaces/IDistribution.sol(1 hunks)src/interfaces/IDistributor.sol(8 hunks)src/middleware/InstallerClonable.sol(1 hunks)src/middleware/InstallerOwnable.sol(1 hunks)src/mocks/MockFailingDistribution.sol(1 hunks)src/mocks/MockFailingMigration.sol(1 hunks)src/mocks/MockInitializer.sol(1 hunks)src/mocks/MockInstaller.sol(2 hunks)src/mocks/MockLatestVersionDistribution.sol(1 hunks)src/mocks/MockLowLevelDistribution.sol(1 hunks)src/mocks/MockMigration.sol(1 hunks)src/mocks/MockNoReasonInitializer.sol(1 hunks)src/mocks/MockOwnableDistributor.sol(1 hunks)src/mocks/MockPanicDistribution.sol(1 hunks)src/mocks/MockRepository.sol(1 hunks)src/mocks/MockTransferOwnershipProxy.sol(1 hunks)src/mocks/TestFacet.sol(1 hunks)src/proxies/WrappedTransparentUpgradeableProxy.sol(1 hunks)src/repositories/Repository.sol(6 hunks)test/eds/Distributor.ts(6 hunks)test/eds/LatestVersionDistribution.ts(1 hunks)test/eds/Repository.ts(5 hunks)test/eds/UpgradabilityWorkflows.ts(1 hunks)test/eds/VersionDistributor.ts(8 hunks)
✅ Files skipped from review due to trivial changes (6)
- docs/guides/Versioning.md
- docs/guides/Distributors.md
- docs/guides/Repositories.md
- src/mocks/MockRepository.sol
- docs/guides/Distributions.md
- docs/guides/Indexer.md
🚧 Files skipped from review as they are similar to previous changes (9)
- src/distributions/CloneDistribution.sol
- src/distributions/LatestVersionDistribution.sol
- src/mocks/MockPanicDistribution.sol
- test/eds/LatestVersionDistribution.ts
- src/mocks/MockMigration.sol
- src/mocks/MockLowLevelDistribution.sol
- src/mocks/MockInstaller.sol
- src/mocks/MockFailingDistribution.sol
- src/distributions/UpgradableDistribution.sol
🧰 Additional context used
🧬 Code Graph Analysis (1)
hardhat.config.ts (1)
deploy/ERC7744.js (2)
result(11-15)hre(4-4)
🔇 Additional comments (21)
src/mocks/TestFacet.sol (1)
6-6: Event declaration looks good.The addition of the
Initializedevent aligns with the implementation of initialization patterns for upgradeable contracts being introduced in this PR.src/mocks/MockLatestVersionDistribution.sol (2)
1-24: LGTM! The mock contract implementation is clean and minimal.The contract correctly inherits from
LatestVersionDistributionand provides the necessary overrides for testing. The use of OpenZeppelin'sShortStringsutility for metadata conversion is appropriate.
19-23:✅ Verification successful
Verify the intent of ignoring input parameters in
instantiate().The
instantiate()function ignores its input parameter and callssuper._instantiate()instead of passing the parameters to the parent's implementation. If this is intentional for testing purposes, consider adding a comment explaining why.
🏁 Script executed:
#!/bin/bash # Check how the parent's instantiate function differs from _instantiate to understand the bypass rg -A 10 "function instantiate\(" src/distributions/LatestVersionDistribution.sol rg -A 10 "function _instantiate\(" src/distributions/LatestVersionDistribution.solLength of output: 726
Confirmed:
instantiateintentionally ignores its input
BothLatestVersionDistributionandMockLatestVersionDistributionoverrideinstantiate(bytes)to callsuper._instantiate()and drop the provided data, because the distribution always pulls the latest version. No changes are required.src/interfaces/IDistribution.sol (1)
4-4: Good addition of ERC-165 support.Adding the IERC165 interface to IDistribution improves standardization and enables interface detection through the ERC-165 standard. This aligns with best practices for contract interoperability.
Also applies to: 11-11
src/repositories/Repository.sol (4)
80-80: Improved latestVersion update logicGood enhancement to only update latestVersion when the new version is greater. This prevents accidentally "rolling back" to an older version.
92-156: Well-implemented version resolution logicThe new
resolveVersionfunction properly implements all semantic versioning requirement types, including the previously unimplementedLESSER_EQUALandLESSERrequirements. This provides a complete version resolution mechanism.
157-167: Good refactoring of the get functionSplitting the version resolution logic into a separate function improves code organization and reusability. The new implementation properly checks that the resolved version exists before returning.
212-214: Getter function for migration scriptGood addition of the
getMigrationScriptfunction to access the migration hash for a given major version.src/mocks/MockFailingMigration.sol (1)
1-34: Well-structured mock for testing migration failuresThis mock implementation correctly implements the IMigration interface for testing failure scenarios. The code follows best practices with:
- Proper documentation
- Implementation of the ERC-165 interface detection
- Clear error message that indicates intentional failure
This will be useful for testing error handling in migration workflows.
src/mocks/MockTransferOwnershipProxy.sol (1)
1-22: LGTM: Well documented mock for proxy admin testingThis mock is well implemented with clear NatSpec documentation, proper inheritance, and a focused implementation that serves its stated purpose. The contract correctly implements the
IAdminGetterinterface and extendsOwnableto simulate proxy admin ownership transfer scenarios.src/mocks/MockInitializer.sol (1)
10-20: Good implementation of conditional failure with proper error propagationThe implementation correctly checks for a specific failure condition and uses assembly to properly propagate the error message back through a delegatecall. This is a good pattern for testing error handling in delegate calls.
test/eds/VersionDistributor.ts (1)
162-165: The event name changed – adjust expectationThe contract now emits
DistributionDisabledbut the expectation still listens
fordisableDistribution’s genericDistributionDisabled?
Verify the event signature (case-sensitive) and make the assertion explicit to
avoid false positives.await expect(distributor.disableDistribution(id)) .to.emit(distributor, "DistributionDisabled");hardhat.config.ts (1)
176-181: VerifycancunEVM target is supportedNot all tool-chains or networks currently support the forthcoming Cancun
changes. Make sure:
- The Hardhat version in
package.jsonis ≥^2.22.0(first to add Cancun).- All CI environments use the matching solc build.
- You do not deploy to networks whose Berlin/London forks reject Cancun byte-code.
Please double-check before merge.
test/eds/UpgradabilityWorkflows.ts (1)
32-47: LGTM: Clean helper function implementationThis helper function provides a clean way to create version requirements throughout the tests.
docs/guides/Upgradability.md (3)
5-5: Clear introduction to the EDS upgrade patternThe introduction effectively outlines the key value proposition of EDS's upgrade pattern with proper balance of ownership between stakeholders.
10-28: Well-structured trust domain breakdownThe clear delineation of trust assumptions across the Ethereum, Developer, Distributor, and End-user domains creates a solid foundation for understanding the security model.
80-103: Well-documented migration strategiesThe three different migration strategies (Call, Delegatecall, Repository managed) are clearly explained with their respective behaviors and use cases.
src/middleware/InstallerOwnable.sol (3)
1-15: Good implementation of initializable ownership patternThe contract correctly inherits from both InstallerClonable and Ownable, with proper initialization sequence that ensures the owner is set properly in the constructor and initialize function.
17-27: Well-designed permission control in install functionThe separation of installation logic between owner and non-owner callers allows for flexible permission management with different installation paths.
29-55: Consistent use of access modifiersAll management functions are appropriately protected with the onlyOwner modifier and properly delegate to parent contract implementations.
src/distributors/Distributor.sol (1)
292-306:VersionChangedarguments swappedThe event is defined as
(distributionId, newRequirement, newRequirementData)
but_changeVersionemits(distributionId, oldRequirement, newRequirement).
This misleads off-chain indexers.- emit VersionChanged(distributionId, oldRequirement, newRequirement); + emit VersionChanged(distributionId, newRequirement, newRequirement);(Or amend the event signature to match the intended semantics.)
- Renamed `SelfInstaller` to `OwnableInstaller` for clarity and consistency. - Introduced `enforceActiveDistribution` function to validate distributor and distribution ID permissions. - Updated installation logic to streamline the process for both owner and public installations. - Adjusted tests to reflect changes in the installer contract and ensure proper functionality. - Removed outdated mock installer references and replaced them with the new `OwnableInstaller`.
- Updated the README to clarify the purpose and features of the Ethereum Distribution System (EDS), emphasizing its decentralized nature and version management capabilities. - Improved use case descriptions for better clarity and added links to relevant documentation. - Expanded the overview section to define user roles and interactions more clearly. - Enhanced documentation for Distributions, Distributors, Repositories, and Upgradability, detailing their functionalities and providing examples for better understanding. - Updated CLI commands for managing distributions and distributors to reflect recent changes in functionality.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Tests
Chores