Merge pull request #6 from ethereumfollowprotocol/review

review

Author: 0xthrpw

CLAUDE.md

@@ -0,0 +1,93 @@
+# CLAUDE.md
+
+This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository.
+
+## Project Overview
+
+The Ethereum Follow Protocol (EFP) ListRecordsV2 repository contains updated smart contracts for EFP's list minting and record management system. This is a Solidity project using the Foundry framework for smart contract development, with TypeScript tooling for ABI generation and testing.
+
+## Common Commands
+
+### Build and Development
+- `bun run build` - Compiles smart contracts using Foundry
+- `bun run test` - Runs all tests using Foundry with forked environment (`--fork-url ${ETH_RPC_URL} -vv`)
+- `bun run coverage` - Generates test coverage report and lcov file
+- `forge test --match-path test/specific.t.sol` - Run specific test file
+- `forge test --match-test testFunctionName` - Run specific test function
+
+### Deployment
+- `bun run deploy` - Deploy to mainnet using `${ETH_RPC_URL}`
+- `bun run deploy:testnet` - Deploy to testnet using `${TESTNET_RPC_URL}`  
+- `bun run deploy:local` - Deploy to local anvil instance
+- `bun run testnet` - Start local anvil fork on chain ID 8453
+
+### ABI Generation
+- `bun wagmi generate` - Generate TypeScript ABIs, actions, and React hooks from compiled contracts
+
+## Architecture Overview
+
+### Core Contracts
+
+**EFPListRecordsV2** (`src/EFPListRecordsV2.sol`): The main list records contract that stores and manages list operations and metadata. Key features:
+- **Slot-based access control**: Uses first 20 bytes of slot to match message sender address, preventing front-running of slot claims
+- **List operations storage**: Maintains a history of operations applied to each list
+- **Metadata management**: Key-value store for list metadata with manager-only access
+- **Manager system**: Claims management through `onlyListManager` modifier
+
+**EFPListMinterV2** (`src/EFPListMinterV2.sol`): Handles minting of new lists with improved validation. Key features:
+- **Chain ID validation**: Ensures list records are stored on the intended chain when using native storage
+- **List storage location decoding**: Validates encoded storage locations before minting
+- **Primary list assignment**: Sets default lists for accounts via account metadata contract
+
+### Contract Hierarchy
+
+```
+ListMetadata (abstract)
+├── Manages metadata key-value pairs
+├── Implements manager claiming and validation
+└── Extends Pausable, Ownable
+
+ListRecordsV2 (abstract)
+├── Extends ListMetadata
+├── Manages list operations history
+└── Provides batch operations
+
+EFPListRecordsV2 (concrete)
+└── Final implementation with ENS reverse claiming
+```
+
+### Key Dependencies
+
+- **External contracts**: Interfaces to EFPListRegistry, EFPAccountMetadata
+- **OpenZeppelin**: Uses Ownable, Pausable, Context
+- **ENS integration**: ENSReverseClaimer for reverse name resolution
+- **Foundry libraries**: forge-std for testing and deployment
+
+### Directory Structure
+
+- `src/` - Main contract source files
+- `src/interfaces/` - Contract interfaces
+- `src/lib/` - Shared libraries (ENSReverseClaimer)
+- `script/` - Deployment scripts and utilities
+- `test/` - Foundry test files
+- `out/` - Compiled contract artifacts
+- `lib/` - Git submodules (forge-std, openzeppelin-contracts)
+
+### Testing Approach
+
+Tests use Foundry's testing framework with mainnet forking. Test files follow the pattern `*.t.sol` and are located in the `test/` directory. Coverage reports are generated using `forge coverage` with lcov output.
+
+### Environment Setup
+
+Requires `.env` file with:
+- `PRIVATE_KEY` - Deployment private key
+- `ETH_RPC_URL` - Mainnet RPC endpoint
+- `TESTNET_RPC_URL` - Testnet RPC endpoint  
+- `ETHERSCAN_API_KEY` - For contract verification
+
+### Important Security Features
+
+1. **Slot validation**: EFPListRecordsV2 validates that the first 20 bytes of a slot match the caller's address
+2. **Chain ID checks**: EFPListMinterV2 verifies chain ID matches current chain for native storage
+3. **Manager-only operations**: All list modifications require manager privileges
+4. **Pausable contracts**: Admin can pause operations in emergency situations

package.json

