Rename: zos-lib contracts and zos command #1074
Conversation
| @@ -40,7 +38,7 @@ contract('ProxyAdmin', function(accounts) { | |||
| this.ownable = this.proxyAdmin; | |||
| }); | |||
|
|
|||
| shouldBehaveLikeOwnable(proxyAdminOwner, anotherAccount); | |||
| // shouldBehaveLikeOwnable(proxyAdminOwner, anotherAccount); | |||
There was a problem hiding this comment.
Ha! nice catch. I left it commented while debugging 😢
There was a problem hiding this comment.
Great work Jota! Thanks for taking care of this massive change! I flagged a few changes, mostly around implementing EIP 1967 (as linked in the issue) instead of a custom label for the proxy storage slots. Let me know if you have any questions regarding that EIP!
| @@ -20,10 +20,10 @@ contract DeprecatedAdminUpgradeabilityProxy is UpgradeabilityProxy { | |||
|
|
|||
| /** | |||
| * @dev Storage slot with the admin of the contract. | |||
| * This is the keccak-256 hash of "org.zeppelinos.proxy.admin", and is | |||
| * This is the keccak-256 hash of "com.openzeppelinupgrades.proxy.admin", and is | |||
There was a problem hiding this comment.
Let's make use of the renaming to push EIP 1967 instead. Please double check on your end that the hashes were calculated correctly!
Logic contract address
Storage slot 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc (obtained as bytes32(uint256(keccak256('eip1967.proxy.implementation')) - 1)).
Admin address
Storage slot 0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103 (obtained as bytes32(uint256(keccak256('eip1967.proxy.admin')) - 1)).
| sinon.restore(); | ||
| }); | ||
|
|
||
| it('uses the correct admin storage slot', async function() { |
There was a problem hiding this comment.
Note that this test does not check that the Proxy class falls back to the deprecated label. I'd suggest adding a LegacyProxy contract in the mocks folder, and testing against it to ensure both the storage and admin are correctly retrieved.
There was a problem hiding this comment.
Yeap, I'm working on it. I created a zos folder inside the mock contracts folder and added the Zos prefix to the upgradeability contract names. Is that fine?
| @@ -46,6 +54,16 @@ export default function shouldBehaveLikeUpgradeabilityProxy( | |||
| it('has expected balance', async function() { | |||
| (await ZWeb3.getBalance(this.proxy)).should.eq(balance.toString()); | |||
| }); | |||
|
|
|||
| it('uses the correct implementation storage slot', async function() { | |||
for both admin and implementation slots
Co-Authored-By: Santiago Palladino <[email protected]>
|
@spalladino should be ready for another review. |
jbcarpanelli
force-pushed
the
rename/commands-and-contracts
branch
from
e8a0dad
to
8c6cb04
Compare
|
|
||
| const DummyImplementation = Contracts.getFromLocal('DummyImplementation'); | ||
|
|
||
| export function shouldUseEIP1967StorageSlot(createProxy, accounts, labels, fnName) { |
There was a problem hiding this comment.
I'd say that the tests in this file are already covered by the shouldBehaveLikeProxy tests run on both the new and the legacy implementations, so they should not be needed - but they definitely not hurt, so let's keep them.
This PR implements:
zos-libcontract references:zos-libcontracts, giving support for those proxies already created with the old slot labels references.zos-cli.tstooz-cli.ts. Also changed references incli/package.jsonfilesopenzeppelinandozas valid binaries, and addedzosandozas commander aliases.bin/zos-cli.jsreferences in zepkit integration tests andfirst-projectsample.zosinstead ofozoropenzeppelinTests are failing but they should be not very difficult to fix, as almost all are throwing the same error.