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.

Sign up for GitHub

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

Jump to bottom

manifest: add NEP-24 #3560

Open

AliceInHunterland wants to merge 1 commit into master from nep24

+558 −0

Contributor

AliceInHunterland commented Aug 19, 2024 •

edited

Loading

Close #3451

wrapper generates:

// RoyaltyInfo invokes `royaltyInfo` method of contract.
func (c *ContractReader) RoyaltyInfo(tokenID []byte, royaltyToken util.Uint160, salePrice *big.Int) ([]stackitem.Item, error) {
	return unwrap.Array(c.invoker.Call(c.hash, "royaltyInfo", tokenID, royaltyToken, salePrice))
}

but it should be different.

add tests with smartcontact/testdata/

AliceInHunterland force-pushed the nep24 branch from 40c7142 to 3a9fdda Compare

September 9, 2024 06:12

AliceInHunterland marked this pull request as ready for review

September 9, 2024 06:14

AliceInHunterland requested review from AnnaShaleva and roman-khimov as code owners

September 9, 2024 06:14

codecov bot commented Sep 9, 2024

Codecov Report

Attention: Patch coverage is 61.72840% with 31 lines in your changes missing coverage. Please review.

Project coverage is 85.74%. Comparing base (d9a6a7c) to head (3a9fdda).

Files with missing lines	Patch %	Lines
pkg/rpcclient/nep11/royalty.go	64.10%	15 Missing and 13 partials ⚠️
pkg/smartcontract/rpcbinding/binding.go	0.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3560      +/-   ##
==========================================
- Coverage   85.79%   85.74%   -0.05%     
==========================================
  Files         330      331       +1     
  Lines       38536    38617      +81     
==========================================
+ Hits        33062    33113      +51     
- Misses       3928     3943      +15     
- Partials     1546     1561      +15

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AnnaShaleva reviewed

View reviewed changes

pkg/rpcclient/nep11/royalty.go Outdated Show resolved Hide resolved

pkg/rpcclient/nep11/royalty.go Outdated Show resolved Hide resolved

pkg/rpcclient/nep11/royalty.go Outdated Show resolved Hide resolved

pkg/rpcclient/nep11/royalty.go Outdated Show resolved Hide resolved

pkg/rpcclient/nep11/royalty.go Outdated Show resolved Hide resolved

pkg/rpcclient/nep11/royalty.go Outdated Show resolved Hide resolved

pkg/rpcclient/nep11/royalty_test.go Outdated Show resolved Hide resolved

pkg/rpcclient/nep11/royalty.go Outdated Show resolved Hide resolved

pkg/smartcontract/manifest/standard/nep24.go Outdated Show resolved Hide resolved

pkg/smartcontract/rpcbinding/binding.go Outdated Show resolved Hide resolved

AnnaShaleva reviewed

View reviewed changes

pkg/smartcontract/manifest/standard/nep24.go Outdated Show resolved Hide resolved


          manifest: add NEP-24

317b65d

Close #3451

Signed-off-by: Ekaterina Pavlova <[email protected]>

AliceInHunterland force-pushed the nep24 branch from 3a9fdda to 317b65d Compare

October 16, 2024 13:27

AnnaShaleva reviewed

View reviewed changes

pkg/rpcclient/nep24/royalty.go

		@@ -0,0 +1,182 @@
		// Package nep24 provides RPC wrappers for NEP-24 contracts, which define NFT royalties.
		package nep24

Member

AnnaShaleva Oct 17, 2024

manifest: add NEP-24

manifest: support NEP-24?

pkg/rpcclient/nep24/royalty.go

		@@ -0,0 +1,182 @@
		// Package nep24 provides RPC wrappers for NEP-24 contracts, which define NFT royalties.

Member

AnnaShaleva Oct 17, 2024

Take a look at documentation of nep17 and nep11 packages. Extend documentation for nep24 in compatible way.

NFT

NEP-11

pkg/rpcclient/nep24/royalty.go

+              	Amount  *big.Int
+              }
+              // RoyaltiesTransferredEvent represents a RoyaltiesTransferred event as defined in the NEP-24 standard.

Member

AnnaShaleva Oct 17, 2024

Line width.

pkg/rpcclient/nep24/royalty.go

Comment on lines +31 to +32

		// RoyaltyReader is an interface for contracts implementing NEP-24 royalties.
		type RoyaltyReader struct {

Member

AnnaShaleva Oct 17, 2024

It's not just an interface. Documentation should be meaningful.

neo-go/pkg/rpcclient/nep17/nep17.go

Lines 36 to 38 in 14ea5a8

    
           // TokenReader represents safe (read-only) methods of NEP-17 token. It can be 
        
           // used to query various data. 
        
           type TokenReader struct {

pkg/rpcclient/nep24/royalty.go

Comment on lines +33 to +34

		neptoken.Base
		Invoker nep11.Invoker

Member

AnnaShaleva Oct 17, 2024

It should not work like this, because the base for NEP24 is either NEP11 divisible or NEP11 non-divisible. I'd suggest:

Remove neptoken.Base from RoyaltyReader.
Replace nep11.Invoker with Invoker.
Adjust RPC binding in the following way: if contract implements NEP-11, then only structures from the nep11 package to the contract reader. If contract implements both NEP-11 and NEP-24, then include both nep11 and nep24 structures.

pkg/rpcclient/nep24/royalty.go

+              		return errors.New("invalid event structure: expected array of 5 items")
+              	}
+              	// Decoding RoyaltyToken

Member

AnnaShaleva Oct 17, 2024

Where did you get these comments from? They are not a part of generator.

pkg/rpcclient/nep24/royalty_test.go

Comment on lines +23 to +41

+              func (t *testAct) Call(contract util.Uint160, operation string, params ...any) (*result.Invoke, error) {
+              	return t.res, t.err
+              }
+              func (t *testAct) SendRun(script []byte) (util.Uint256, uint32, error) {
+              	return t.txh, t.vub, t.err
+              }
+              func (t *testAct) CallAndExpandIterator(contract util.Uint160, method string, maxItems int, params ...any) (*result.Invoke, error) {
+              	return t.res, t.err
+              }
+              func (t *testAct) TerminateSession(sessionID uuid.UUID) error {
+              	return t.err
+              }
+              func (t *testAct) TraverseIterator(sessionID uuid.UUID, iterator *result.Iterator, num int) ([]stackitem.Item, error) {
+              	return t.res.Stack, t.err
+              }

Member

AnnaShaleva Oct 17, 2024

No actor should be needed, this standard has a single method that is safe.

pkg/rpcclient/nep24/royalty_test.go

		@@ -0,0 +1,330 @@
		package nep24

Member

AnnaShaleva Oct 17, 2024

Include doc_test.go, create example with documentation based on https://github.com/nspcc-dev/neo-go/blob/master/pkg/rpcclient/nep17/doc_test.go.

pkg/smartcontract/manifest/standard/nep24.go

+              // Nep24Royalty is a NEP-24 Standard for NFT royalties.
+              var Nep24Royalty = &Standard{
+              	//Base: DecimalTokenBase,

Member

AnnaShaleva Oct 17, 2024

Remove it.

pkg/smartcontract/rpcbinding/binding.go

@@ @@ -413,6 +413,10 @@ func Generate(cfg binding.Config) error { @@
               				mfst.ABI.Methods = dropStdMethods(mfst.ABI.Methods, standard.Nep11NonDivisible)
               				ctr.IsNep11ND = true
               			}
+              			if standard.ComplyABI(cfg.Manifest, standard.Nep24Royalty) == nil {
+              				mfst.ABI.Methods = dropStdMethods(mfst.ABI.Methods, standard.Nep24Royalty)
+              				ctr.IsNep11ND = true

Member

AnnaShaleva Oct 17, 2024

ctr.IsNep11ND = true

It's not.

Member

AnnaShaleva Oct 17, 2024

And you need to fix RPC bindings generator. Based on ctr.IsNep24 flag RPC bindings generator must include package nep24 to the generated contract reader.

AnnaShaleva reviewed

View reviewed changes

Member

AnnaShaleva left a comment

And you need to adjust

neo-go/pkg/smartcontract/manifest/standard/comply.go

Line 68 in 14ea5a8

func comply(m *manifest.Manifest, checkNames bool, st *Standard) error {

in the following way:

Extend standard.Standard structure with Required []string field. This field includes standards required by this standard.
In comply() check that standards marked as "Required" are present in the manifest. No compliance check is needed for them, the presence check is enough since compliance is checked by the calling code for all standards.

AnnaShaleva reviewed

View reviewed changes

Member

AnnaShaleva left a comment

Also, the test of RPC bindings generator based on the existing NEP contract is missing, we discussed it in DM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet