Review feedback

src/OptionSettlementEngine.sol

@@ -145,7 +145,7 @@ contract OptionSettlementEngine is ERC1155, IOptionSettlementEngine {
 
     /// @inheritdoc IOptionSettlementEngine
     function isOptionInitialized(uint160 optionKey) public view returns (bool) {
-        return _option[optionKey].underlyingAsset != address(0x0);
+        return _option[optionKey].underlyingAsset != address(0);
     }
 
     /*//////////////////////////////////////////////////////////////
@@ -190,10 +190,10 @@ contract OptionSettlementEngine is ERC1155, IOptionSettlementEngine {
 
     /// @inheritdoc IOptionSettlementEngine
     function decodeTokenId(uint256 tokenId) public pure returns (uint160 optionKey, uint96 claimNum) {
-        // move key to LSB to fit into uint160
+        // move key to lsb to fit into uint160
         optionKey = uint160(tokenId >> 96);
 
-        // grab lower 96b of id for claim index
+        // grab lower 96b of id for claim number
         uint256 claimNumMask = 0xFFFFFFFFFFFFFFFFFFFFFFFF;
         claimNum = uint96(tokenId & claimNumMask);
     }
@@ -444,11 +444,6 @@ contract OptionSettlementEngine is ERC1155, IOptionSettlementEngine {
         }
 
         OptionLotClaim storage claimRecord = _claim[claimId];
-
-        if (claimRecord.claimed) {
-            revert AlreadyClaimed(claimId);
-        }
-
         Option storage optionRecord = _option[optionKey];
 
         if (optionRecord.expiryTimestamp > block.timestamp) {
@@ -540,11 +535,11 @@ contract OptionSettlementEngine is ERC1155, IOptionSettlementEngine {
     /// are then iterated from oldest to newest (looping if we reach "today") if the
     /// exercise amount overflows into another bucket. The seed for the pseudorandom
     /// index is updated accordingly on the option type.
-    function _assignExercise(uint160 optionId, Option storage optionRecord, uint112 amount) internal {
+    function _assignExercise(uint160 optionKey, Option storage optionRecord, uint112 amount) internal {
         // A bucket of the overall amounts written and exercised for all claims
         // on a given day
-        OptionsDayBucket[] storage claimBucketArray = _claimBucketByOption[optionId];
-        uint16[] storage unexercisedBucketIndices = _unexercisedBucketsByOption[optionId];
+        OptionsDayBucket[] storage claimBucketArray = _claimBucketByOption[optionKey];
+        uint16[] storage unexercisedBucketIndices = _unexercisedBucketsByOption[optionKey];
         uint16 unexercisedBucketsMod = uint16(unexercisedBucketIndices.length);
         uint16 unexercisedBucketsIndex = uint16(optionRecord.settlementSeed % unexercisedBucketsMod);
         while (amount > 0) {
@@ -562,7 +557,7 @@ contract OptionSettlementEngine is ERC1155, IOptionSettlementEngine {
                 unexercisedBucketIndices[unexercisedBucketsIndex] = overwrite;
                 unexercisedBucketIndices.pop();
                 unexercisedBucketsMod -= 1;
-                _doesBucketIndexHaveUnexercisedOptions[optionId][bucketIndex] = false;
+                _doesBucketIndexHaveUnexercisedOptions[optionKey][bucketIndex] = false;
             } else {
                 amountPresentlyExercised = amount;
                 amount = 0;
@@ -606,49 +601,49 @@ contract OptionSettlementEngine is ERC1155, IOptionSettlementEngine {
     }
 
     /// @dev Get the exercise and underlying amounts for a claim
-    function _getPositionsForClaim(uint160 optionId, uint256 claimId, Option storage optionRecord)
+    function _getPositionsForClaim(uint160 optionKey, uint256 claimId, Option storage optionRecord)
         internal
         view
         returns (uint256 exerciseAmount, uint256 underlyingAmount)
     {
         OptionLotClaimIndex[] storage claimIndexArray = _claimIdToClaimIndexArray[claimId];
         for (uint256 i = 0; i < claimIndexArray.length; i++) {
             OptionLotClaimIndex storage claimIndex = claimIndexArray[i];
-            OptionsDayBucket storage claimBucketInfo = _claimBucketByOption[optionId][claimIndex.bucketIndex];
+            OptionsDayBucket storage claimBucketInfo = _claimBucketByOption[optionKey][claimIndex.bucketIndex];
             (uint256 amountExercised, uint256 amountUnexercised) = _getAmountExercised(claimIndex, claimBucketInfo);
             exerciseAmount += optionRecord.exerciseAmount * amountExercised;
             underlyingAmount += optionRecord.underlyingAmount * amountUnexercised;
         }
     }
 
     /// @dev Help with internal options bucket accounting
-    function _addOrUpdateClaimBucket(uint160 optionId, uint112 amount) internal returns (uint16) {
-        OptionsDayBucket[] storage claimBucketsInfo = _claimBucketByOption[optionId];
-        uint16[] storage unexercised = _unexercisedBucketsByOption[optionId];
+    function _addOrUpdateClaimBucket(uint160 optionKey, uint112 amount) internal returns (uint16) {
+        OptionsDayBucket[] storage claimBucketsInfo = _claimBucketByOption[optionKey];
+        uint16[] storage unexercised = _unexercisedBucketsByOption[optionKey];
         OptionsDayBucket storage currentBucket;
         uint16 daysAfterEpoch = _getDaysBucket();
         uint16 bucketIndex = uint16(claimBucketsInfo.length);
         if (claimBucketsInfo.length == 0) {
             // add a new bucket none exist
             claimBucketsInfo.push(OptionsDayBucket(amount, 0, daysAfterEpoch));
             // update _unexercisedBucketsByOption and corresponding index mapping
-            _updateUnexercisedBucketIndices(optionId, bucketIndex, unexercised);
+            _updateUnexercisedBucketIndices(optionKey, bucketIndex, unexercised);
             return bucketIndex;
         }
 
         currentBucket = claimBucketsInfo[bucketIndex - 1];
         if (currentBucket.daysAfterEpoch < daysAfterEpoch) {
             claimBucketsInfo.push(OptionsDayBucket(amount, 0, daysAfterEpoch));
-            _updateUnexercisedBucketIndices(optionId, bucketIndex, unexercised);
+            _updateUnexercisedBucketIndices(optionKey, bucketIndex, unexercised);
         } else {
             // Update claim bucket for today
             currentBucket.amountWritten += amount;
             bucketIndex -= 1;
 
             // This block is executed if a bucket has been previously fully exercised
             // and now more options are being written into it
-            if (!_doesBucketIndexHaveUnexercisedOptions[optionId][bucketIndex]) {
-                _updateUnexercisedBucketIndices(optionId, bucketIndex, unexercised);
+            if (!_doesBucketIndexHaveUnexercisedOptions[optionKey][bucketIndex]) {
+                _updateUnexercisedBucketIndices(optionKey, bucketIndex, unexercised);
             }
         }
 
@@ -657,12 +652,12 @@ contract OptionSettlementEngine is ERC1155, IOptionSettlementEngine {
 
     /// @dev Help with internal claim bucket accounting
     function _updateUnexercisedBucketIndices(
-        uint160 optionId,
+        uint160 optionKey,
         uint16 bucketIndex,
         uint16[] storage unexercisedBucketIndices
     ) internal {
         unexercisedBucketIndices.push(bucketIndex);
-        _doesBucketIndexHaveUnexercisedOptions[optionId][bucketIndex] = true;
+        _doesBucketIndexHaveUnexercisedOptions[optionKey][bucketIndex] = true;
     }
 
     /// @dev Help with internal claim bucket accounting

src/interfaces/IOptionSettlementEngine.sol

@@ -1,8 +1,6 @@
 // SPDX-License-Identifier: BUSL 1.1
 pragma solidity 0.8.11;
 
-import "./IERC1155Metadata.sol";
-
 /// @title A settlement engine for options
 /// @author 0xAlcibiades
 /// @author Flip-Liquid
@@ -200,12 +198,6 @@ interface IOptionSettlementEngine {
      */
     error CallerHoldsInsufficientOptions(uint256 optionId, uint112 amount);
 
-    /**
-     * @notice This claimId has already been claimed.
-     * @param claimId Supplied claim ID.
-     */
-    error AlreadyClaimed(uint256 claimId);
-
     /**
      * @notice You can't claim before expiry.
      * @param claimId Supplied claim ID.

src/interfaces/ITokenURIGenerator.sol

@@ -1,7 +1,6 @@
 // SPDX-License-Identifier: BUSL 1.1
 pragma solidity 0.8.11;
 
-import "./IERC1155Metadata.sol";
 import "./IOptionSettlementEngine.sol";
 
 /// @title A token URI geneartor for Claim NFTs

test/OptionSettlementEngine.t.sol

@@ -317,6 +317,19 @@ contract OptionSettlementTest is Test, NFTreceiver {
         _assertPosition(underlyingPositions.exercisePosition, 2 * testExerciseAmount);
     }
 
+    function testWriteAfterFullyExercisingDay() public {
+        uint256 claim1 = _writeAndExerciseOption(testOptionId, ALICE, BOB, 1, 1);
+        uint256 claim2 = _writeAndExerciseOption(testOptionId, ALICE, BOB, 1, 1);
+
+        IOptionSettlementEngine.Underlying memory underlyingPositions = engine.underlying(claim1);
+        _assertPosition(underlyingPositions.underlyingPosition, 0);
+        _assertPosition(underlyingPositions.exercisePosition, testExerciseAmount);
+
+        underlyingPositions = engine.underlying(claim2);
+        _assertPosition(underlyingPositions.underlyingPosition, 0);
+        _assertPosition(underlyingPositions.exercisePosition, testExerciseAmount);
+    }
+
     function testUnderlyingForFungibleOptionToken() public {
         IOptionSettlementEngine.Underlying memory underlying = engine.underlying(testOptionId);
         // before expiry, position is entirely the underlying amount
@@ -1082,8 +1095,8 @@ contract OptionSettlementTest is Test, NFTreceiver {
         engine.write(invalidOptionId, 1);
 
         // Option ID not initialized
-        invalidOptionId = testOptionId / 2;
-        vm.expectRevert(abi.encodeWithSelector(IOptionSettlementEngine.InvalidOption.selector, invalidOptionId));
+        invalidOptionId = engine.encodeTokenId(0x1, 0x0);
+        vm.expectRevert(abi.encodeWithSelector(IOptionSettlementEngine.InvalidOption.selector, invalidOptionId >> 96));
         engine.write(invalidOptionId, 1);
     }