Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test storage persistence after registry upgrade #2

Open
wants to merge 6 commits into
base: mdt/fixed-storage-pattern
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 21 additions & 17 deletions src/VerifiableFactory.sol
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,6 @@ contract VerifiableFactory {
* @return proxy The address of the deployed `TransparentVerifiableProxy`.
*/
function deployProxy(address implementation, uint256 salt) external returns (address) {
console.log("deploys");
console.logAddress(msg.sender);
bytes32 outerSalt = keccak256(abi.encode(msg.sender, salt));

TransparentVerifiableProxy proxy = new TransparentVerifiableProxy{salt: outerSalt}(address(this));
Expand Down Expand Up @@ -74,26 +72,32 @@ contract VerifiableFactory {
return false;
}
try IProxy(proxy).salt() returns (uint256 salt) {
try IProxy(proxy).creator() returns (address creator) {
// verify the creator matches this factory
if (address(this) != creator) {
return false;
}
try IProxy(proxy).owner() returns (address owner) {
try IProxy(proxy).creator() returns (address creator) {
return _verifyContract(proxy, creator, owner, salt);
} catch {}
} catch {}
} catch {}

return false;
}

// reconstruct the address using CREATE2 and verify it matches
bytes32 outerSalt = keccak256(abi.encode(msg.sender, salt));
function _verifyContract(address proxy, address creator, address owner, uint256 salt) private view returns (bool) {
// verify the creator matches this factory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any need for this check, or for the proxy to be able to return the creator? If it doesn't match, the address will not validate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh right, since creator is the part of the address creation, this is unnecessary. gonna remove it.

if (address(this) != creator) {
return false;
}

// get creation bytecode with constructor arguments
bytes memory bytecode =
abi.encodePacked(type(TransparentVerifiableProxy).creationCode, abi.encode(address(this)));
// reconstruct the address using CREATE2 and verify it matches
bytes32 outerSalt = keccak256(abi.encode(owner, salt));

address expectedProxyAddress = Create2.computeAddress(outerSalt, keccak256(bytecode), address(this));
// get creation bytecode with constructor arguments
bytes memory bytecode =
abi.encodePacked(type(TransparentVerifiableProxy).creationCode, abi.encode(address(this)));

return expectedProxyAddress == proxy;
} catch {}
} catch {}
address expectedProxyAddress = Create2.computeAddress(outerSalt, keccak256(bytecode), address(this));

return false;
return expectedProxyAddress == proxy;
}

function isContract(address account) internal view returns (bool) {
Expand Down
41 changes: 41 additions & 0 deletions test/VerifiableFactory.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,47 @@ contract VerifiableFactoryTest is Test {
assertEq(proxy.creator(), address(factory), "Wrong creator");
}

function test_StoragePersistenceAfterUpgrade() public {
uint256 salt = 1;
address testAccount = makeAddr("testAccount");

// deploy proxy
vm.prank(owner);
address proxyAddress = factory.deployProxy(address(implementation), salt);

// initialize v1 implementation
MockRegistry proxyV1 = MockRegistry(proxyAddress);

// initialize registry
vm.prank(owner);
proxyV1.initialize(owner);
assertEq(proxyV1.admin(), owner, "Admin should be set");

// register an address
vm.prank(owner);
proxyV1.register(testAccount);
assertTrue(proxyV1.registeredAddresses(testAccount), "Address should be registered in V1");
assertEq(proxyV1.getRegistryVersion(), 1, "Should be V1 implementation");

// upgrade to v2
vm.prank(owner);
factory.upgradeImplementation(proxyAddress, address(implementationV2), "");

// verify state persists after upgrade
MockRegistryV2 proxyV2 = MockRegistryV2(proxyAddress);

// check storage persistence
assertTrue(proxyV2.registeredAddresses(testAccount), "Address registration should persist after upgrade");
assertEq(proxyV2.admin(), owner, "Admin should persist after upgrade");
assertEq(proxyV2.getRegistryVersion(), 2, "Should be V2 implementation");

// verify v2 functionality still works as it should be
address newTestAccount = makeAddr("newTestAccount");
vm.prank(owner);
proxyV2.register(newTestAccount);
assertTrue(proxyV2.registeredAddresses(newTestAccount), "Should be able to register new address in V2");
}

// ### Helpers
function isContract(address account) internal view returns (bool) {
uint256 size;
Expand Down