Comments and documentation to deposits

src/TheCompact.sol

@@ -25,47 +25,65 @@ import { ISignatureTransfer } from "permit2/src/interfaces/ISignatureTransfer.so
  *         This contract has not yet been properly tested, audited, or reviewed.
  */
 contract TheCompact is ITheCompact, ERC6909, TheCompactLogic {
+    // NOTIFY READERS OF THE NATSPEC FOR THE FUNCTIONS IN THE INTERFACE
     function deposit(address allocator) external payable returns (uint256) {
-        return _performBasicNativeTokenDeposit(allocator);
+        // COMPLETED PR
+        return _performBasicNativeTokenDeposit(allocator); // DirectDepositLogic.sol
     }
 
+    // THE CLAIM REGISTERED IS NOT A SECRET IN THIS CASE, SO COULD WE VERIFY THE CLAIMHASH?
     function depositAndRegister(address allocator, bytes32 claimHash, bytes32 typehash) external payable returns (uint256 id) {
-        id = _performBasicNativeTokenDeposit(allocator);
+        // COMPLETED PR
+        id = _performBasicNativeTokenDeposit(allocator); // DirectDepositLogic.sol
 
-        _registerWithDefaults(claimHash, typehash);
+        _registerWithDefaults(claimHash, typehash); // RegistrationLogic.sol
     }
 
+    // COULD ALSO BE USED FOR NATIVE TOKENS TO REDUCE NUMBER OF FUNCTIONS - 'AMOUNT' COULD BE USED TO VERIFY THE INTENDED AMOUNT OF NATIVE TOKENS TO BE DEPOSITED
     function deposit(address token, address allocator, uint256 amount) external returns (uint256) {
-        return _performBasicERC20Deposit(token, allocator, amount);
+        // Completed PR
+        return _performBasicERC20Deposit(token, allocator, amount); // DirectDepositLogic.sol
     }
 
+    // THE CLAIM REGISTERED IS NOT A SECRET IN THIS CASE, SO COULD WE VERIFY THE CLAIMHASH?
     function depositAndRegister(address token, address allocator, uint256 amount, bytes32 claimHash, bytes32 typehash) external returns (uint256 id) {
-        id = _performBasicERC20Deposit(token, allocator, amount);
+        // Completed PR
+        id = _performBasicERC20Deposit(token, allocator, amount); // DirectDepositLogic.sol
 
-        _registerWithDefaults(claimHash, typehash);
+        _registerWithDefaults(claimHash, typehash); // RegistrationLogic.sol
     }
 
+    // HAVING THE RECIPIENT AS A PARAMETER ALLOWS US TO OUTSOURCE depositAndRegister FUNCTIONS TO HELPER CONTRACTS
+    // IF WE USE A CALLBACK FOR THE USER TO SEND IN THE TOKENS (LIKE IN UNISWAP V3), WE SAVE ON GAS BY SKIPPING APPROVAL FOR THIS CONTRACT.
+    // THE CONTRACT FEELS A LITTLE LIKE UNISWAP V3 CORE AND PERIPHERY CONTRACTS WERE COMBINED IN ONE. COULD BE INTERESTING TO EXPLORE
+    // WHERE TO SEPERATE THE LOGIC TO KEEP IT EASY TO READ AND MODULAR AT THE SAME TIME.
     function deposit(address allocator, ResetPeriod resetPeriod, Scope scope, address recipient) external payable returns (uint256) {
-        return _performCustomNativeTokenDeposit(allocator, resetPeriod, scope, recipient);
+        // Completed PR
+        // COULD ELIMINATE A LOT OF THE FUNCTIONS IN THE DEPOSIT CONTRACTS BY HANDELING DEFAULT PARAMETERS IN THIS CONTRACT. COULD INCREASE READABILITY.
+        return _performCustomNativeTokenDeposit(allocator, resetPeriod, scope, recipient); // DirectDepositLogic.sol
     }
 
     function deposit(address token, address allocator, ResetPeriod resetPeriod, Scope scope, uint256 amount, address recipient) external returns (uint256) {
-        return _performCustomERC20Deposit(token, allocator, resetPeriod, scope, amount, recipient);
+        // Completed PR
+        // COULD ELIMINATE A LOT OF THE FUNCTIONS IN THE DEPOSIT CONTRACTS BY HANDELING DEFAULT PARAMETERS IN THIS CONTRACT. COULD INCREASE READABILITY.
+        return _performCustomERC20Deposit(token, allocator, resetPeriod, scope, amount, recipient); // DirectDepositLogic.sol
     }
 
-    function deposit(uint256[2][] calldata idsAndAmounts, address recipient) external payable returns (bool) {
-        _processBatchDeposit(idsAndAmounts, recipient);
+    // IN HERE, WE ARE ACTUALLY COMBINING NATIVE AND ERC20 TOKENS IN THE SAME FUNCTION, WE CAN DO THE SAME ABOVE.
+    function deposit(uint256[2][] calldata idsAndAmounts, address recipient) external payable returns (bool) { 
+        // Completed PR
+        _processBatchDeposit(idsAndAmounts, recipient); // DirectDepositLogic.sol
 
         return true;
     }
 
     function depositAndRegister(uint256[2][] calldata idsAndAmounts, bytes32[2][] calldata claimHashesAndTypehashes, uint256 duration) external payable returns (bool) {
         _processBatchDeposit(idsAndAmounts, msg.sender);
 
-        return _registerBatch(claimHashesAndTypehashes, duration);
+        return _registerBatch(claimHashesAndTypehashes, duration); // NEED TO LOOK INTO THIS
     }
 
-    function deposit(
+    function deposit( // RENAME TO depositViaPermit2 FOR CLEAR SEPARATION?
         address token,
         uint256, // amount
         uint256, // nonce
@@ -76,11 +94,12 @@ contract TheCompact is ITheCompact, ERC6909, TheCompactLogic {
         Scope, //scope
         address recipient,
         bytes calldata signature
-    ) external returns (uint256) {
-        return _depositViaPermit2(token, recipient, signature);
+    ) external returns (uint256) { // Completed PR
+        return _depositViaPermit2(token, recipient, signature); // DepositViaPermit2Logic.sol
     }
 
-    function depositAndRegister(
+    // ITS HARD TO FOLLOW A PATTERN HERE... DEPOSIT AND REGISTER VIA PERMIT2 HAS SUCH A DIFFERENT INTERNAL PROCESS COMPARED TO ONLY DEPOSITING (NOT JUST THE REGISTERING PART ADDED).
+    function depositAndRegister( // RENAME TO depositAndRegisterViaPermit2 FOR CLEAR SEPARATION?
         address token,
         uint256, // amount
         uint256, // nonce

src/interfaces/ITheCompact.sol

@@ -57,6 +57,8 @@ interface ITheCompact {
      */
     event AllocatorRegistered(uint96 allocatorId, address allocator);
 
+    // COULD WE ADD ALL THE CUSTOM ERRORS HERE FOR CLARITY, (EVEN THOUGH THE CONTRACT REVERTS IN ASSEMBLY)?
+
     /**
      * @notice External payable function for depositing native tokens into a resource lock
      * and receiving back ERC6909 tokens representing the underlying locked balance controlled
@@ -143,7 +145,7 @@ interface ITheCompact {
 
     /**
      * @notice External payable function for depositing multiple tokens in a single transaction.
-     * The first entry in idsAndAmounts can optionally represent native tokens by providing the
+     * The first entry (and ONLY the first entry) in idsAndAmounts can optionally represent native tokens by providing the
      * null address and an amount matching msg.value. For ERC20 tokens, the caller must directly
      * approve The Compact to transfer sufficient amounts on its behalf. The ERC6909 token amounts
      * received by the recipient are derived from the differences between starting and ending

src/lib/ConstructorLogic.sol

@@ -26,7 +26,7 @@ contract ConstructorLogic is Tstorish {
     using IdLib for uint256;
 
     // Address of the Permit2 contract, optionally used for depositing ERC20 tokens.
-    address private constant _PERMIT2 = 0x000000000022D473030F116dDEE9F6B43aC78BA3;
+    address private constant _PERMIT2 = 0x000000000022D473030F116dDEE9F6B43aC78BA3; // BETTER AS IMMUTABLE TO BE MORE FLEXIBLE WITH OTHER CHAINS 
 
     // Storage slot used for the reentrancy guard, whether using TSTORE or SSTORE.
     uint256 private constant _REENTRANCY_GUARD_SLOT = 0x929eee149b4bd21268;
@@ -41,7 +41,7 @@ contract ConstructorLogic is Tstorish {
     MetadataRenderer private immutable _METADATA_RENDERER;
 
     // Whether Permit2 was deployed on the chain at construction time.
-    bool private immutable _PERMIT2_INITIALLY_DEPLOYED;
+    bool private immutable _PERMIT2_INITIALLY_DEPLOYED; // IF WE MAKE _PERMIT2 AN IMMUTABLE, WE CAN JUST CHECK IF _PERMIT2 IS NOT ADDRESS ZERO
 
     /**
      * @notice Constructor that initializes immutable variables and deploys the metadata

src/lib/DepositLogic.sol

@@ -14,7 +14,7 @@ contract DepositLogic is ConstructorLogic {
     using SafeTransferLib for address;
 
     // Storage slot seed for ERC6909 state, used in computing balance slots.
-    uint256 private constant _ERC6909_MASTER_SLOT_SEED = 0xedcaa89a82293940;
+    uint256 private constant _ERC6909_MASTER_SLOT_SEED = 0xedcaa89a82293940; // WHERE IS THIS COMING FROM?
 
     // keccak256(bytes("Transfer(address,address,address,uint256,uint256)")).
     uint256 private constant _TRANSFER_EVENT_SIGNATURE = 0x1b3d7edb2e9c0b0e7c525b20aaaef0f5940d2ed71663c7d39266ecafac728859;
@@ -36,7 +36,7 @@ contract DepositLogic is ConstructorLogic {
         // Revert if the balance hasn't increased.
         assembly ("memory-safe") {
             if iszero(lt(initialBalance, tokenBalance)) {
-                // revert InvalidDepositBalanceChange()
+                // revert InvalidDepositBalanceChange() // COULD PROVIDE INFORMATION ABOUT INIT AND NEW TOKEN BALANCE IN THE ERROR
                 mstore(0, 0x426d8dcf)
                 revert(0x1c, 0x04)
             }
@@ -60,9 +60,14 @@ contract DepositLogic is ConstructorLogic {
     function _deposit(address to, uint256 id, uint256 amount) internal {
         assembly ("memory-safe") {
             // Compute the recipient's balance slot using the master slot seed.
-            mstore(0x20, _ERC6909_MASTER_SLOT_SEED)
-            mstore(0x14, to)
-            mstore(0x00, id)
+            mstore(0x20, _ERC6909_MASTER_SLOT_SEED) // length of 64 bits
+            mstore(0x14, to) // Length of 160 bits
+            mstore(0x00, id) // length of 256 bits
+            //           -----------SLOT 1-----------   -----------SLOT 2-----------
+            // master:  |        - 256 bytes -         | [0000000000000000000][--64 bits--]
+            // to:      |    - 160 bytes -     [[0000] | [---160 bits---]]
+            // id:      | [---------256 bits---------] |        - 256 bytes -
+
             let toBalanceSlot := keccak256(0x00, 0x40)
 
             // Load current balance and compute new balance.
@@ -87,6 +92,7 @@ contract DepositLogic is ConstructorLogic {
             mstore(0x00, caller())
             mstore(0x20, amount)
             log4(0, 0x40, _TRANSFER_EVENT_SIGNATURE, 0, shr(0x60, shl(0x60, to)), id)
+            // IS IT REQUIRED TO SANITIZE AN INTERNAL INPUT?
         }
     }
 }

src/lib/DirectDepositLogic.sol

@@ -33,6 +33,7 @@ contract DirectDepositLogic is DepositLogic {
      * default reset period (ten minutes) and scope (multichain) will be used for the resource
      * lock. The ERC6909 token amount received by the caller will match the amount of native
      * tokens sent with the transaction.
+     * @dev will revert if the allocator address was not previously registered
      * @param allocator The address of the allocator.
      * @return id The ERC6909 token identifier of the associated resource lock.
      */
@@ -46,7 +47,7 @@ contract DirectDepositLogic is DepositLogic {
 
     /**
      * @notice Internal function for depositing multiple tokens in a single transaction. The
-     * first entry in idsAndAmounts can optionally represent native tokens by providing the null
+     * first entry (and ONLY the first entry) in idsAndAmounts can optionally represent native tokens by providing the null
      * address and an amount matching msg.value. For ERC20 tokens, the caller must directly
      * approve The Compact to transfer sufficient amounts on its behalf. The ERC6909 token amounts
      * received by the recipient are derived from the differences between starting and ending
@@ -74,14 +75,29 @@ contract DirectDepositLogic is DepositLogic {
             // Load the first ID from idsAndAmounts.
             id := calldataload(idsAndAmountsOffset)
 
+            // Example of how the calldata is structured and the difference between the array pointer and .offset in assembly.
+            //
+            // The example calldata for this function has the following inputs:
+            // idsAndAmounts = [[1, 2],[3, 4]], recipient = 0x3482f06bbd1cc4ae5b13b3f63a5373752a7819e0
+            //
+            // 0x5943672c|                                      Function selector
+            // 0...0000000000000000000000000000000000000040|    Pointer to idsAndAmounts
+            // 0...3482f06bbd1cc4ae5b13b3f63a5373752a7819e0|    Recipient
+            // 0...0000000000000000000000000000000000000002|    Length of idsAndAmounts <--- Pointer to idsAndAmounts
+            // 0...0000000000000000000000000000000000000001|    First ID <--- idsAndAmounts.offset points here
+            // 0...0000000000000000000000000000000000000002|    First Amount
+            // 0...0000000000000000000000000000000000000003|    Second ID
+            // 0...0000000000000000000000000000000000000004|    Second Amount
+
             // Determine if token encoded in first ID is the null address.
             firstUnderlyingTokenIsNative := iszero(shr(96, shl(96, id)))
+            // shift 96 to left to clear [1 bit scope][3 bits resetPeriod][92 bits allocatorId], so only [160 bits token] remains
 
             // Revert if:
             //  * the array is empty
             //  * the callvalue is zero but the first token is native
             //  * the callvalue is nonzero but the first token is non-native
-            //  * the first token is non-native and the callvalue doesn't equal the first amount
+            //  * the first token is native and the callvalue doesn't equal the first amount
             if or(iszero(totalIds), or(eq(firstUnderlyingTokenIsNative, iszero(callvalue())), and(firstUnderlyingTokenIsNative, iszero(eq(callvalue(), calldataload(add(idsAndAmountsOffset, 0x20))))))) {
                 // revert InvalidBatchDepositStructure()
                 mstore(0, 0xca0fc08e)
@@ -103,13 +119,23 @@ contract DirectDepositLogic is DepositLogic {
         // Iterate over remaining IDs and amounts.
         unchecked {
             for (uint256 i = firstUnderlyingTokenIsNative.asUint256(); i < totalIds; ++i) {
+                // LOVE THAT BOOL TO UINT USAGE
                 // Navigate to the current ID and amount pair in calldata.
                 uint256[2] calldata idAndAmount = idsAndAmounts[i];
 
                 // Retrieve the current ID and amount.
                 id = idAndAmount[0];
                 amount = idAndAmount[1];
 
+                // ADDITIONAL CHECK TO REVERT IF ID OR AMOUNT IS 0
+                assembly ("memory-safe") {
+                    if or(iszero(id), iszero(amount)) {
+                        // revert InvalidBatchDepositStructure()
+                        mstore(0, 0xca0fc08e)
+                        revert(0x1c, 0x04)
+                    }
+                }
+
                 // Derive new allocator ID from current resource lock ID.
                 newAllocatorId = id.toAllocatorId();
 
@@ -138,6 +164,8 @@ contract DirectDepositLogic is DepositLogic {
      * default reset period (ten minutes) and scope (multichain) will be used for the resource
      * lock. The ERC6909 token amount received by the caller will match the amount of native
      * tokens sent with the transaction.
+     * @dev will revert if the allocator address was not previously registered
+     * @dev will revert if the token is address 0 (which is used to represent native tokens)
      * @param allocator The address of the allocator.
      * @return id The ERC6909 token identifier of the associated resource lock.
      */