feat: streamline document store versions (#122)

* feat: remove legacy DocumentStore.js * refactor: switch out document store * feat: remove OwnableDocumentStore.sol * feat: add owner required * fix: owner in gsn document store * refactor: clean up
Open-Attestation · Nov 9, 2022 · 99db55e · 99db55e
1 parent 38ae341
commit 99db55e
Show file tree

Hide file tree

Showing 13 changed files with 42 additions and 152 deletions.
diff --git a/benchmark/GasCostBenchmark.js b/benchmark/GasCostBenchmark.js
@@ -138,23 +138,20 @@ describe("Gas Cost Benchmarks", () => {
 
   let Accounts;
   let DocumentStore;
-  let UpgradableDocumentStore;
   let DocumentStoreCreator;
 
   before(async () => {
     Accounts = await ethers.getSigners();
     DocumentStore = await ethers.getContractFactory("DocumentStore");
-    UpgradableDocumentStore = await ethers.getContractFactory("UpgradableDocumentStore");
     DocumentStoreCreator = await ethers.getContractFactory("DocumentStoreCreator");
-    UpgradableDocumentStore.numberFormat = "String";
   });
 
   describe("DocumentStore", () => {
     const contractName = "DocumentStore";
 
     it("runs benchmark", async () => {
       // Deploy & initialize document store contract
-      const documentStoreInstance = await DocumentStore.deploy(contractName);
+      const documentStoreInstance = await DocumentStore.deploy(contractName, Accounts[0].address);
       const tx = await documentStoreInstance.deployed();
       recordGasCost(contractName, "deployment", await getCumulativeGasUsed(tx));
 

diff --git a/contracts/DocumentStore.sol b/contracts/DocumentStore.sol
@@ -2,81 +2,36 @@
 
 pragma solidity ^0.8.0;
 
-import "@openzeppelin/contracts/access/Ownable.sol";
+import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
+import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
 
-/*
- * Legacy version for reference and backward compatibility, similar to OwnableDocumentStore
- */
-contract DocumentStore is Ownable {
-  string public name;
-  string public version = "2.3.0";
+import "./BaseDocumentStore.sol";
 
-  /// A mapping of the document hash to the block number that was issued
-  mapping(bytes32 => uint256) public documentIssued;
-  /// A mapping of the hash of the claim being revoked to the revocation block number
-  mapping(bytes32 => uint256) public documentRevoked;
-
-  event DocumentIssued(bytes32 indexed document);
-  event DocumentRevoked(bytes32 indexed document);
-
-  constructor(string memory _name) {
-    name = _name;
-  }
-
-  function issue(bytes32 document) public onlyOwner onlyNotIssued(document) {
-    documentIssued[document] = block.number;
-    emit DocumentIssued(document);
+contract DocumentStore is BaseDocumentStore, OwnableUpgradeable {
+  constructor(string memory _name, address owner) {
+    initialize(_name, owner);
   }
 
-  function bulkIssue(bytes32[] memory documents) public {
-    for (uint256 i = 0; i < documents.length; i++) {
-      issue(documents[i]);
-    }
+  function initialize(string memory _name, address owner) internal initializer {
+    require(owner != address(0), "Owner is required");
+    super.__Ownable_init();
+    super.transferOwnership(owner);
+    BaseDocumentStore.initialize(_name);
   }
 
-  function getIssuedBlock(bytes32 document) public view onlyIssued(document) returns (uint256) {
-    return documentIssued[document];
-  }
-
-  function isIssued(bytes32 document) public view returns (bool) {
-    return (documentIssued[document] != 0);
+  function issue(bytes32 document) public onlyOwner onlyNotIssued(document) {
+    BaseDocumentStore._issue(document);
   }
 
-  function isIssuedBefore(bytes32 document, uint256 blockNumber) public view returns (bool) {
-    return documentIssued[document] != 0 && documentIssued[document] <= blockNumber;
+  function bulkIssue(bytes32[] memory documents) public onlyOwner {
+    BaseDocumentStore._bulkIssue(documents);
   }
 
   function revoke(bytes32 document) public onlyOwner onlyNotRevoked(document) returns (bool) {
-    documentRevoked[document] = block.number;
-    emit DocumentRevoked(document);
-  }
-
-  function bulkRevoke(bytes32[] memory documents) public {
-    for (uint256 i = 0; i < documents.length; i++) {
-      revoke(documents[i]);
-    }
-  }
-
-  function isRevoked(bytes32 document) public view returns (bool) {
-    return documentRevoked[document] != 0;
-  }
-
-  function isRevokedBefore(bytes32 document, uint256 blockNumber) public view returns (bool) {
-    return documentRevoked[document] <= blockNumber && documentRevoked[document] != 0;
-  }
-
-  modifier onlyIssued(bytes32 document) {
-    require(isIssued(document), "Error: Only issued document hashes can be revoked");
-    _;
-  }
-
-  modifier onlyNotIssued(bytes32 document) {
-    require(!isIssued(document), "Error: Only hashes that have not been issued can be issued");
-    _;
+    return BaseDocumentStore._revoke(document);
   }
 
-  modifier onlyNotRevoked(bytes32 claim) {
-    require(!isRevoked(claim), "Error: Hash has been revoked previously");
-    _;
+  function bulkRevoke(bytes32[] memory documents) public onlyOwner {
+    return BaseDocumentStore._bulkRevoke(documents);
   }
 }
diff --git a/contracts/DocumentStoreCreator.sol b/contracts/DocumentStoreCreator.sol
@@ -2,7 +2,7 @@
 
 pragma solidity ^0.8.0;
 
-import "./UpgradableDocumentStore.sol";
+import "./DocumentStore.sol";
 
 // Naming this factory contract as DocumentStoreCreator so that typechain can name the factory of this
 // contract as DocumentStoreCreatorFactory and it does not collide with the automatically generated
@@ -12,7 +12,7 @@ contract DocumentStoreCreator {
 
   function deploy(string memory name) public returns (address) {
     // solhint-disable-next-line mark-callable-contracts
-    UpgradableDocumentStore instance = new UpgradableDocumentStore(name, msg.sender);
+    DocumentStore instance = new DocumentStore(name, msg.sender);
     emit DocumentStoreDeployed(address(instance), msg.sender);
     return address(instance);
   }

diff --git a/contracts/DocumentStoreWithRevokeReasons.sol b/contracts/DocumentStoreWithRevokeReasons.sol
@@ -2,15 +2,15 @@
 
 pragma solidity ^0.8.0;
 
-import "./UpgradableDocumentStore.sol";
+import "./DocumentStore.sol";
 
-contract DocumentStoreWithRevokeReasons is UpgradableDocumentStore {
+contract DocumentStoreWithRevokeReasons is DocumentStore {
   /// A mapping of the document hash to the block number that was issued
   mapping(bytes32 => uint256) public revokeReason;
 
   event DocumentRevokedWithReason(bytes32 indexed document, uint256 reason);
 
-  constructor(string memory _name, address owner) UpgradableDocumentStore(_name, owner) {}
+  constructor(string memory _name, address owner) DocumentStore(_name, owner) {}
 
   function revoke(bytes32 document, uint256 reason) public onlyOwner onlyNotRevoked(document) returns (bool) {
     revoke(document);

diff --git a/contracts/GsnCapable.sol b/contracts/GsnCapable.sol
@@ -2,10 +2,10 @@
 
 pragma solidity ^0.8.0;
 
-import "@openzeppelin/contracts/access/Ownable.sol";
+import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
 import "@openzeppelin/contracts/utils/introspection/ERC165Storage.sol";
 
-contract GsnCapable is ERC165Storage, Ownable {
+contract GsnCapable is ERC165Storage, OwnableUpgradeable {
   address public paymaster;
   bytes4 private constant _INTERFACE_ID_GSN_CAPABLE = 0xa5a23640;
 

diff --git a/contracts/GsnCapableDocumentStore.sol b/contracts/GsnCapableDocumentStore.sol
@@ -4,22 +4,22 @@ pragma solidity ^0.8.0;
 
 import "@opengsn/contracts/src/BaseRelayRecipient.sol";
 
-import "./OwnableDocumentStore.sol";
+import "./DocumentStore.sol";
 import "./GsnCapable.sol";
 import "../interfaces/IKnowForwarderAddress.sol";
 
-contract GsnCapableDocumentStore is OwnableDocumentStore, BaseRelayRecipient, IKnowForwarderAddress, GsnCapable {
+contract GsnCapableDocumentStore is DocumentStore, BaseRelayRecipient, IKnowForwarderAddress, GsnCapable {
   string public override versionRecipient = "2.0.0";
 
-  constructor(string memory _name, address _forwarder) OwnableDocumentStore(_name) {
+  constructor(string memory name, address owner, address _forwarder) DocumentStore(name, owner) {
     _setTrustedForwarder(_forwarder);
   }
 
-  function _msgSender() internal view override(Context, BaseRelayRecipient) returns (address) {
+  function _msgSender() internal view override(ContextUpgradeable, BaseRelayRecipient) returns (address) {
     return BaseRelayRecipient._msgSender();
   }
 
-  function _msgData() internal view override(Context, BaseRelayRecipient) returns (bytes memory) {
+  function _msgData() internal view override(ContextUpgradeable, BaseRelayRecipient) returns (bytes memory) {
     return BaseRelayRecipient._msgData();
   }
 

diff --git a/contracts/OwnableDocumentStore.sol b/contracts/OwnableDocumentStore.sol
diff --git a/contracts/UpgradableDocumentStore.sol b/contracts/UpgradableDocumentStore.sol
diff --git a/src/index.test.ts b/src/index.test.ts
@@ -16,14 +16,14 @@ beforeAll(async () => {
 });
 
 describe("deploy", () => {
-  it("deploys a new UpgradableDocumentStore contract without waiting for confirmation", async () => {
+  it("deploys a new DocumentStore contract without waiting for confirmation", async () => {
     const receipt = await deploy("My Store", signer, { documentStoreCreatorAddressOverride });
     expect(receipt.from).toBe(account);
   });
 });
 
 describe("deployAndWait", () => {
-  it("deploys a new UpgradableDocumentStore contract", async () => {
+  it("deploys a new DocumentStore contract", async () => {
     const instance = await deployAndWait("My Store", signer, { documentStoreCreatorAddressOverride });
     const owner = await instance.owner();
     expect(owner).toBe(account);

diff --git a/src/index.ts b/src/index.ts
@@ -3,7 +3,7 @@
 import { Signer, providers, ContractTransaction } from "ethers";
 import {
   DocumentStoreCreator__factory as DocumentStoreCreatorFactory,
-  UpgradableDocumentStore__factory as UpgradableDocumentStoreFactory,
+  DocumentStore__factory as DocumentStoreFactory,
 } from "./contracts";
 import { getDocumentStoreCreatorAddress } from "./config";
 
@@ -24,11 +24,11 @@ export const deploy = async (name: string, signer: Signer, options?: DeployOptio
 export const deployAndWait = async (name: string, signer: Signer, options?: DeployOptions) => {
   const receipt = await (await deploy(name, signer, options)).wait();
   if (!receipt.logs || !receipt.logs[0].address) throw new Error("Fail to detect deployed contract address");
-  return UpgradableDocumentStoreFactory.connect(receipt.logs![0].address, signer);
+  return DocumentStoreFactory.connect(receipt.logs![0].address, signer);
 };
 
 export const connect = async (address: string, signerOrProvider: Signer | providers.Provider) => {
-  return UpgradableDocumentStoreFactory.connect(address, signerOrProvider);
+  return DocumentStoreFactory.connect(address, signerOrProvider);
 };
 
 // Export typechain classes for distribution purposes

diff --git a/test/DocumentStore.js b/test/DocumentStore.js
@@ -11,7 +11,7 @@ describe("DocumentStore", async () => {
   beforeEach("", async () => {
     Accounts = await ethers.getSigners();
     DocumentStore = await ethers.getContractFactory("DocumentStore");
-    DocumentStoreInstance = await DocumentStore.connect(Accounts[0]).deploy(config.INSTITUTE_NAME);
+    DocumentStoreInstance = await DocumentStore.connect(Accounts[0]).deploy(config.INSTITUTE_NAME, Accounts[0].address);
     await DocumentStoreInstance.deployed();
   });
 

diff --git a/test/DocumentStoreCreator.js b/test/DocumentStoreCreator.js
@@ -4,12 +4,12 @@ const config = require("../config.js");
 
 describe("DocumentStoreCreator", async () => {
   let Accounts;
-  let UpgradableDocumentStore;
+  let DocumentStore;
   let DocumentStoreCreator;
 
   before("", async () => {
     Accounts = await ethers.getSigners();
-    UpgradableDocumentStore = await ethers.getContractFactory("UpgradableDocumentStore");
+    DocumentStore = await ethers.getContractFactory("DocumentStore");
     DocumentStoreCreator = await ethers.getContractFactory("DocumentStoreCreator");
   });
 
@@ -29,7 +29,7 @@ describe("DocumentStoreCreator", async () => {
         "Emitted contract creator does not match"
       );
       // Test correctness of deployed DocumentStore
-      const deployedDocumentStore = await UpgradableDocumentStore.attach(receipt.events[2].args.instance);
+      const deployedDocumentStore = await DocumentStore.attach(receipt.events[2].args.instance);
       const name = await deployedDocumentStore.name();
       expect(name).to.be.equal(config.INSTITUTE_NAME, "Name of institute does not match");
       const owner = await deployedDocumentStore.owner();

diff --git a/test/GsnCapableDocumentStore.js b/test/GsnCapableDocumentStore.js
@@ -20,6 +20,7 @@ describe("GsnCapableDocumentStore", async () => {
   beforeEach("", async () => {
     GsnCapableDocumentStoreInstance = await GsnCapableDocumentStore.connect(Accounts[0]).deploy(
       config.INSTITUTE_NAME,
+      Accounts[0].address,
       Accounts[1].address
     );
     CalculatorSelectorInstance = await CalculateSelector.connect(Accounts[0]).deploy();
@@ -91,6 +92,7 @@ describe("GsnCapableDocumentStore", async () => {
       configurableForwarder = await ConfigurableTrustForwarder.deploy();
       configurableInstance = await GsnCapableDocumentStore.connect(owner).deploy(
         config.INSTITUTE_NAME,
+        owner.address,
         configurableForwarder.address
       );
     });