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 Nep11Token #1670

Closed
wants to merge 132 commits into from
Closed

Add Nep11Token #1670

wants to merge 132 commits into from

Conversation

doubiliu
Copy link
Contributor

@doubiliu doubiliu commented May 26, 2020

required for #1644

This is the pre-PR of Nns. The token prototype is implemented according to the Nep11 standard, and other NFT assets can inherit this class, simplifying the implementation

@doubiliu
Copy link
Contributor Author

doubiliu commented Jun 3, 2020

Can we merge it?

@erikzhang
Copy link
Member

Can we merge it?

Of course not. I'm still reviewing. And there is no approval at all.

public abstract string Symbol { get; }
public BigInteger Factor { get; }

public const byte Decimals = 0;
Copy link
Member

Choose a reason for hiding this comment

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

We can remove this const

return new InteropInterface(OwnerOf(engine.Snapshot, tokenId));
}

public virtual IEnumerator OwnerOf(StoreView snapshot, byte[] tokenId)
Copy link
Member

Choose a reason for hiding this comment

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

IEnumerable<StackItem> ?

IEnumerator enumerator = OwnerOf(engine.Snapshot, tokenId);
if (!enumerator.Next()) return false;
UInt160 owner = new UInt160(enumerator.Value().GetSpan().ToArray());
if (!engine.CheckWitnessInternal(owner)) return false;
Copy link
Member

Choose a reason for hiding this comment

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

If the owner it's the second in the enumerator?

Copy link
Contributor

Choose a reason for hiding this comment

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

It has only one owner, when Decimals = 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will not happen, because in this contract NFT is restricted to be indivisible, so there is no second owner.

Copy link
Member

Choose a reason for hiding this comment

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

Then why the enumerator?

Copy link
Contributor

Choose a reason for hiding this comment

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

We need IEnumerator for tokensOf methods of nep11 .

public static enumerator tokensOf(byte[] owner)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can use UInt160 as the return type of the internal tokensOf, it's ok for me.

src/neo/SmartContract/Native/Tokens/Nep11Token.cs Outdated Show resolved Hide resolved
src/neo/SmartContract/Native/Tokens/Nep11Token.cs Outdated Show resolved Hide resolved
@corollari
Copy link

Any thoughts on #1670 (comment)?

@erikzhang
Copy link
Member

Wait for #1629

@doubiliu
Copy link
Contributor Author

doubiliu commented Jul 2, 2020

go?

@erikzhang
Copy link
Member

Please wait.

[ContractMethod(0_08000000, CallFlags.AllowModifyStates)]
public virtual bool Transfer(ApplicationEngine engine, UInt160 to, byte[] tokenId)
{
if (tokenId.Length > MaxTokenIdLength) return false;
Copy link
Member

Choose a reason for hiding this comment

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

Accorging to
https://github.com/neo-project/proposals/pull/41/files?short_path=afd2225#diff-afd22253b102da74b022d8c022201ee6

The parameter tokenid SHOULD be a valid NFT. If not, this method SHOULD throw an exception.

[ContractMethod(0_01000000, CallFlags.AllowStates)]
public virtual BigInteger BalanceOf(StoreView snapshot, UInt160 account, byte[] tokenId)
{
if (tokenId.Length > MaxTokenIdLength) return BigInteger.Zero;
Copy link
Member

Choose a reason for hiding this comment

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

The parameter owner SHOULD be a 20-byte address. If not, this method SHOULD throw an exception.

We should unify it like https://github.com/neo-project/neo/pull/1670/files#diff-f7ac0fe60b0bcd46ff4d867627d38bf6R112

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not necessary, because the incorrectly formatted parameters cannot call the method

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I copied the wrong text, i means that token id should be checked in all methods in the same way, throwing an exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK.Got it.

@EdgeDLT
Copy link

EdgeDLT commented Aug 27, 2020

Happy to see this, a check from @hal0x2328 would be a good to see.

@doubiliu doubiliu closed this Dec 25, 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.

8 participants