---
# System prepended metadata

title: '**Repository.huff**'

---

**dpd-assembly review**
by @devtooligan

# **Repository.huff**

**(INFORMATIONAL)** **l2** Address.huff imported but not used 

**(INFORMATIONAL)** **l5-18** Interface
 - missing `owners(uint256)`
 - incorrect fn sig sb `addDpd(uint256,bytes32,address,address)`
 - extranneous fn `updateDpd(uint256,bytes32)`
 - missing `updateDpdData(uint256, bytes32)`
 - incorrect fn name `setDpdOwner` sb `updateDpdOwner`
 - incorrect fn name `setDpdUpdater` sb `updateDpdUpdater`
 - inccorect event sig sb `DPDUpdated(uint,bytes32)`
 - incorrect event name `DPDOwnerChanged` sb `DPDOwnerUpdated`
 - incorrect event name `DPDUpdaterChanged` sb `DPDUpdaterUpdated`

**(INFORMATIONAL)** **l25** Incorrect event sig in comment sb `DPDUpdated(uint,bytes32)`

**(INFORMATIONAL)** **l28-29** Should use correct fn name in comment and as constant name for consistency.  sb `DPDOwnerUpdated` and `DPD_OWNER_UPDATED` respectively

**(INFORMATIONAL)** **l31-32** Should use correct fn name in comment and as constant name for consistency.  sb `DPDUpdaterUpdated` and `DPD_UPDATER_UPDATED` respectively

**(INFORMATIONAL)** **l35** Unclear/inconsistent inline comment - Is there a reason only this one has a comment after?

**(INFORMATIONAL)** **l36** Unused storage pointer `DPD_ARRAY_LENGTH_SLOT` - related to l72

**(INFORMATIONAL)** **l44 & l49** Inconsistent comments - The two similar, subsequent view functions don't have the same comments as this fn.

**(INFORMATIONAL)** **l51** Inconsistent with subsequent 2 view fns which put the return on the next line.

