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

add example nep11 #1009

Merged
merged 27 commits into from
Apr 17, 2024
Merged

add example nep11 #1009

merged 27 commits into from
Apr 17, 2024

Conversation

vikkkko
Copy link
Contributor

@vikkkko vikkkko commented Mar 29, 2024

No description provided.

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.

Perfect, can you verify the example https://github.com/neo-project/neo-devpack-dotnet/tree/master/examples/Example.SmartContract.NFT and maybe modify it as well?

I think there can be two examples there. Maybe give another name to each of them and move to the same folder.

Otherwise we can merge this example and do this merge later as well.

@Jim8y
Copy link
Contributor

Jim8y commented Mar 29, 2024

@vikkkko nice work, may you please use dotnet format to update the format?

Copy link
Member

@cschuchardt88 cschuchardt88 left a comment

Choose a reason for hiding this comment

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

Make changes and You need to write tests in order to approve this.

@cschuchardt88
Copy link
Member

Change project name to non-divisible NEP11.

Jim8y and others added 2 commits March 31, 2024 14:55
…11.csproj

Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com>
@vikkkko
Copy link
Contributor Author

vikkkko commented Apr 1, 2024

Make changes and You need to write tests in order to approve this.

Some tests will be added later.

public static bool Update(ByteString nefFile, string manifest)
{
if (IsOwner() == false)
throw new InvalidOperationException("No Authorization!");
Copy link
Member

Choose a reason for hiding this comment

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

I prefer Asserts because are not cacheable.

throw new InvalidOperationException("No Authorization!");
for (uint i = 0; i < royaltyInfos.Length; i++)
{
if (((UInt160)royaltyInfos[i]["royaltyRecipient"]).IsValid == false ||
Copy link
Member

Choose a reason for hiding this comment

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

check existence?

Copy link
Member

Choose a reason for hiding this comment

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

check existence?

How? Maybe just a new address.

Copy link
Contributor

Choose a reason for hiding this comment

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

Royalties are not part of the NEP-11 standard. Isn't this going to confuse people?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should either add a comment to the contract code that it (additionally) implements neo-project/proposals#155 or we should create one more separate example that includes Royalty standard implementation (and remove this royalty-related code from this contract).

)
throw new InvalidOperationException("Parameter error");
}
Storage.Put(PrefixRoyalty + tokenId, StdLib.Serialize(royaltyInfos));
Copy link
Member

Choose a reason for hiding this comment

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

Check tokenId format

[Safe]
public static bool Verify() => IsOwner();

public static bool Update(ByteString nefFile, string manifest)
Copy link
Member

Choose a reason for hiding this comment

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

Add data and send it to Update method

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a side note: The ContractManagement will use byte[] and not string in the manifest parameter. I can see many examples using it this way. I'm not sure why, maybe it's because of Neo Express, but I can't be sure.

{
if (IsOwner() == false)
throw new InvalidOperationException("No Authorization!");
if (newOwner != null && newOwner.IsValid)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (newOwner != null && newOwner.IsValid)
if (newOwner != null && newOwner.IsValid && !newOwner.IsZero)

{
/// <inheritdoc />
[DisplayName("SampleNonDivisibleNEP11Token")]
[ContractAuthor("core-dev", "dev@neo.org")]
Copy link
Contributor

Choose a reason for hiding this comment

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

The author and email can be yours.

Copy link
Contributor

@lock9 lock9 left a comment

Choose a reason for hiding this comment

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

Hello,
Some extra feedback:

  • This example will only work with C#. It would be helpful if there were examples that didn't use inheritance.
  • The following contract can't receive payments. I would one that uses the user data parameter.

throw new InvalidOperationException("No Authorization!");
for (uint i = 0; i < royaltyInfos.Length; i++)
{
if (((UInt160)royaltyInfos[i]["royaltyRecipient"]).IsValid == false ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Royalties are not part of the NEP-11 standard. Isn't this going to confuse people?

throw new InvalidOperationException("No Authorization!");
for (uint i = 0; i < royaltyInfos.Length; i++)
{
if (((UInt160)royaltyInfos[i]["royaltyRecipient"]).IsValid == false ||
Copy link
Member

Choose a reason for hiding this comment

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

I think we should either add a comment to the contract code that it (additionally) implements neo-project/proposals#155 or we should create one more separate example that includes Royalty standard implementation (and remove this royalty-related code from this contract).

@Jim8y
Copy link
Contributor

Jim8y commented Apr 11, 2024

@AnnaShaleva please check it again, the name has being updated.

@Jim8y
Copy link
Contributor

Jim8y commented Apr 11, 2024

please relove conflict @vikkkko

@superboyiii
Copy link
Member

superboyiii commented Apr 11, 2024

All done. Ready for merge.

<TargetFramework>net8.0</TargetFramework>
<Nullable>enable</Nullable>
</PropertyGroup>

Copy link
Member

Choose a reason for hiding this comment

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

This won't generate the *.nef file.

Copy link
Member

Choose a reason for hiding this comment

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

This won't generate the *.nef file.

If you want, we can add PostBuild in Target like:

  <Target Name="PostBuild" AfterTargets="PostBuildEvent">
    <Message Text="Start NeoContract converter, Source File: $(TargetPath)" Importance="high">
    </Message>
    <Exec Command="nccs &quot;$(ProjectPath)&quot;" />
  </Target>

But maybe just keep in the same formation with other examples is better? People can go to project path and just type ncss.

Copy link
Member

@shargon shargon Apr 12, 2024

Choose a reason for hiding this comment

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

We should add it in all of them, but I think that we should find a different way to execute nccs, using a package we can do it without require user installation maybe.

(Not related to this PR)

Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com>
Jim8y
Jim8y previously approved these changes Apr 12, 2024
{
if (IsOwner() == false)
throw new InvalidOperationException("No Authorization!");
if (newOwner != null && newOwner.IsValid && newOwner.IsZero == false)
Copy link
Member

Choose a reason for hiding this comment

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

It's better to use an assert, otherwise if the validation fail, the user don't get any error

Copy link
Member

Choose a reason for hiding this comment

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

reason?

Copy link
Member

Choose a reason for hiding this comment

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

if (myValueIsgood){ Action }

If your value is bad, you get the same result, the user should check the storage changes after any call?

Jim8y
Jim8y previously approved these changes Apr 16, 2024
@superboyiii
Copy link
Member

@shargon All done

@Jim8y
Copy link
Contributor

Jim8y commented Apr 17, 2024

this is an example and unnecessarily blocked the proposal. exception or assert all make sense, not worth a debating here. Thus, i will merge.

@Jim8y Jim8y merged commit 35ee6f5 into neo-project:master Apr 17, 2024
3 checks passed
@roman-khimov
Copy link

this is an example

Yeah, this is an example and examples get copy-pasted into production code. Which means https://github.com/neo-project/neo-devpack-dotnet/pull/1009/files#r1562443533 will soon be in many C# NEP-11 implementations. And people will refer to this as the way to do things even though we know it's not the best one.

@shargon
Copy link
Member

shargon commented Apr 17, 2024

this is an example

Yeah, this is an example and examples get copy-pasted into production code. Which means https://github.com/neo-project/neo-devpack-dotnet/pull/1009/files#r1562443533 will soon be in many C# NEP-11 implementations. And people will refer to this as the way to do things even though we know it's not the best one.

I can't agree more, an example in the official repository is not an example, it is a template

@Jim8y
Copy link
Contributor

Jim8y commented Apr 18, 2024

Yeah, this is an example and examples get copy-pasted into production code. Which means https://github.com/neo-project/neo-devpack-dotnet/pull/1009/files#r1562443533 will soon be in many C# NEP-11 implementations. And people will refer to this as the way to do things even though we know it's not the best one.

�I wont block your best one example practice if you have one in your mind. But blocking a pr without telling what is your best practice is not ok for me. And i sort of not believing there exists a best one example, discussion here more like either or.

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.

9 participants