[eth] contract improvement (#348)

* Make Pyth.initialize private

* Make contract upgrade more resillient + add fail test

* Remove deployCommitHash

The deployCommitHash process is error-prone and it's alternatives
require changing many parts of the code.
And as it is not used anywhere. I believe it is not
worth the effort.

* Improve price not found log

Author: ali-behjati

ethereum/contracts/pyth/Pyth.sol

@@ -14,11 +14,11 @@ import "./PythInternalStructs.sol";
 abstract contract Pyth is PythGetters, PythSetters, AbstractPyth {
     using BytesLib for bytes;
 
-    function initialize(
+    function _initialize(
         address wormhole,
         uint16 pyth2WormholeChainId,
         bytes32 pyth2WormholeEmitter
-    ) virtual public {
+    ) internal {
         setWormhole(wormhole);
         setPyth2WormholeChainId(pyth2WormholeChainId);
         setPyth2WormholeEmitter(pyth2WormholeEmitter);
@@ -222,7 +222,7 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth {
     function queryPriceFeed(bytes32 id) public view override returns (PythStructs.PriceFeed memory priceFeed){
         // Look up the latest price info for the given ID
         PythInternalStructs.PriceInfo memory info = latestPriceInfo(id);
-        require(info.priceFeed.id != 0, "no price feed found for the given price id");
+        require(info.priceFeed.id != 0, "price feed for the given id is not pushed or does not exist");
 
         return info.priceFeed;
     }
@@ -236,17 +236,7 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth {
         return validTimePeriodSeconds();
     }
 
-    function isPyth() internal pure returns (bool) {
-        return true;
-    }
-
     function version() public pure returns (string memory) {
         return "1.1.0";
     }
-
-    function deployCommitHash() public pure returns (string memory) {
-        // This is a place holder for the commit hash and will be replaced
-        // with the commit hash upon deployment.
-        return "__DEPLOY_COMMIT_HASH_PLACEHOLER__";
-    }
 }

ethereum/contracts/pyth/PythUpgradable.sol

@@ -18,11 +18,11 @@ contract PythUpgradable is Initializable, OwnableUpgradeable, UUPSUpgradeable, P
         address wormhole,
         uint16 pyth2WormholeChainId,
         bytes32 pyth2WormholeEmitter
-    ) initializer override public {
+    ) initializer public {
         __Ownable_init();
         __UUPSUpgradeable_init();
 
-        Pyth.initialize(wormhole, pyth2WormholeChainId, pyth2WormholeEmitter);
+        Pyth._initialize(wormhole, pyth2WormholeChainId, pyth2WormholeEmitter);
     }
 
     /// Privileged function to specify additional data sources in the contract
@@ -81,11 +81,18 @@ contract PythUpgradable is Initializable, OwnableUpgradeable, UUPSUpgradeable, P
     // Only allow the owner to upgrade the proxy to a new implementation.
     function _authorizeUpgrade(address) internal override onlyOwner {}
 
+    function pythUpgradableMagic() public pure returns (uint32) {
+        return 0x97a6f304;
+    }
+
     // Execute a UpgradeContract governance message
     function upgradeUpgradableContract(UpgradeContractPayload memory payload) override internal {
         address oldImplementation = _getImplementation();
         _upgradeToAndCallUUPS(payload.newImplementation, new bytes(0), false);
-        require(isPyth(), "the new implementation is not a Pyth contract");
+
+        // Calling a method using `this.<method>` will cause a contract call that will use
+        // the new contract.
+        require(this.pythUpgradableMagic() == 0x97a6f304, "the new implementation is not a Pyth contract");
 
         emit ContractUpgraded(oldImplementation, _getImplementation());
     }

ethereum/contracts/pyth/mock/MockUpgradeableProxy.sol

@@ -0,0 +1,14 @@
+// contracts/Implementation.sol
+// SPDX-License-Identifier: Apache 2
+
+pragma solidity ^0.8.0;
+
+import "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
+
+contract MockUpgradeableProxy is UUPSUpgradeable {
+    function isUpgradeActive() external pure returns (bool) {
+        return true;
+    }
+
+    function _authorizeUpgrade(address) internal override {}
+}

ethereum/deploy.sh

@@ -13,10 +13,6 @@ set -euo pipefail
 
 echo "=========== Compiling ==========="
 
-echo "Replacing the deploy commit hash..."
-# Set the deploy commit hash in the contract (used for debugging purposes)
-sed -i "s/__DEPLOY_COMMIT_HASH_PLACEHOLER__/$(git rev-parse HEAD)/g" ./contracts/pyth/Pyth.sol
-
 echo "Building the contract..."
 # Ensure that we deploy a fresh build with up-to-date dependencies.
 rm -rf build && npx truffle compile --all
@@ -41,6 +37,4 @@ while [[ $# -ne 0 ]]; do
     echo "Deployment to $NETWORK finished successfully"
 done
 
-echo "=========== Cleaning up ==========="
-echo "Reverting back the deploy commit hash..."
-sed -i "s/$(git rev-parse HEAD)/__DEPLOY_COMMIT_HASH_PLACEHOLER__/g" ./contracts/pyth/Pyth.sol
+echo "=========== Finished ==========="

ethereum/test/pyth.js

@@ -15,6 +15,7 @@ const Wormhole = artifacts.require("Wormhole");
 
 const PythUpgradable = artifacts.require("PythUpgradable");
 const MockPythUpgrade = artifacts.require("MockPythUpgrade");
+const MockUpgradeableProxy = artifacts.require("MockUpgradeableProxy");
 
 const testSigner1PK =
     "cfb12303a19cde580bb4dd771639b0d26bc68353645571a8cff516ab2ee113a0";
@@ -594,7 +595,7 @@ contract("Pyth", function () {
             this.pythProxy.queryPriceFeed(
                 "0xdeadfeeddeadfeeddeadfeeddeadfeeddeadfeeddeadfeeddeadfeeddeadfeed"
             ),
-            "no price feed found for the given price id"
+            "price feed for the given id is not pushed or does not exist"
         );
     });
 
@@ -982,6 +983,27 @@ contract("Pyth", function () {
         });
     });
 
+    it("Upgrading the contract to a non-pyth contract won't work", async function () {
+        const newImplementation = await MockUpgradeableProxy.new();
+        
+        const data = new governance.EthereumUpgradeContractInstruction(
+            governance.CHAINS.ethereum,
+            new governance.HexString20Bytes(newImplementation.address),
+        ).serialize();
+
+        const vaa = await createVAAFromUint8Array(data,
+            testGovernanceChainId, 
+            testGovernanceEmitter,
+            1
+        );
+
+        // Calling a non-existing method will cause a revert with no explanation.
+        await expectRevert(
+            this.pythProxy.executeGovernanceInstruction(vaa),
+            "revert"
+        );
+    });
+
     it("Setting governance data source should work", async function () {
         const data = new governance.SetGovernanceDataSourceInstruction(
             governance.CHAINS.ethereum,