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

ReadOnly methods #93

Closed
wants to merge 34 commits into from
Closed

ReadOnly methods #93

wants to merge 34 commits into from

Conversation

shargon
Copy link
Member

@shargon shargon commented Aug 24, 2019

Required for neo-project/neo#1052

Fix some issues:

  • entrypoint to entryPoint (from string to method)
  • functions to methods
  • returntype to returnType
  • Unit test for cover all the abi

Copy link
Contributor

@igormcoelho igormcoelho left a comment

Choose a reason for hiding this comment

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

Please let's merge #94 first... too complicated to review now.

@igormcoelho
Copy link
Contributor

I'll have to reproduce your commits one by one... the conflicting file is exactly the most complicated one........ another excellent reason for doing 94 before, it would be impossible to review.

@igormcoelho
Copy link
Contributor

igormcoelho commented Aug 26, 2019

Can you merge @shargon? It's about 5 blocks of code... for me I'm blind, have to understand everything step by step to fix this... perhaps you can do it quicker, as you precisely know the changes.

@shargon
Copy link
Member Author

shargon commented Aug 26, 2019

@igormcoelho is done

@erikzhang
Copy link
Member

Can you give an example of the output json from the changes?

@shargon
Copy link
Member Author

shargon commented Aug 28, 2019

This is the final format (was adjusted considering that neo have the good one)

{
    "hash": "0x77811b3127dea2df1a18230f91396fbcf8c648f4",
    "methods": [
        {
            "name": "readOnlyTrue",
            "parameters": [],
            "returnType": "Void"
        },
        {
            "name": "readOnlyFalse1",
            "parameters": [],
            "returnType": "Void"
        },
        {
            "name": "readOnlyFalse2",
            "parameters": [],
            "returnType": "Void"
        }
    ],
    "readOnlyMethods": [
        "readOnlyTrue"
    ],
    "entryPoint": {
        "name": "Main",
        "parameters": [
            {
                "name": "method",
                "type": "String"
            },
            {
                "name": "args",
                "type": "Array"
            }
        ],
        "returnType": "Void"
    },
    "events": [
        {
            "name": "transfer",
            "parameters": [
                {
                    "name": "arg1",
                    "type": "ByteArray"
                },
                {
                    "name": "arg2",
                    "type": "ByteArray"
                },
                {
                    "name": "arg3",
                    "type": "Integer"
                }
            ]
        }
    ]
}

for the code:

public class Contract_Abi : SmartContract.Framework.SmartContract
    {
        [DisplayName("transfer")]
        public static event Action<byte[], byte[], BigInteger> Transferred;

        public static void Main(string method, object[] args) { }

        [ReadOnly(true)]
        public static void readOnlyTrue() { }

        [ReadOnly(false)]
        public static void readOnlyFalse1() { }

        public static void readOnlyFalse2() { }
    }

Note that Main is not in methods

@erikzhang
Copy link
Member

I think we don't have readOnlyMethods in ABI specification.

@shargon
Copy link
Member Author

shargon commented Aug 28, 2019

These changes are precisely for this neo-project/neo#927

@erikzhang
Copy link
Member

Maybe we can output in this format:

{
    "methods": [
        {
            "name": "readOnlyTrue",
            "parameters": [],
            "returnType": "Void",
            "readonly": true,
        }
    ]
}

Or just not modify the ABI, we keep readonly in the manifest.

@shargon
Copy link
Member Author

shargon commented Aug 28, 2019

I prefer to modify the abi, is more readable, i like your way.
If is on the abi, the compiler can deal with it

@erikzhang
Copy link
Member

Okay. Let's amend the NEP-3 specification first.

@igormcoelho
Copy link
Contributor

Fully agree on "readonly": true. Just one camel case standard... shouldn't it be readOnly:true ? Let's ammend NEP-3.

@shargon
Copy link
Member Author

shargon commented Sep 3, 2019

@igormcoelho could amend the NEP-3? and as you said, yes, must be in camel case

@shargon
Copy link
Member Author

shargon commented Sep 9, 2019

Changes made, requires updating the NEP-3 and NEO manifest before merging

@lock9 lock9 removed their request for review November 17, 2019 22:03
@shargon shargon closed this Dec 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants