Added validator class for NEP-17 contract

[cschuchardt88]

Closes #194

[ixje]

Some observations, thoughts and questions I have on this project and related to this PR

• Why are we doing contract standard validation in the ListBalances function? Validation feels like something we want to do PRIOR to deploying a contract not once it's already deployed.
• When doing validation we should probably make it log warnings by default instead of rejecting because otherwise it's going to be very hard to develop your own NEP-17 token if it needs to be compliant from the first moment you try to deploy.
• I don't really see much validation code beyond verifying that part of the 'interface' exists and it returns some values. I haven't checked if there exists code elsewhere but we could

  1. validate all required functions (and correct args/return types) exist based on the manifest. Right now for example we don't even check if there's a transfer function

  2. call such functions and validate they ad-here to the specification e.g.

     • symbol states

     This string MUST be valid ASCII, MUST NOT contain whitespace or control characters

     • decimals states

     This method MUST always return the same value every time it is invoked.

[cschuchardt88]

You are completely right, It's all up for discussion.

Why are we doing contract standard validation in the ListBalances function? Validation feels like something we want to do PRIOR to deploying a contract not once it's already deployed.

Only place i could current find NEP-17 stuff.

validate all required functions (and correct args/return types) exist based on the manifest.

When we deploy a contract in neo-express you don't provide manifest file? So one would have to add a command named validate maybe or check every contract to see if it is NEP-17/NEP-11 even tho the contract may not be one the user is deploying.

[ixje]

When we deploy a contract in neo-express you don't provide manifest file? So one would have to add a command named validate maybe or check every contract to see if it is NEP-17/NEP-11 even tho the contract may not be one the user is deploying.

We don't provide a manifest, but it is expected to be present in the same directory as the .nef and is loaded implicitly. It's not possible to deploy a contract without a manifest.

[chenzhitong]

The symbol method MUST always return the same value every time it is invoked.

How do you verify this?

[cschuchardt88]

The symbol method MUST always return the same value every time it is invoked.

How do you verify this?

One could check the script assembly. But i don't think we need to go that far.

[ixje]

The symbol method MUST always return the same value every time it is invoked.

How do you verify this?

Deploy the contract to a separate snapshot, and call the symbol method n times. It's not 100% perfect, but a best effort.
Nothing stops somebody from updating a contract later on and changing the symbol.

[cschuchardt88]

Nothing stops somebody from updating a contract later on and changing the symbol.

Not if we stop them at deploy or redeploy. 😃

[chenzhitong]

using Neo;
using Neo.SmartContract.Framework;
using Neo.SmartContract.Framework.Services;
using System;
using System.ComponentModel;
using System.Numerics;

namespace Test
{
    public class Contract1 : SmartContract
    {
        [DisplayName("Transfer")]
        public static event Action<UInt160, UInt160, BigInteger>  OnTransfer;

        public static byte Decimals() => 1;

        public static string Symbol() => "TEST";

        public static BigInteger BalanceOf(UInt160 _) => 100000000;

        public static bool Transfer(UInt160 from, UInt160 to, BigInteger amount, object data)
        {
            if (from is null || !from.IsValid) throw new Exception("The argument \"from\" is invalid.");
            if (to is null || !to.IsValid) throw new Exception("The argument \"to\" is invalid.");
            if (amount < 0) throw new Exception("The amount must be a positive number.");
            if (!Runtime.CheckWitness(from)) return false;
            OnTransfer(from, to, amount);
            return true;
        }
    }
}

In addition, contracts such as the one above, which have no storage area but are essentially NEP-17 compliant, are also difficult to verify. balanceOf and transfer notification summarize inconsistent results, which will be difficult to verify.

[ixje]

using Neo;
using Neo.SmartContract.Framework;
using Neo.SmartContract.Framework.Services;
using System;
using System.ComponentModel;
using System.Numerics;

namespace Test
{
    public class Contract1 : SmartContract
    {
        [DisplayName("Transfer")]
        public static event Action<UInt160, UInt160, BigInteger>  OnTransfer;

        public static byte Decimals() => 1;

        public static string Symbol() => "TEST";

        public static BigInteger BalanceOf(UInt160 _) => 100000000;

        public static bool Transfer(UInt160 from, UInt160 to, BigInteger amount, object data)
        {
            if (from is null || !from.IsValid) throw new Exception("The argument \"from\" is invalid.");
            if (to is null || !to.IsValid) throw new Exception("The argument \"to\" is invalid.");
            if (amount < 0) throw new Exception("The amount must be a positive number.");
            if (!Runtime.CheckWitness(from)) return false;
            OnTransfer(from, to, amount);
            return true;
        }
    }
}

In addition, contracts such as the one above, which have no storage area but are essentially NEP-17 compliant, are also difficult to verify. balanceOf and transfer notification summarize inconsistent results, which will be difficult to verify.

I'm not sure where the problem is? If they use storage doesn't matter. We should create a separate snapshot, provide that snapshot to the ApplicationEngine, then deploy and test against that engine. This simulates a "live environment" and doesn't care about how the contract internally arranges it's data.

Incorrect working of balanceOf can be detected by providing it a scripthash of 0x00..00, that should never have a balance and will thus fail this requirement

If the account is an unused address, this method MUST return 0.

Validating the correct behaviour of transfer is likely impossible because we don't know how to obtain a balance for an account. It could be using OnNEP17Payment, OnNEP11Payment or even a custom function.

[ixje]

Perhaps we should not try to solve implementation validation in this project, but have that as a separate tool. We can do manifest validation here as a basic validation. A separate test suite can be created to specifically to test the implementation. It can have requirements such as a callback function which ensures an account has balance or whatever format.

[cschuchardt88]

This is getting too complex to fast. I think we should validate what the proposals say and not assume how we think it should work. for example

Incorrect working of balanceOf can be detected by providing it a scripthash of 0x00..00, that should never have a balance and will thus fail this requirement

[cschuchardt88]

Problem here, some entities including flamingo.finance think you they don't need to follow the NEP-17 spec. For example their tokens use non-uppercase characters and non-latin alphabet. Also have a length of more than 3-8 characters as well. What do we do for that? see neo-project/proposals#164

[cschuchardt88]

This is for NEP-17 of #194