Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Numerous modifications and optimization for Template.NEP5.CSharp #304

Merged
merged 24 commits into from
Jul 31, 2020

Conversation

chenzhitong
Copy link
Member

@chenzhitong chenzhitong commented Jun 23, 2020

Adjust the code structure:

  • Extract AssetStorage class and TotalSupplyStorage class to make it more versatile
  • Remove StoragePrefixBalance property and StoragePrefixContract property
  • Rename NEP5.Admin.cs to NEP5.Owner.cs
  • Remove redundant solution files

Optimize the writing:

  • Change ?.ToBigInteger() ?? 0 to TryToBigInteger()
  • Change
    new byte[] { 0xf6, 0x64, 0x43, 0x49, 0x8d, 0x38, 0x78, 0xd3, 0x2b, 0x99, 0x4e, 0x4e, 0x12, 0x83, 0xc6, 0x93, 0x44, 0x21, 0xda, 0xfe };
    
    new byte[] { 0x89, 0x77, 0x20, 0xd8, 0xcd, 0x76, 0xf4, 0xf0, 0x0a, 0xbf, 0xa3, 0x7c, 0x0e, 0xdd, 0x88, 0x9c, 0x20, 0x8f, 0xde, 0x9b };
    
    to
    "NiNmXL8FjEUEs1nfX9uHFBNaenxDHJtmuB".ToScriptHash();
    "0xde5f57d430d3dece511cf975a8d37848cb9e0525".HexToBytes();
    

Fix the balance error in Mint method:

  • Double counting balance OnTransfer(null, tx.Sender, balance + contribution);

Feature improvements:

  • Transfer method allows contract invocation
    change
    if (!Runtime.CheckWitness(from)) return false;
    
    to
    if (!Runtime.CheckWitness(from) && !from.Equals(ExecutionEngine.CallingScriptHash))
    {
        Error("No authorization.");
        return false;
    }
    
  • During contract migration, determine whether the new contract is the same as the old contract
    if (script != null && script.Equals(Blockchain.GetContract(ExecutionEngine.ExecutingScriptHash))) return true;
    

@chenzhitong chenzhitong changed the title Numerous modifications and optimization Numerous modifications and optimization for Template.NEP5.CSharp Jun 23, 2020
{
balances.Delete(from);
Error("The parameters from and to SHOULD be 20-byte addresses.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, should keep it, throw an exception. This is defined in nep5 standard.

The parameters from and to SHOULD be 20-byte addresses. If not, this method SHOULD throw an exception.

@Tommo-L
Copy link
Contributor

Tommo-L commented Jun 23, 2020

Waiting for #302

Notify method will be updated.

public static extern void Notify(string eventName, params object[] state);

Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great changes, @chenzhitong!

if (amount <= 0) throw new Exception("The parameter amount MUST be greater than 0.");
if (!IsPayable(to)) throw new Exception("Receiver cannot receive.");
if (!Runtime.CheckWitness(from) && !from.Equals(ExecutionEngine.CallingScriptHash)) throw new Exception("No authorization.");
if (AssetStorage.Get(from) < amount) throw new Exception("Insufficient balance.");
if (amount == 0 || from == to) return true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

amount <= 0 will throw exception, so amount == 0 is impossible in there?

@shargon
Copy link
Member

shargon commented Jul 23, 2020

@chenzhitong could you check our reviews?

@ProDog
Copy link
Contributor

ProDog commented Jul 24, 2020

Pass test, it's down @shargon .

@shargon
Copy link
Member

shargon commented Jul 28, 2020

@ProDog done, could you check it?

@ProDog
Copy link
Contributor

ProDog commented Jul 30, 2020

This can merge.

@erikzhang erikzhang merged commit ab40f26 into neo-project:master Jul 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants