Skip to content

Add ContractMethodAttribute#778

Merged
erikzhang merged 7 commits intomasterfrom
3.0/ContractMethodAttribute
May 25, 2019
Merged

Add ContractMethodAttribute#778
erikzhang merged 7 commits intomasterfrom
3.0/ContractMethodAttribute

Conversation

@erikzhang
Copy link
Copy Markdown
Member

@erikzhang erikzhang commented May 24, 2019

  1. Use ContractMethodAttribute to replace the Main() method of native contracts.
  2. Use ContractMethodAttribute to create ABI for native contracts.
  3. Use ContractMethodAttribute to set safemethods for native contracts.
  4. Use ContractMethodAttribute to describe the price of native contract's method.
  5. Use ContractMethodAttribute to describe the allowed triggers of native contract's method.

@erikzhang erikzhang added this to the NEO 3.0 milestone May 24, 2019
@erikzhang erikzhang changed the title Create ContractMethodAttribute to replace the Main() method of native contracts Add ContractMethodAttribute May 24, 2019
@codecov-io
Copy link
Copy Markdown

codecov-io commented May 24, 2019

Codecov Report

Merging #778 into master will decrease coverage by 1.36%.
The diff coverage is 86.32%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #778      +/-   ##
==========================================
- Coverage   37.95%   36.58%   -1.37%     
==========================================
  Files         174      175       +1     
  Lines       12661    12401     -260     
==========================================
- Hits         4805     4537     -268     
- Misses       7856     7864       +8
Impacted Files Coverage Δ
neo/Cryptography/ECC/ECPoint.cs 74.9% <ø> (+0.9%) ⬆️
...eo/SmartContract/Native/ContractMethodAttribute.cs 100% <100%> (ø)
neo/SmartContract/Native/PolicyContract.cs 100% <100%> (ø) ⬆️
neo/SmartContract/Native/Tokens/GasToken.cs 38.23% <20%> (-28.44%) ⬇️
neo/SmartContract/Native/Tokens/NeoToken.cs 47.41% <75%> (-14.97%) ⬇️
neo/SmartContract/Native/Tokens/Nep5Token.cs 96.4% <86.95%> (-1.84%) ⬇️
neo/SmartContract/Native/NativeContract.cs 85.26% <90.32%> (+1.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f26968f...6e967e2. Read the comment docs.

Copy link
Copy Markdown
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

Good refactor

Co-Authored-By: Shargon <shargon@gmail.com>
Copy link
Copy Markdown
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.

I agree with all 3 points, @erikzhang.
Nice insight. This way we gonna keep a good standard that will be quite readable.

Copy link
Copy Markdown
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.

It is much clear as it is now with StackItem being return and the same standard as contracts.

The other 2 points are going to be done here as well, right?
Later we check again.

{
ContractMethodAttribute attribute = method.GetCustomAttribute<ContractMethodAttribute>();
if (attribute is null) continue;
string name = attribute.Name ?? (method.Name.ToLower()[0] + method.Name.Substring(1));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
string name = attribute.Name ?? (method.Name.ToLower()[0] + method.Name.Substring(1));
string name = attribute.Name ?? method.Name[0].ToString().ToLowerInvariant() + method.Name.Substring(1));

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It won't be faster.

return true;
}

[ContractMethod]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that we can set the name as mandatory

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Why?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Because we can prevent future issues like:

Question: I call OnXXXX and it doesn't work
Answer: Is onXXX

😆

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It automatically set the method names in the ABI, so it won't be a problem.

@vncoelho
Copy link
Copy Markdown
Member

Nice commit, Erik: 0f9758b

@erikzhang erikzhang marked this pull request as ready for review May 24, 2019 13:57
@erikzhang erikzhang requested review from shargon and vncoelho May 25, 2019 04:09
@erikzhang erikzhang merged commit 56866f4 into master May 25, 2019
@erikzhang erikzhang deleted the 3.0/ContractMethodAttribute branch May 25, 2019 11:22
@erikzhang erikzhang removed this from the NEO 3.0 milestone Dec 6, 2019
Thacryba pushed a commit to simplitech/neo that referenced this pull request Feb 17, 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.

4 participants