From 910c0e87a4fd5ebd615850110139f33985995413 Mon Sep 17 00:00:00 2001
From: Alysia Huggins <hi@alysia.tech>
Date: Mon, 29 Jul 2024 20:54:22 -0400
Subject: [PATCH 1/7] switched to using contract inheritance for new versions
 of the LC contract

---
 contracts/src/LightClient.sol           |  32 ++--
 contracts/test/LightClientUpgrade.t.sol |  13 +-
 contracts/test/LightClientV2.sol        | 205 ++++--------------------
 3 files changed, 59 insertions(+), 191 deletions(-)

diff --git a/contracts/src/LightClient.sol b/contracts/src/LightClient.sol
index f3110da1fa..e01cd73994 100644
--- a/contracts/src/LightClient.sol
+++ b/contracts/src/LightClient.sol
@@ -174,19 +174,23 @@ contract LightClient is Initializable, OwnableUpgradeable, UUPSUpgradeable {
     function getVersion()
         public
         pure
+        virtual
         returns (uint8 majorVersion, uint8 minorVersion, uint8 patchVersion)
     {
         return (1, 0, 0);
     }
 
     /// @notice only the owner can authorize an upgrade
-    function _authorizeUpgrade(address newImplementation) internal override onlyOwner {
+    function _authorizeUpgrade(address newImplementation) internal virtual override onlyOwner {
         emit Upgrade(newImplementation);
     }
 
     // @dev Initialization of contract variables happens in this method because the LightClient
     // contract is upgradable and thus has its constructor method disabled.
-    function _initializeState(LightClientState memory genesis, uint32 numBlockPerEpoch) internal {
+    function _initializeState(LightClientState memory genesis, uint32 numBlockPerEpoch)
+        internal
+        virtual
+    {
         // stake table commitments and threshold cannot be zero, otherwise it's impossible to
         // generate valid proof to move finalized state forward.
         // Whereas blockCommRoot can be zero, if we use special value zero to denote empty tree.
@@ -237,7 +241,7 @@ contract LightClient is Initializable, OwnableUpgradeable, UUPSUpgradeable {
     function newFinalizedState(
         LightClientState memory newState,
         IPlonkVerifier.PlonkProof memory proof
-    ) external {
+    ) external virtual {
         //revert if we're in permissionedProver mode and the permissioned prover has not been set
         if (permissionedProverEnabled && msg.sender != permissionedProver) {
             if (permissionedProver == address(0)) {
@@ -287,12 +291,12 @@ contract LightClient is Initializable, OwnableUpgradeable, UUPSUpgradeable {
     }
 
     /// @dev Simple getter function for the genesis state
-    function getGenesisState() public view returns (LightClientState memory) {
+    function getGenesisState() public view virtual returns (LightClientState memory) {
         return states[genesisState];
     }
 
     /// @dev Simple getter function for the finalized state
-    function getFinalizedState() public view returns (LightClientState memory) {
+    function getFinalizedState() public view virtual returns (LightClientState memory) {
         return states[finalizedState];
     }
 
@@ -322,7 +326,7 @@ contract LightClient is Initializable, OwnableUpgradeable, UUPSUpgradeable {
 
     /// @notice Advance to the next epoch (without any precondition check!)
     /// @dev This meant to be invoked only internally after appropriate precondition checks are done
-    function _advanceEpoch() private {
+    function _advanceEpoch() internal virtual {
         bytes32 newStakeTableComm = computeStakeTableComm(states[finalizedState]);
         votingStakeTableCommitment = frozenStakeTableCommitment;
         frozenStakeTableCommitment = newStakeTableComm;
@@ -335,7 +339,12 @@ contract LightClient is Initializable, OwnableUpgradeable, UUPSUpgradeable {
     }
 
     /// @notice Given the light client state, compute the short commitment of the stake table
-    function computeStakeTableComm(LightClientState memory state) public pure returns (bytes32) {
+    function computeStakeTableComm(LightClientState memory state)
+        public
+        pure
+        virtual
+        returns (bytes32)
+    {
         return keccak256(
             abi.encodePacked(
                 state.stakeTableBlsKeyComm,
@@ -349,7 +358,7 @@ contract LightClient is Initializable, OwnableUpgradeable, UUPSUpgradeable {
     /// non-zero address provided
     /// @dev this function can also be used to update the permissioned prover once it's a different
     /// address
-    function setPermissionedProver(address prover) public onlyOwner {
+    function setPermissionedProver(address prover) public virtual onlyOwner {
         if (prover == address(0)) {
             revert InvalidAddress();
         }
@@ -363,7 +372,7 @@ contract LightClient is Initializable, OwnableUpgradeable, UUPSUpgradeable {
 
     /// @notice set the permissionedProverMode to false and set the permissionedProver to address(0)
     /// @dev if it was already disabled (permissioneProverMode == false), then revert with
-    function disablePermissionedProverMode() public onlyOwner {
+    function disablePermissionedProverMode() public virtual onlyOwner {
         if (permissionedProverEnabled) {
             permissionedProver = address(0);
             permissionedProverEnabled = false;
@@ -419,7 +428,7 @@ contract LightClient is Initializable, OwnableUpgradeable, UUPSUpgradeable {
     }
 
     /// @notice get the number of L1 block updates
-    function getStateUpdateBlockNumbersCount() public view returns (uint256) {
+    function getStateUpdateBlockNumbersCount() public view virtual returns (uint256) {
         return stateUpdateBlockNumbers.length;
     }
 
@@ -429,6 +438,7 @@ contract LightClient is Initializable, OwnableUpgradeable, UUPSUpgradeable {
     function getHotShotCommitment(uint256 hotShotBlockHeight)
         public
         view
+        virtual
         returns (HotShotCommitment memory)
     {
         uint256 commitmentsHeight = hotShotCommitments.length;
@@ -447,7 +457,7 @@ contract LightClient is Initializable, OwnableUpgradeable, UUPSUpgradeable {
     }
 
     /// @notice get the number of HotShot block commitments
-    function getHotShotBlockCommitmentsCount() public view returns (uint256) {
+    function getHotShotBlockCommitmentsCount() public view virtual returns (uint256) {
         return hotShotCommitments.length;
     }
 }
diff --git a/contracts/test/LightClientUpgrade.t.sol b/contracts/test/LightClientUpgrade.t.sol
index b24fe3dd38..75ca4a1430 100644
--- a/contracts/test/LightClientUpgrade.t.sol
+++ b/contracts/test/LightClientUpgrade.t.sol
@@ -52,7 +52,7 @@ contract LightClientUpgradeTest is Test {
         assertEq(lcV2Proxy.blocksPerEpoch(), 10);
         assertEq(lcV2Proxy.currentEpoch(), 0);
 
-        LCV2.LightClientState memory expectedLightClientState = LCV2.LightClientState(
+        LCV1.LightClientState memory expectedLightClientState = LCV1.LightClientState(
             stateV1.viewNum,
             stateV1.blockHeight,
             stateV1.blockCommRoot,
@@ -60,11 +60,18 @@ contract LightClientUpgradeTest is Test {
             stateV1.stakeTableBlsKeyComm,
             stateV1.stakeTableSchnorrKeyComm,
             stateV1.stakeTableAmountComm,
-            stateV1.threshold,
-            0 // New field for testing purposes
+            stateV1.threshold //,
+                // 0 // New field for testing purposes
         );
 
+        LCV2.ExtendedLightClientState memory expectedExtendedLightClientState =
+            LCV2.ExtendedLightClientState(0);
+
         assertEq(abi.encode(lcV2Proxy.getFinalizedState()), abi.encode(expectedLightClientState));
+        assertEq(
+            abi.encode(lcV2Proxy.getExtendedFinalizedState()),
+            abi.encode(expectedExtendedLightClientState)
+        );
     }
 
     // check that the proxy address remains the same
diff --git a/contracts/test/LightClientV2.sol b/contracts/test/LightClientV2.sol
index f95f514745..894e1b1708 100644
--- a/contracts/test/LightClientV2.sol
+++ b/contracts/test/LightClientV2.sol
@@ -13,91 +13,22 @@ import { BN254 } from "bn254/BN254.sol";
 import { IPlonkVerifier } from "../src/interfaces/IPlonkVerifier.sol";
 import { PlonkVerifier } from "../src/libraries/PlonkVerifier.sol";
 import { LightClientStateUpdateVK as VkLib } from "../src/libraries/LightClientStateUpdateVK.sol";
+import { LightClient } from "../src/LightClient.sol";
 
 /// @notice A light client for HotShot consensus. Keeping track of its finalized states in safe,
 /// authenticated ways.
-contract LightClientV2 is Initializable, OwnableUpgradeable, UUPSUpgradeable {
-    // === Events ===
-    //
-    // @notice Notify a new epoch is starting
-    event EpochChanged(uint64);
-
-    /// @notice upgrade event when the proxy updates the implementation it's pointing to
-    event Upgrade(address implementation);
-
-    // === Constants ===
-    //
-    /// @notice System parameter: number of blocks per epoch
-    /// @dev This variable cannot be made immutable due to how UUPS contracts work. See
-    /// https://forum.openzeppelin.com/t/upgradable-contracts-instantiating-an-immutable-value/28763/2#why-cant-i-use-immutable-variables-1
-    uint32 public blocksPerEpoch;
-
-    /// @notice genesis block commitment index
-    uint32 internal genesisState;
-
-    /// @notice Finalized HotShot's light client state index
-    uint32 internal finalizedState;
-
-    // === Storage ===
-    //
-    /// @notice current (finalized) epoch number
-    uint64 public currentEpoch;
-    /// @notice The commitment of the stake table used in current voting (i.e. snapshot at the start
-    /// of last epoch)
-    bytes32 public votingStakeTableCommitment;
-    /// @notice The quorum threshold for the stake table used in current voting
-    uint256 public votingThreshold;
-    /// @notice The commitment of the stake table frozen for change (i.e. snapshot at the start of
-    /// last epoch)
-    bytes32 public frozenStakeTableCommitment;
-    /// @notice The quorum threshold for the frozen stake table
-    uint256 public frozenThreshold;
-
-    /// @notice mapping to store light client states in order to simplify upgrades
-    mapping(uint32 index => LightClientState value) public states;
-
+contract LightClientV2 is LightClient {
     /// @notice new field for testing purposes
     /// @dev In order to add a field to LightClientState struct one can: add a new contract variable
     /// that has the new struct type, or put the struct inside a map.
     uint256 public newField;
 
-    // === Data Structure ===
-    //
-    /// @notice The finalized HotShot state (as the digest of the entire HotShot state)
-    /// @param viewNum The latest view number of the finalized HotShot chain
-    /// @param blockHeight The block height of the latest finalized block
-    /// @param blockCommRoot The merkle root of historical block commitments (BN254::ScalarField)
-    /// @param feeLedgerComm The commitment to the fee ledger state (type: BN254::ScalarField)
-    /// @param stakeTableBlsKeyComm The commitment to the BlsVerKey column of the stake table
-    /// @param stakeTableSchnorrKeyComm The commitment to the SchnorrVerKey column of the table
-    /// @param stakeTableAmountComm The commitment to the stake amount column of the stake table
-    /// @param threshold The (stake-weighted) quorum threshold for a QC to be considered as valid
-    struct LightClientState {
-        uint64 viewNum;
-        uint64 blockHeight;
-        BN254.ScalarField blockCommRoot;
-        BN254.ScalarField feeLedgerComm;
-        BN254.ScalarField stakeTableBlsKeyComm;
-        BN254.ScalarField stakeTableSchnorrKeyComm;
-        BN254.ScalarField stakeTableAmountComm;
-        uint256 threshold;
-        uint32 extraField; // New field for testing purposes
+    struct ExtendedLightClientState {
+        uint256 extraField;
     }
 
-    /// @notice Event that a new finalized state has been successfully verified and updated
-    event NewState(
-        uint64 indexed viewNum, uint64 indexed blockHeight, BN254.ScalarField blockCommRoot
-    );
-
-    /// @notice The state is outdated and older than currently known `finalizedState`
-    error OutdatedState();
-    /// @notice Warning that the last block of the current epoch is not yet submitted before newer
-    /// states are proposed.
-    error MissingLastBlockForCurrentEpoch(uint64 expectedBlockHeight);
-    /// @notice Invalid user inputs: wrong format or non-sensible arguments
-    error InvalidArgs();
-    /// @notice Wrong plonk proof or public inputs.
-    error InvalidProof();
+    /// @notice mapping to store the extended light client states in order to simplify upgrades
+    mapping(uint32 index => ExtendedLightClientState state) public extendedStates;
 
     /// @notice since the constructor initializes storage on this contract we disable it
     /// @dev storage is on the proxy contract since it calls this contract via delegatecall
@@ -110,47 +41,13 @@ contract LightClientV2 is Initializable, OwnableUpgradeable, UUPSUpgradeable {
     function getVersion()
         public
         pure
+        virtual
+        override
         returns (uint8 majorVersion, uint8 minorVersion, uint8 patchVersion)
     {
         return (2, 0, 0);
     }
 
-    /// @notice only the owner can authorize an upgrade
-    function _authorizeUpgrade(address newImplementation) internal override onlyOwner {
-        emit Upgrade(newImplementation);
-    }
-
-    function _initializeState(LightClientState memory genesis, uint32 numBlockPerEpoch)
-        internal
-        onlyOwner
-    {
-        // stake table commitments and threshold cannot be zero, otherwise it's impossible to
-        // generate valid proof to move finalized state forward.
-        // Whereas blockCommRoot can be zero, if we use special value zero to denote empty tree.
-        // feeLedgerComm can be zero, if we optionally support fee ledger yet.
-        if (
-            genesis.viewNum != 0 || genesis.blockHeight != 0
-                || BN254.ScalarField.unwrap(genesis.stakeTableBlsKeyComm) == 0
-                || BN254.ScalarField.unwrap(genesis.stakeTableSchnorrKeyComm) == 0
-                || BN254.ScalarField.unwrap(genesis.stakeTableAmountComm) == 0 || genesis.threshold == 0
-                || numBlockPerEpoch == 0
-        ) {
-            revert InvalidArgs();
-        }
-
-        states[genesisState] = genesis;
-        states[finalizedState] = genesis;
-        currentEpoch = 0;
-
-        blocksPerEpoch = numBlockPerEpoch;
-
-        bytes32 initStakeTableComm = computeStakeTableComm(genesis);
-        votingStakeTableCommitment = initStakeTableComm;
-        votingThreshold = genesis.threshold;
-        frozenStakeTableCommitment = initStakeTableComm;
-        frozenThreshold = genesis.threshold;
-    }
-
     // === State Modifying APIs ===
     //
     /// @notice Update the latest finalized light client state. It is expected to be updated
@@ -160,7 +57,7 @@ contract LightClientV2 is Initializable, OwnableUpgradeable, UUPSUpgradeable {
     function newFinalizedState(
         LightClientState memory newState,
         IPlonkVerifier.PlonkProof memory proof
-    ) external {
+    ) external virtual override {
         if (
             newState.viewNum <= getFinalizedState().viewNum
                 || newState.blockHeight <= getFinalizedState().blockHeight
@@ -193,76 +90,30 @@ contract LightClientV2 is Initializable, OwnableUpgradeable, UUPSUpgradeable {
         require(newField == 0, "newField can only be set to 0");
 
         // upon successful verification, update the latest finalized state
-        states[finalizedState] = newState;
-        emit NewState(newState.viewNum, newState.blockHeight, newState.blockCommRoot);
-    }
+        // because newState is in memory and states[finalizedState] is in storage, they have
+        // different data handling mechanisms
+        // and this each field needs to be assigned individually
+        states[finalizedState].viewNum = newState.viewNum;
+        states[finalizedState].blockHeight = newState.blockHeight;
+        states[finalizedState].blockCommRoot = newState.blockCommRoot;
+        states[finalizedState].feeLedgerComm = newState.feeLedgerComm;
+        states[finalizedState].stakeTableBlsKeyComm = newState.stakeTableBlsKeyComm;
+        states[finalizedState].stakeTableSchnorrKeyComm = newState.stakeTableSchnorrKeyComm;
+        states[finalizedState].stakeTableAmountComm = newState.stakeTableAmountComm;
+        states[finalizedState].threshold = newState.threshold;
+
+        extendedStates[finalizedState].extraField = 2;
 
-    /// @dev Simple getter function for the genesis state
-    function getGenesisState() public view returns (LightClientState memory) {
-        return states[genesisState];
-    }
-
-    /// @dev Simple getter function for the finalized state
-    function getFinalizedState() public view returns (LightClientState memory) {
-        return states[finalizedState];
+        emit NewState(newState.viewNum, newState.blockHeight, newState.blockCommRoot);
     }
 
-    // === Pure or View-only APIs ===
-    /// @dev Transform a state into an array of field elements, prepared as public inputs of the
-    /// plonk proof verification
-    function preparePublicInput(LightClientState memory state)
-        internal
+    /// @dev Simple getter function for the extended finalized state
+    function getExtendedFinalizedState()
+        public
         view
-        returns (uint256[] memory)
-    {
-        uint256[] memory publicInput = new uint256[](8);
-        publicInput[0] = votingThreshold;
-        publicInput[1] = uint256(state.viewNum);
-        publicInput[2] = uint256(state.blockHeight);
-        publicInput[3] = BN254.ScalarField.unwrap(state.blockCommRoot);
-        publicInput[4] = BN254.ScalarField.unwrap(state.feeLedgerComm);
-        publicInput[5] = BN254.ScalarField.unwrap(state.stakeTableBlsKeyComm);
-        publicInput[6] = BN254.ScalarField.unwrap(state.stakeTableSchnorrKeyComm);
-        publicInput[7] = BN254.ScalarField.unwrap(state.stakeTableAmountComm);
-        return publicInput;
-    }
-
-    /// @dev Verify the Plonk proof, marked as `virtual` for easier testing as we can swap VK used
-    /// in inherited contracts.
-    function verifyProof(LightClientState memory state, IPlonkVerifier.PlonkProof memory proof)
-        internal
         virtual
+        returns (ExtendedLightClientState memory)
     {
-        IPlonkVerifier.VerifyingKey memory vk = VkLib.getVk();
-        uint256[] memory publicInput = preparePublicInput(state);
-
-        if (!PlonkVerifier.verify(vk, publicInput, proof)) {
-            revert InvalidProof();
-        }
-    }
-
-    /// @notice Advance to the next epoch (without any precondition check!)
-    /// @dev This meant to be invoked only internally after appropriate precondition checks are done
-    function _advanceEpoch() private {
-        bytes32 newStakeTableComm = computeStakeTableComm(getFinalizedState());
-        votingStakeTableCommitment = frozenStakeTableCommitment;
-        frozenStakeTableCommitment = newStakeTableComm;
-
-        votingThreshold = frozenThreshold;
-        frozenThreshold = getFinalizedState().threshold;
-
-        currentEpoch += 1;
-        emit EpochChanged(currentEpoch);
-    }
-
-    /// @notice Given the light client state, compute the short commitment of the stake table
-    function computeStakeTableComm(LightClientState memory state) public pure returns (bytes32) {
-        return keccak256(
-            abi.encodePacked(
-                state.stakeTableBlsKeyComm,
-                state.stakeTableSchnorrKeyComm,
-                state.stakeTableAmountComm
-            )
-        );
+        return extendedStates[finalizedState];
     }
 }

From 35a6dd9f9d884bb09ce9fc93a40d794958cfacce Mon Sep 17 00:00:00 2001
From: Alysia Huggins <hi@alysia.tech>
Date: Wed, 7 Aug 2024 10:00:29 -0400
Subject: [PATCH 2/7] added initialize methods for each version and a third
 version of the LC contract

---
 contracts/script/UpgradeLightClientToV2.s.sol | 10 +--
 contracts/script/UpgradeLightClientToV3.s.sol | 43 ++++++++++++
 contracts/src/LightClient.sol                 |  5 +-
 ...oV2.t.sol => LightClientUpgradeToVx.t.sol} | 68 ++++++++++++++++---
 contracts/test/LightClientV2.sol              | 22 +++---
 contracts/test/LightClientV3.sol              | 30 ++++++++
 6 files changed, 151 insertions(+), 27 deletions(-)
 create mode 100644 contracts/script/UpgradeLightClientToV3.s.sol
 rename contracts/test/{LightClientUpgradeToV2.t.sol => LightClientUpgradeToVx.t.sol} (57%)
 create mode 100644 contracts/test/LightClientV3.sol

diff --git a/contracts/script/UpgradeLightClientToV2.s.sol b/contracts/script/UpgradeLightClientToV2.s.sol
index a608786212..df73cb5fe4 100644
--- a/contracts/script/UpgradeLightClientToV2.s.sol
+++ b/contracts/script/UpgradeLightClientToV2.s.sol
@@ -11,14 +11,14 @@ contract UpgradeLightClientScript is Script {
     /// @param mostRecentlyDeployedProxy address of deployed proxy
     /// @return address of the proxy
     /// TODO get the most recent deployment from the devops tooling
-    function run(uint32 seedPhraseOffset, address mostRecentlyDeployedProxy)
+    function run(uint32 seedPhraseOffset, address mostRecentlyDeployedProxy, uint256 newField)
         external
         returns (address)
     {
         string memory seedPhrase = vm.envString("MNEMONIC");
         (address admin,) = deriveRememberKey(seedPhrase, seedPhraseOffset);
         vm.startBroadcast(admin);
-        address proxy = upgradeLightClient(mostRecentlyDeployedProxy, address(new LCV2()));
+        address proxy = upgradeLightClient(mostRecentlyDeployedProxy, address(new LCV2()), newField);
         return proxy;
     }
 
@@ -28,12 +28,14 @@ contract UpgradeLightClientScript is Script {
     /// @param proxyAddress address of proxy
     /// @param newLightClient address of new implementation
     /// @return address of the proxy
-    function upgradeLightClient(address proxyAddress, address newLightClient)
+    function upgradeLightClient(address proxyAddress, address newLightClient, uint256 newField)
         public
         returns (address)
     {
         LC proxy = LC(proxyAddress); //make the function call on the previous implementation
-        proxy.upgradeToAndCall(newLightClient, ""); //proxy address now points to the new
+
+        proxy.upgradeToAndCall(newLightClient, abi.encodeCall(LCV2.initializeV2, newField)); //proxy
+            // address now points to the new
             // implementation
         vm.stopBroadcast();
         return address(proxy);
diff --git a/contracts/script/UpgradeLightClientToV3.s.sol b/contracts/script/UpgradeLightClientToV3.s.sol
new file mode 100644
index 0000000000..19bf8aa91f
--- /dev/null
+++ b/contracts/script/UpgradeLightClientToV3.s.sol
@@ -0,0 +1,43 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.19;
+
+import { Script } from "forge-std/Script.sol";
+
+import { LightClientV2 as LCV2 } from "../test/LightClientV2.sol";
+import { LightClientV3 as LCV3 } from "../test/LightClientV3.sol";
+
+contract UpgradeLightClientScript is Script {
+    /// @notice runs the upgrade
+    /// @param mostRecentlyDeployedProxy address of deployed proxy
+    /// @return address of the proxy
+    /// TODO get the most recent deployment from the devops tooling
+    function run(uint32 seedPhraseOffset, address mostRecentlyDeployedProxy, uint256 newField)
+        external
+        returns (address)
+    {
+        string memory seedPhrase = vm.envString("MNEMONIC");
+        (address admin,) = deriveRememberKey(seedPhrase, seedPhraseOffset);
+        vm.startBroadcast(admin);
+        address proxy = upgradeLightClient(mostRecentlyDeployedProxy, address(new LCV3()), newField);
+        return proxy;
+    }
+
+    /// @notice upgrades the light client contract by calling the upgrade function the
+    /// implementation contract via
+    /// the proxy
+    /// @param proxyAddress address of proxy
+    /// @param newLightClient address of new implementation
+    /// @return address of the proxy
+    function upgradeLightClient(address proxyAddress, address newLightClient, uint256 newField)
+        public
+        returns (address)
+    {
+        LCV2 proxy = LCV2(proxyAddress); //make the function call on the previous implementation
+
+        proxy.upgradeToAndCall(newLightClient, abi.encodeCall(LCV3.initializeV3, newField)); //proxy
+            // address now points to the new
+            // implementation
+        vm.stopBroadcast();
+        return address(proxy);
+    }
+}
diff --git a/contracts/src/LightClient.sol b/contracts/src/LightClient.sol
index e01cd73994..4778465ec7 100644
--- a/contracts/src/LightClient.sol
+++ b/contracts/src/LightClient.sol
@@ -187,10 +187,7 @@ contract LightClient is Initializable, OwnableUpgradeable, UUPSUpgradeable {
 
     // @dev Initialization of contract variables happens in this method because the LightClient
     // contract is upgradable and thus has its constructor method disabled.
-    function _initializeState(LightClientState memory genesis, uint32 numBlockPerEpoch)
-        internal
-        virtual
-    {
+    function _initializeState(LightClientState memory genesis, uint32 numBlockPerEpoch) internal {
         // stake table commitments and threshold cannot be zero, otherwise it's impossible to
         // generate valid proof to move finalized state forward.
         // Whereas blockCommRoot can be zero, if we use special value zero to denote empty tree.
diff --git a/contracts/test/LightClientUpgradeToV2.t.sol b/contracts/test/LightClientUpgradeToVx.t.sol
similarity index 57%
rename from contracts/test/LightClientUpgradeToV2.t.sol
rename to contracts/test/LightClientUpgradeToVx.t.sol
index dae4f1bd02..6e5e5687bc 100644
--- a/contracts/test/LightClientUpgradeToV2.t.sol
+++ b/contracts/test/LightClientUpgradeToVx.t.sol
@@ -5,15 +5,19 @@ pragma experimental ABIEncoderV2;
 import { Test } /*, console2*/ from "forge-std/Test.sol";
 import { LightClient as LCV1 } from "../src/LightClient.sol";
 import { LightClientV2 as LCV2 } from "../test/LightClientV2.sol";
+import { LightClientV3 as LCV3 } from "../test/LightClientV3.sol";
 import { DeployLightClientContractScript } from "../script/LightClient.s.sol";
 import { UpgradeLightClientScript } from "../script/UpgradeLightClientToV2.s.sol";
+import { UpgradeLightClientScript as ULCV3 } from "../script/UpgradeLightClientToV3.s.sol";
 
-contract LightClientUpgradeToV2Test is Test {
+contract LightClientUpgradeToVxTest is Test {
     LCV1 public lcV1Proxy;
     LCV2 public lcV2Proxy;
+    LCV3 public lcV3Proxy;
 
     DeployLightClientContractScript public deployer = new DeployLightClientContractScript();
-    UpgradeLightClientScript public upgrader = new UpgradeLightClientScript();
+    UpgradeLightClientScript public upgraderV2 = new UpgradeLightClientScript();
+    ULCV3 public upgraderV3 = new ULCV3();
 
     LCV1.LightClientState public stateV1;
 
@@ -46,9 +50,10 @@ contract LightClientUpgradeToV2Test is Test {
         // Upgrade LightClient and check that the genesis state is not changed and that the new
         // field
         // of the upgraded contract is set to 0
-        lcV2Proxy = LCV2(upgrader.run(0, proxy));
+        uint256 myNewField = 123;
+        lcV2Proxy = LCV2(upgraderV2.run(0, proxy, myNewField));
 
-        assertEq(lcV2Proxy.newField(), 0);
+        assertEq(lcV2Proxy.newField(), myNewField);
         assertEq(lcV2Proxy.blocksPerEpoch(), 10);
         assertEq(lcV2Proxy.currentEpoch(), 0);
 
@@ -60,8 +65,7 @@ contract LightClientUpgradeToV2Test is Test {
             stateV1.stakeTableBlsKeyComm,
             stateV1.stakeTableSchnorrKeyComm,
             stateV1.stakeTableAmountComm,
-            stateV1.threshold //,
-                // 0 // New field for testing purposes
+            stateV1.threshold
         );
 
         LCV2.ExtendedLightClientState memory expectedExtendedLightClientState =
@@ -74,6 +78,54 @@ contract LightClientUpgradeToV2Test is Test {
         );
     }
 
+    // that the data remains the same after upgrading the implementation
+    function testUpgradeSameDataV2ToV3() public {
+        // Upgrade LightClient and check that the genesis state is not changed and that the new
+        // field
+        // of the upgraded contract is set to 0
+        uint256 myNewField = 123;
+        uint256 myNewFieldV3 = 456;
+        lcV2Proxy = LCV2(upgraderV2.run(0, proxy, myNewField));
+
+        assertEq(lcV2Proxy.newField(), myNewField);
+        assertEq(lcV2Proxy.blocksPerEpoch(), 10);
+        assertEq(lcV2Proxy.currentEpoch(), 0);
+
+        LCV1.LightClientState memory expectedLightClientState = LCV1.LightClientState(
+            stateV1.viewNum,
+            stateV1.blockHeight,
+            stateV1.blockCommRoot,
+            stateV1.feeLedgerComm,
+            stateV1.stakeTableBlsKeyComm,
+            stateV1.stakeTableSchnorrKeyComm,
+            stateV1.stakeTableAmountComm,
+            stateV1.threshold
+        );
+
+        LCV2.ExtendedLightClientState memory expectedExtendedLightClientState =
+            LCV2.ExtendedLightClientState(0);
+
+        assertEq(abi.encode(lcV2Proxy.getFinalizedState()), abi.encode(expectedLightClientState));
+        assertEq(
+            abi.encode(lcV2Proxy.getExtendedFinalizedState()),
+            abi.encode(expectedExtendedLightClientState)
+        );
+
+        // upgrade to v3
+        lcV3Proxy = LCV3(upgraderV3.run(0, proxy, myNewFieldV3));
+
+        assertEq(lcV3Proxy.newField(), myNewField);
+        assertEq(lcV3Proxy.anotherField(), myNewFieldV3);
+        assertEq(lcV3Proxy.blocksPerEpoch(), 10);
+        assertEq(lcV3Proxy.currentEpoch(), 0);
+
+        assertEq(abi.encode(lcV3Proxy.getFinalizedState()), abi.encode(expectedLightClientState));
+        assertEq(
+            abi.encode(lcV3Proxy.getExtendedFinalizedState()),
+            abi.encode(expectedExtendedLightClientState)
+        );
+    }
+
     // check that the proxy address remains the same
     function testUpgradesSameProxyAddress() public {
         (uint8 major, uint8 minor, uint8 patch) = lcV1Proxy.getVersion();
@@ -82,7 +134,7 @@ contract LightClientUpgradeToV2Test is Test {
         assertEq(patch, 0);
 
         //upgrade box
-        lcV2Proxy = LCV2(upgrader.run(0, proxy));
+        lcV2Proxy = LCV2(upgraderV2.run(0, proxy, 123));
         assertEq(address(lcV2Proxy), address(lcV1Proxy));
         (uint8 majorV2, uint8 minorV2, uint8 patchV2) = lcV2Proxy.getVersion();
         assertEq(majorV2, 2);
@@ -96,6 +148,6 @@ contract LightClientUpgradeToV2Test is Test {
         //attempted upgrade as attacker will revert
         vm.prank(attacker);
         vm.expectRevert();
-        lcV2Proxy = LCV2(upgrader.run(0, address(proxy)));
+        lcV2Proxy = LCV2(upgraderV2.run(0, address(proxy), 123));
     }
 }
diff --git a/contracts/test/LightClientV2.sol b/contracts/test/LightClientV2.sol
index 894e1b1708..b49fcf492e 100644
--- a/contracts/test/LightClientV2.sol
+++ b/contracts/test/LightClientV2.sol
@@ -3,12 +3,6 @@
 pragma solidity ^0.8.0;
 pragma experimental ABIEncoderV2;
 
-import { OwnableUpgradeable } from
-    "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
-import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
-import { UUPSUpgradeable } from
-    "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
-
 import { BN254 } from "bn254/BN254.sol";
 import { IPlonkVerifier } from "../src/interfaces/IPlonkVerifier.sol";
 import { PlonkVerifier } from "../src/libraries/PlonkVerifier.sol";
@@ -23,6 +17,10 @@ contract LightClientV2 is LightClient {
     /// that has the new struct type, or put the struct inside a map.
     uint256 public newField;
 
+    /// @notice this field is used to check initialized versions so that one can ensure that the
+    /// initialization only happens once
+    uint8 internal _initializedVersion;
+
     struct ExtendedLightClientState {
         uint256 extraField;
     }
@@ -30,11 +28,13 @@ contract LightClientV2 is LightClient {
     /// @notice mapping to store the extended light client states in order to simplify upgrades
     mapping(uint32 index => ExtendedLightClientState state) public extendedStates;
 
-    /// @notice since the constructor initializes storage on this contract we disable it
-    /// @dev storage is on the proxy contract since it calls this contract via delegatecall
-    /// @custom:oz-upgrades-unsafe-allow constructor
-    constructor() {
-        _disableInitializers();
+    /// @notice Initialize v2
+    /// @param _newField   New field amount
+    ///
+    function initializeV2(uint256 _newField) external {
+        require(_initializedVersion == 0);
+        newField = _newField;
+        _initializedVersion = 2;
     }
 
     /// @notice Use this to get the implementation contract version
diff --git a/contracts/test/LightClientV3.sol b/contracts/test/LightClientV3.sol
new file mode 100644
index 0000000000..d9a4bb4515
--- /dev/null
+++ b/contracts/test/LightClientV3.sol
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: UNLICENSED
+
+pragma solidity ^0.8.0;
+pragma experimental ABIEncoderV2;
+
+import { LightClientV2 } from "./LightClientV2.sol";
+/// @notice A light client for HotShot consensus. Keeping track of its finalized states in safe,
+/// authenticated ways.
+
+contract LightClientV3 is LightClientV2 {
+    uint256 public anotherField;
+
+    /// @param _newField   New field amount
+    function initializeV3(uint256 _newField) external {
+        require(_initializedVersion == 2);
+        anotherField = _newField;
+        _initializedVersion = 3;
+    }
+
+    /// @notice Use this to get the implementation contract version
+    function getVersion()
+        public
+        pure
+        virtual
+        override
+        returns (uint8 majorVersion, uint8 minorVersion, uint8 patchVersion)
+    {
+        return (3, 0, 0);
+    }
+}

From 4a0b8ca58bcc4ffcd5822cc76daa4226fca9968d Mon Sep 17 00:00:00 2001
From: Alysia Huggins <hi@alysia.tech>
Date: Wed, 7 Aug 2024 16:48:23 -0400
Subject: [PATCH 3/7] added documentation about smart contract upgrades to read
 me and modified tests

---
 contracts/test/LightClientUpgradeToVx.t.sol   |  37 +++-
 contracts/test/LightClientV2.sol              |   1 -
 contracts/test/LightClientV3.sol              |   2 +-
 .../UpgradeLightClientToV2.s.sol              |   8 +-
 .../UpgradeLightClientToV3.s.sol              |  10 +-
 doc/smart-contract-upgrades.md                | 194 ++++++++++++++++++
 6 files changed, 228 insertions(+), 24 deletions(-)
 rename contracts/{script => test}/UpgradeLightClientToV2.s.sol (82%)
 rename contracts/{script => test}/UpgradeLightClientToV3.s.sol (78%)
 create mode 100644 doc/smart-contract-upgrades.md

diff --git a/contracts/test/LightClientUpgradeToVx.t.sol b/contracts/test/LightClientUpgradeToVx.t.sol
index 6e5e5687bc..e6525fe944 100644
--- a/contracts/test/LightClientUpgradeToVx.t.sol
+++ b/contracts/test/LightClientUpgradeToVx.t.sol
@@ -7,8 +7,10 @@ import { LightClient as LCV1 } from "../src/LightClient.sol";
 import { LightClientV2 as LCV2 } from "../test/LightClientV2.sol";
 import { LightClientV3 as LCV3 } from "../test/LightClientV3.sol";
 import { DeployLightClientContractScript } from "../script/LightClient.s.sol";
-import { UpgradeLightClientScript } from "../script/UpgradeLightClientToV2.s.sol";
-import { UpgradeLightClientScript as ULCV3 } from "../script/UpgradeLightClientToV3.s.sol";
+import { UpgradeLightClientScript } from "./UpgradeLightClientToV2.s.sol";
+import { UpgradeLightClientScript as ULCV3 } from "./UpgradeLightClientToV3.s.sol";
+import { OwnableUpgradeable } from
+    "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
 
 contract LightClientUpgradeToVxTest is Test {
     LCV1 public lcV1Proxy;
@@ -46,12 +48,12 @@ contract LightClientUpgradeToVxTest is Test {
     }
 
     // that the data remains the same after upgrading the implementation
-    function testUpgradeSameData() public {
+    function testUpgradeSameDataV1ToV2() public {
         // Upgrade LightClient and check that the genesis state is not changed and that the new
         // field
         // of the upgraded contract is set to 0
         uint256 myNewField = 123;
-        lcV2Proxy = LCV2(upgraderV2.run(0, proxy, myNewField));
+        lcV2Proxy = LCV2(upgraderV2.run(proxy, myNewField, admin));
 
         assertEq(lcV2Proxy.newField(), myNewField);
         assertEq(lcV2Proxy.blocksPerEpoch(), 10);
@@ -85,7 +87,7 @@ contract LightClientUpgradeToVxTest is Test {
         // of the upgraded contract is set to 0
         uint256 myNewField = 123;
         uint256 myNewFieldV3 = 456;
-        lcV2Proxy = LCV2(upgraderV2.run(0, proxy, myNewField));
+        lcV2Proxy = LCV2(upgraderV2.run(proxy, myNewField, admin));
 
         assertEq(lcV2Proxy.newField(), myNewField);
         assertEq(lcV2Proxy.blocksPerEpoch(), 10);
@@ -112,7 +114,7 @@ contract LightClientUpgradeToVxTest is Test {
         );
 
         // upgrade to v3
-        lcV3Proxy = LCV3(upgraderV3.run(0, proxy, myNewFieldV3));
+        lcV3Proxy = LCV3(upgraderV3.run(proxy, myNewFieldV3, admin));
 
         assertEq(lcV3Proxy.newField(), myNewField);
         assertEq(lcV3Proxy.anotherField(), myNewFieldV3);
@@ -134,7 +136,7 @@ contract LightClientUpgradeToVxTest is Test {
         assertEq(patch, 0);
 
         //upgrade box
-        lcV2Proxy = LCV2(upgraderV2.run(0, proxy, 123));
+        lcV2Proxy = LCV2(upgraderV2.run(proxy, 123, admin));
         assertEq(address(lcV2Proxy), address(lcV1Proxy));
         (uint8 majorV2, uint8 minorV2, uint8 patchV2) = lcV2Proxy.getVersion();
         assertEq(majorV2, 2);
@@ -142,12 +144,25 @@ contract LightClientUpgradeToVxTest is Test {
         assertEq(patchV2, 0);
     }
 
-    function testMaliciousUpgradeFails() public {
+    function testMaliciousUpgradeToV2Fails() public {
         address attacker = makeAddr("attacker");
 
         //attempted upgrade as attacker will revert
-        vm.prank(attacker);
-        vm.expectRevert();
-        lcV2Proxy = LCV2(upgraderV2.run(0, address(proxy), 123));
+        vm.expectRevert(
+            abi.encodeWithSelector(OwnableUpgradeable.OwnableUnauthorizedAccount.selector, attacker)
+        );
+
+        lcV2Proxy = LCV2(upgraderV2.run(address(proxy), 123, attacker));
+    }
+
+    function testMaliciousUpgradeToV32Fails() public {
+        address attacker = makeAddr("attacker");
+
+        //attempted upgrade as attacker will revert
+
+        vm.expectRevert(
+            abi.encodeWithSelector(OwnableUpgradeable.OwnableUnauthorizedAccount.selector, attacker)
+        );
+        lcV3Proxy = LCV3(upgraderV3.run(address(proxy), 456, attacker));
     }
 }
diff --git a/contracts/test/LightClientV2.sol b/contracts/test/LightClientV2.sol
index b49fcf492e..5f64210917 100644
--- a/contracts/test/LightClientV2.sol
+++ b/contracts/test/LightClientV2.sol
@@ -30,7 +30,6 @@ contract LightClientV2 is LightClient {
 
     /// @notice Initialize v2
     /// @param _newField   New field amount
-    ///
     function initializeV2(uint256 _newField) external {
         require(_initializedVersion == 0);
         newField = _newField;
diff --git a/contracts/test/LightClientV3.sol b/contracts/test/LightClientV3.sol
index d9a4bb4515..eead886633 100644
--- a/contracts/test/LightClientV3.sol
+++ b/contracts/test/LightClientV3.sol
@@ -12,7 +12,7 @@ contract LightClientV3 is LightClientV2 {
 
     /// @param _newField   New field amount
     function initializeV3(uint256 _newField) external {
-        require(_initializedVersion == 2);
+        require(_initializedVersion == 2, "already initialized");
         anotherField = _newField;
         _initializedVersion = 3;
     }
diff --git a/contracts/script/UpgradeLightClientToV2.s.sol b/contracts/test/UpgradeLightClientToV2.s.sol
similarity index 82%
rename from contracts/script/UpgradeLightClientToV2.s.sol
rename to contracts/test/UpgradeLightClientToV2.s.sol
index df73cb5fe4..7eb152fcd6 100644
--- a/contracts/script/UpgradeLightClientToV2.s.sol
+++ b/contracts/test/UpgradeLightClientToV2.s.sol
@@ -3,7 +3,7 @@ pragma solidity ^0.8.19;
 
 import { Script } from "forge-std/Script.sol";
 
-import { LightClientV2 as LCV2 } from "../test/LightClientV2.sol";
+import { LightClientV2 as LCV2 } from "./LightClientV2.sol";
 import { LightClient as LC } from "../src/LightClient.sol";
 
 contract UpgradeLightClientScript is Script {
@@ -11,14 +11,13 @@ contract UpgradeLightClientScript is Script {
     /// @param mostRecentlyDeployedProxy address of deployed proxy
     /// @return address of the proxy
     /// TODO get the most recent deployment from the devops tooling
-    function run(uint32 seedPhraseOffset, address mostRecentlyDeployedProxy, uint256 newField)
+    function run(address mostRecentlyDeployedProxy, uint256 newField, address admin)
         external
         returns (address)
     {
-        string memory seedPhrase = vm.envString("MNEMONIC");
-        (address admin,) = deriveRememberKey(seedPhrase, seedPhraseOffset);
         vm.startBroadcast(admin);
         address proxy = upgradeLightClient(mostRecentlyDeployedProxy, address(new LCV2()), newField);
+        vm.stopBroadcast();
         return proxy;
     }
 
@@ -37,7 +36,6 @@ contract UpgradeLightClientScript is Script {
         proxy.upgradeToAndCall(newLightClient, abi.encodeCall(LCV2.initializeV2, newField)); //proxy
             // address now points to the new
             // implementation
-        vm.stopBroadcast();
         return address(proxy);
     }
 }
diff --git a/contracts/script/UpgradeLightClientToV3.s.sol b/contracts/test/UpgradeLightClientToV3.s.sol
similarity index 78%
rename from contracts/script/UpgradeLightClientToV3.s.sol
rename to contracts/test/UpgradeLightClientToV3.s.sol
index 19bf8aa91f..3e6158ee99 100644
--- a/contracts/script/UpgradeLightClientToV3.s.sol
+++ b/contracts/test/UpgradeLightClientToV3.s.sol
@@ -3,22 +3,21 @@ pragma solidity ^0.8.19;
 
 import { Script } from "forge-std/Script.sol";
 
-import { LightClientV2 as LCV2 } from "../test/LightClientV2.sol";
-import { LightClientV3 as LCV3 } from "../test/LightClientV3.sol";
+import { LightClientV2 as LCV2 } from "./LightClientV2.sol";
+import { LightClientV3 as LCV3 } from "./LightClientV3.sol";
 
 contract UpgradeLightClientScript is Script {
     /// @notice runs the upgrade
     /// @param mostRecentlyDeployedProxy address of deployed proxy
     /// @return address of the proxy
     /// TODO get the most recent deployment from the devops tooling
-    function run(uint32 seedPhraseOffset, address mostRecentlyDeployedProxy, uint256 newField)
+    function run(address mostRecentlyDeployedProxy, uint256 newField, address admin)
         external
         returns (address)
     {
-        string memory seedPhrase = vm.envString("MNEMONIC");
-        (address admin,) = deriveRememberKey(seedPhrase, seedPhraseOffset);
         vm.startBroadcast(admin);
         address proxy = upgradeLightClient(mostRecentlyDeployedProxy, address(new LCV3()), newField);
+        vm.stopBroadcast();
         return proxy;
     }
 
@@ -37,7 +36,6 @@ contract UpgradeLightClientScript is Script {
         proxy.upgradeToAndCall(newLightClient, abi.encodeCall(LCV3.initializeV3, newField)); //proxy
             // address now points to the new
             // implementation
-        vm.stopBroadcast();
         return address(proxy);
     }
 }
diff --git a/doc/smart-contract-upgrades.md b/doc/smart-contract-upgrades.md
new file mode 100644
index 0000000000..f4aaedaf0c
--- /dev/null
+++ b/doc/smart-contract-upgrades.md
@@ -0,0 +1,194 @@
+# Smart Contract Upgrades
+
+Code on the EVM is immutable but via proxies, smart contracts can be “upgraded”. A proxy contract acts as an
+intermediary that can delegate calls to other contracts, called implementation contracts. This allows the logic of a
+smart contract to be upgraded while keeping the same address and state. This is made possible by two functions in
+Solidity:
+
+- `delegatecall`: contractA can delegatecall contractB but execute the logic within the context of contractA. This means
+  that the called contract's code is executed with the storage, caller, and balance of the calling contract.
+- `fallback`: executed when a function doesn't exist. This is often used in proxy contracts to catch all function calls
+  and delegate them to the implementation contract.
+
+OpenZeppelin's [implementation](https://docs.openzeppelin.com/contracts/4.x/api/proxy#UUPSUpgradeable) for the UUPS
+proxy pattern was used.
+
+## Writing Upgradable Smart Contracts
+
+This is written with developers in mind to explain the process for upgrading contracts within our repo to ensure safe
+upgrades.
+
+### Upgrading via Contract Inheritance
+
+Solidity supports inheritance which means that a function call always executes the function of the same name (and
+parameter types) in the most derived contract in the inheritance hierarchy.
+
+**Pros**:
+
+- **Modularity**: each contract version can focus on specific features or upgrades
+- **Reusability:** code doesn’t have to be duplicated in newer versions which reduces the likelihood of errors
+- **Ease of extension:** easier to extend features without worrying about the rest of the implementation
+- **C3 linearization**: C3 linearization provides a deterministic way to determine the storage layout of a contract
+  based on its inheritance hierarchy. This means that if we maintain the same inheritance order in subsequent contract
+  versions, the storage layout will remain consistent, even if we add new state variables.
+
+**Cons**:
+
+- **Deployment size:** Contracts that inherit from multiple base contracts can become large, potentially exceeding the
+  maximum contract size limit on the Ethereum blockchain
+- **State variable shadowing:** When a new version of a contract inherits from an older version, introducing a state
+  variable with the same name as one in the base contract will lead to a compilation error due to shadowing. Therefore,
+  one cannot simply redefine an existing struct with new members if inheritance is used for upgrades. This can become
+  cumbersome in large projects with many inherited contracts
+- **Multiple inheritance:** Solidity’s C3 linearization predicts the order in which base contracts are inherited, and
+  their functions are called. So the order inherited is the order they will be in the storage layout and this needs to
+  be taken under consideration. When multiple base contracts define state variables, the order of inheritance can affect
+  the storage layout. Mismanaging this can lead to overlapping storage slots, causing data corruption.
+
+### Upgrades supported
+
+- new state variables
+- new logic
+- modifying existing logic (function overriding)
+
+#### New State Variables
+
+Thanks to C3 linearization, the order in which you inherit from the previous version of the contract dictates the
+storage layout so be sure to be mindful when declaring new variables.
+
+Criteria:
+
+- New variable names must be used otherwise it leads to a compilation error due to shadowing.
+- New variables added _must_ be declared as type `internal` or `public`, `private` can never be used.
+- If the new variable needs to be initialized, then the initialize function with a separate initV[X] function should be
+  added. The initV[X] function allows the contract to initialize only the new variables added in the new contract.
+
+_Example:_
+
+```solidity
+// Version 1
+contract V1 {
+    uint public oldValue;
+    uint8 internal _initializedVersion
+
+    function initialize(uint _oldValue) external {
+        require(_initializedVersion == 0);
+        oldValue = _oldValue;
+        _initializedVersion = 1;
+    }
+}
+
+// Version 2
+contract V2 is V1 {
+    uint public newValue;
+
+    function initializeV2(uint _newValue) external {
+        require(_initializedVersion == 1);
+        newValue = _newValue;
+        _initializedVersion = 2;
+    }
+}
+```
+
+#### New Logic
+
+Base functions can be overridden by inheriting contracts to change their behavior if they are marked as virtual. Thus if
+you plan to keep a function upgradable, always mark it as virtual in new versions of the contract.
+
+_Example:_
+
+```solidity
+// Version 1
+contract V1 {
+    function getValue() public view virtual returns (uint) {
+        return 1;
+    }
+}
+
+// Version 2
+contract V2 is V1 {
+    function getValue() public view virtual override returns (uint) {
+        return 2;
+    }
+}
+```
+
+#### Modifying Existing Logic (Function Overriding)
+
+The logic in exiting functions can be overridden but be sure not to introduce breaking changes.
+
+Criteria:
+
+- a function must be `virtual` to be able to be overridden in a derived contract.
+- a function with `private` visibility cannot be overridden or inherited.
+- Public state variables can override `external` functions if the parameter and return types of the function matches the
+  getter function of the variable:
+
+_Example:_
+
+```solidity
+// SPDX-License-Identifier: GPL-3.0
+pragma solidity >=0.6.0 <0.9.0;
+
+contract A {
+    function f() external view virtual returns(uint) { return 5; }
+}
+
+contract B is A {
+    uint public override f;
+}
+```
+
+- The overriding function may only change the visibility of the overridden function from `external` to `public`.
+- The mutability may be changed to a more strict one following the order: `nonpayable` can be overridden by `view` and
+  `pure`. `view` can be overridden by `pure`. `payable` is an exception and cannot be changed to any other mutability.
+
+### Upgrades that become complicated through inheritance
+
+#### Upgrading Structs
+
+Unfortunately, directly upgrading structs through inheritance is not straightforward due to Solidity's storage layout
+rules. When a new version of a contract inherits from an older version, introducing a state variable with the same name
+as one in the base contract will lead to a compilation error due to shadowing.
+
+**Potential workarounds:**
+
+- Additional struct: create a new struct with additional fields that extends the original struct so that both can be
+  used in tandem.
+- New Struct: Create a new struct with additional fields and migrate existing data to the new struct during the upgrade
+  process. _Example:_
+
+```solidity
+
+// Version 1
+contract V1 {
+    struct Data {
+        uint value;
+    }
+
+    Data public data;
+}
+
+// Version 2
+contract V2 is V1 {
+    struct NewData {
+        uint value;
+        uint newValue;
+    }
+
+    NewData public newData;
+
+    function migrateData() public {
+        newData = NewData(data.value, 0);
+    }
+}
+```
+
+## Deployment of Upgrades via the Proxy
+
+- Deploy the new (modified) implementation contract If new state variables that need initialization were added then:
+- Prepare an `upgradeToAndCall` transaction from the multisig admin account which is parameterized with the address of
+  the new implementation contract and an internal call to invoke initializeV[X] with the new state variables data. Else:
+- Prepare an `upgradeTo` transaction from the multisig admin account parameterized with the address of the new
+  implementation contract: Then:
+- Broadcast the transaction

From 09f3732ded62943bbcbb6afa6a35f07f007b858d Mon Sep 17 00:00:00 2001
From: Alysia Huggins <hi@alysia.tech>
Date: Tue, 20 Aug 2024 09:26:52 -0400
Subject: [PATCH 4/7] adding more context to the docs with example that include
 the LC contract

---
 doc/smart-contract-upgrades.md | 106 +++++++++++++++++++++++++++++----
 1 file changed, 96 insertions(+), 10 deletions(-)

diff --git a/doc/smart-contract-upgrades.md b/doc/smart-contract-upgrades.md
index f4aaedaf0c..fcc671af4a 100644
--- a/doc/smart-contract-upgrades.md
+++ b/doc/smart-contract-upgrades.md
@@ -5,21 +5,96 @@ intermediary that can delegate calls to other contracts, called implementation c
 smart contract to be upgraded while keeping the same address and state. This is made possible by two functions in
 Solidity:
 
-- `delegatecall`: contractA can delegatecall contractB but execute the logic within the context of contractA. This means
+- `delegatecall`: ContractA can delegatecall ContractB but execute the logic within the context of ContractA. This means
   that the called contract's code is executed with the storage, caller, and balance of the calling contract.
-- `fallback`: executed when a function doesn't exist. This is often used in proxy contracts to catch all function calls
+- `fallback`: executed when a function does not exist. This is often used in proxy contracts to catch all function calls
   and delegate them to the implementation contract.
 
 OpenZeppelin's [implementation](https://docs.openzeppelin.com/contracts/4.x/api/proxy#UUPSUpgradeable) for the UUPS
 proxy pattern was used.
 
+At the time of writing, we have two upgradable smart contracts:
+
+- LightClient.sol
+- FeeContract.sol
+
+They both use OpenZeppelin's [implementation](https://docs.openzeppelin.com/contracts/4.x/api/proxy#UUPSUpgradeable) for
+the UUPS proxy pattern and is deployed using OpenZeppelin Upgrades library.
+
 ## Writing Upgradable Smart Contracts
 
-This is written with developers in mind to explain the process for upgrading contracts within our repo to ensure safe
+This document is intended for developers to explain the process for upgrading contracts within our repo to ensure safe
 upgrades.
 
 ### Upgrading via Contract Inheritance
 
+When writing the upgrade for upgradable contracts such as the light client contract and the fee contract, the new
+implementation inherits from the latest implementation contract. This new implementation only implements the new or
+modified features or new variables. If any variables have to be initialized, the new implementation contract should
+create a `initializeVx` function, where x is the version number. Here's an example:
+
+The original `LightClient` contract looks like this:
+
+```solidity
+contract LightClient is Initializable, OwnableUpgradeable, UUPSUpgradeable {
+  //...
+}
+```
+
+The 2nd implementation, LightClientV2, that adds a new variable is implemented like this:
+
+```solidity
+contract LightClientV2 is LightClient {
+    uint256 public newField;
+
+    /// @notice this field is used to check initialized versions so that one can ensure that the
+    /// initialization only happens once
+    uint8 internal _initializedVersion;
+
+    /// @notice Initialize v2
+    /// @param _newField   New field amount
+    function initializeV2(uint256 _newField) external {
+        require(_initializedVersion == 0);
+        newField = _newField;
+        _initializedVersion = 2;
+    }
+
+    function newFinalizedState(
+        LightClientState memory newState,
+        IPlonkVerifier.PlonkProof memory proof
+    ) external virtual override {
+      //...
+    }
+
+    //...
+}
+```
+
+The 3rd implementation will now inherit from LightClientV2, ensure that functions are marked as virtual in previous
+versions of the contract if you want to continue overriding it in child contracts
+
+```solidity
+contract LightClientV3 is LightClientV2 {
+    uint256 public anotherField;
+
+    /// @param _newField   New field amount
+    function initializeV3(uint256 _newField) external {
+        require(_initializedVersion == 2, "already initialized");
+        anotherField = _newField;
+        _initializedVersion = 3;
+    }
+
+    function newFinalizedState(
+        LightClientState memory newState,
+        IPlonkVerifier.PlonkProof memory proof
+    ) external virtual override {
+      //...
+    }
+
+    //...
+}
+```
+
 Solidity supports inheritance which means that a function call always executes the function of the same name (and
 parameter types) in the most derived contract in the inheritance hierarchy.
 
@@ -56,7 +131,7 @@ parameter types) in the most derived contract in the inheritance hierarchy.
 Thanks to C3 linearization, the order in which you inherit from the previous version of the contract dictates the
 storage layout so be sure to be mindful when declaring new variables.
 
-Criteria:
+Criteria for New State Variables:
 
 - New variable names must be used otherwise it leads to a compilation error due to shadowing.
 - New variables added _must_ be declared as type `internal` or `public`, `private` can never be used.
@@ -115,11 +190,12 @@ contract V2 is V1 {
 
 #### Modifying Existing Logic (Function Overriding)
 
-The logic in exiting functions can be overridden but be sure not to introduce breaking changes.
+The logic in existing functions can be overridden but be sure not to introduce breaking changes.
 
-Criteria:
+Criteria for Overriding Functions::
 
-- a function must be `virtual` to be able to be overridden in a derived contract.
+- a function must be marked as `virtual` in the base contract so that it can be eligible to be overridden in a derived
+  contract.
 - a function with `private` visibility cannot be overridden or inherited.
 - Public state variables can override `external` functions if the parameter and return types of the function matches the
   getter function of the variable:
@@ -131,11 +207,15 @@ _Example:_
 pragma solidity >=0.6.0 <0.9.0;
 
 contract A {
-    function f() external view virtual returns(uint) { return 5; }
+     function getValue() public view virtual returns (uint) {
+        return 1;
+    }
 }
 
 contract B is A {
-    uint public override f;
+    function getValue() public view virtual override returns (uint) {
+        return 2;
+    }
 }
 ```
 
@@ -143,6 +223,12 @@ contract B is A {
 - The mutability may be changed to a more strict one following the order: `nonpayable` can be overridden by `view` and
   `pure`. `view` can be overridden by `pure`. `payable` is an exception and cannot be changed to any other mutability.
 
+##### Avoiding breaking changes when overriding functions
+
+- When overriding a function, consider whether it’s necessary to call the parent contract’s implementation using
+  `super`. This can preserve the original logic as you extend with new functionality.
+- Ensure that any side effects are handled appropriately, e.g. event emissions, state variable modifications etc
+
 ### Upgrades that become complicated through inheritance
 
 #### Upgrading Structs
@@ -186,7 +272,7 @@ contract V2 is V1 {
 
 ## Deployment of Upgrades via the Proxy
 
-- Deploy the new (modified) implementation contract If new state variables that need initialization were added then:
+- Deploy the new (modified) implementation contract. If new state variables that need initialization were added then:
 - Prepare an `upgradeToAndCall` transaction from the multisig admin account which is parameterized with the address of
   the new implementation contract and an internal call to invoke initializeV[X] with the new state variables data. Else:
 - Prepare an `upgradeTo` transaction from the multisig admin account parameterized with the address of the new

From 105e26ae0990735738032a0c5e4264e1655d3ea3 Mon Sep 17 00:00:00 2001
From: Alysia Huggins <hi@alysia.tech>
Date: Tue, 20 Aug 2024 09:56:12 -0400
Subject: [PATCH 5/7] add more deployment notes

---
 doc/smart-contract-upgrades.md | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/doc/smart-contract-upgrades.md b/doc/smart-contract-upgrades.md
index fcc671af4a..44ae6d6ead 100644
--- a/doc/smart-contract-upgrades.md
+++ b/doc/smart-contract-upgrades.md
@@ -270,7 +270,21 @@ contract V2 is V1 {
 }
 ```
 
-## Deployment of Upgrades via the Proxy
+## Deploying the Upgradable Contract
+
+The [OpenZeppelin Upgrade plugins](https://docs.openzeppelin.com/upgrades-plugins/1.x/) are used to assist with upgrades
+so that we can ensure that the base and future implementation contracts are upgrade safe. For example, the
+implementation contract:
+
+- cannot have a constructor.
+- should not use the `selfdestruct` or `delegatecall`.
+- should not initialize state variables.
+- should not make state variables `immutable`.
+
+You can learn more about deploying and upgrading upgradable contracts with OpenZeppelin and Foundry
+[here](https://docs.openzeppelin.com/upgrades-plugins/1.x/foundry-upgrades).
+
+### Upgrading via the Proxy
 
 - Deploy the new (modified) implementation contract. If new state variables that need initialization were added then:
 - Prepare an `upgradeToAndCall` transaction from the multisig admin account which is parameterized with the address of
@@ -278,3 +292,5 @@ contract V2 is V1 {
 - Prepare an `upgradeTo` transaction from the multisig admin account parameterized with the address of the new
   implementation contract: Then:
 - Broadcast the transaction
+
+For more details, check out this [README](../contracts/script/README.md).

From 50dec6911459403d8c662ae7ac331fb00730888e Mon Sep 17 00:00:00 2001
From: Alysia Huggins <hi@alysia.tech>
Date: Tue, 20 Aug 2024 11:17:28 -0400
Subject: [PATCH 6/7] use the reinitializer modifier to safeguard against more
 that one re-initialization in new implementations of an upgradable contract

---
 contracts/test/LightClientUpgradeToVx.t.sol | 91 +++++++++++++++++++++
 contracts/test/LightClientV2.sol            | 14 ++--
 contracts/test/LightClientV3.sol            | 12 ++-
 doc/smart-contract-upgrades.md              | 38 +++++----
 4 files changed, 130 insertions(+), 25 deletions(-)

diff --git a/contracts/test/LightClientUpgradeToVx.t.sol b/contracts/test/LightClientUpgradeToVx.t.sol
index e6525fe944..04890520f9 100644
--- a/contracts/test/LightClientUpgradeToVx.t.sol
+++ b/contracts/test/LightClientUpgradeToVx.t.sol
@@ -11,6 +11,7 @@ import { UpgradeLightClientScript } from "./UpgradeLightClientToV2.s.sol";
 import { UpgradeLightClientScript as ULCV3 } from "./UpgradeLightClientToV3.s.sol";
 import { OwnableUpgradeable } from
     "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
+import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
 
 contract LightClientUpgradeToVxTest is Test {
     LCV1 public lcV1Proxy;
@@ -81,6 +82,43 @@ contract LightClientUpgradeToVxTest is Test {
     }
 
     // that the data remains the same after upgrading the implementation
+    function testExpectRevertUpgradeSameDataV1ToV2ReinitializeTwice() public {
+        // Upgrade LightClient and check that the genesis state is not changed and that the new
+        // field
+        // of the upgraded contract is set to 0
+        uint256 myNewField = 123;
+        lcV2Proxy = LCV2(upgraderV2.run(proxy, myNewField, admin));
+
+        assertEq(lcV2Proxy.newField(), myNewField);
+        assertEq(lcV2Proxy.blocksPerEpoch(), 10);
+        assertEq(lcV2Proxy.currentEpoch(), 0);
+
+        LCV1.LightClientState memory expectedLightClientState = LCV1.LightClientState(
+            stateV1.viewNum,
+            stateV1.blockHeight,
+            stateV1.blockCommRoot,
+            stateV1.feeLedgerComm,
+            stateV1.stakeTableBlsKeyComm,
+            stateV1.stakeTableSchnorrKeyComm,
+            stateV1.stakeTableAmountComm,
+            stateV1.threshold
+        );
+
+        LCV2.ExtendedLightClientState memory expectedExtendedLightClientState =
+            LCV2.ExtendedLightClientState(0);
+
+        assertEq(abi.encode(lcV2Proxy.getFinalizedState()), abi.encode(expectedLightClientState));
+        assertEq(
+            abi.encode(lcV2Proxy.getExtendedFinalizedState()),
+            abi.encode(expectedExtendedLightClientState)
+        );
+
+        // expect a revert when we try to reinitialize
+        vm.expectRevert(Initializable.InvalidInitialization.selector);
+        lcV2Proxy.initializeV2(5);
+    }
+
+    // test that the data remains the same after upgrading the implementation
     function testUpgradeSameDataV2ToV3() public {
         // Upgrade LightClient and check that the genesis state is not changed and that the new
         // field
@@ -128,6 +166,59 @@ contract LightClientUpgradeToVxTest is Test {
         );
     }
 
+    // test that the tx reverts if we try to reinitialize more than once
+    function testExpectRevertUpgradeSameDataV2ToV3ReinitializeTwice() public {
+        // Upgrade LightClient and check that the genesis state is not changed and that the new
+        // field
+        // of the upgraded contract is set to 0
+        uint256 myNewField = 123;
+        uint256 myNewFieldV3 = 456;
+        // upgrade to v2 first
+        lcV2Proxy = LCV2(upgraderV2.run(proxy, myNewField, admin));
+
+        assertEq(lcV2Proxy.newField(), myNewField);
+        assertEq(lcV2Proxy.blocksPerEpoch(), 10);
+        assertEq(lcV2Proxy.currentEpoch(), 0);
+
+        LCV1.LightClientState memory expectedLightClientState = LCV1.LightClientState(
+            stateV1.viewNum,
+            stateV1.blockHeight,
+            stateV1.blockCommRoot,
+            stateV1.feeLedgerComm,
+            stateV1.stakeTableBlsKeyComm,
+            stateV1.stakeTableSchnorrKeyComm,
+            stateV1.stakeTableAmountComm,
+            stateV1.threshold
+        );
+
+        LCV2.ExtendedLightClientState memory expectedExtendedLightClientState =
+            LCV2.ExtendedLightClientState(0);
+
+        assertEq(abi.encode(lcV2Proxy.getFinalizedState()), abi.encode(expectedLightClientState));
+        assertEq(
+            abi.encode(lcV2Proxy.getExtendedFinalizedState()),
+            abi.encode(expectedExtendedLightClientState)
+        );
+
+        // upgrade to v3
+        lcV3Proxy = LCV3(upgraderV3.run(proxy, myNewFieldV3, admin));
+
+        assertEq(lcV3Proxy.newField(), myNewField);
+        assertEq(lcV3Proxy.anotherField(), myNewFieldV3);
+        assertEq(lcV3Proxy.blocksPerEpoch(), 10);
+        assertEq(lcV3Proxy.currentEpoch(), 0);
+
+        assertEq(abi.encode(lcV3Proxy.getFinalizedState()), abi.encode(expectedLightClientState));
+        assertEq(
+            abi.encode(lcV3Proxy.getExtendedFinalizedState()),
+            abi.encode(expectedExtendedLightClientState)
+        );
+
+        // expect a revert when we try to reinitialize
+        vm.expectRevert(Initializable.InvalidInitialization.selector);
+        lcV3Proxy.initializeV3(6);
+    }
+
     // check that the proxy address remains the same
     function testUpgradesSameProxyAddress() public {
         (uint8 major, uint8 minor, uint8 patch) = lcV1Proxy.getVersion();
diff --git a/contracts/test/LightClientV2.sol b/contracts/test/LightClientV2.sol
index 5f64210917..75fe943aa3 100644
--- a/contracts/test/LightClientV2.sol
+++ b/contracts/test/LightClientV2.sol
@@ -17,10 +17,6 @@ contract LightClientV2 is LightClient {
     /// that has the new struct type, or put the struct inside a map.
     uint256 public newField;
 
-    /// @notice this field is used to check initialized versions so that one can ensure that the
-    /// initialization only happens once
-    uint8 internal _initializedVersion;
-
     struct ExtendedLightClientState {
         uint256 extraField;
     }
@@ -30,10 +26,14 @@ contract LightClientV2 is LightClient {
 
     /// @notice Initialize v2
     /// @param _newField   New field amount
-    function initializeV2(uint256 _newField) external {
-        require(_initializedVersion == 0);
+    /// @dev the reinitializer modifier is used to reinitialize new versions of a contract and
+    /// is called at most once. The modifier has an uint64 argument which indicates the next
+    /// contract version.
+    /// when the base implementation contract is initialized for the first time, the _initialized
+    /// version
+    /// is set to 1. Since this is the 2nd implementation, the next contract version is 2.
+    function initializeV2(uint256 _newField) external reinitializer(2) {
         newField = _newField;
-        _initializedVersion = 2;
     }
 
     /// @notice Use this to get the implementation contract version
diff --git a/contracts/test/LightClientV3.sol b/contracts/test/LightClientV3.sol
index eead886633..ffeabc54ce 100644
--- a/contracts/test/LightClientV3.sol
+++ b/contracts/test/LightClientV3.sol
@@ -4,17 +4,23 @@ pragma solidity ^0.8.0;
 pragma experimental ABIEncoderV2;
 
 import { LightClientV2 } from "./LightClientV2.sol";
+
 /// @notice A light client for HotShot consensus. Keeping track of its finalized states in safe,
 /// authenticated ways.
 
 contract LightClientV3 is LightClientV2 {
     uint256 public anotherField;
 
+    /// @notice Initialize v3
     /// @param _newField   New field amount
-    function initializeV3(uint256 _newField) external {
-        require(_initializedVersion == 2, "already initialized");
+    /// @dev the reinitializer modifier is used to reinitialize new versions of a contract and
+    /// is called at most once. The modifier has an uint64 argument which indicates the next
+    /// contract version.
+    /// when the base implementation contract is initialized for the first time, the _initialized
+    /// version
+    /// is set to 1. Since this is the 3rd implementation, the next contract version is 3.
+    function initializeV3(uint256 _newField) external reinitializer(3) {
         anotherField = _newField;
-        _initializedVersion = 3;
     }
 
     /// @notice Use this to get the implementation contract version
diff --git a/doc/smart-contract-upgrades.md b/doc/smart-contract-upgrades.md
index 44ae6d6ead..4558073924 100644
--- a/doc/smart-contract-upgrades.md
+++ b/doc/smart-contract-upgrades.md
@@ -31,7 +31,19 @@ upgrades.
 When writing the upgrade for upgradable contracts such as the light client contract and the fee contract, the new
 implementation inherits from the latest implementation contract. This new implementation only implements the new or
 modified features or new variables. If any variables have to be initialized, the new implementation contract should
-create a `initializeVx` function, where x is the version number. Here's an example:
+create a `initializeVx` function, where x is the version number.
+
+The `reinitializer` modifier, provided by the
+[`Initializable`](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/proxy/utils/Initializable.sol)
+library, ensures that the new implementation is reinitialized at most once. The modifier takes a `uint64` argument,
+which specifies the contract's version.
+
+When the base implementation contract is initialized, its `_initializedVersion` is set to 1. The `reinitializer`
+modifier ensures that the initialization logic is only executed if the \_initializedVersion is less than the specified
+version number. Therefore, when upgrading a contract, the `reinitializer` modifier should be set to the previous
+`_initializedVersion` plus 1 to indicate the new version.
+
+Here's an example:
 
 The original `LightClient` contract looks like this:
 
@@ -47,16 +59,14 @@ The 2nd implementation, LightClientV2, that adds a new variable is implemented l
 contract LightClientV2 is LightClient {
     uint256 public newField;
 
-    /// @notice this field is used to check initialized versions so that one can ensure that the
-    /// initialization only happens once
-    uint8 internal _initializedVersion;
-
     /// @notice Initialize v2
     /// @param _newField   New field amount
-    function initializeV2(uint256 _newField) external {
-        require(_initializedVersion == 0);
+    /// @dev the reinitializer modifier is used to reinitialize new versions of a contract and
+    /// is called at most once. The modifier has an uint64 argument which indicates the next contract version.
+    /// when the base implementation contract is initialized for the first time, the _initialized version
+    /// is set to 1. Since this is the 2nd implementation, the next contract version is 2.
+    function initializeV2(uint256 _newField) external reinitializer(2){
         newField = _newField;
-        _initializedVersion = 2;
     }
 
     function newFinalizedState(
@@ -70,18 +80,16 @@ contract LightClientV2 is LightClient {
 }
 ```
 
-The 3rd implementation will now inherit from LightClientV2, ensure that functions are marked as virtual in previous
-versions of the contract if you want to continue overriding it in child contracts
+The third implementation, LightClientV3, will inherit from LightClientV2. Ensure that functions are marked as `virtual`
+in previous versions of the contract if you intend to override them in child contracts.
 
 ```solidity
 contract LightClientV3 is LightClientV2 {
     uint256 public anotherField;
 
     /// @param _newField   New field amount
-    function initializeV3(uint256 _newField) external {
-        require(_initializedVersion == 2, "already initialized");
+    function initializeV3(uint256 _newField) external reinitializer(3){
         anotherField = _newField;
-        _initializedVersion = 3;
     }
 
     function newFinalizedState(
@@ -136,7 +144,7 @@ Criteria for New State Variables:
 - New variable names must be used otherwise it leads to a compilation error due to shadowing.
 - New variables added _must_ be declared as type `internal` or `public`, `private` can never be used.
 - If the new variable needs to be initialized, then the initialize function with a separate initV[X] function should be
-  added. The initV[X] function allows the contract to initialize only the new variables added in the new contract.
+  added. The initializeVx function allows the contract to initialize only the new variables added in the new contract.
 
 _Example:_
 
@@ -288,7 +296,7 @@ You can learn more about deploying and upgrading upgradable contracts with OpenZ
 
 - Deploy the new (modified) implementation contract. If new state variables that need initialization were added then:
 - Prepare an `upgradeToAndCall` transaction from the multisig admin account which is parameterized with the address of
-  the new implementation contract and an internal call to invoke initializeV[X] with the new state variables data. Else:
+  the new implementation contract and an internal call to invoke initializeVx with the new state variables data. Else:
 - Prepare an `upgradeTo` transaction from the multisig admin account parameterized with the address of the new
   implementation contract: Then:
 - Broadcast the transaction

From 7c3569c83e067dbb49c1521f67895de69bffed8c Mon Sep 17 00:00:00 2001
From: Alysia Huggins <hi@alysia.tech>
Date: Tue, 20 Aug 2024 12:39:04 -0400
Subject: [PATCH 7/7] improving comments

---
 contracts/test/LightClientUpgradeToVx.t.sol | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contracts/test/LightClientUpgradeToVx.t.sol b/contracts/test/LightClientUpgradeToVx.t.sol
index 04890520f9..0dc6c56e25 100644
--- a/contracts/test/LightClientUpgradeToVx.t.sol
+++ b/contracts/test/LightClientUpgradeToVx.t.sol
@@ -48,7 +48,7 @@ contract LightClientUpgradeToVxTest is Test {
         assertEq(lcV1Proxy.frozenThreshold(), stateV1.threshold);
     }
 
-    // that the data remains the same after upgrading the implementation
+    // test that the data remains the same after upgrading the implementation
     function testUpgradeSameDataV1ToV2() public {
         // Upgrade LightClient and check that the genesis state is not changed and that the new
         // field
@@ -81,7 +81,7 @@ contract LightClientUpgradeToVxTest is Test {
         );
     }
 
-    // that the data remains the same after upgrading the implementation
+    // test that the data remains the same after upgrading the implementation
     function testExpectRevertUpgradeSameDataV1ToV2ReinitializeTwice() public {
         // Upgrade LightClient and check that the genesis state is not changed and that the new
         // field