Avoiding ZWeb3 globals in Proxy helper class

[miohtama]

Proxy helper class wraps the management of Proxy contract.

This helper class refers to ZWeb3 which has globals variables including ZWeb3.web3. This ZWeb3.web3 is used in Proxy.admin().

The usage of globals makes harder to reuse this class, e.g. in testing. One has to manually set globals first, before calling Proxy.admin().

A cleaner solution, both usage and API wise, is that one can pass Web3 instance to the Proxy helper class constructor.

Here is a demostration of the problem from a Jest testing code

import { accounts, contract } from '@openzeppelin/test-environment';
import { Proxy } from '@openzeppelin/upgrades';
import { ZWeb3 } from '@openzeppelin/upgrades';

// Ethereum accounts used in these tests
const [
  deployer,  // Deploys the smart contract
  owner, // Token owner - an imaginary multisig wallet
  proxyOwner, // Who owns the proxy contract - an imaginary multisig wallet
  user2 // Random dude who wants play with tokens
] = accounts;

// Loads a compiled contract using OpenZeppelin test-environment
const DawnTokenImpl = contract.fromArtifact('DawnTokenImpl');   // ERC20Pausable subclass
const DawnTokenProxy = contract.fromArtifact('DawnTokenProxy');  // AdminUpgradeabilityProxy subclass

let token = null;  // ERC20Pausable
let proxyContract = null;  // DawnTokenProxy depoyment, AdminUpgradeabilityProxy
let proxy: Proxy = null;  // Zeppelin Proxy helper class

beforeEach(async () => {

  // Fix global usage of ZWeb3.provider in Proxy.admin() call
  ZWeb3.initialize(DawnTokenImpl.web3.currentProvider);

  // Here we refer the token contract directly without going through the proxy
  token = await DawnTokenImpl.new(owner, { from: deployer });

  // https://github.com/OpenZeppelin/openzeppelin-sdk/blob/master/packages/lib/test/contracts/upgradeability/AdminUpgradeabilityProxy.test.js
  const initializeData = Buffer.from('');
  proxyContract = await DawnTokenProxy.new(token.address, proxyOwner, initializeData, { from: deployer });

  assert(proxyContract.address != null);

  // TODO: I want to pass my own web3 to Proxy here
  proxy = new Proxy(proxyContract.address);

});

test('Proxy should have an admin right', async () => {
  // This will trigger the read of global ZWeb3.web3
  console.log(await proxy.admin());
  assert(await proxy.admin() == proxyOwner);
});

[abcoathup]

Hi @miohtama! Thanks for the suggestion, it is really appreciated.

The project owner should review your suggestion during the next week.

Please wait until we have discussed this idea before writing any code or submitting a Pull Request, so we can go through the design beforehand. We don’t want you to waste your time!

[frangio]

Hi @miohtama! Thank you for bringing this up. We completely agree that the use of global state here is really bad, and it's one of the things we want to improve for the next releases. I'm closing this issue in favor of https://github.com/OpenZeppelin/openzeppelin-sdk/issues/1308. Make sure to subscribe to that one for any updates.