Merge pull request #1 from filecoin-project/biglep/reviewing-initial-code

Small contract cleanups when reading through the code and configuration

Author: Kubuxu

README.md

@@ -10,6 +10,12 @@ For the full motivation and detailed design, please refer to the [FIP discussion
 - **Controlled Update Mechanism:** Requires consensus from Lotus, Forest, and Venus teams.
 - **Finalization and Self-Disabling:** Automatically disables after six months if not activated.
 
+## Development
+1. Install yarn (e.g., `brew install yarn`)
+2. Initialize yarn in the repo: `yarn`
+3. Set `PRIVATE_KEY` environment variable (e.g., `export PRIVATE_KEY='0000000000000000000000000000000000000000000000000000000000000000'`)
+4. Run tests: `yarn hardhat test`
+
 ## License
 
 This project is licensed under the MIT License - see the [LICENSE](LICENSE) file for details.

contracts/F3Parameters.sol

@@ -4,17 +4,21 @@ pragma solidity 0.8.23;
 /// @title F3Parameters
 /// @dev This contract manages parameters for the F3 system, allowing changes until the activation epoch.
 ///      It ensures a review period for the community before activation.
+/// @notice https://github.com/filecoin-project/FIPs/discussions/1102
 
 import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
 
 contract F3Parameters is Ownable {
     /// @dev The expiry timestamp after which updates are not allowed.
+    ///      Note: updates also aren't allowed after the _activationEpoch.
     uint64 private immutable _expiry;
 
-    /// @dev The block number at which the parameters become active.
+    /// @dev The block number at which the parameters become active,
+    ///      and no further updates to _manifestData are accepted.
     uint64 private _activationEpoch;
 
     /// @dev The data associated with the manifest for the parameters.
+    ///      It is up to consumers (e.g., Lotus) to parse this data and be defensive in what they allowed be mutated as a result.
     bytes private _manifesetData;
 
     /// @notice Initializes the contract with the owner and expiry block number.
@@ -81,10 +85,10 @@ contract F3Parameters is Ownable {
             revert UpdateAlreadyActive();
         }
         if (activationEpoch <= block.number) {
-            revert UpdateActivationEpochInvalid(uint64(block.number), activationEpoch, "before current block");
+            revert UpdateActivationEpochInvalid(uint64(block.number), activationEpoch, "activationEpoch is before current block");
         }
         if (uint128(activationEpoch - block.number) < MIN_ACTIVATION_HEADROOM_BLOCKS) {
-            revert UpdateActivationEpochInvalid(uint64(block.number), activationEpoch, "based on block time");
+            revert UpdateActivationEpochInvalid(uint64(block.number), activationEpoch, "activationEpoch is within minActivationHeadroomBlocks from the current block");
         }
 
         _activationEpoch = activationEpoch;

ignition/modules/F3Parameters.ts

@@ -1,12 +1,12 @@
 
 import { buildModule } from "@nomicfoundation/hardhat-ignition/modules";
 
-const JAN_1ST_2030 = 1893456000;
+const JAN_1ST_2030_EPOCH_SECONDS = Math.floor(new Date('2030-01-01T00:00:00Z').getTime() / 1000);
 const ONE_GWEI = 1_000_000_000n;
 
 export default buildModule("F3ParametersModule", (m) => {
   const owner = m.getParameter("owner", m.getAccount(0));
-  const expiration = m.getParameter("expirationTime", JAN_1ST_2030);
+  const expiration = m.getParameter("expirationTime", JAN_1ST_2030_EPOCH_SECONDS);
 
   const f3params = m.contract("F3Parameters", [owner, expiration]);
   return { f3params };

test/F3Parameters.js

@@ -16,10 +16,10 @@ describe("F3Parameters", function () {
     // Contracts are deployed using the first signer/account by default
     const [owner, otherAccount] = await ethers.getSigners();
 
-    const F3Param = await ethers.getContractFactory("F3Parameters");
-    const f3param = await F3Param.deploy(owner, expireTime);
+    const f3ParamContractFactory = await ethers.getContractFactory("F3Parameters");
+    const f3ParamContract = await f3ParamContractFactory.deploy(owner, expireTime);
 
-    return { f3param, expireTime, owner, otherAccount };
+    return { f3param: f3ParamContract, expireTime, owner, otherAccount };
   }
 
   describe("Deployment", function () {
@@ -59,44 +59,29 @@ describe("F3Parameters", function () {
       expect(data).to.equal(manifestData);
     });
 
-  it("Should revert if activation epoch is less than minActivationHeadroomBlocks ahead of current block", async function () {
-    const { f3param, owner } = await loadFixture(deployOneYearExpireFixture);
-    const currentBlockNumber = BigInt(await ethers.provider.getBlockNumber());
-    const minActivationHeadroomBlocks = await f3param.getMinActivationHeadroomBlocks();
-    const newActivationEpoch = currentBlockNumber + minActivationHeadroomBlocks - BigInt(1);
-    const manifestData = "0x123456";
-
-    await expect(
-      f3param.connect(owner).updateActivationInformation(newActivationEpoch, manifestData)
-    ).to.be.revertedWithCustomError(f3param, "UpdateActivationEpochInvalid")
-      .withArgs(anyValue, newActivationEpoch, "based on block time");
-  });
-
-    it("Should revert if update is attempted after expiry", async function () {
+    it("Should revert if activation epoch is set to a past block", async function () {
       const { f3param, owner } = await loadFixture(deployOneYearExpireFixture);
-      const currentBlockNumber = BigInt(await ethers.provider.getBlockNumber());
-      const minActivationHeadroomBlocks = await f3param.getMinActivationHeadroomBlocks();
-      const newActivationEpoch = currentBlockNumber + minActivationHeadroomBlocks + BigInt(1);
+      await mine(10)
+      const pastBlock = BigInt(await ethers.provider.getBlockNumber() - 2);
       const manifestData = "0x123456";
 
-      const expiryTime = BigInt(await f3param.expiresAt());
-      await time.increaseTo(expiryTime + BigInt(100));
-
       await expect(
-        f3param.connect(owner).updateActivationInformation(newActivationEpoch, manifestData)
-      ).to.be.revertedWithCustomError(f3param, "UpdateExpired");
+        f3param.connect(owner).updateActivationInformation(pastBlock, manifestData)
+      ).to.be.revertedWithCustomError(f3param, "UpdateActivationEpochInvalid")
+        .withArgs(anyValue, pastBlock, "activationEpoch is before current block");
     });
 
-    it("Should revert if activation epoch is set to a past block", async function () {
+    it("Should revert if activation epoch is less than minActivationHeadroomBlocks ahead of current block", async function () {
       const { f3param, owner } = await loadFixture(deployOneYearExpireFixture);
-      await mine(10)
-      const pastBlock = BigInt(await ethers.provider.getBlockNumber() - 2);
+      const currentBlockNumber = BigInt(await ethers.provider.getBlockNumber());
+      const minActivationHeadroomBlocks = await f3param.getMinActivationHeadroomBlocks();
+      const newActivationEpoch = currentBlockNumber + minActivationHeadroomBlocks - BigInt(1);
       const manifestData = "0x123456";
 
       await expect(
-        f3param.connect(owner).updateActivationInformation(pastBlock, manifestData)
+        f3param.connect(owner).updateActivationInformation(newActivationEpoch, manifestData)
       ).to.be.revertedWithCustomError(f3param, "UpdateActivationEpochInvalid")
-        .withArgs(anyValue, pastBlock, "before current block");
+        .withArgs(anyValue, newActivationEpoch, "activationEpoch is within minActivationHeadroomBlocks from the current block")
     });
 
     it("Should revert if update is attempted after activation", async function () {
@@ -115,6 +100,21 @@ describe("F3Parameters", function () {
       ).to.be.revertedWithCustomError(f3param, "UpdateAlreadyActive");
     });
 
+    it("Should revert if update is attempted after expiry", async function () {
+      const { f3param, owner } = await loadFixture(deployOneYearExpireFixture);
+      const currentBlockNumber = BigInt(await ethers.provider.getBlockNumber());
+      const minActivationHeadroomBlocks = await f3param.getMinActivationHeadroomBlocks();
+      const newActivationEpoch = currentBlockNumber + minActivationHeadroomBlocks + BigInt(1);
+      const manifestData = "0x123456";
+
+      const expiryTime = BigInt(await f3param.expiresAt());
+      await time.increaseTo(expiryTime + BigInt(100));
+
+      await expect(
+        f3param.connect(owner).updateActivationInformation(newActivationEpoch, manifestData)
+      ).to.be.revertedWithCustomError(f3param, "UpdateExpired");
+    });
+
     it("Should revert if non-owner attempts to updateActivationInformation", async function() {
       const { f3param, otherAccount } = await loadFixture(deployOneYearExpireFixture);
       const currentBlockNumber = BigInt(await ethers.provider.getBlockNumber());