**(MEDIUM)** **l71** Missing view functions:
 - Consider adding a `GET_ARRAY` function that returns the entire array. [see Solcurity Standard V9](https://github.com/Rari-Capital/solcurity)
 - Consider adding a `TOTAL_SUPPLY` function that returns the total number of DPD's (the length of the array).

**(CRITICAL)** **l74** `ADD_DPD`
Function comment says `Get the DPD array length, which serves as the DPD ID.` but in actuality, an index is passed in.  **NOTE: SEE BELOW RECOMMENDATION FOR OVERHAULING THIS FUNCTION AND REPLACING WITH ADD_DPD() AND UPDATE_DPD()**

**(INFORMATIONAL)** **l95-104** For some reason in the stack comment `length*32, length` gets added to the end.

**(INFORMATIONAL)** **l117** Confirming we don't allow zero's for any of the fields `updater`, `data`, or `owner` (so no "burning" by sending to zero address).

**(INFORMATIONAL)** **l144** `UPDATE_DPD` - Consider renaming this to `UPDATE_DPD_DATA` or `UPDATE_DPD_CID` to be consistent.

**(HIGH)** **l144** We revert if any zeros are found during ADD_DPD but we allow zeros to go through here and in the other single attribute update fns.  Recommend iszero check for all 3 update functions.

**(INFORMATIONAL)** **l158-l170** Consider abstracting this logic to a macro `ONLY_OWNER_OR_UPDATER`.

**(MEDIUM)** **l199** Recommend allowing owner to change `updater`.

**(INFORMATIONAL)** **l233** Consider allowing the `updater` to update the `owner` as well. This would allow for an approved `udpater` proxy to transfer the DPD on behalf of the owner.

**(GAS OPTIMIZATION)** **l260** The fns in MAIN() could be sorted, transactional first then sorted by most often called.  View functions that would likely be viewed offchain can go last.

**RECOMMENDATION FOR OVERHAULING ADD_DPD()**
```
/* DPD Overwrite existing */
/// @dev 🔫 This low level fn has no auth, validation, or other error checking
#define macro SET_DPD() = takes(0) returns (0) {
    // Store the DPD data.
    0x24 calldataload                           // [data, dpd_id]
    dup2                                        // [dpd_id, data, dpd_id]
    [DPD_ARRAY_SLOT]                            // [salt, dpd_id, data, dpd_id]
    STORE_ELEMENT_FROM_KEYS(0x00)               // [dpd_id]

    // Store the address of the owner.
    0x44 calldataload                           // [owner, dpd_id]
    dup2                                        // [dpd_id, owner, dpd_io]
    [DPD_OWNER_MAP_SLOT]                        // [salt, dpd_id, owner, dpd_io]
    STORE_ELEMENT_FROM_KEYS(0x00)               // [dpd_id]

    // Store the address of the updater.
    0x64 calldataload                           // [updater, dpd_id]
    dup2                                        // [dpd_id, updater, dpd_id]
    [DPD_UPDATER_MAP_SLOT]                      // [salt, dpd_id, updater, dpd_id]
    STORE_ELEMENT_FROM_KEYS(0x00)               // [dpd_id]



/* DPD Add New */
#define macro ADD_DPD() = takes(0) returns (0) {
    // Get the DPD array length, which serves as the DPD ID.
    [DPD_ARRAY_LENGTH_SLOT]                     // [salt]
    LOAD_ELEMENT(0x00)                          // [dpd_id]
    dup1                                        // [dpd_id, dpd_id]

    // Update array length
    0x1 add                                     // [dpd_id + 1, dpd_id]
    [DPD_ARRAY_LENGTH_SLOT]                     // [salt, dpd_id + 1, dpd_id]
    STORE_ELEMENT(0x00)                         // [dpd_id]

    // Checks
    ID_CHECKS(error)                            // [dpd_id]

    // Emit the event.
    // DPDAdded(uint256 indexed dpdId, address owner, address updater, bytes32 cid)
    dup1                                        // [dpd_id, dpd_id]
    [DPD_ADDED_EVENT_SIGNATURE]                 // [sig, dpd_id, dpd_id]
    0x44 calldataload 0x00 mstore               // [sig, dpd_id, dpd_id]
    0x64 calldataload 0x20 mstore               // [sig, dpd_id, dpd_id]
    0x24 calldataload 0x40 mstore               // [sig, dpd_id, dpd_id]
    0x60 0x00                                   // [offset, size, sig, dpd_id, dpd_id]
    log2                                        // [dpd_id]

    SET_DPD()                                   // [dpd_id]

    // Stop the program.
    stop

    // Revert if the checks are wrong.
    error:
        0x00 0x00 revert
}

/* DPD Edit */
#define macro UPDATE_DPD() = takes(0) returns (0) {
    0x04 calldataload                           // [dpd_id]

    // Checks
    // by checking isOwner we also get an in bounds check
    ONLY_OWNER_OR_UPDATER()                     // [dpi_id]
    ID_CHECKS(error)                            // [dpd_id]

    // Emit the event.
    // DPDAdded(uint256 indexed dpdId, address owner, address updater, bytes32 cid)
    dup1                                        // [dpd_id, dpd_id]
    [DPD_UPDATED_EVENT_SIGNATURE]                 // [sig, dpd_id, dpd_id]
    0x44 calldataload 0x00 mstore               // [sig, dpd_id, dpd_id]
    0x64 calldataload 0x20 mstore               // [sig, dpd_id, dpd_id]
    0x24 calldataload 0x40 mstore               // [sig, dpd_id, dpd_id]
    0x60 0x00                                   // [offset, size, sig, dpd_id, dpd_id]
    log2                                        // [dpd_id]

    SET_DPD()                                   // [dpd_id]

    // Stop the program.
    stop

    // Revert if the checks are wrong.
    error:
        0x00 0x00 revert
}

```

# **Repository.t.sol -- TESTS**

**Interface**
  - **(INFORMATIONAL)** Recommend to split up the interface into events and functions.  Not only does it make it easier to import events into the test ctrct, but [there's an argument for not including events in interfaces](https://twitter.com/alcueca/status/1466152649015836673?s=20&t=iaEVlpqN6NEWCsVi_c2NSg). Rename interface `Repository` to `IRepository` and move all events from the newly renamed `IRepository` to a new abstract contract `RepositoryEvents`. Can move these two to a new file or to the top of test file.
  - **(INFORMATIONAL)** Have the test contract inherit `RepositoryEvents` and delete the duped events currently on **l9-l12** within the test contract.
  - **(MEDIUM)** All `address` args in all events should be indexed.  This will mean updating the `log` code in the contract and the `vm.expectEmit` code in the tests as well. [see Solcurity Standard E5](https://github.com/Rari-Capital/solcurity)
  - **(INFORMATIONAL)** Incorrect natspec for event`DPDUpdated` -- sb `Event emitted when a DPD's CID is updated.`
  - **(INFORMATIONAL)** Typo in natspec for event`DPDUpdated` -- "upgrader"
  - **(INFORMATIONAL)** Consider adding the param names to the signatures for readability like in contract in comment on l79 `DPDAdded(uint256 indexed dpdId, address owner, address updater, bytes32 cid)`

**Tests**
  - **(HIGH)** Auth checks are not tested.
  - **(MEDIUM)** View fns not tested.
  - **(HIGH)** Unit testing (or fuzzing) for ID_CHECKS.
  - **(MEDIUM)** Currently only one event `DPDAdded` is tested. There should be at least 1 test for each event.
  - **(MEDIUM)** Ensure that there is a test case for every where that the contract could revert.
  - **(INFORMATIONAL)** The setup for the `vm.expectRevert` is incorrect in its current state. Since there is only 1 topic area so it should be `vm.expectRevert(true, false, false, true)`
  - **(INFORMATIONAL)** The assertion made in the comment on **l45** is not entirely true. `@dev Ensure that you cannot create two DPDs with the same ID.` You can create a DPD with id of 1 and update all fields to zeros for `data`, `owner`, and `updater`. You could then call `ADD_DPD` again with the same dpd_id and it would not revert. Prefer `vm.expectRevert` to `testFail` for tighter control and less false positives.
 
 

