refactor(zrc1): remove unnecessary validation

[ghost]

https://github.com/Zilliqa/ZRC/blob/master/reference/nonfungible-token.scilla#L374-L380

  current_token_id_count <- token_id_count;
  new_token_id_count = builtin add current_token_id_count one;
  token_id_count := new_token_id_count;
  token_id = new_token_id_count;
  IsTokenNotFound token_id;
  (* Mint new non-fungible token *)
  token_owners[token_id] := to;

Because of the above logic, IsTokenNotFound() will never throw an error. Even with the overflow case, there will be the CALL_CONTRACT_FAILED error with Cannot apply built-in Uint.add: an overflow/underflow occurred to a list of arguments message.

Also, IsTokenNotFound is only used for above.

procedure IsTokenNotFound(token_id: Uint256)
  token_exist <- exists token_owners[token_id];
  match token_exist with
  | False =>
  | True =>
    err = CodeTokenExists;
    ThrowError err
  end
end

Therefore, we can remove IsTokenNotFound() and simplify the logic in Minting().

[bb111189]

@noel-yoo On second thought, IsTokenNotFound token_id; might be actually checking whether a token exists or not. If exists, don't mint a minted token and throw an exception. i.e don't overwrite a minted token

[ghost]

@noel-yoo On second thought, IsTokenNotFound token_id; might be actually checking whether a token exists or not. If exists, don't mint a minted token and throw an exception.

Yes, IsTokenNotFound token_id is valid only if it's possible to throw the CodeTokenExists. Can you explain when that is possible?

The new token id can never conflict with another token id because

• the field token_id_count is increased by 1 each time
• the field token_id_count is mutated only by the Minting() procedure
• increment with overflow is not possible because of the run-time error CALL_CONTRACT_FAILED (i.e MAX-1 -> MAX -> 0 -> 1 -> 2 is not possible)
• Burn() doesn't mutate the the field token_id_count

If the above is correct then IsTokenNotFound token_id; will never throw the CodeTokenExists error because the following part never runs:

  | True =>
    err = CodeTokenExists;
    ThrowError err

If unnecessary validations are allowed

1. it will make engineers hard to test the codes when they should test every possible case
2. a flawed logic can be acceptable since the flawed logic can rely on this kind of validation

Therefore, if some validation is unnecessary, then we should consider removing it.

Although the validation is unnecessary and cannot be tested, still we have a strong reason to keep it then we can comment that why the code is unnecessary and what is the strong reason to keep it.

[bb111189]

I was thinking that that check could be a constraint. More for safety purposes.

While the reference contract has no issue, if any future developer is modifying the reference code and calling the procedure from other transitions carelessly, it might be dangerous.

With this constraint, it is guaranteed that this procedure cannot double mint regardless of who is calling the procedure.

I went to check the solidity code too also.

  function _mint(
    address _to,
    uint256 _tokenId
  )
    internal
    virtual
  {
    require(_to != address(0), ZERO_ADDRESS);
    require(idToOwner[_tokenId] == address(0), NFT_ALREADY_EXISTS);

    _addNFToken(_to, _tokenId);

    emit Transfer(address(0), _to, _tokenId);
  }

[ghost]

In the _mint() case, the validation makes more sense since it's not clear what _tokenId param will be.

In the Minting() case, the new token ID is quite clear because we have all context.