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

Get contract name in ToJson #2039

Closed
wants to merge 14 commits into from
Closed

Get contract name in ToJson #2039

wants to merge 14 commits into from

Conversation

superboyiii
Copy link
Member

@superboyiii superboyiii commented Oct 30, 2020

Related: neo-project/neo-modules#378
Partly support: neo-project/neo-modules#355
Enable get name in ToJson.

@roman-khimov
Copy link
Contributor

We might want to extend manifests to include some name for all contracts, btw. Neo 2 had that metadata and it allowed contract explorers to show something more meaningful than just hashes.

@superboyiii
Copy link
Member Author

We might want to extend manifests to include some name for all contracts, btw. Neo 2 had that metadata and it allowed contract explorers to show something more meaningful than just hashes.

Yes, might add it later.

superboyiii and others added 2 commits November 2, 2020 16:50
@superboyiii
Copy link
Member Author

Done

Tommo-L
Tommo-L previously approved these changes Nov 3, 2020
@superboyiii
Copy link
Member Author

@shargon Shall we merge?

@shargon
Copy link
Member

shargon commented Nov 3, 2020

Please take a look, I don't like to make difference between native and not native.

@erikzhang
Copy link
Member

May I ask why you need to get the name of the contract in json? Isn't this public information? They can be hard-coded into the SDK.

@roman-khimov
Copy link
Contributor

I think any contract should have a name.

@superboyiii superboyiii changed the title Get native name in ToJson Get contract name in ToJson Nov 4, 2020
@erikzhang
Copy link
Member

I think any contract should have a name.

I also think so. But how should they display their names? Some contracts use the name() method, and some use the manifest. If all contracts are required to have a name, then a unified way must be used to display the names.

@superboyiii
Copy link
Member Author

superboyiii commented Nov 4, 2020

May I ask why you need to get the name of the contract in json? Isn't this public information? They can be hard-coded into the SDK.

As neo-project/neo-modules#355 said, although hash is necessary, name/ID is more convenient to remember as one of the contract infos. I think we have duty to make it more friendly for development. And as @roman-khimov said, maybe we could make name and contact as required when deploying as neo2 did (neo2 get name in contract state from what they input when deploying) or we could make a check on name from manifest to ensure it contains name and return error msg when it's null.

@superboyiii
Copy link
Member Author

Please take a look, I don't like to make difference between native and not native.

Agree

@roman-khimov
Copy link
Contributor

Some contracts use the name() method, and some use the manifest

Manifests are probably more appropriate for this, both native and non-native contracts could have a field there for name.

@erikzhang
Copy link
Member

Manifests are probably more appropriate for this, both native and non-native contracts could have a field there for name.

Then we should modify NEP-5 and other standards, and remove the name() method.

@shargon
Copy link
Member

shargon commented Nov 5, 2020

I think that it will be easier with name method, I can change the code to execute name if we are agree

@roman-khimov
Copy link
Contributor

name() method requires spawning a VM, running some code and then dealing with the output, to me it's a bit harder than getting data from a field in the manifest. At the same time, name() is something available to other contracts unlike manifest, but that can be solved with some System.Contract.GetManifest syscall probably.

@Tommo-L
Copy link
Contributor

Tommo-L commented Nov 6, 2020

although hash is necessary, name/ID is more convenient to remember as one of the contract infos.

Wouldn't it be more convenient to use NNS.

For me, we'd better keep name method, like NEP-5/11, we already have symbol, decimals methods. If just move name method to manifest file, it doesn't look consistent.

BTW, multiple contracts can also have the same name, and a query by name is not very reliable.

@Tommo-L
Copy link
Contributor

Tommo-L commented Nov 13, 2020

Now, we can move on

@shargon
Copy link
Member

shargon commented Nov 13, 2020

Now, we can move on
yes, we can take it from the manifest

@superboyiii
Copy link
Member Author

@roman-khimov @shargon @erikzhang Please review again.

@superboyiii superboyiii requested a review from Tommo-L November 19, 2020 06:50

json["isnative"] = false;
}

json["hash"] = ScriptHash.ToString();
json["script"] = Convert.ToBase64String(Script);
json["manifest"] = Manifest.ToJson();
Copy link
Member

Choose a reason for hiding this comment

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

The name is included in the manifest, and I think we don't need to add it to the top level of the json.

Copy link
Member Author

Choose a reason for hiding this comment

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

The name is included in the manifest, and I think we don't need to add it to the top level of the json.

Agree. I'll close this.

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.

5 participants