Non-fungible contract according to erc-721 standard #7
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mrtpain
reviewed
Sep 7, 2019
|
Hey! Overall, looking good! I briefly looked over the code but at the moment, I'm unfamiliar with the erc-721 standard. Give me some time to look over that and pull this PR in locally and I can review on a more granular level. |
Contributor
Author
|
A few things I noticed whilst using the sct tool: see comments on files. |
Neurosploit
commented
Sep 16, 2019
| /// <summary> | ||
| /// A non fungible token contract. | ||
| /// </summary> | ||
| public class NonFungibleToken : SmartContract, INonFungibleToken, ISupportsInterface |
Contributor
Author
There was a problem hiding this comment.
Why are multiple files and interfaces on the smartcontract class not supported by sct? Hard to keep things reusable and structured otherwise. I can create an abstract base class but interfaces are way nicer to work with and are easily copied between projects.
Collaborator
There was a problem hiding this comment.
We tried to keep the amount of functionality minimal in the first iteration. I definitely hear what you're saying about the interfaces, this is something we could look into support for.
Contributor
Author
There was a problem hiding this comment.
What should I do about this now? Leave it out?
Contributor
Author
There was a problem hiding this comment.
Also look into namespaces, why can't that work if the class is public?
Neurosploit
commented
Sep 16, 2019
| if (this.PersistentState.IsContract(to)) | ||
| { | ||
| ITransferResult result = this.Call(to, 0, "OnNonFungibleTokenReceived", new object[] { this.Message.Sender, from, tokenId, data }, 0); | ||
| Assert(Convert.ToBoolean(result.ReturnValue)); |
Contributor
Author
There was a problem hiding this comment.
From SCT:
System.Convert is not supported so I also have no way of converting this with a system class.
Why does Call return an result.ReturnValue object which I can't cast in any way too a bool? I also don't have a generic type of any sorts to pass an assumed value.
I can also not cast to Nullable < bool > to resolve the compiler error I get thus making it pretty hard to work with.
Collaborator
There was a problem hiding this comment.
You should be able to cast it using (bool)result.ReturnValue.
Contributor
Author
There was a problem hiding this comment.
Done, see commit after this comment.
Contributor
Author
|
After some more changes, removing the namespace and the interfaces this now validates: |
Neurosploit
commented
Sep 17, 2019
| } | ||
| catch (Exception ex) | ||
| { | ||
| if (ex is InvalidCastException || ex is NullReferenceException) |
Contributor
Author
There was a problem hiding this comment.
try catching also not supported, well then, the framework can deal with it.
codingupastorm
approved these changes
Nov 11, 2019
Collaborator
codingupastorm
left a comment
codingupastorm
left a comment
There was a problem hiding this comment.
Sorry this has taken me so long to have a look at, I wasn't even aware of it.
Looks great to me, can't fault it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request hopes to deliver a smart contract sample that implements the erc-721 standard as a stratis smart contract equivalent.
Notes:
and try it on a local testnet sidechain.