@@ -6,7 +6,7 @@
     "type": "module",
     "scripts": {
         "build": "forge build",
-        "test": "forge test --fork-url ${ETH_RPC_URL} -vv",
+        "test": "forge test --match-path test/EFPListRecords.t.sol --fork-url ${ETH_RPC_URL} -vv && forge test --match-path test/EFPListMinter.t.sol  --fork-url ${ETH_RPC_URL} -vv ",
         "coverage": "forge coverage --fork-url ${ETH_RPC_URL} --report lcov && lcov-viewer lcov -o ./coverage ./lcov.info",
         "deploy": "forge script script/deploy.s.sol --fork-url ${ETH_RPC_URL} --broadcast --private-key ${PRIVATE_KEY}",
         "deploy:testnet": "forge script script/deploy.s.sol --fork-url ${TESTNET_RPC_URL} --broadcast --private-key ${PRIVATE_KEY}",

src/EFPListMinterV2.sol

@@ -25,10 +25,12 @@ interface IEFPListRegistryERC721 is IEFPListRegistry {
  * EFP List metadata.
  */
 contract EFPListMinterV2 is ENSReverseClaimer, Pausable {
-  IEFPListRegistryERC721 public registry;
-  IEFPAccountMetadata public accountMetadata;
+  IEFPListRegistryERC721 public immutable registry;
+  IEFPAccountMetadata public immutable accountMetadata;
   IEFPListRecords public listRecordsL1;
 
+  event Minted(string method, address indexed to, bytes listStorageLocation);
+
   constructor(address _registryAddress, address _accountMetadataAddress, address _listRecordsL1) {
     registry = IEFPListRegistryERC721(_registryAddress);
     accountMetadata = IEFPAccountMetadata(_accountMetadataAddress);
@@ -70,25 +72,48 @@ contract EFPListMinterV2 is ENSReverseClaimer, Pausable {
   /////////////////////////////////////////////////////////////////////////////
 
   /**
-   * @dev Decode a list storage location with no metadata.
+   * @dev Decode a list storage location 
+   * @param listStorageLocation The storage location of the list.
+   * @return chain The chain ID of the list.
+   * @return slot The slot of the list.
+   * @return contractAddress The contract address of the list.
+   */
+  function decodeLSL(bytes calldata listStorageLocation) public pure returns (uint256, uint256, address) {
+    address contractAddress = _bytesToAddress(listStorageLocation, 34);
+    uint256 chain = _bytesToUint(listStorageLocation, 2);
+    uint256 slot = _bytesToUint(listStorageLocation, 54);
+    return (chain, slot, contractAddress);
+  }
+
+  /**
+   * @dev Encode a list storage location. Note this has a fixed version and type.
+   * @param chain The chain ID of the list.
+   * @param slot The slot of the list.
+   * @param contractAddress The contract address of the list.
+   * @return The encoded list storage location.
+   */
+  function encodeLSL(uint256 chain, uint256 slot, address contractAddress) public pure returns (bytes memory) {
+    return abi.encodePacked(bytes1(0x01), bytes1(0x01), bytes32(chain), contractAddress, bytes32(slot));
+  }
+
+  /**
+   * @dev Validate and decode a list storage location 
    * @param listStorageLocation The storage location of the list.
+   * @return chain The chain ID of the list.
    * @return slot The slot of the list.
    * @return contractAddress The contract address of the list.
    */
-  function decodeL1ListStorageLocation(bytes calldata listStorageLocation) internal pure returns (uint256, uint256, address) {
+  function validateAndDecodeLSL(bytes calldata listStorageLocation) internal pure returns (uint256, uint256, address) {
     // the list storage location is
     // - version (1 byte)
-    // - list storate location type (1 byte)
+    // - list storage location type (1 byte)
     // - chain id (32 bytes)
     // - contract address (20 bytes)
     // - slot (32 bytes)
     require(listStorageLocation.length == 1 + 1 + 32 + 20 + 32, 'EFPListMinter: invalid list storage location');
     require(listStorageLocation[0] == 0x01, 'EFPListMinter: invalid list storage location version');
     require(listStorageLocation[1] == 0x01, 'EFPListMinter: invalid list storage location type');
-    address contractAddress = _bytesToAddress(listStorageLocation, 34);
-
-    uint256 chain = _bytesToUint(listStorageLocation, 2);
-    uint256 slot = _bytesToUint(listStorageLocation, 54);
+    (uint256 chain, uint256 slot, address contractAddress) = decodeLSL(listStorageLocation);
     return (chain, slot, contractAddress);
   }
 
@@ -98,7 +123,7 @@ contract EFPListMinterV2 is ENSReverseClaimer, Pausable {
    */
   function easyMint(bytes calldata listStorageLocation) public payable whenNotPaused {
     // validate the list storage location
-    (uint256 chain, uint256 slot, address recordsContract) = decodeL1ListStorageLocation(listStorageLocation);
+    (uint256 chain, uint256 slot, address recordsContract) = validateAndDecodeLSL(listStorageLocation);
 
     uint256 tokenId = registry.totalSupply();
     uint256 currentChain = block.chainid;
@@ -107,6 +132,7 @@ contract EFPListMinterV2 is ENSReverseClaimer, Pausable {
     if (recordsContract == address(listRecordsL1) && currentChain == chain) {
       listRecordsL1.claimListManagerForAddress(slot, msg.sender);
     }
+    emit Minted('easyMint', msg.sender, listStorageLocation);
   }
 
   /**
@@ -116,7 +142,7 @@ contract EFPListMinterV2 is ENSReverseClaimer, Pausable {
    */
   function easyMintTo(address to, bytes calldata listStorageLocation) public payable whenNotPaused {
     // validate the list storage location
-    (uint256 chain, uint256 slot, address recordsContract) = decodeL1ListStorageLocation(listStorageLocation);
+    (uint256 chain, uint256 slot, address recordsContract) = validateAndDecodeLSL(listStorageLocation);
 
     uint256 tokenId = registry.totalSupply();
     uint256 currentChain = block.chainid;
@@ -125,6 +151,7 @@ contract EFPListMinterV2 is ENSReverseClaimer, Pausable {
     if (recordsContract == address(listRecordsL1) && currentChain == chain) {
       listRecordsL1.claimListManagerForAddress(slot, msg.sender);
     }
+    emit Minted('easyMintTo', to, listStorageLocation);
   }
 
   /**
@@ -133,10 +160,11 @@ contract EFPListMinterV2 is ENSReverseClaimer, Pausable {
    */
   function mintPrimaryListNoMeta(bytes calldata listStorageLocation) public payable whenNotPaused {
     // validate the list storage location
-    decodeL1ListStorageLocation(listStorageLocation);
+    validateAndDecodeLSL(listStorageLocation);
     uint256 tokenId = registry.totalSupply();
     _setDefaultListForAccount(msg.sender, tokenId);
     registry.mintTo{value: msg.value}(msg.sender, listStorageLocation);
+    emit Minted('mintPrimaryListNoMeta', msg.sender, listStorageLocation);
   }
 
   /**
@@ -145,9 +173,10 @@ contract EFPListMinterV2 is ENSReverseClaimer, Pausable {
    */
   function mintNoMeta(bytes calldata listStorageLocation) public payable whenNotPaused {
     // validate the list storage location
-    decodeL1ListStorageLocation(listStorageLocation);
+    validateAndDecodeLSL(listStorageLocation);
 
     registry.mintTo{value: msg.value}(msg.sender, listStorageLocation);
+    emit Minted('mintNoMeta', msg.sender, listStorageLocation);
   }
 
   /**
@@ -157,9 +186,10 @@ contract EFPListMinterV2 is ENSReverseClaimer, Pausable {
    */
   function mintToNoMeta(address to, bytes calldata listStorageLocation) public payable whenNotPaused {
     // validate the list storage location
-    decodeL1ListStorageLocation(listStorageLocation);
+    validateAndDecodeLSL(listStorageLocation);
 
     registry.mintTo{value: msg.value}(to, listStorageLocation);
+    emit Minted('mintToNoMeta', to, listStorageLocation);
   }
 
   /**

src/EFPListRecordsV2.sol

@@ -20,8 +20,8 @@ abstract contract ListMetadata is IEFPListMetadata, Pausable, Ownable {
   // Data Structures
   ///////////////////////////////////////////////////////////////////////////
 
-  /// @dev The key-value set for each token ID
-  mapping(uint256 => mapping(string => bytes)) private values;
+  /// @dev The key-value set for each slot
+  mapping(uint256 slot => mapping(string key => bytes value)) private values;
 
   /////////////////////////////////////////////////////////////////////////////
   // Pausable
@@ -61,30 +61,27 @@ abstract contract ListMetadata is IEFPListMetadata, Pausable, Ownable {
   /////////////////////////////////////////////////////////////////////////////
 
   /**
-   * @dev Retrieves metadata value for token ID and key.
-   * @param tokenId The token Id to query.
+   * @dev Retrieves metadata value for slot and key.
+   * @param slot The slot to query.
    * @param key The key to query.
    * @return The associated value.
    */
-  function getMetadataValue(uint256 tokenId, string calldata key) external view returns (bytes memory) {
-    return values[tokenId][key];
+  function getMetadataValue(uint256 slot, string calldata key) external view returns (bytes memory) {
+    return values[slot][key];
   }
 
   /**
-   * @dev Retrieves metadata values for token ID and keys.
-   * @param tokenId The token Id to query.
+   * @dev Retrieves metadata values for slot and keys.
+   * @param slot The slot to query.
    * @param keys The keys to query.
    * @return The associated values.
    */
-  function getMetadataValues(uint256 tokenId, string[] calldata keys) external view returns (bytes[] memory) {
+  function getMetadataValues(uint256 slot, string[] calldata keys) external view returns (bytes[] memory) {
     uint256 length = keys.length;
     bytes[] memory result = new bytes[](length);
-    for (uint256 i = 0; i < length;) {
+    for (uint256 i = 0; i < length; ++i) {
       string calldata key = keys[i];
-      result[i] = values[tokenId][key];
-      unchecked {
-        ++i;
-      }
+      result[i] = values[slot][key];
     }
     return result;
   }
@@ -94,8 +91,8 @@ abstract contract ListMetadata is IEFPListMetadata, Pausable, Ownable {
   /////////////////////////////////////////////////////////////////////////////
 
   /**
-   * @dev Sets metadata records for token ID with the unique key key to value,
-   * overwriting anything previously stored for token ID and key. To clear a
+   * @dev Sets metadata records for slot with the unique key key to value,
+   * overwriting anything previously stored for slot and key. To clear a
    * field, set it to the empty string.
    * @param slot The slot corresponding to the list to update.
    * @param key The key to set.
@@ -107,8 +104,8 @@ abstract contract ListMetadata is IEFPListMetadata, Pausable, Ownable {
   }
 
   /**
-   * @dev Sets metadata records for token ID with the unique key key to value,
-   * overwriting anything previously stored for token ID and key. To clear a
+   * @dev Sets metadata records for slot with the unique key key to value,
+   * overwriting anything previously stored for slot and key. To clear a
    * field, set it to the empty string. Only callable by the list manager.
    * @param slot The slot corresponding to the list to update.
    * @param key The key to set.
@@ -123,24 +120,21 @@ abstract contract ListMetadata is IEFPListMetadata, Pausable, Ownable {
   }
 
   /**
-   * @dev Sets an array of metadata records for a token ID. Each record is a
+   * @dev Sets an array of metadata records for a slot. Each record is a
    * key/value pair.
    * @param slot The slot corresponding to the list to update.
    * @param records The records to set.
    */
   function _setMetadataValues(uint256 slot, KeyValue[] calldata records) internal {
     uint256 length = records.length;
-    for (uint256 i = 0; i < length;) {
+    for (uint256 i = 0; i < length; ++i) {
       KeyValue calldata record = records[i];
       _setMetadataValue(slot, record.key, record.value);
-      unchecked {
-        ++i;
-      }
     }
   }
 
   /**
-   * @dev Sets an array of metadata records for a token ID. Each record is a
+   * @dev Sets an array of metadata records for a slot. Each record is a
    * key/value pair. Only callable by the list manager.
    * @param slot The slot corresponding to the list to update.
    * @param records The records to set.
@@ -161,7 +155,7 @@ abstract contract ListMetadata is IEFPListMetadata, Pausable, Ownable {
   modifier onlyListManager(uint256 slot) {
     bytes memory existing = values[slot]['manager'];
     // if not set, claim for msg.sender
-    if (existing.length != 20) {
+    if (existing.length == 0) {
       _claimListManager(slot, msg.sender);
     } else {
       address existingManager = bytesToAddress(existing);
@@ -290,7 +284,7 @@ abstract contract ListRecordsV2 is IEFPListRecords, ListMetadata {
 
   /// @notice Stores a sequence of operations for each list identified by its slot.
   /// @dev Each list can have multiple operations performed over time.
-  mapping(uint256 => bytes[]) public listOps;
+  mapping(uint256 slot => bytes[] listOps) public listOps;
 
   ///////////////////////////////////////////////////////////////////////////
   // List Operation Functions -  Read
@@ -328,12 +322,8 @@ abstract contract ListRecordsV2 is IEFPListRecords, ListMetadata {
     }
 
     bytes[] memory ops = new bytes[](end - start);
-    for (uint256 i = start; i < end;) {
+    for (uint256 i = start; i < end; ++i) {
       ops[i - start] = listOps[slot][i];
-
-      unchecked {
-        ++i;
-      }
     }
     return ops;
   }
@@ -377,11 +367,8 @@ abstract contract ListRecordsV2 is IEFPListRecords, ListMetadata {
    */
   function _applyListOps(uint256 slot, bytes[] calldata ops) internal {
     uint256 len = ops.length;
-    for (uint256 i = 0; i < len;) {
+    for (uint256 i = 0; i < len; ++i) {
       _applyListOp(slot, ops[i]);
-      unchecked {
-        ++i;
-      }
     }
   }