From 036e38b5cf2ffdceb541bf4590e66a40e96dd754 Mon Sep 17 00:00:00 2001 From: purp Date: Fri, 21 Jul 2023 10:41:29 -0600 Subject: [PATCH 01/12] naming convention --- .../contracts/DependencyRegistryV0.sol | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/packages/contracts/contracts/DependencyRegistryV0.sol b/packages/contracts/contracts/DependencyRegistryV0.sol index 48724c5ca..83ed32be9 100644 --- a/packages/contracts/contracts/DependencyRegistryV0.sol +++ b/packages/contracts/contracts/DependencyRegistryV0.sol @@ -134,10 +134,10 @@ contract DependencyRegistryV0 is ); _dependencyTypes.add(_dependencyType); - Dependency storage dependencyType = dependencyDetails[_dependencyType]; - dependencyType.preferredCDN = _preferredCDN; - dependencyType.preferredRepository = _preferredRepository; - dependencyType.referenceWebsite = _referenceWebsite; + Dependency storage dependency = dependencyDetails[_dependencyType]; + dependency.preferredCDN = _preferredCDN; + dependency.preferredRepository = _preferredRepository; + dependency.referenceWebsite = _referenceWebsite; emit DependencyAdded( _dependencyType, @@ -205,14 +205,11 @@ contract DependencyRegistryV0 is _onlyAdminACL(this.updateDependencyScript.selector); _onlyNonEmptyString(_script); _onlyExistingDependencyType(_dependencyType); - Dependency storage dependencyType = dependencyDetails[_dependencyType]; - require( - _scriptId < dependencyType.scriptCount, - "scriptId out of range" - ); + Dependency storage dependency = dependencyDetails[_dependencyType]; + require(_scriptId < dependency.scriptCount, "scriptId out of range"); // store script in contract bytecode, replacing reference address from // the contract that no longer exists with the newly created one - dependencyType.scriptBytecodeAddresses[_scriptId] = _script + dependency.scriptBytecodeAddresses[_scriptId] = _script .writeToBytecode(); emit DependencyScriptUpdated(_dependencyType); From 7f1766b3c59e4ac049aced96cdc40006d75ec4c1 Mon Sep 17 00:00:00 2001 From: purp Date: Fri, 21 Jul 2023 10:42:01 -0600 Subject: [PATCH 02/12] isSupportedCoreContract helper --- packages/contracts/contracts/DependencyRegistryV0.sol | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/packages/contracts/contracts/DependencyRegistryV0.sol b/packages/contracts/contracts/DependencyRegistryV0.sol index 83ed32be9..183975a54 100644 --- a/packages/contracts/contracts/DependencyRegistryV0.sol +++ b/packages/contracts/contracts/DependencyRegistryV0.sol @@ -646,6 +646,17 @@ contract DependencyRegistryV0 is return _supportedCoreContracts.at(_index); } + /** + * @notice Returns whether the given contract address is a supported core contract. + * @param coreContractAddress Address of the core contract to be queried. + * @return True if the given contract address is a supported core contract. + */ + function isSupportedCoreContract( + address coreContractAddress + ) external view returns (bool) { + return _supportedCoreContracts.contains(coreContractAddress); + } + /** * @notice Returns a list of supported core contracts. * @return List of supported core contracts. From caf5d0bce639a5dfb1b67c7a8014159b888c9347 Mon Sep 17 00:00:00 2001 From: purp Date: Fri, 21 Jul 2023 10:46:59 -0600 Subject: [PATCH 03/12] additional test coverage --- .../test/dependency-registry/DependencyRegistryV0.test.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/contracts/test/dependency-registry/DependencyRegistryV0.test.ts b/packages/contracts/test/dependency-registry/DependencyRegistryV0.test.ts index 466bc02c0..3efad7fc9 100644 --- a/packages/contracts/test/dependency-registry/DependencyRegistryV0.test.ts +++ b/packages/contracts/test/dependency-registry/DependencyRegistryV0.test.ts @@ -1447,6 +1447,12 @@ describe(`DependencyRegistryV0`, async function () { expect(supportedCoreContracts).to.deep.eq([ config.genArt721Core.address, ]); + + const isSupportedCoreContract = + await config.dependencyRegistry.isSupportedCoreContract( + config.genArt721Core.address + ); + expect(isSupportedCoreContract).to.eq(true); }); }); describe("removeSupportedCoreContract", function () { From 771815c838d23fded0a85776b96abe224534fa5f Mon Sep 17 00:00:00 2001 From: purp Date: Fri, 21 Jul 2023 11:55:30 -0600 Subject: [PATCH 04/12] Add backwards compatibility for pointer-writing to DependencyRegistryV0 --- .../contracts/DependencyRegistryV0.sol | 103 +++++++++++++++--- 1 file changed, 88 insertions(+), 15 deletions(-) diff --git a/packages/contracts/contracts/DependencyRegistryV0.sol b/packages/contracts/contracts/DependencyRegistryV0.sol index 183975a54..4ddf714be 100644 --- a/packages/contracts/contracts/DependencyRegistryV0.sol +++ b/packages/contracts/contracts/DependencyRegistryV0.sol @@ -169,7 +169,8 @@ contract DependencyRegistryV0 is } /** - * @notice Adds a script to dependency `_dependencyType`. + * @notice Adds a script to dependency `_dependencyType`, by way of + * providing a string to write to bytecode storage. * @param _dependencyType dependency to be updated. * @param _script Script to be added. Required to be a non-empty string, * but no further validation is performed. @@ -191,11 +192,12 @@ contract DependencyRegistryV0 is } /** - * @notice Updates script for dependencyType `_dependencyType` at script ID `_scriptId`. + * @notice Updates script for dependencyType `_dependencyType` at script ID `_scriptId`, + * by way of providing a string to write to bytecode storage. * @param _dependencyType dependency to be updated. * @param _scriptId Script ID to be updated. * @param _script The updated script value. Required to be a non-empty - * string, but no further validation is performed. + * string, but no further validation is performed. */ function updateDependencyScript( bytes32 _dependencyType, @@ -215,6 +217,53 @@ contract DependencyRegistryV0 is emit DependencyScriptUpdated(_dependencyType); } + /** + * @notice Adds a script to dependency `_dependencyType`, by way of + * providing an already written chunk of bytecode storage. + * @param _dependencyType dependency to be updated. + * @param _scriptPointer Address of script to be added. Required to be a non-zero address, + * but no further validation is performed. + */ + function addDependencyScriptPointer( + bytes32 _dependencyType, + address _scriptPointer + ) external { + _onlyAdminACL(this.addDependencyScriptPointer.selector); + _onlyNonZeroAddress(_scriptPointer); + _onlyExistingDependencyType(_dependencyType); + Dependency storage dependency = dependencyDetails[_dependencyType]; + // store script in contract bytecode + dependency.scriptBytecodeAddresses[ + dependency.scriptCount + ] = _scriptPointer; + dependency.scriptCount = dependency.scriptCount + 1; + + emit DependencyScriptUpdated(_dependencyType); + } + + /** + * @notice Updates script for dependencyType `_dependencyType` at script ID `_scriptId`, + * by way of providing an already written chunk of bytecode storage. + * @param _dependencyType dependency to be updated. + * @param _scriptId Script ID to be updated. + * @param _scriptPointer The updated script pointer (address of bytecode storage). + * Required to be a non-zero address, but no further validation is performed. + */ + function updateDependencyScriptPointer( + bytes32 _dependencyType, + uint256 _scriptId, + address _scriptPointer + ) external { + _onlyAdminACL(this.updateDependencyScriptPointer.selector); + _onlyNonZeroAddress(_scriptPointer); + _onlyExistingDependencyType(_dependencyType); + Dependency storage dependency = dependencyDetails[_dependencyType]; + require(_scriptId < dependency.scriptCount, "scriptId out of range"); + dependency.scriptBytecodeAddresses[_scriptId] = _scriptPointer; + + emit DependencyScriptUpdated(_dependencyType); + } + /** * @notice Removes last script from dependency `_dependencyType`. * @param _dependencyType dependency to be updated. @@ -717,7 +766,7 @@ contract DependencyRegistryV0 is /** * @notice Returns address with bytecode containing script for - * dependency `_dependencyTypes` at script index `_index`. + * dependency `_dependencyTypes` at script index `_index`. */ function getDependencyScriptBytecodeAddressAtIndex( bytes32 _dependencyType, @@ -727,10 +776,31 @@ contract DependencyRegistryV0 is dependencyDetails[_dependencyType].scriptBytecodeAddresses[_index]; } + /** + * @notice Returns the storage library version for + * dependency `_dependencyTypes` at script index `_index`. + */ + function getDependencyScriptBytecodeStorageVersionAtIndex( + bytes32 _dependencyType, + uint256 _index + ) external view returns (bytes32) { + return + BytecodeStorageReader.getLibraryVersionForBytecode( + dependencyDetails[_dependencyType].scriptBytecodeAddresses[ + _index + ] + ); + } + /** * @notice Returns script for dependency `_dependencyType` at script index `_index`. * @param _dependencyType dependency to be queried. * @param _index Index of script to be queried. + * @dev This method attempts to introspectively determine which library version of + * `BytecodeStorage` was used to write the stored script string that is being + * read back, in order to use the proper read approach. If the version is + * non-determinate, a fall-back to reading using the assumption that the bytes + * were written with `SSTORE2` is used. */ function getDependencyScriptAtIndex( bytes32 _dependencyType, @@ -742,7 +812,20 @@ contract DependencyRegistryV0 is return ""; } - return _readFromBytecode(dependency.scriptBytecodeAddresses[_index]); + address scriptAddress = dependency.scriptBytecodeAddresses[_index]; + bytes32 storageVersion = BytecodeStorageReader + .getLibraryVersionForBytecode(scriptAddress); + + if (storageVersion == BytecodeStorageReader.UNKNOWN_VERSION_STRING) { + return + string( + BytecodeStorageReader.readBytesFromSSTORE2Bytecode( + scriptAddress + ) + ); + } else { + return BytecodeStorageReader.readFromBytecode(scriptAddress); + } } /** @@ -836,14 +919,4 @@ contract DependencyRegistryV0 is OwnableUpgradeable._transferOwnership(newOwner); adminACLContract = IAdminACLV0(newOwner); } - - /** - * Helper for calling `BytecodeStorageReader` external library reader method, - * added for bytecode size reduction purposes. - */ - function _readFromBytecode( - address _address - ) internal view returns (string memory) { - return BytecodeStorageReader.readFromBytecode(_address); - } } From a79c2946030ad91f67c94e9f6a4b10c1cb772eb8 Mon Sep 17 00:00:00 2001 From: purp Date: Mon, 24 Jul 2023 13:49:05 -0600 Subject: [PATCH 05/12] basic test scafolding --- .../DependencyRegistryV0.test.ts | 58 +++++++++++++++++++ ...StorageV1_BackwardsCompatibleReads.test.ts | 14 +---- 2 files changed, 59 insertions(+), 13 deletions(-) diff --git a/packages/contracts/test/dependency-registry/DependencyRegistryV0.test.ts b/packages/contracts/test/dependency-registry/DependencyRegistryV0.test.ts index 3efad7fc9..a8e9b52eb 100644 --- a/packages/contracts/test/dependency-registry/DependencyRegistryV0.test.ts +++ b/packages/contracts/test/dependency-registry/DependencyRegistryV0.test.ts @@ -1,14 +1,18 @@ import { expectRevert, constants } from "@openzeppelin/test-helpers"; import { expect } from "chai"; import { ethers } from "hardhat"; +import { Contract } from "ethers"; import Mocha from "mocha"; import { loadFixture } from "@nomicfoundation/hardhat-network-helpers"; + import { AdminACLV0, AdminACLV0__factory, DependencyRegistryV0, GenArt721CoreV1, GenArt721CoreV3, + BytecodeV1TextCR_DMock, + SSTORE2Mock, } from "../../scripts/contracts"; import { @@ -47,6 +51,31 @@ describe(`DependencyRegistryV0`, async function () { const preferredRepository = "https://github.com/processing/p5.js"; const referenceWebsite = "https://p5js.org/"; + // Helper that retrieves the address of the most recently deployed contract + // containing bytecode for storage, from the SSTORE2 library. + async function getLatestTextDeploymentAddressSSTORE2(sstore2Mock: Contract) { + const nextTextSlotId = await sstore2Mock.nextTextSlotId(); + // decrement from `nextTextSlotId` to get last updated slot + const textSlotId = nextTextSlotId - 1; + const textBytecodeAddress = await sstore2Mock.storedTextBytecodeAddresses( + textSlotId + ); + return textBytecodeAddress; + } + + // Helper that retrieves the address of the most recently deployed contract + // containing bytecode for storage, from the V0 ByteCode storage library. + async function getLatestTextDeploymentAddressV1( + bytecodeV1TextCR_DMock: Contract + ) { + const nextTextSlotId = await bytecodeV1TextCR_DMock.nextTextSlotId(); + // decrement from `nextTextSlotId` to get last updated slot + const textSlotId = nextTextSlotId - 1; + const textBytecodeAddress = + await bytecodeV1TextCR_DMock.storedTextBytecodeAddresses(textSlotId); + return textBytecodeAddress; + } + async function _beforeEach() { let config: T_Config = { accounts: await getAccounts(), @@ -104,6 +133,21 @@ describe(`DependencyRegistryV0`, async function () { await config.minter .connect(config.accounts.artist) .updatePricePerTokenInWei(config.projectZero, 0); + + // set up library mocks + // deploy the V1 library mock + config.bytecodeV1TextCR_DMock = await deployWithStorageLibraryAndGet( + config, + "BytecodeV1TextCR_DMock", + [] // no deployment args + ); + // deploy the SSTORE2 library mock + config.sstore2Mock = await deployAndGet( + config, + "SSTORE2Mock", + [] // no deployment args + ); + return config; } @@ -525,6 +569,20 @@ describe(`DependencyRegistryV0`, async function () { this.config = config; }); + // TODO! + describe("direct pointer compatibility", function () { + it("does not allow non-admins to add a script pointer", async function () { + // get config from beforeEach + const config = this.config; + await expectRevert( + config.dependencyRegistry + .connect(config.accounts.user) + .addDependencyScriptPointer(dependencyTypeBytes, "on-chain script"), + ONLY_ADMIN_ACL_ERROR + ); + }); + }); + describe("addDependencyScript", function () { it("does not allow non-admins to add a script", async function () { // get config from beforeEach diff --git a/packages/contracts/test/libs/BytecodeStorageV1_BackwardsCompatibleReads.test.ts b/packages/contracts/test/libs/BytecodeStorageV1_BackwardsCompatibleReads.test.ts index ef0299cdc..88f52bf61 100644 --- a/packages/contracts/test/libs/BytecodeStorageV1_BackwardsCompatibleReads.test.ts +++ b/packages/contracts/test/libs/BytecodeStorageV1_BackwardsCompatibleReads.test.ts @@ -62,7 +62,6 @@ describe("BytecodeStorageV1 Backwards Compatible Reads Tests", async function () // Retrieve the address of the written target text from the SSTORE2 library. const textBytecodeAddress = getLatestTextDeploymentAddressSSTORE2( - config, config.sstore2Mock ); @@ -96,7 +95,6 @@ describe("BytecodeStorageV1 Backwards Compatible Reads Tests", async function () // Retrieve the address of the written target text from the V0 library. const textBytecodeAddress = getLatestTextDeploymentAddressV0( - config, config.bytecodeV0TextCR_DMock ); @@ -116,10 +114,7 @@ describe("BytecodeStorageV1 Backwards Compatible Reads Tests", async function () // Helper that retrieves the address of the most recently deployed contract // containing bytecode for storage, from the SSTORE2 library. - async function getLatestTextDeploymentAddressSSTORE2( - config: BytecodeStorageBackwardsCompatibleTestConfig, - sstore2Mock: Contract - ) { + async function getLatestTextDeploymentAddressSSTORE2(sstore2Mock: Contract) { const nextTextSlotId = await sstore2Mock.nextTextSlotId(); // decrement from `nextTextSlotId` to get last updated slot const textSlotId = nextTextSlotId - 1; @@ -132,7 +127,6 @@ describe("BytecodeStorageV1 Backwards Compatible Reads Tests", async function () // Helper that retrieves the address of the most recently deployed contract // containing bytecode for storage, from the V0 ByteCode storage library. async function getLatestTextDeploymentAddressV0( - config: BytecodeStorageBackwardsCompatibleTestConfig, bytecodeV0TextCR_DMock: Contract ) { const nextTextSlotId = await bytecodeV0TextCR_DMock.nextTextSlotId(); @@ -146,7 +140,6 @@ describe("BytecodeStorageV1 Backwards Compatible Reads Tests", async function () // Helper that retrieves the address of the most recently deployed contract // containing bytecode for storage, from the V0 ByteCode storage library. async function getLatestTextDeploymentAddressV1( - config: BytecodeStorageBackwardsCompatibleTestConfig, bytecodeV1TextCR_DMock: Contract ) { const nextTextSlotId = await bytecodeV1TextCR_DMock.nextTextSlotId(); @@ -280,7 +273,6 @@ describe("BytecodeStorageV1 Backwards Compatible Reads Tests", async function () .connect(config.accounts.deployer) .createText("zip zipppity zoooop zop"); const textBytecodeAddress = getLatestTextDeploymentAddressV0( - config, config.bytecodeV0TextCR_DMock ); @@ -305,7 +297,6 @@ describe("BytecodeStorageV1 Backwards Compatible Reads Tests", async function () .connect(config.accounts.deployer) .createText("zip zipppity zoooop zop"); const textBytecodeAddress = getLatestTextDeploymentAddressSSTORE2( - config, config.sstore2Mock ); @@ -326,7 +317,6 @@ describe("BytecodeStorageV1 Backwards Compatible Reads Tests", async function () .connect(config.accounts.deployer) .createText("zip zipppity zoooop zop"); const textBytecodeAddress = getLatestTextDeploymentAddressSSTORE2( - config, config.sstore2Mock ); @@ -350,7 +340,6 @@ describe("BytecodeStorageV1 Backwards Compatible Reads Tests", async function () .connect(config.accounts.deployer) .createText("zip zipppity zoooop zop"); const textBytecodeAddress = getLatestTextDeploymentAddressV0( - config, config.bytecodeV0TextCR_DMock ); @@ -373,7 +362,6 @@ describe("BytecodeStorageV1 Backwards Compatible Reads Tests", async function () .connect(config.accounts.deployer) .createText("zip zipppity zoooop zop"); const textBytecodeAddress = getLatestTextDeploymentAddressV1( - config, config.bytecodeV1TextCR_DMock ); From bbefb52486f48d6b5ae65202ae50ece344045b0f Mon Sep 17 00:00:00 2001 From: purp Date: Tue, 25 Jul 2023 08:35:40 -0600 Subject: [PATCH 06/12] additional test coverage --- .../test/dependency-registry/DependencyRegistryV0.test.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/contracts/test/dependency-registry/DependencyRegistryV0.test.ts b/packages/contracts/test/dependency-registry/DependencyRegistryV0.test.ts index 3efad7fc9..368a2fab3 100644 --- a/packages/contracts/test/dependency-registry/DependencyRegistryV0.test.ts +++ b/packages/contracts/test/dependency-registry/DependencyRegistryV0.test.ts @@ -1503,6 +1503,12 @@ describe(`DependencyRegistryV0`, async function () { const supportedCoreContracts = await config.dependencyRegistry.getSupportedCoreContracts(); expect(supportedCoreContracts).to.deep.eq([]); + + const isSupportedCoreContract = + await config.dependencyRegistry.isSupportedCoreContract( + config.genArt721Core.address + ); + expect(isSupportedCoreContract).to.eq(false); }); }); describe("addProjectDependencyTypeOverride", function () { From 911bba9464153b0975b44b506a78e62b771d7249 Mon Sep 17 00:00:00 2001 From: purp Date: Tue, 25 Jul 2023 09:22:32 -0600 Subject: [PATCH 07/12] Test coverageeeeeeeee; https://www.youtube.com/watch\?v\=vmgp7MHzmBM --- .../DependencyRegistryV0.test.ts | 410 +++++++++++++++++- 1 file changed, 391 insertions(+), 19 deletions(-) diff --git a/packages/contracts/test/dependency-registry/DependencyRegistryV0.test.ts b/packages/contracts/test/dependency-registry/DependencyRegistryV0.test.ts index 6ec456620..75f372429 100644 --- a/packages/contracts/test/dependency-registry/DependencyRegistryV0.test.ts +++ b/packages/contracts/test/dependency-registry/DependencyRegistryV0.test.ts @@ -23,10 +23,12 @@ import { deployWithStorageLibraryAndGet, deployCoreWithMinterFilter, } from "../util/common"; +import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; const ONLY_ADMIN_ACL_ERROR = "Only Admin ACL allowed"; const ONLY_EXISTING_DEPENDENCY_TYPE_ERROR = "Dependency type does not exist"; const ONLY_NON_EMPTY_STRING_ERROR = "Must input non-empty string"; +const ONLY_NON_ZERO_ADDRESS_ERROR = "Must input non-zero address"; // test the following V3 core contract derivatives: const coreContractsToTest = [ @@ -51,9 +53,16 @@ describe(`DependencyRegistryV0`, async function () { const preferredRepository = "https://github.com/processing/p5.js"; const referenceWebsite = "https://p5js.org/"; - // Helper that retrieves the address of the most recently deployed contract - // containing bytecode for storage, from the SSTORE2 library. - async function getLatestTextDeploymentAddressSSTORE2(sstore2Mock: Contract) { + // Helper that retrieves writes content to blockchain bytecode storage using SSTORE2, + // and returns the address of that content. + async function writeContentWithSSTORE2( + content: string, + sstore2Mock: Contract, + writer: SignerWithAddress + ) { + // write + await sstore2Mock.connect(writer).createText(content); + const nextTextSlotId = await sstore2Mock.nextTextSlotId(); // decrement from `nextTextSlotId` to get last updated slot const textSlotId = nextTextSlotId - 1; @@ -65,9 +74,14 @@ describe(`DependencyRegistryV0`, async function () { // Helper that retrieves the address of the most recently deployed contract // containing bytecode for storage, from the V0 ByteCode storage library. - async function getLatestTextDeploymentAddressV1( - bytecodeV1TextCR_DMock: Contract + async function writeContentWithBytecodeStorageV1( + content: string, + bytecodeV1TextCR_DMock: Contract, + writer: SignerWithAddress ) { + // write + await bytecodeV1TextCR_DMock.connect(writer).createText(content); + const nextTextSlotId = await bytecodeV1TextCR_DMock.nextTextSlotId(); // decrement from `nextTextSlotId` to get last updated slot const textSlotId = nextTextSlotId - 1; @@ -569,20 +583,6 @@ describe(`DependencyRegistryV0`, async function () { this.config = config; }); - // TODO! - describe("direct pointer compatibility", function () { - it("does not allow non-admins to add a script pointer", async function () { - // get config from beforeEach - const config = this.config; - await expectRevert( - config.dependencyRegistry - .connect(config.accounts.user) - .addDependencyScriptPointer(dependencyTypeBytes, "on-chain script"), - ONLY_ADMIN_ACL_ERROR - ); - }); - }); - describe("addDependencyScript", function () { it("does not allow non-admins to add a script", async function () { // get config from beforeEach @@ -825,6 +825,321 @@ describe(`DependencyRegistryV0`, async function () { expect(storedScript).to.eq(updatedScript); }); }); + + describe("addDependencyScriptPointer", function () { + it("does not allow non-admins to add a script pointer", async function () { + // get config from beforeEach + const config = this.config; + + const contentAddress = writeContentWithBytecodeStorageV1( + "on-chain script", + config.bytecodeV1TextCR_DMock, + config.accounts.deployer + ); + + await expectRevert( + config.dependencyRegistry + .connect(config.accounts.user) + .addDependencyScriptPointer(dependencyTypeBytes, contentAddress), + ONLY_ADMIN_ACL_ERROR + ); + }); + + it("does not allow adding a script to a dependency that does not exist", async function () { + // get config from beforeEach + const config = this.config; + + const contentAddress = writeContentWithBytecodeStorageV1( + "on-chain script", + config.bytecodeV1TextCR_DMock, + config.accounts.deployer + ); + + await expectRevert( + config.dependencyRegistry + .connect(config.accounts.deployer) + .addDependencyScriptPointer( + ethers.utils.formatBytes32String("nonExistentDependencyType"), + contentAddress + ), + ONLY_EXISTING_DEPENDENCY_TYPE_ERROR + ); + }); + + it("does not allow specifying zero address as script pointer", async function () { + // get config from beforeEach + const config = this.config; + await expectRevert( + config.dependencyRegistry + .connect(config.accounts.deployer) + .addDependencyScriptPointer( + dependencyTypeBytes, + constants.ZERO_ADDRESS + ), + ONLY_NON_ZERO_ADDRESS_ERROR + ); + }); + + it("supports BytecodeStorageV1 reads/writes", async function () { + // get config from beforeEach + const config = this.config; + const script = "on-chain script"; + const contentAddress = writeContentWithBytecodeStorageV1( + script, + config.bytecodeV1TextCR_DMock, + config.accounts.deployer + ); + + await expect( + config.dependencyRegistry.addDependencyScriptPointer( + dependencyTypeBytes, + contentAddress + ) + ) + .to.emit(config.dependencyRegistry, "DependencyScriptUpdated") + .withArgs(dependencyTypeBytes); + + const dependencyDetails = + await config.dependencyRegistry.getDependencyDetails( + dependencyTypeBytes + ); + + expect(dependencyDetails.scriptCount).to.eq(1); + + const scriptCount = + await config.dependencyRegistry.getDependencyScriptCount( + dependencyTypeBytes + ); + expect(scriptCount).to.eq(1); + + const storedScript = + await config.dependencyRegistry.getDependencyScriptAtIndex( + dependencyTypeBytes, + 0 + ); + expect(storedScript).to.eq(script); + }); + + it("supports SSTORE2 reads/writes", async function () { + // get config from beforeEach + const config = this.config; + const script = "on-chain script"; + const contentAddress = writeContentWithSSTORE2( + script, + config.sstore2Mock, + config.accounts.deployer + ); + + await expect( + config.dependencyRegistry.addDependencyScriptPointer( + dependencyTypeBytes, + contentAddress + ) + ) + .to.emit(config.dependencyRegistry, "DependencyScriptUpdated") + .withArgs(dependencyTypeBytes); + + const dependencyDetails = + await config.dependencyRegistry.getDependencyDetails( + dependencyTypeBytes + ); + + expect(dependencyDetails.scriptCount).to.eq(1); + + const scriptCount = + await config.dependencyRegistry.getDependencyScriptCount( + dependencyTypeBytes + ); + expect(scriptCount).to.eq(1); + + const storedScript = + await config.dependencyRegistry.getDependencyScriptAtIndex( + dependencyTypeBytes, + 0 + ); + expect(storedScript).to.eq(script); + }); + }); + + describe("updateDependencyScriptPointer", function () { + it("does not allow non-admins to update a script pointer", async function () { + // get config from beforeEach + const config = this.config; + + const contentAddress = writeContentWithBytecodeStorageV1( + "on-chain script", + config.bytecodeV1TextCR_DMock, + config.accounts.deployer + ); + + await expectRevert( + config.dependencyRegistry + .connect(config.accounts.user) + .updateDependencyScriptPointer( + dependencyTypeBytes, + 0, + contentAddress + ), + ONLY_ADMIN_ACL_ERROR + ); + }); + + it("does not allow update a script for a dependency that does not exist", async function () { + // get config from beforeEach + const config = this.config; + + const contentAddress = writeContentWithBytecodeStorageV1( + "on-chain script", + config.bytecodeV1TextCR_DMock, + config.accounts.deployer + ); + + await expectRevert( + config.dependencyRegistry + .connect(config.accounts.deployer) + .updateDependencyScriptPointer( + ethers.utils.formatBytes32String("nonExistentDependencyType"), + 0, + contentAddress + ), + ONLY_EXISTING_DEPENDENCY_TYPE_ERROR + ); + }); + + it("does not allow updating a script that is out of range", async function () { + // get config from beforeEach + const config = this.config; + + const contentAddress = writeContentWithBytecodeStorageV1( + "on-chain script", + config.bytecodeV1TextCR_DMock, + config.accounts.deployer + ); + + await expectRevert( + config.dependencyRegistry + .connect(config.accounts.deployer) + .updateDependencyScriptPointer( + dependencyTypeBytes, + 0, + contentAddress + ), + "scriptId out of range" + ); + }); + + it("does not allow specifying zero address as script pointer", async function () { + // get config from beforeEach + const config = this.config; + await expectRevert( + config.dependencyRegistry + .connect(config.accounts.deployer) + .updateDependencyScriptPointer( + dependencyTypeBytes, + 0, + constants.ZERO_ADDRESS + ), + ONLY_NON_ZERO_ADDRESS_ERROR + ); + }); + + it("supports BytecodeStorageV1 reads/writes", async function () { + // get config from beforeEach + const config = this.config; + const script = "on-chain script"; + const contentAddress = writeContentWithBytecodeStorageV1( + script, + config.bytecodeV1TextCR_DMock, + config.accounts.deployer + ); + + // add modified script first, before then updating it + const garbledScript = script.split("").reverse().join(""); + await config.dependencyRegistry + .connect(config.accounts.deployer) + .addDependencyScript(dependencyTypeBytes, garbledScript); + + await expect( + config.dependencyRegistry.updateDependencyScriptPointer( + dependencyTypeBytes, + 0, + contentAddress + ) + ) + .to.emit(config.dependencyRegistry, "DependencyScriptUpdated") + .withArgs(dependencyTypeBytes); + + const dependencyDetails = + await config.dependencyRegistry.getDependencyDetails( + dependencyTypeBytes + ); + + expect(dependencyDetails.scriptCount).to.eq(1); + + const scriptCount = + await config.dependencyRegistry.getDependencyScriptCount( + dependencyTypeBytes + ); + expect(scriptCount).to.eq(1); + + const storedScript = + await config.dependencyRegistry.getDependencyScriptAtIndex( + dependencyTypeBytes, + 0 + ); + expect(storedScript).to.eq(script); + expect(garbledScript).to.not.eq(script); + }); + + it("supports SSTORE2 reads/writes", async function () { + // get config from beforeEach + const config = this.config; + const script = "on-chain script"; + const contentAddress = writeContentWithSSTORE2( + script, + config.sstore2Mock, + config.accounts.deployer + ); + + // add modified script first, before then updating it + const garbledScript = script.split("").reverse().join(""); + await config.dependencyRegistry + .connect(config.accounts.deployer) + .addDependencyScript(dependencyTypeBytes, garbledScript); + + await expect( + config.dependencyRegistry.updateDependencyScriptPointer( + dependencyTypeBytes, + 0, + contentAddress + ) + ) + .to.emit(config.dependencyRegistry, "DependencyScriptUpdated") + .withArgs(dependencyTypeBytes); + + const dependencyDetails = + await config.dependencyRegistry.getDependencyDetails( + dependencyTypeBytes + ); + + expect(dependencyDetails.scriptCount).to.eq(1); + + const scriptCount = + await config.dependencyRegistry.getDependencyScriptCount( + dependencyTypeBytes + ); + expect(scriptCount).to.eq(1); + + const storedScript = + await config.dependencyRegistry.getDependencyScriptAtIndex( + dependencyTypeBytes, + 0 + ); + expect(storedScript).to.eq(script); + expect(garbledScript).to.not.eq(script); + }); + }); + describe("views", function () { it("getDependencyDetails", async function () { // get config from beforeEach @@ -883,6 +1198,63 @@ describe(`DependencyRegistryV0`, async function () { ethers.utils.hexlify(ethers.utils.toUtf8Bytes(script)).substring(2) ); }); + + it("getDependencyScriptBytecodeStorageVersionAtIndex (BytecodeStorageV1)", async function () { + // get config from beforeEach + const config = this.config; + const script = "on-chain script"; + const contentAddress = writeContentWithBytecodeStorageV1( + script, + config.bytecodeV1TextCR_DMock, + config.accounts.deployer + ); + + await config.dependencyRegistry.addDependencyScriptPointer( + dependencyTypeBytes, + contentAddress + ); + + const bytecodeStorageVersionBytes = + await config.dependencyRegistry.getDependencyScriptBytecodeStorageVersionAtIndex( + dependencyTypeBytes, + 0 + ); + let bytecodeStorageVersionUTF8 = ethers.utils.toUtf8String( + bytecodeStorageVersionBytes + ); + expect(bytecodeStorageVersionUTF8).to.eq( + "BytecodeStorage_V1.0.0_________ " + ); + }); + + it("getDependencyScriptBytecodeStorageVersionAtIndex (SSTORE2)", async function () { + // get config from beforeEach + const config = this.config; + const script = "on-chain script"; + const contentAddress = writeContentWithSSTORE2( + script, + config.sstore2Mock, + config.accounts.deployer + ); + + await config.dependencyRegistry.addDependencyScriptPointer( + dependencyTypeBytes, + contentAddress + ); + + const bytecodeStorageVersionBytes = + await config.dependencyRegistry.getDependencyScriptBytecodeStorageVersionAtIndex( + dependencyTypeBytes, + 0 + ); + + let bytecodeStorageVersionUTF8 = ethers.utils.toUtf8String( + bytecodeStorageVersionBytes + ); + expect(bytecodeStorageVersionUTF8).to.eq( + "UNKNOWN_VERSION_STRING_________ " + ); + }); }); }); describe("dependency additional cdns", function () { From 064dda759cec027e0fb3f9c3843cca3d8403fd8a Mon Sep 17 00:00:00 2001 From: purp Date: Tue, 25 Jul 2023 13:52:21 -0600 Subject: [PATCH 08/12] better documentation --- .../contracts/DependencyRegistryV0.sol | 54 +++++++++++-------- 1 file changed, 31 insertions(+), 23 deletions(-) diff --git a/packages/contracts/contracts/DependencyRegistryV0.sol b/packages/contracts/contracts/DependencyRegistryV0.sol index 4ddf714be..c4f6d6a82 100644 --- a/packages/contracts/contracts/DependencyRegistryV0.sol +++ b/packages/contracts/contracts/DependencyRegistryV0.sol @@ -149,7 +149,7 @@ contract DependencyRegistryV0 is /** * @notice Removes a dependency. - * @param _dependencyType Name of dependency type (i.e. "type@version") + * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. */ function removeDependency(bytes32 _dependencyType) external { _onlyAdminACL(this.removeDependency.selector); @@ -171,7 +171,7 @@ contract DependencyRegistryV0 is /** * @notice Adds a script to dependency `_dependencyType`, by way of * providing a string to write to bytecode storage. - * @param _dependencyType dependency to be updated. + * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. * @param _script Script to be added. Required to be a non-empty string, * but no further validation is performed. */ @@ -194,7 +194,7 @@ contract DependencyRegistryV0 is /** * @notice Updates script for dependencyType `_dependencyType` at script ID `_scriptId`, * by way of providing a string to write to bytecode storage. - * @param _dependencyType dependency to be updated. + * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. * @param _scriptId Script ID to be updated. * @param _script The updated script value. Required to be a non-empty * string, but no further validation is performed. @@ -220,7 +220,7 @@ contract DependencyRegistryV0 is /** * @notice Adds a script to dependency `_dependencyType`, by way of * providing an already written chunk of bytecode storage. - * @param _dependencyType dependency to be updated. + * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. * @param _scriptPointer Address of script to be added. Required to be a non-zero address, * but no further validation is performed. */ @@ -244,7 +244,7 @@ contract DependencyRegistryV0 is /** * @notice Updates script for dependencyType `_dependencyType` at script ID `_scriptId`, * by way of providing an already written chunk of bytecode storage. - * @param _dependencyType dependency to be updated. + * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. * @param _scriptId Script ID to be updated. * @param _scriptPointer The updated script pointer (address of bytecode storage). * Required to be a non-zero address, but no further validation is performed. @@ -266,7 +266,7 @@ contract DependencyRegistryV0 is /** * @notice Removes last script from dependency `_dependencyType`. - * @param _dependencyType dependency to be updated. + * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. */ function removeDependencyLastScript(bytes32 _dependencyType) external { _onlyAdminACL(this.removeDependencyLastScript.selector); @@ -284,7 +284,7 @@ contract DependencyRegistryV0 is /** * @notice Updates preferred CDN for dependency `_dependencyType`. - * @param _dependencyType dependency to be updated. + * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. * @param _preferredCDN URL for preferred CDN. */ function updateDependencyPreferredCDN( @@ -300,7 +300,7 @@ contract DependencyRegistryV0 is /** * @notice Updates preferred repository for dependency `_dependencyType`. - * @param _dependencyType dependency to be updated. + * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. * @param _preferredRepository URL for preferred repository. */ function updateDependencyPreferredRepository( @@ -320,7 +320,7 @@ contract DependencyRegistryV0 is /** * @notice Updates project website for dependency `_dependencyType`. - * @param _dependencyType dependency to be updated. + * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. * @param _referenceWebsite URL for project website. */ function updateDependencyReferenceWebsite( @@ -339,7 +339,7 @@ contract DependencyRegistryV0 is /** * @notice Adds a new CDN url to `_dependencyType`. - * @param _dependencyType dependency to be updated. + * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. * @param _additionalCDN CDN URL to be added. Required to be a non-empty string, * but no further validation is performed. */ @@ -367,7 +367,7 @@ contract DependencyRegistryV0 is * @notice Removes additional CDN for dependency `_dependencyId` at index `_index`. * Removal is done by swapping the element to be removed with the last element in the array, then deleting this last element. * Assets with indices higher than `_index` can have their indices adjusted as a result of this operation. - * @param _dependencyType dependency to be updated. + * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. * @param _index Additional CDN index */ function removeDependencyAdditionalCDNAtIndex( @@ -395,7 +395,7 @@ contract DependencyRegistryV0 is /** * @notice Updates additional CDN for dependency `_dependencyType` at `_index`. - * @param _dependencyType dependency to be updated. + * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. * @param _index Additional CDN index. * @param _additionalCDN New CDN URL. */ @@ -422,7 +422,7 @@ contract DependencyRegistryV0 is /** * @notice Adds a new repository URL to dependency `_dependencyType`. - * @param _dependencyType dependency to be updated. + * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. * @param _additionalRepository Repository URL to be added. Required to be a non-empty string, * but no further validation is performed. */ @@ -455,7 +455,7 @@ contract DependencyRegistryV0 is * @notice Removes additional repository for depenency `_dependencyId` at index `_index`. * Removal is done by swapping the element to be removed with the last element in the array, then deleting this last element. * Assets with indices higher than `_index` can have their indices adjusted as a result of this operation. - * @param _dependencyType dependency to be updated. + * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. * @param _index Additional repository index. */ function removeDependencyAdditionalRepositoryAtIndex( @@ -486,7 +486,7 @@ contract DependencyRegistryV0 is /** * @notice Updates additional repository for dependency `_dependencyType` at `_index`. - * @param _dependencyType dependency to be updated. + * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. * @param _index Additional repository index. * @param _additionalRepository New Repository URL. */ @@ -549,7 +549,7 @@ contract DependencyRegistryV0 is * type (`_dependencyType`). * @param _contractAddress Core contract address. * @param _projectId Project to override script type and version for. - * @param _dependencyType Dependency type to return for project. + * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. */ function addProjectDependencyTypeOverride( address _contractAddress, @@ -630,8 +630,8 @@ contract DependencyRegistryV0 is } /** - * @notice Returns details for depedency type `_dependencyType`. - * @param _dependencyType Dependency type to be queried. + * @notice Returns details for a given depedency type `_dependencyType`. + * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. * @return typeAndVersion String representation of `_dependencyType`. * (e.g. "p5js(atSymbol)1.0.0") * @return preferredCDN Preferred CDN URL for dependency @@ -731,7 +731,7 @@ contract DependencyRegistryV0 is /** * @notice Returns the additional CDN URL at index `_index` for dependency `_dependencyType`. - * @param _dependencyType Dependency type to be queried. + * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. * @param _index Index of the additional CDN URL to be returned. */ function getDependencyAdditionalCDNAtIndex( @@ -743,7 +743,7 @@ contract DependencyRegistryV0 is /** * @notice Returns the additional repository URL at index `_index` for dependency `_dependencyType`. - * @param _dependencyType Dependency type to be queried. + * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. * @param _index Index of the additional repository URL to be returned. */ function getDependencyAdditionalRepositoryAtIndex( @@ -756,7 +756,7 @@ contract DependencyRegistryV0 is /** * @notice Returns the count of scripts for dependency `_dependencyType`. - * @param _dependencyType Dependency type to be queried. + * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. */ function getDependencyScriptCount( bytes32 _dependencyType @@ -779,6 +779,14 @@ contract DependencyRegistryV0 is /** * @notice Returns the storage library version for * dependency `_dependencyTypes` at script index `_index`. + * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. + * @param _index Index of script to be queried in the array of dependency script chunks. + * @return The storage library version for the script at the given index, if it can be determined. + * @dev Note that we only expect this to be determinable if the script was written using a version + * of the Art Blocks `BytecodeStorage` library, and in other cases the fallback will be the + * unknown version string, as defined by the `BytecodeStorage` UNKNOWN_VERSION_STRING – this + * is inclusive of the in the case of `SSTORE2` written data blobs, which are an unknown version + * that can be fallback-read optimistically. */ function getDependencyScriptBytecodeStorageVersionAtIndex( bytes32 _dependencyType, @@ -794,7 +802,7 @@ contract DependencyRegistryV0 is /** * @notice Returns script for dependency `_dependencyType` at script index `_index`. - * @param _dependencyType dependency to be queried. + * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. * @param _index Index of script to be queried. * @dev This method attempts to introspectively determine which library version of * `BytecodeStorage` was used to write the stored script string that is being @@ -836,7 +844,7 @@ contract DependencyRegistryV0 is * an override set, this will revert. * @param _contractAddress Core contract address. * @param _projectId Project to return dependency type for. - * @return dependencyType Dependency type used by project. + * @return dependencyType Identifier for the dependency (i.e. "type@version") used by project. */ function getDependencyTypeForProject( address _contractAddress, From 1a31ce4f5372b0d7cb9bafb7c1a927156753e6df Mon Sep 17 00:00:00 2001 From: purp Date: Tue, 25 Jul 2023 13:53:56 -0600 Subject: [PATCH 09/12] return docs --- packages/contracts/contracts/DependencyRegistryV0.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/contracts/contracts/DependencyRegistryV0.sol b/packages/contracts/contracts/DependencyRegistryV0.sol index c4f6d6a82..ab6f020d0 100644 --- a/packages/contracts/contracts/DependencyRegistryV0.sol +++ b/packages/contracts/contracts/DependencyRegistryV0.sol @@ -804,6 +804,7 @@ contract DependencyRegistryV0 is * @notice Returns script for dependency `_dependencyType` at script index `_index`. * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. * @param _index Index of script to be queried. + * @return A string containing the script content at the given script chunk index for a given dependency. * @dev This method attempts to introspectively determine which library version of * `BytecodeStorage` was used to write the stored script string that is being * read back, in order to use the proper read approach. If the version is From fde83f864c5d88168582f3b21ae4150ef7983954 Mon Sep 17 00:00:00 2001 From: purp Date: Tue, 25 Jul 2023 16:11:04 -0600 Subject: [PATCH 10/12] Update pass for consistency in nomenclature and test adjustment in kind --- .../contracts/DependencyRegistryV0.sol | 139 ++++++++-------- .../v0.8.x/IDependencyRegistryV0.sol | 4 +- .../DependencyRegistryV0.test.ts | 153 +++++++++--------- 3 files changed, 150 insertions(+), 146 deletions(-) diff --git a/packages/contracts/contracts/DependencyRegistryV0.sol b/packages/contracts/contracts/DependencyRegistryV0.sol index ab6f020d0..47172ef5b 100644 --- a/packages/contracts/contracts/DependencyRegistryV0.sol +++ b/packages/contracts/contracts/DependencyRegistryV0.sol @@ -47,8 +47,10 @@ contract DependencyRegistryV0 is struct Dependency { string preferredCDN; + // mapping from additional CDN index to CDN URLr mapping(uint256 => string) additionalCDNs; string preferredRepository; + // mapping from additional repository index to repository URL mapping(uint256 => string) additionalRepositories; string referenceWebsite; // mapping from script index to address storing script in bytecode @@ -97,6 +99,13 @@ contract DependencyRegistryV0 is ); } + function _onlyInRangeIndex( + uint256 _index, + uint256 _maxIndex + ) internal pure { + require(_index < _maxIndex, "Index out of range"); + } + /** * @notice Initializes contract. * @param _adminACLContract Address of admin access control contract, to be @@ -173,7 +182,7 @@ contract DependencyRegistryV0 is * providing a string to write to bytecode storage. * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. * @param _script Script to be added. Required to be a non-empty string, - * but no further validation is performed. + * but no further validation is performed. */ function addDependencyScript( bytes32 _dependencyType, @@ -192,27 +201,26 @@ contract DependencyRegistryV0 is } /** - * @notice Updates script for dependencyType `_dependencyType` at script ID `_scriptId`, + * @notice Updates script for dependencyType `_dependencyType` at script `_index`, * by way of providing a string to write to bytecode storage. * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. - * @param _scriptId Script ID to be updated. + * @param _index The index of a given script, relative to the overall `scriptCount`. * @param _script The updated script value. Required to be a non-empty * string, but no further validation is performed. */ function updateDependencyScript( bytes32 _dependencyType, - uint256 _scriptId, + uint256 _index, string memory _script ) external { _onlyAdminACL(this.updateDependencyScript.selector); _onlyNonEmptyString(_script); _onlyExistingDependencyType(_dependencyType); Dependency storage dependency = dependencyDetails[_dependencyType]; - require(_scriptId < dependency.scriptCount, "scriptId out of range"); + _onlyInRangeIndex(_index, dependency.scriptCount); // store script in contract bytecode, replacing reference address from // the contract that no longer exists with the newly created one - dependency.scriptBytecodeAddresses[_scriptId] = _script - .writeToBytecode(); + dependency.scriptBytecodeAddresses[_index] = _script.writeToBytecode(); emit DependencyScriptUpdated(_dependencyType); } @@ -242,24 +250,24 @@ contract DependencyRegistryV0 is } /** - * @notice Updates script for dependencyType `_dependencyType` at script ID `_scriptId`, + * @notice Updates script for dependencyType `_dependencyType` at script `_index`, * by way of providing an already written chunk of bytecode storage. * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. - * @param _scriptId Script ID to be updated. + * @param _index The index of a given script, relative to the overall `scriptCount`. * @param _scriptPointer The updated script pointer (address of bytecode storage). - * Required to be a non-zero address, but no further validation is performed. + * Required to be a non-zero address, but no further validation is performed. */ function updateDependencyScriptPointer( bytes32 _dependencyType, - uint256 _scriptId, + uint256 _index, address _scriptPointer ) external { _onlyAdminACL(this.updateDependencyScriptPointer.selector); _onlyNonZeroAddress(_scriptPointer); _onlyExistingDependencyType(_dependencyType); Dependency storage dependency = dependencyDetails[_dependencyType]; - require(_scriptId < dependency.scriptCount, "scriptId out of range"); - dependency.scriptBytecodeAddresses[_scriptId] = _scriptPointer; + _onlyInRangeIndex(_index, dependency.scriptCount); + dependency.scriptBytecodeAddresses[_index] = _scriptPointer; emit DependencyScriptUpdated(_dependencyType); } @@ -341,7 +349,7 @@ contract DependencyRegistryV0 is * @notice Adds a new CDN url to `_dependencyType`. * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. * @param _additionalCDN CDN URL to be added. Required to be a non-empty string, - * but no further validation is performed. + * but no further validation is performed. */ function addDependencyAdditionalCDN( bytes32 _dependencyType, @@ -368,18 +376,18 @@ contract DependencyRegistryV0 is * Removal is done by swapping the element to be removed with the last element in the array, then deleting this last element. * Assets with indices higher than `_index` can have their indices adjusted as a result of this operation. * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. - * @param _index Additional CDN index + * @param _index The index of an additional CDN, relative to the overall `additionalCDNCount`. */ - function removeDependencyAdditionalCDNAtIndex( + function removeDependencyAdditionalCDN( bytes32 _dependencyType, uint256 _index ) external { - _onlyAdminACL(this.removeDependencyAdditionalCDNAtIndex.selector); + _onlyAdminACL(this.removeDependencyAdditionalCDN.selector); _onlyExistingDependencyType(_dependencyType); Dependency storage dependency = dependencyDetails[_dependencyType]; uint256 additionalCDNCount = dependency.additionalCDNCount; - require(_index < additionalCDNCount, "Asset index out of range"); + _onlyInRangeIndex(_index, additionalCDNCount); uint256 lastElementIndex = additionalCDNCount - 1; @@ -396,20 +404,19 @@ contract DependencyRegistryV0 is /** * @notice Updates additional CDN for dependency `_dependencyType` at `_index`. * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. - * @param _index Additional CDN index. + * @param _index The index of an additional CDN, relative to the overall `additionalCDNCount`. * @param _additionalCDN New CDN URL. */ - function updateDependencyAdditionalCDNAtIndex( + function updateDependencyAdditionalCDN( bytes32 _dependencyType, uint256 _index, string memory _additionalCDN ) external { - _onlyAdminACL(this.updateDependencyAdditionalCDNAtIndex.selector); + _onlyAdminACL(this.updateDependencyAdditionalCDN.selector); _onlyNonEmptyString(_additionalCDN); _onlyExistingDependencyType(_dependencyType); Dependency storage dependency = dependencyDetails[_dependencyType]; - uint24 additionalCDNCount = dependency.additionalCDNCount; - require(_index < additionalCDNCount, "Asset index out of range"); + _onlyInRangeIndex(_index, dependency.additionalCDNCount); dependency.additionalCDNs[_index] = _additionalCDN; @@ -456,21 +463,18 @@ contract DependencyRegistryV0 is * Removal is done by swapping the element to be removed with the last element in the array, then deleting this last element. * Assets with indices higher than `_index` can have their indices adjusted as a result of this operation. * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. - * @param _index Additional repository index. + * @param _index The index of an additional repository, relative to the overall `additionalRepositoryCount`. */ - function removeDependencyAdditionalRepositoryAtIndex( + function removeDependencyAdditionalRepository( bytes32 _dependencyType, uint256 _index ) external { - _onlyAdminACL( - this.removeDependencyAdditionalRepositoryAtIndex.selector - ); + _onlyAdminACL(this.removeDependencyAdditionalRepository.selector); _onlyExistingDependencyType(_dependencyType); Dependency storage dependency = dependencyDetails[_dependencyType]; - uint256 additionalRepositoryCount = uint256( - dependency.additionalRepositoryCount - ); - require(_index < additionalRepositoryCount, "Asset index out of range"); + uint256 additionalRepositoryCount = dependency + .additionalRepositoryCount; + _onlyInRangeIndex(_index, additionalRepositoryCount); uint256 lastElementIndex = additionalRepositoryCount - 1; @@ -487,22 +491,19 @@ contract DependencyRegistryV0 is /** * @notice Updates additional repository for dependency `_dependencyType` at `_index`. * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. - * @param _index Additional repository index. + * @param _index The index of an additional repository, relative to the overall `additionalRepositoryCount`. * @param _additionalRepository New Repository URL. */ - function updateDependencyAdditionalRepositoryAtIndex( + function updateDependencyAdditionalRepository( bytes32 _dependencyType, uint256 _index, string memory _additionalRepository ) external { - _onlyAdminACL( - this.updateDependencyAdditionalRepositoryAtIndex.selector - ); + _onlyAdminACL(this.updateDependencyAdditionalRepository.selector); _onlyNonEmptyString(_additionalRepository); _onlyExistingDependencyType(_dependencyType); Dependency storage dependency = dependencyDetails[_dependencyType]; - uint24 additionalRepositoryCount = dependency.additionalRepositoryCount; - require(_index < additionalRepositoryCount, "Asset index out of range"); + _onlyInRangeIndex(_index, dependency.additionalRepositoryCount); dependency.additionalRepositories[_index] = _additionalRepository; @@ -620,12 +621,12 @@ contract DependencyRegistryV0 is /** * @notice Returns registered depenedency type at index `_index`. - * @return Registered dependency at `_index`. + * @return Registered dependency at `_index`, relative to the overall length of the dependency type set. */ - function getDependencyTypeAtIndex( + function getDependencyType( uint256 _index ) external view returns (string memory) { - require(_dependencyTypes.length() > _index, "Index out of range"); + _onlyInRangeIndex(_index, _dependencyTypes.length()); return _dependencyTypes.at(_index).toString(); } @@ -633,7 +634,7 @@ contract DependencyRegistryV0 is * @notice Returns details for a given depedency type `_dependencyType`. * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. * @return typeAndVersion String representation of `_dependencyType`. - * (e.g. "p5js(atSymbol)1.0.0") + * (e.g. "p5js(atSymbol)1.0.0") * @return preferredCDN Preferred CDN URL for dependency * @return additionalCDNCount Count of additional CDN URLs for dependency * @return preferredRepository Preferred repository URL for dependency @@ -682,16 +683,14 @@ contract DependencyRegistryV0 is /** * @notice Returns the address of the supported core contract at index `_index`. - * @param _index Index of the core contract to be returned. + * @param _index Index of the core contract to be returned, relative to the overall + * list of supported core contracts. * @return address of the core contract. */ - function getSupportedCoreContractAtIndex( + function getSupportedCoreContract( uint256 _index ) external view returns (address) { - require( - _supportedCoreContracts.length() > _index, - "Index out of bounds" - ); + _onlyInRangeIndex(_index, _supportedCoreContracts.length()); return _supportedCoreContracts.at(_index); } @@ -732,26 +731,29 @@ contract DependencyRegistryV0 is /** * @notice Returns the additional CDN URL at index `_index` for dependency `_dependencyType`. * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. - * @param _index Index of the additional CDN URL to be returned. + * @param _index The index of an additional CDN, relative to the overall `additionalCDNCount`. */ - function getDependencyAdditionalCDNAtIndex( + function getDependencyAdditionalCDN( bytes32 _dependencyType, uint256 _index ) external view returns (string memory) { - return dependencyDetails[_dependencyType].additionalCDNs[_index]; + Dependency storage dependency = dependencyDetails[_dependencyType]; + _onlyInRangeIndex(_index, dependency.additionalCDNCount); + return dependency.additionalCDNs[_index]; } /** * @notice Returns the additional repository URL at index `_index` for dependency `_dependencyType`. * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. - * @param _index Index of the additional repository URL to be returned. + * @param _index The index of an additional repository, relative to the overall `additionalRepositoryCount`. */ - function getDependencyAdditionalRepositoryAtIndex( + function getDependencyAdditionalRepository( bytes32 _dependencyType, uint256 _index ) external view returns (string memory) { - return - dependencyDetails[_dependencyType].additionalRepositories[_index]; + Dependency storage dependency = dependencyDetails[_dependencyType]; + _onlyInRangeIndex(_index, dependency.additionalRepositoryCount); + return dependency.additionalRepositories[_index]; } /** @@ -767,20 +769,24 @@ contract DependencyRegistryV0 is /** * @notice Returns address with bytecode containing script for * dependency `_dependencyTypes` at script index `_index`. + * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. + * @param _index The index of a given script, relative to the overall `scriptCount`. + * @return The address of the bytecode storage for the script at the given index, if it can be determined. */ - function getDependencyScriptBytecodeAddressAtIndex( + function getDependencyScriptBytecodeAddress( bytes32 _dependencyType, uint256 _index ) external view returns (address) { - return - dependencyDetails[_dependencyType].scriptBytecodeAddresses[_index]; + Dependency storage dependency = dependencyDetails[_dependencyType]; + _onlyInRangeIndex(_index, dependency.scriptCount); + return dependency.scriptBytecodeAddresses[_index]; } /** * @notice Returns the storage library version for * dependency `_dependencyTypes` at script index `_index`. * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. - * @param _index Index of script to be queried in the array of dependency script chunks. + * @param _index The index of a given script, relative to the overall `scriptCount`. * @return The storage library version for the script at the given index, if it can be determined. * @dev Note that we only expect this to be determinable if the script was written using a version * of the Art Blocks `BytecodeStorage` library, and in other cases the fallback will be the @@ -788,10 +794,12 @@ contract DependencyRegistryV0 is * is inclusive of the in the case of `SSTORE2` written data blobs, which are an unknown version * that can be fallback-read optimistically. */ - function getDependencyScriptBytecodeStorageVersionAtIndex( + function getDependencyScriptBytecodeStorageVersion( bytes32 _dependencyType, uint256 _index ) external view returns (bytes32) { + Dependency storage dependency = dependencyDetails[_dependencyType]; + _onlyInRangeIndex(_index, dependency.scriptCount); return BytecodeStorageReader.getLibraryVersionForBytecode( dependencyDetails[_dependencyType].scriptBytecodeAddresses[ @@ -803,7 +811,7 @@ contract DependencyRegistryV0 is /** * @notice Returns script for dependency `_dependencyType` at script index `_index`. * @param _dependencyType Name of dependency type (i.e. "type@version") used to identify dependency. - * @param _index Index of script to be queried. + * @param _index The index of a given script, relative to the overall `scriptCount`. * @return A string containing the script content at the given script chunk index for a given dependency. * @dev This method attempts to introspectively determine which library version of * `BytecodeStorage` was used to write the stored script string that is being @@ -811,15 +819,12 @@ contract DependencyRegistryV0 is * non-determinate, a fall-back to reading using the assumption that the bytes * were written with `SSTORE2` is used. */ - function getDependencyScriptAtIndex( + function getDependencyScript( bytes32 _dependencyType, uint256 _index ) external view returns (string memory) { Dependency storage dependency = dependencyDetails[_dependencyType]; - // If trying to access an out-of-index script, return the empty string. - if (_index >= dependency.scriptCount) { - return ""; - } + _onlyInRangeIndex(_index, dependency.scriptCount); address scriptAddress = dependency.scriptBytecodeAddresses[_index]; bytes32 storageVersion = BytecodeStorageReader diff --git a/packages/contracts/contracts/interfaces/v0.8.x/IDependencyRegistryV0.sol b/packages/contracts/contracts/interfaces/v0.8.x/IDependencyRegistryV0.sol index 0e2476816..678f7417d 100644 --- a/packages/contracts/contracts/interfaces/v0.8.x/IDependencyRegistryV0.sol +++ b/packages/contracts/contracts/interfaces/v0.8.x/IDependencyRegistryV0.sol @@ -78,7 +78,7 @@ interface IDependencyRegistryV0 { * @notice Returns address with bytecode containing script for * dependency type `_dependencyTypes` at script index `_index`. */ - function getDependencyScriptBytecodeAddressAtIndex( + function getDependencyScriptBytecodeAddress( bytes32 _dependencyType, uint256 _index ) external view returns (address); @@ -88,7 +88,7 @@ interface IDependencyRegistryV0 { * @param _dependencyType Dependency type to be queried. * @param _index Index of script to be queried. */ - function getDependencyScriptAtIndex( + function getDependencyScript( bytes32 _dependencyType, uint256 _index ) external view returns (string memory); diff --git a/packages/contracts/test/dependency-registry/DependencyRegistryV0.test.ts b/packages/contracts/test/dependency-registry/DependencyRegistryV0.test.ts index 75f372429..8ac984cd0 100644 --- a/packages/contracts/test/dependency-registry/DependencyRegistryV0.test.ts +++ b/packages/contracts/test/dependency-registry/DependencyRegistryV0.test.ts @@ -29,6 +29,7 @@ const ONLY_ADMIN_ACL_ERROR = "Only Admin ACL allowed"; const ONLY_EXISTING_DEPENDENCY_TYPE_ERROR = "Dependency type does not exist"; const ONLY_NON_EMPTY_STRING_ERROR = "Must input non-empty string"; const ONLY_NON_ZERO_ADDRESS_ERROR = "Must input non-zero address"; +const INDEX_OUT_OF_RANGE_ERROR = "Index out of range"; // test the following V3 core contract derivatives: const coreContractsToTest = [ @@ -235,8 +236,9 @@ describe(`DependencyRegistryV0`, async function () { await config.dependencyRegistry.getDependencyTypeCount(); expect(registeredDependencyCount).to.eq(1); - const storedDepType = - await config.dependencyRegistry.getDependencyTypeAtIndex(0); + const storedDepType = await config.dependencyRegistry.getDependencyType( + 0 + ); expect(storedDepType).to.eq(dependencyType); const dependencyTypes = @@ -319,7 +321,7 @@ describe(`DependencyRegistryV0`, async function () { ); // Remove additional CDNs - await config.dependencyRegistry.removeDependencyAdditionalCDNAtIndex( + await config.dependencyRegistry.removeDependencyAdditionalCDN( dependencyTypeBytes, 0 ); @@ -338,7 +340,7 @@ describe(`DependencyRegistryV0`, async function () { ); // Remove additional repositories - await config.dependencyRegistry.removeDependencyAdditionalRepositoryAtIndex( + await config.dependencyRegistry.removeDependencyAdditionalRepository( dependencyTypeBytes, 0 ); @@ -647,7 +649,7 @@ describe(`DependencyRegistryV0`, async function () { expect(scriptCount).to.eq(1); const storedScript = - await config.dependencyRegistry.getDependencyScriptAtIndex( + await config.dependencyRegistry.getDependencyScript( dependencyTypeBytes, 0 ); @@ -721,12 +723,10 @@ describe(`DependencyRegistryV0`, async function () { ); expect(scriptCount).to.eq(0); - const storedScript = - await config.dependencyRegistry.getDependencyScriptAtIndex( - dependencyTypeBytes, - 0 - ); - expect(storedScript).to.eq(""); + await expectRevert( + config.dependencyRegistry.getDependencyScript(dependencyTypeBytes, 0), + INDEX_OUT_OF_RANGE_ERROR + ); }); }); @@ -764,7 +764,7 @@ describe(`DependencyRegistryV0`, async function () { config.dependencyRegistry .connect(config.accounts.deployer) .updateDependencyScript(dependencyTypeBytes, 0, "on-chain script"), - "scriptId out of range" + INDEX_OUT_OF_RANGE_ERROR ); }); @@ -818,7 +818,7 @@ describe(`DependencyRegistryV0`, async function () { expect(scriptCount).to.eq(1); const storedScript = - await config.dependencyRegistry.getDependencyScriptAtIndex( + await config.dependencyRegistry.getDependencyScript( dependencyTypeBytes, 0 ); @@ -913,7 +913,7 @@ describe(`DependencyRegistryV0`, async function () { expect(scriptCount).to.eq(1); const storedScript = - await config.dependencyRegistry.getDependencyScriptAtIndex( + await config.dependencyRegistry.getDependencyScript( dependencyTypeBytes, 0 ); @@ -953,7 +953,7 @@ describe(`DependencyRegistryV0`, async function () { expect(scriptCount).to.eq(1); const storedScript = - await config.dependencyRegistry.getDependencyScriptAtIndex( + await config.dependencyRegistry.getDependencyScript( dependencyTypeBytes, 0 ); @@ -1024,7 +1024,7 @@ describe(`DependencyRegistryV0`, async function () { 0, contentAddress ), - "scriptId out of range" + INDEX_OUT_OF_RANGE_ERROR ); }); @@ -1083,7 +1083,7 @@ describe(`DependencyRegistryV0`, async function () { expect(scriptCount).to.eq(1); const storedScript = - await config.dependencyRegistry.getDependencyScriptAtIndex( + await config.dependencyRegistry.getDependencyScript( dependencyTypeBytes, 0 ); @@ -1131,7 +1131,7 @@ describe(`DependencyRegistryV0`, async function () { expect(scriptCount).to.eq(1); const storedScript = - await config.dependencyRegistry.getDependencyScriptAtIndex( + await config.dependencyRegistry.getDependencyScript( dependencyTypeBytes, 0 ); @@ -1159,7 +1159,7 @@ describe(`DependencyRegistryV0`, async function () { expect(dependencyDetails.availableOnChain).to.eq(true); }); - it("getDependencyScriptAtIndex", async function () { + it("getDependencyScript", async function () { // get config from beforeEach const config = this.config; const script = "on-chain script"; @@ -1169,14 +1169,14 @@ describe(`DependencyRegistryV0`, async function () { .addDependencyScript(dependencyTypeBytes, script); const storedScript = - await config.dependencyRegistry.getDependencyScriptAtIndex( + await config.dependencyRegistry.getDependencyScript( dependencyTypeBytes, 0 ); expect(storedScript).to.eq(script); }); - it("getDependencyScriptBytecodeAddressAtIndex", async function () { + it("getDependencyScriptBytecodeAddress", async function () { // get config from beforeEach const config = this.config; const script = "on-chain script"; @@ -1186,7 +1186,7 @@ describe(`DependencyRegistryV0`, async function () { .addDependencyScript(dependencyTypeBytes, script); const storedScriptByteCodeAddress = - await config.dependencyRegistry.getDependencyScriptBytecodeAddressAtIndex( + await config.dependencyRegistry.getDependencyScriptBytecodeAddress( dependencyTypeBytes, 0 ); @@ -1199,7 +1199,7 @@ describe(`DependencyRegistryV0`, async function () { ); }); - it("getDependencyScriptBytecodeStorageVersionAtIndex (BytecodeStorageV1)", async function () { + it("getDependencyScriptBytecodeStorageVersion (BytecodeStorageV1)", async function () { // get config from beforeEach const config = this.config; const script = "on-chain script"; @@ -1215,7 +1215,7 @@ describe(`DependencyRegistryV0`, async function () { ); const bytecodeStorageVersionBytes = - await config.dependencyRegistry.getDependencyScriptBytecodeStorageVersionAtIndex( + await config.dependencyRegistry.getDependencyScriptBytecodeStorageVersion( dependencyTypeBytes, 0 ); @@ -1227,7 +1227,7 @@ describe(`DependencyRegistryV0`, async function () { ); }); - it("getDependencyScriptBytecodeStorageVersionAtIndex (SSTORE2)", async function () { + it("getDependencyScriptBytecodeStorageVersion (SSTORE2)", async function () { // get config from beforeEach const config = this.config; const script = "on-chain script"; @@ -1243,7 +1243,7 @@ describe(`DependencyRegistryV0`, async function () { ); const bytecodeStorageVersionBytes = - await config.dependencyRegistry.getDependencyScriptBytecodeStorageVersionAtIndex( + await config.dependencyRegistry.getDependencyScriptBytecodeStorageVersion( dependencyTypeBytes, 0 ); @@ -1328,21 +1328,21 @@ describe(`DependencyRegistryV0`, async function () { expect(dependencyDetails.additionalCDNCount).to.eq(1); const storedCDN = - await config.dependencyRegistry.getDependencyAdditionalCDNAtIndex( + await config.dependencyRegistry.getDependencyAdditionalCDN( dependencyTypeBytes, 0 ); expect(storedCDN).to.eq("https://cdn.com"); }); }); - describe("removeDependencyAdditionalCDNAtIndex", function () { + describe("removeDependencyAdditionalCDN", function () { it("does not allow non-admins to remove a cdn", async function () { // get config from beforeEach const config = this.config; await expectRevert( config.dependencyRegistry .connect(config.accounts.user) - .removeDependencyAdditionalCDNAtIndex(dependencyTypeBytes, 0), + .removeDependencyAdditionalCDN(dependencyTypeBytes, 0), ONLY_ADMIN_ACL_ERROR ); }); @@ -1353,7 +1353,7 @@ describe(`DependencyRegistryV0`, async function () { await expectRevert( config.dependencyRegistry .connect(config.accounts.deployer) - .removeDependencyAdditionalCDNAtIndex( + .removeDependencyAdditionalCDN( ethers.utils.formatBytes32String("nonExistentDependencyType"), 0 ), @@ -1367,8 +1367,8 @@ describe(`DependencyRegistryV0`, async function () { await expectRevert( config.dependencyRegistry .connect(config.accounts.deployer) - .removeDependencyAdditionalCDNAtIndex(dependencyTypeBytes, 0), - "Asset index out of range" + .removeDependencyAdditionalCDN(dependencyTypeBytes, 0), + INDEX_OUT_OF_RANGE_ERROR ); }); @@ -1382,7 +1382,7 @@ describe(`DependencyRegistryV0`, async function () { await expect( config.dependencyRegistry .connect(config.accounts.deployer) - .removeDependencyAdditionalCDNAtIndex(dependencyTypeBytes, 0) + .removeDependencyAdditionalCDN(dependencyTypeBytes, 0) ) .to.emit(config.dependencyRegistry, "DependencyAdditionalCDNRemoved") .withArgs(dependencyTypeBytes, 0); @@ -1393,17 +1393,25 @@ describe(`DependencyRegistryV0`, async function () { ); expect(dependencyDetails.additionalCDNCount).to.eq(0); + + expectRevert( + config.dependencyRegistry.getDependencyAdditionalCDN( + dependencyTypeBytes, + 0 + ), + INDEX_OUT_OF_RANGE_ERROR + ); }); }); - describe("updateDependencyAdditionalCDNAtIndex", function () { + describe("updateDependencyAdditionalCDN", function () { it("does not allow non-admins to update a cdn", async function () { // get config from beforeEach const config = this.config; await expectRevert( config.dependencyRegistry .connect(config.accounts.user) - .updateDependencyAdditionalCDNAtIndex( + .updateDependencyAdditionalCDN( dependencyTypeBytes, 0, "https://cdn.com" @@ -1417,7 +1425,7 @@ describe(`DependencyRegistryV0`, async function () { await expectRevert( config.dependencyRegistry .connect(config.accounts.deployer) - .updateDependencyAdditionalCDNAtIndex( + .updateDependencyAdditionalCDN( ethers.utils.formatBytes32String("nonExistentDependencyType"), 0, "https://cdn.com" @@ -1431,12 +1439,12 @@ describe(`DependencyRegistryV0`, async function () { await expectRevert( config.dependencyRegistry .connect(config.accounts.deployer) - .updateDependencyAdditionalCDNAtIndex( + .updateDependencyAdditionalCDN( dependencyTypeBytes, 0, "https://cdn.com" ), - "Asset index out of range" + INDEX_OUT_OF_RANGE_ERROR ); }); it("does not allow updating a cdn with empty string", async function () { @@ -1445,7 +1453,7 @@ describe(`DependencyRegistryV0`, async function () { await expectRevert( config.dependencyRegistry .connect(config.accounts.deployer) - .updateDependencyAdditionalCDNAtIndex(dependencyTypeBytes, 0, ""), + .updateDependencyAdditionalCDN(dependencyTypeBytes, 0, ""), ONLY_NON_EMPTY_STRING_ERROR ); }); @@ -1459,7 +1467,7 @@ describe(`DependencyRegistryV0`, async function () { await expect( config.dependencyRegistry .connect(config.accounts.deployer) - .updateDependencyAdditionalCDNAtIndex( + .updateDependencyAdditionalCDN( dependencyTypeBytes, 0, "https://cdn2.com" @@ -1476,7 +1484,7 @@ describe(`DependencyRegistryV0`, async function () { expect(dependencyDetails.additionalCDNCount).to.eq(1); const storedCDN = - await config.dependencyRegistry.getDependencyAdditionalCDNAtIndex( + await config.dependencyRegistry.getDependencyAdditionalCDN( dependencyTypeBytes, 0 ); @@ -1499,7 +1507,7 @@ describe(`DependencyRegistryV0`, async function () { expect(dependencyDetails.additionalCDNCount).to.eq(1); }); - it("getDependencyAdditionalCDNAtIndex", async function () { + it("getDependencyAdditionalCDN", async function () { // get config from beforeEach const config = this.config; await config.dependencyRegistry @@ -1507,7 +1515,7 @@ describe(`DependencyRegistryV0`, async function () { .addDependencyAdditionalCDN(dependencyTypeBytes, "https://cdn.com"); const storedCDN = - await config.dependencyRegistry.getDependencyAdditionalCDNAtIndex( + await config.dependencyRegistry.getDependencyAdditionalCDN( dependencyTypeBytes, 0 ); @@ -1592,24 +1600,21 @@ describe(`DependencyRegistryV0`, async function () { expect(dependencyDetails.additionalRepositoryCount).to.eq(1); const storedRepository = - await config.dependencyRegistry.getDependencyAdditionalRepositoryAtIndex( + await config.dependencyRegistry.getDependencyAdditionalRepository( dependencyTypeBytes, 0 ); expect(storedRepository).to.eq("https://github.com"); }); }); - describe("removeDependencyAdditionalRepositoryAtIndex", function () { + describe("removeDependencyAdditionalRepository", function () { it("does not allow non-admins to remove additional repository", async function () { // get config from beforeEach const config = this.config; await expectRevert( config.dependencyRegistry .connect(config.accounts.user) - .removeDependencyAdditionalRepositoryAtIndex( - dependencyTypeBytes, - 0 - ), + .removeDependencyAdditionalRepository(dependencyTypeBytes, 0), ONLY_ADMIN_ACL_ERROR ); }); @@ -1619,7 +1624,7 @@ describe(`DependencyRegistryV0`, async function () { await expectRevert( config.dependencyRegistry .connect(config.accounts.deployer) - .removeDependencyAdditionalRepositoryAtIndex( + .removeDependencyAdditionalRepository( ethers.utils.formatBytes32String("nonExistentDependencyType"), 0 ), @@ -1632,11 +1637,8 @@ describe(`DependencyRegistryV0`, async function () { await expectRevert( config.dependencyRegistry .connect(config.accounts.deployer) - .removeDependencyAdditionalRepositoryAtIndex( - dependencyTypeBytes, - 1 - ), - "Asset index out of range" + .removeDependencyAdditionalRepository(dependencyTypeBytes, 1), + INDEX_OUT_OF_RANGE_ERROR ); }); it("allows admin to remove additional repository", async function () { @@ -1652,7 +1654,7 @@ describe(`DependencyRegistryV0`, async function () { await expect( config.dependencyRegistry .connect(config.accounts.deployer) - .removeDependencyAdditionalRepositoryAtIndex(dependencyTypeBytes, 0) + .removeDependencyAdditionalRepository(dependencyTypeBytes, 0) ) .to.emit( config.dependencyRegistry, @@ -1667,22 +1669,23 @@ describe(`DependencyRegistryV0`, async function () { expect(dependencyDetails.additionalRepositoryCount).to.eq(0); - const storedRepository = - await config.dependencyRegistry.getDependencyAdditionalRepositoryAtIndex( + expectRevert( + config.dependencyRegistry.getDependencyAdditionalRepository( dependencyTypeBytes, 0 - ); - expect(storedRepository).to.eq(""); + ), + INDEX_OUT_OF_RANGE_ERROR + ); }); }); - describe("updateDependencyAdditionalRepositoryAtIndex", function () { + describe("updateDependencyAdditionalRepository", function () { it("does not allow non-admins to update additional repository", async function () { // get config from beforeEach const config = this.config; await expectRevert( config.dependencyRegistry .connect(config.accounts.user) - .updateDependencyAdditionalRepositoryAtIndex( + .updateDependencyAdditionalRepository( dependencyTypeBytes, 0, "https://github.com" @@ -1696,7 +1699,7 @@ describe(`DependencyRegistryV0`, async function () { await expectRevert( config.dependencyRegistry .connect(config.accounts.deployer) - .updateDependencyAdditionalRepositoryAtIndex( + .updateDependencyAdditionalRepository( ethers.utils.formatBytes32String("nonExistentDependencyType"), 0, "https://github.com" @@ -1710,12 +1713,12 @@ describe(`DependencyRegistryV0`, async function () { await expectRevert( config.dependencyRegistry .connect(config.accounts.deployer) - .updateDependencyAdditionalRepositoryAtIndex( + .updateDependencyAdditionalRepository( dependencyTypeBytes, 1, "https://github.com" ), - "Asset index out of range" + INDEX_OUT_OF_RANGE_ERROR ); }); it("does not allow updating additional repository to empty string", async function () { @@ -1724,11 +1727,7 @@ describe(`DependencyRegistryV0`, async function () { await expectRevert( config.dependencyRegistry .connect(config.accounts.deployer) - .updateDependencyAdditionalRepositoryAtIndex( - dependencyTypeBytes, - 0, - "" - ), + .updateDependencyAdditionalRepository(dependencyTypeBytes, 0, ""), ONLY_NON_EMPTY_STRING_ERROR ); }); @@ -1745,7 +1744,7 @@ describe(`DependencyRegistryV0`, async function () { await expect( config.dependencyRegistry .connect(config.accounts.deployer) - .updateDependencyAdditionalRepositoryAtIndex( + .updateDependencyAdditionalRepository( dependencyTypeBytes, 0, "https://bitbucket.com" @@ -1758,7 +1757,7 @@ describe(`DependencyRegistryV0`, async function () { .withArgs(dependencyTypeBytes, "https://bitbucket.com", 0); const storedRepository = - await config.dependencyRegistry.getDependencyAdditionalRepositoryAtIndex( + await config.dependencyRegistry.getDependencyAdditionalRepository( dependencyTypeBytes, 0 ); @@ -1784,7 +1783,7 @@ describe(`DependencyRegistryV0`, async function () { expect(dependencyDetails.additionalRepositoryCount).to.eq(1); }); - it("getDependencyAdditionalRepositoryAtIndex", async function () { + it("getDependencyAdditionalRepository", async function () { // get config from beforeEach const config = this.config; await config.dependencyRegistry @@ -1795,7 +1794,7 @@ describe(`DependencyRegistryV0`, async function () { ); const storedRepository = - await config.dependencyRegistry.getDependencyAdditionalRepositoryAtIndex( + await config.dependencyRegistry.getDependencyAdditionalRepository( dependencyTypeBytes, 0 ); @@ -1869,7 +1868,7 @@ describe(`DependencyRegistryV0`, async function () { expect(supportedCoreContractCount).to.eq(1); const storedCoreContract = - await config.dependencyRegistry.getSupportedCoreContractAtIndex(0); + await config.dependencyRegistry.getSupportedCoreContract(0); expect(storedCoreContract).to.eq(config.genArt721Core.address); const supportedCoreContracts = @@ -1926,8 +1925,8 @@ describe(`DependencyRegistryV0`, async function () { expect(supportedCoreContractCount).to.eq(0); await expectRevert( - config.dependencyRegistry.getSupportedCoreContractAtIndex(0), - "Index out of bounds" + config.dependencyRegistry.getSupportedCoreContract(0), + INDEX_OUT_OF_RANGE_ERROR ); const supportedCoreContracts = From 518690ce4229533151600e5fef3f71a35f1298e3 Mon Sep 17 00:00:00 2001 From: purp Date: Tue, 25 Jul 2023 16:16:52 -0600 Subject: [PATCH 11/12] Additional dev note --- packages/contracts/contracts/DependencyRegistryV0.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/contracts/contracts/DependencyRegistryV0.sol b/packages/contracts/contracts/DependencyRegistryV0.sol index 47172ef5b..b7a3476be 100644 --- a/packages/contracts/contracts/DependencyRegistryV0.sol +++ b/packages/contracts/contracts/DependencyRegistryV0.sol @@ -818,6 +818,8 @@ contract DependencyRegistryV0 is * read back, in order to use the proper read approach. If the version is * non-determinate, a fall-back to reading using the assumption that the bytes * were written with `SSTORE2` is used. + * Also note that in this `SSTORE2` fallback handling, the approach of casting bytes to string + * can cause failure (e.g. unexpected continuation byte). */ function getDependencyScript( bytes32 _dependencyType, From 938e3436a20412c2cb2b411ab3dfc6dd7cc096b6 Mon Sep 17 00:00:00 2001 From: purp Date: Wed, 26 Jul 2023 14:32:20 -0600 Subject: [PATCH 12/12] Named args; rename args --- .../contracts/DependencyRegistryV0.sol | 48 ++++++++++++------- 1 file changed, 30 insertions(+), 18 deletions(-) diff --git a/packages/contracts/contracts/DependencyRegistryV0.sol b/packages/contracts/contracts/DependencyRegistryV0.sol index b7a3476be..b0d4d5f2e 100644 --- a/packages/contracts/contracts/DependencyRegistryV0.sol +++ b/packages/contracts/contracts/DependencyRegistryV0.sol @@ -99,11 +99,8 @@ contract DependencyRegistryV0 is ); } - function _onlyInRangeIndex( - uint256 _index, - uint256 _maxIndex - ) internal pure { - require(_index < _maxIndex, "Index out of range"); + function _onlyInRangeIndex(uint256 _index, uint256 _length) internal pure { + require(_index < _length, "Index out of range"); } /** @@ -217,7 +214,7 @@ contract DependencyRegistryV0 is _onlyNonEmptyString(_script); _onlyExistingDependencyType(_dependencyType); Dependency storage dependency = dependencyDetails[_dependencyType]; - _onlyInRangeIndex(_index, dependency.scriptCount); + _onlyInRangeIndex({_index: _index, _length: dependency.scriptCount}); // store script in contract bytecode, replacing reference address from // the contract that no longer exists with the newly created one dependency.scriptBytecodeAddresses[_index] = _script.writeToBytecode(); @@ -266,7 +263,7 @@ contract DependencyRegistryV0 is _onlyNonZeroAddress(_scriptPointer); _onlyExistingDependencyType(_dependencyType); Dependency storage dependency = dependencyDetails[_dependencyType]; - _onlyInRangeIndex(_index, dependency.scriptCount); + _onlyInRangeIndex({_index: _index, _length: dependency.scriptCount}); dependency.scriptBytecodeAddresses[_index] = _scriptPointer; emit DependencyScriptUpdated(_dependencyType); @@ -387,7 +384,7 @@ contract DependencyRegistryV0 is Dependency storage dependency = dependencyDetails[_dependencyType]; uint256 additionalCDNCount = dependency.additionalCDNCount; - _onlyInRangeIndex(_index, additionalCDNCount); + _onlyInRangeIndex({_index: _index, _length: additionalCDNCount}); uint256 lastElementIndex = additionalCDNCount - 1; @@ -416,7 +413,10 @@ contract DependencyRegistryV0 is _onlyNonEmptyString(_additionalCDN); _onlyExistingDependencyType(_dependencyType); Dependency storage dependency = dependencyDetails[_dependencyType]; - _onlyInRangeIndex(_index, dependency.additionalCDNCount); + _onlyInRangeIndex({ + _index: _index, + _length: dependency.additionalCDNCount + }); dependency.additionalCDNs[_index] = _additionalCDN; @@ -474,7 +474,7 @@ contract DependencyRegistryV0 is Dependency storage dependency = dependencyDetails[_dependencyType]; uint256 additionalRepositoryCount = dependency .additionalRepositoryCount; - _onlyInRangeIndex(_index, additionalRepositoryCount); + _onlyInRangeIndex({_index: _index, _length: additionalRepositoryCount}); uint256 lastElementIndex = additionalRepositoryCount - 1; @@ -503,7 +503,10 @@ contract DependencyRegistryV0 is _onlyNonEmptyString(_additionalRepository); _onlyExistingDependencyType(_dependencyType); Dependency storage dependency = dependencyDetails[_dependencyType]; - _onlyInRangeIndex(_index, dependency.additionalRepositoryCount); + _onlyInRangeIndex({ + _index: _index, + _length: dependency.additionalRepositoryCount + }); dependency.additionalRepositories[_index] = _additionalRepository; @@ -626,7 +629,7 @@ contract DependencyRegistryV0 is function getDependencyType( uint256 _index ) external view returns (string memory) { - _onlyInRangeIndex(_index, _dependencyTypes.length()); + _onlyInRangeIndex({_index: _index, _length: _dependencyTypes.length()}); return _dependencyTypes.at(_index).toString(); } @@ -690,7 +693,10 @@ contract DependencyRegistryV0 is function getSupportedCoreContract( uint256 _index ) external view returns (address) { - _onlyInRangeIndex(_index, _supportedCoreContracts.length()); + _onlyInRangeIndex({ + _index: _index, + _length: _supportedCoreContracts.length() + }); return _supportedCoreContracts.at(_index); } @@ -738,7 +744,10 @@ contract DependencyRegistryV0 is uint256 _index ) external view returns (string memory) { Dependency storage dependency = dependencyDetails[_dependencyType]; - _onlyInRangeIndex(_index, dependency.additionalCDNCount); + _onlyInRangeIndex({ + _index: _index, + _length: dependency.additionalCDNCount + }); return dependency.additionalCDNs[_index]; } @@ -752,7 +761,10 @@ contract DependencyRegistryV0 is uint256 _index ) external view returns (string memory) { Dependency storage dependency = dependencyDetails[_dependencyType]; - _onlyInRangeIndex(_index, dependency.additionalRepositoryCount); + _onlyInRangeIndex({ + _index: _index, + _length: dependency.additionalRepositoryCount + }); return dependency.additionalRepositories[_index]; } @@ -778,7 +790,7 @@ contract DependencyRegistryV0 is uint256 _index ) external view returns (address) { Dependency storage dependency = dependencyDetails[_dependencyType]; - _onlyInRangeIndex(_index, dependency.scriptCount); + _onlyInRangeIndex({_index: _index, _length: dependency.scriptCount}); return dependency.scriptBytecodeAddresses[_index]; } @@ -799,7 +811,7 @@ contract DependencyRegistryV0 is uint256 _index ) external view returns (bytes32) { Dependency storage dependency = dependencyDetails[_dependencyType]; - _onlyInRangeIndex(_index, dependency.scriptCount); + _onlyInRangeIndex({_index: _index, _length: dependency.scriptCount}); return BytecodeStorageReader.getLibraryVersionForBytecode( dependencyDetails[_dependencyType].scriptBytecodeAddresses[ @@ -826,7 +838,7 @@ contract DependencyRegistryV0 is uint256 _index ) external view returns (string memory) { Dependency storage dependency = dependencyDetails[_dependencyType]; - _onlyInRangeIndex(_index, dependency.scriptCount); + _onlyInRangeIndex({_index: _index, _length: dependency.scriptCount}); address scriptAddress = dependency.scriptBytecodeAddresses[_index]; bytes32 storageVersion = BytecodeStorageReader