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

Check NEP11 methods #777

Closed
wants to merge 10 commits into from

Conversation

Ashuaidehao
Copy link
Contributor

No description provided.

@roman-khimov
Copy link
Contributor

Hi, neo-project/neo#1883. You can take a look at https://pkg.go.dev/github.com/nspcc-dev/neo-go/pkg/smartcontract/manifest/standard#CheckABI as well, it has compliance checks implemented properly.

Log($"{state.Hash} is not nft:balance1!", LogLevel.Warning);
continue;
}
if (balanceMethod2 != null &&
Copy link
Member

@cschuchardt88 cschuchardt88 Aug 25, 2023

Choose a reason for hiding this comment

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

should be an else if Because the function is one or the other. Also if the token has both functions you don't check for that either. As the code sits right now, if 1st function exists look for other function. Which isn't valid to what NEP-11 says
https://github.com/neo-project/proposals/blob/master/nep-11.mediawiki#user-content-decimals

You need to get the decimals to determined which function to use.

Edit: this may help you sum up that function into a class
https://github.com/cschuchardt88/neo-modules/blob/RestServer/src/RestServer/Tokens/NEP11Token.cs

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 also balance1 and balance2 is a bad naming.
Let's say something like "balanceInteger" and "balanceDivisible"

Copy link
Member

Choose a reason for hiding this comment

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

Also there is no need to check to abi for these functions.

@vncoelho I think balanceDivisible and balanceNonDivisible would fit better.

@Jim8y Jim8y added Need Active Pr will be closed after one week if no new activity. waiting for review labels Feb 12, 2024
@vncoelho vncoelho removed the Need Active Pr will be closed after one week if no new activity. label Mar 19, 2024
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.

@Ashuaidehao, can you check @cschuchardt88

Otherwise, @cschuchardt88, can you modify the aforementioned if statement and let's finish this PR?

@cschuchardt88
Copy link
Member

@Ashuaidehao @vncoelho

Basic Simple Check

if (Decimals == 0)
{
  return BalanceOf(ownerAddress);
}
else
{
  return BalanceOf(ownerAddress, tokenId);
}

@Jim8y
Copy link
Contributor

Jim8y commented May 11, 2024

close as author agrees this is not necessary

@Jim8y Jim8y closed this May 11, 2024
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.

6 participants