# ERC1820 code review
###### tags: erc1820, rengo-labs, casper-network, rust, test, code-review

## Questions

1. **(Raul)**
Why `CLType::Key` is [used on creation](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/entry_points.rs#L36) of `entry_points` that [return](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/lib.rs#L88) `Result<(), ApiError>`?
In ERC20 package they [use](https://github.com/casper-ecosystem/erc20/blob/d1e0d0b26668e4ca902fe90756be0d28859d447a/erc20/src/entry_points.rs#L91) `CLType::Unit` instead.

2. **CasperLabs**
In [ERC20 package upon entry_point creation](https://github.com/casper-ecosystem/erc20/blob/d1e0d0b26668e4ca902fe90756be0d28859d447a/erc20/src/entry_points.rs#L91) used `CLType::Unit` for [entry_points that dont return any](https://github.com/casper-ecosystem/erc20/blob/d1e0d0b26668e4ca902fe90756be0d28859d447a/example/erc20-token/src/main.rs#L54).
However I [saw usage](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/entry_points.rs#L36) of `CLType::Key` for the same [non-return function](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/example/implementations/erc1820-registry/src/main.rs#L46).

**Question:** What would be a proper choice for `no-return` entry_points, when we coding it?
`CLType::Key` or `CLType::Unit`

**Answer:**
- [A1](https://rengocapital.slack.com/archives/C02B1QEEMHP/p1658927666577949?thread_ts=1658924675.783749&cid=C02B1QEEMHP). `Unit`. The `EntryPoints` structure is not 100% when it comes to the return value.
- [A2](https://rengocapital.slack.com/archives/C02B1QEEMHP/p1658927863395809?thread_ts=1658924675.783749&cid=C02B1QEEMHP). https://docs.rs/casper-types/latest/casper_types/struct.EntryPoints.html
- [A3](https://rengocapital.slack.com/archives/C02B1QEEMHP/p1658927922860099?thread_ts=1658924675.783749&cid=C02B1QEEMHP). This is the struct that is passed in when you create a new contract version. In this the EntryPoint struct writes a lot of things as you have seen. return type, parameters, and security things. the return type as I recall is not enforced.
- [A4](https://rengocapital.slack.com/archives/C02B1QEEMHP/p1658928425277429?thread_ts=1658924675.783749&cid=C02B1QEEMHP). Practically you can put anything there and it will not enforce it.

3. **CasperLabs**
In your ERC20 test examples in [test_fixture.rs](https://github.com/casper-ecosystem/erc20/blob/master/example/erc20-tests/src/test_fixture.rs) were implemented:
- [query_contract()](https://github.com/casper-ecosystem/erc20/blob/d1e0d0b26668e4ca902fe90756be0d28859d447a/example/erc20-tests/src/test_fixture.rs#L87) to recieve values (read from chain). Looks like imitation of `entry_point` call that `return values`.
- [call()](https://github.com/casper-ecosystem/erc20/blob/d1e0d0b26668e4ca902fe90756be0d28859d447a/example/erc20-tests/src/test_fixture.rs#L102) to execute logic of an `entry_point`. And it's used in test for `no-return` entry_points.

**Question:** How could be tested an `entry_point` that returns value ([example](https://github.com/casper-ecosystem/erc20/blob/d1e0d0b26668e4ca902fe90756be0d28859d447a/example/erc20-token/src/main.rs#L41)) through `contract calling` and `not querying`?

**Entry point example:** Return a `keccak256_bytes_hash` for provided `string_value`. In that example I don't want to store any values on chain. But would like to consume a `string`, compute `hash` and return it to a caller.

**Question:** How that logic could be tested using [casper-engine-test-support](https://crates.io/crates/casper-engine-test-support)?

**Answer:**
- [1](https://rengocapital.slack.com/archives/C02B1QEEMHP/p1658927698117709?thread_ts=1658925847.965779&cid=C02B1QEEMHP). Use another session code that calls into the contract. No other option.
- [2](https://rengocapital.slack.com/archives/C02B1QEEMHP/p1658928454586939?thread_ts=1658925847.965779&cid=C02B1QEEMHP). https://github.com/casper-ecosystem/kyc-proxy-contract/blob/master/contract/src/test_contract.rs

## Error codes
### 1820
- 999 - "Not the manager" (managers_registry.rs)
- 1000 - "Not the manager" (implementers_registry.rs)
- 1001 - "Does not implement the interface" (implementers_registry.rs)

## [lib.rs](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/main/erc1820/src/lib.rs)

1. Implement `interfaceHash(String)` function.
That returns `hash`(keccak256 bytes) of provided `string` value.

function interfaceHash(
    string calldata _interfaceName
    ) external pure returns(bytes32) {
        return keccak256(abi.encodePacked(_interfaceName));
}

2. Delete unused codelines [here](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/lib.rs#L14)

mod address;

## [managers_registry.rs](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/main/erc1820/src/managers_registry.rs)

1. Rename `manager` into `new_manager` [here](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/managers_registry.rs#L17) and [here](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/managers_registry.rs#L29)

2. Rename `previous_manager` into `current_manager` [here](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/managers_registry.rs#L19) and [here](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/managers_registry.rs#L20)

3. Rename `manager` into `current_manager` [here](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/managers_registry.rs#L36) and [here](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/managers_registry.rs#L41)

4. Rename `hash_account` into `hash_string` [here](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/managers_registry.rs#L24) and [here](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/managers_registry.rs#L28)

5. Change `unwrap_or_default()` for `unwrap()` [`here`](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/managers_registry.rs#L33)

7. [`get_manager(account)`](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/managers_registry.rs#L33)
Should return `account` if `manager` is not registered.
**Currently**: Returns `Caller`

pub fn get_manager(manager_uref: URef, account: Key) -> Key {
    let caller = runtime::get_caller();
    let hash_string = to_str(account);
    let manager: Key = storage::dictionary_get(
        manager_uref,
        hash_string.as_str()
    ).unwrap_or_default().unwrap_or(Key::Account(caller));
    
    manager
}

Update:

pub fn get_manager(manager_uref: URef, account: Key) -> Key {
    let hash_string = to_str(account);
    let manager: Key = storage::dictionary_get(
        manager_uref,
        hash_string.as_str()
    ).unwrap_or_default().unwrap_or(account);
    
    manager
}

7. [`set_manager(manager_uref, account, manager)`](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/managers_registry.rs#L17)
Should check if `caller` is a current `manager` of `account`.
Throw error `Not the manager` if not.

require(getManager(_addr) == msg.sender, "Not the manager");

Change this:

let previous_manager = get_manager(manager_uref, account);
if previous_manager.ne(&account) {
    return Err(ApiError::User(999))
}

For this:

let caller_hash = runtime::get_caller();
let caller_key = Key::Account(caller);
let previous_manager = get_manager(manager_uref, account);
if previous_manager.ne(caller_key) {
        return Err(ApiError::User(error_code_not_the_manager))
    }

8. Unused codelines [here](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/managers_registry.rs#L6) and [here](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/managers_registry.rs#L8):

use casper_contract::unwrap_or_revert::UnwrapOrRevert,

use casper_types::account::AccountHash;

## [implementers_registry.rs](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/main/erc1820/src/implementers_registry.rs)

1. Update `use ::{detail};` into `use ::detail;` [here](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/implementers_registry.rs#L6)

2. [`get_implementer(implementer_uref, account, interface_hash)`](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/implementers_registry.rs#L43)
Leave option, when user specified `empty` `account` value
Consume `Caller` as `account: Key`.

/// (If '_addr' is the zero address then 'msg.sender' is assumed.)

address addr = _addr == address(0) ? msg.sender : _addr;

Change:

pub fn get_implementer(implementer_uref: URef, account: Key, interface_hash: Bytes) -> Key {
    let hash_string = to_str(account, interface_hash);
    let implementer = storage::dictionary_get(
        implementer_uref,
        hash_string.as_str()
    ).unwrap().unwrap_or(Key::Account(AccountHash::default()));

    implementer
}

For:

pub fn get_implementer(implementer_uref: URef, _account: Key, interface_hash: Bytes) -> Key {
    
    let account: Key;
    If _account.eq(Key::Account(AccountHash::default())) {
        let caller = runtime::get_caller();
        account = Key::Account(caller);
    } else {
        account = _account;
    }
        
    let hash_string = to_str(account, interface_hash);
    let implementer = storage::dictionary_get(
        implementer_uref,
        hash_string.as_str()
    ).unwrap().unwrap_or(Key::Account(AccountHash::default()));

    implementer
}

3. [`create_or_update_implementer(implementer_uref, account, interface_hash, implementer, manager)`](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/implementers_registry.rs#L17)
Leave option, when user specified `empty` `account` value
Consume `Caller` as `account: Key`.

/// (If '_addr' is the zero address then 'msg.sender' is assumed.)

Change:

pub fn create_or_update_implementer(
    implementer_uref: URef,
    account: Key,
    interface_hash: Bytes,
    implementer: Key,
    manager: Key
) -> Result<(), ApiError> {
    let hash_string: String;

    if manager.ne(&account) {
        return Err(ApiError::User(1000));
    }

    if implementer.eq(&manager) || implementer.eq(&Key::Account(AccountHash::default())) {
        return Err(ApiError::User(1001))
    }

    hash_string = to_str(account, interface_hash);

    storage::dictionary_put(
        implementer_uref,
        hash_string.as_str(),
        implementer);
    Ok(())
}

For:

pub fn create_or_update_implementer(
    implementer_uref: URef,
    account: Key,
    interface_hash: Bytes,
    implementer: Key,
    manager: Key
) -> Result<(), ApiError> {
    
    let hash_string: String;

    let caller_hash = runtime::get_caller();
    let caller_key = Key::Account(caller);
    
    let account: Key;
    If _account.eq(Key::Account(AccountHash::default())) {
        account = caller_key;
    } else {
        account = _account;
    }    

    if manager.ne(&account) {
        return Err(ApiError::User(1000));
    }

    if implementer.eq(&manager) || implementer.eq(&Key::Account(AccountHash::default())) {
        return Err(ApiError::User(1001))
    }

    hash_string = to_str(account, interface_hash);

    storage::dictionary_put(
        implementer_uref,
        hash_string.as_str(),
        implementer);
    Ok(())
}

4. [`create_or_update_implementer(implementer_uref, account, interface_hash, implementer, manager)`](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/implementers_registry.rs#L17)
Should check that `caller` is a `manager`
**Currently**: Checks if `account` is a `manager`

Change:

if manager.ne(&account) {
    return Err(ApiError::User(1000));
}

For:

let caller_hash = runtime::get_caller();
let caller_key = Key::Account(caller);
    
if manager.ne(&caller_key) {
    return Err(ApiError::User(error_code_not_the_manager));
}

5. Need to figure out how to:
Check that `implementer` can implement `interface_hash` for `account`
**Before that need to:**
Verify that `implementer` is not `0` or `empty` or `default`

Solidity:

if (_implementer != address(0) && _implementer != msg.sender) {
            require(
                ERC1820ImplementerInterface(_implementer)
                    .canImplementInterfaceForAddress(_interfaceHash, addr) == ERC1820_ACCEPT_MAGIC,
                "Does not implement the interface"
            );
}

You actually can register an `implementer` with `address zero`.
It means you will erase implementation for `account`
**Change:**

if implementer.eq(&manager) || implementer.eq(&Key::Account(AccountHash::default())) {
    return Err(ApiError::User(1001))
}

**For:**

let caller_hash = runtime::get_caller();
let caller_key = Key::Account(caller);
let default_account = Key::Account(AccountHash::default());

/// Verify that `implementer` is not `0` or `empty` or `default`
/// And `implementer` is not a `caller`
if implementer.ne(default_account) && implementer.ne(caller_key) {
    
    /// Check that `implementer` can implement `interface_hash` for `account`
    /// Figure out how to check interface implementation
    return Err(ApiError::User(error_code_does_not_implement_the_interface))
}

## [entry_points.rs](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/entry_points.rs)

1. [`interfaceHash()`](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/entry_points.rs#L71)
Change `SET_INTERFACE_ENTRY_POINT` for `INTERFACE_HASH_ENTRY_POINT`
Update `vec![]` of entry_point arguments:
Eliminate: `ACCOUNT_RUNTIME_ARG_NAME`, `I_HASH_RUNTIME_ARG_NAME`, `IMPLEMENTER_RUNTIME_ARG_NAME`.
Add: `INTERFACE_NAME_ARG_NAME`

2. [`default()`](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/entry_points.rs#L86)
Add `interfaceHash()` function into default `entry_point` list.

3. Change `CLType::Key` for `CLType::Unit`

## [address.rs](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/main/erc1820/src/address.rs)

ERC20 `address` implementation could be deleted, since not used.

## [constants.rs](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/main/erc1820/src/constants.rs)

1. [Next](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/constants.rs#L20) `entry_point` constants could be deleted since not used.
- `UPDATE_ERC165_CACHE`
- `IMPLEMENTERS_ERC165_INTERFACE`
- `IMPLEMENTERS_ERC165_INTERFACE_NO_CACHE`

2. Update [constant name](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/constants.rs#L18)
From `INTERFACE_HASH` into `INTERFACE_HASH_ENTRY_POINT`

3. Update [constant value](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/constants.rs#L31). To suit rest constant values.
From `i_hash` into `interface_hash`

## [lib.rs](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/main/erc1820/src/lib.rs)

1. Unused imports:
- `String` [here](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/lib.rs#L21)

2. Rename:
- `i_hash` for `interface_hash` on linse [64](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/lib.rs#L64), [70](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/lib.rs#L70), [77](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/lib.rs#L77) and [81](https://github.com/Rengo-Labs/CasperLabs-ERC777/blob/9970a180a20f70bf6b0eefb9bd73cd8193e21e8a/erc1820/src/lib.rs#L81).

3. Update comments. Some comments are copy pasted from different sources (erc20). And not enough details about function logic and checks.