Replace NEP-5 with NEP-17 #1558

fyrchik · 2020-11-20T10:05:25Z

Add Name field to manifest.
Fix calling from native contracts (need to discuss with NEO).
NEP-17.

TODO

update transfer costs

pkg/compiler/debug.go

pkg/core/native/native_nep17.go

roman-khimov · 2020-11-20T12:02:13Z

And take a look at RPC client also, NEP5Name is no longer relevant, for example.

AnnaShaleva

There's also a binary file added (cli/testdata/verify.nef ), do we need it?

pkg/smartcontract/manifest/manifest.go

AnnaShaleva · 2020-11-23T08:19:02Z

pkg/core/native/oracle.go

@@ -402,6 +409,10 @@ func (o *Oracle) verify(ic *interop.Context, _ []stackitem.Item) stackitem.Item
 	return stackitem.NewBool(ic.Tx.HasAttribute(transaction.OracleResponseT))
 }

+func (o *Oracle) onPayment(_ *interop.Context, _ []stackitem.Item) stackitem.Item {
+	return stackitem.Null{}


Just a question, don't we need to check that calling script hash is GAS and panic if not? Otherwise, Oracle contract will accept any other token.

I think this needs to be discussed with core devs. I mean, it's correct and expected behavior, but at the same time we need to be compatible.

neo-project/neo#2093

Fixed, allow GAS only.

There is a problem with this: when invoking native methods directly, calling script hash is not updated. And when invoking them via contract.Call, GAS is spent.

Same problem in neo-project/neo#2094.

pkg/smartcontract/manifest/manifest.go

codecov · 2020-11-23T10:43:15Z

Codecov Report

Merging #1558 (31eca34) into master (5f21178) will increase coverage by 0.04%.
The diff coverage is 84.88%.

@@            Coverage Diff             @@
##           master    #1558      +/-   ##
==========================================
+ Coverage   77.54%   77.58%   +0.04%     
==========================================
  Files         233      233              
  Lines       18413    18438      +25     
==========================================
+ Hits        14278    14306      +28     
+ Misses       3120     3116       -4     
- Partials     1015     1016       +1

Impacted Files	Coverage Δ
pkg/core/interop_system.go	`63.20% <0.00%> (+1.72%)`	⬆️
pkg/core/storage/store.go	`75.00% <ø> (ø)`
pkg/rpc/client/nep17.go	`49.05% <58.33%> (ø)`
pkg/core/dao/dao.go	`69.40% <66.66%> (ø)`
pkg/core/dao/cacheddao.go	`76.66% <81.81%> (ø)`
pkg/core/native/native_gas.go	`66.66% <81.81%> (ø)`
pkg/core/native/native_neo.go	`71.59% <81.81%> (ø)`
pkg/core/native/oracle.go	`81.14% <81.81%> (-0.06%)`	⬇️
pkg/rpc/server/server.go	`77.45% <85.71%> (-0.08%)`	⬇️
pkg/core/blockchain.go	`74.10% <86.66%> (ø)`
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f21178...31eca34. Read the comment docs.

roman-khimov · 2020-11-23T15:08:28Z

pkg/vm/context.go

+// InvocationState contains return convention and callback to be executed on context unload.
+type InvocationState struct {
+	// Callback is executed on context unload.
+	Callback func(ctx *Context)


Do we really need it?

We don't need it now, though it exists in C# code.
If we remove it, some tests for native contract will also be removed. I am ok with both variants, as we have it covered with tests.

I think we better not introduce things not need for the protocol to work. I'd say it's an internal C# thingie (that's not really used even there), so we can not complicate our code with it.

pkg/vm/emit/emit.go

roman-khimov · 2020-11-23T15:41:37Z

pkg/core/native/oracle.go

@@ -402,6 +409,10 @@ func (o *Oracle) verify(ic *interop.Context, _ []stackitem.Item) stackitem.Item
 	return stackitem.NewBool(ic.Tx.HasAttribute(transaction.OracleResponseT))
 }

+func (o *Oracle) onPayment(_ *interop.Context, _ []stackitem.Item) stackitem.Item {
+	return stackitem.Null{}


neo-project/neo#2093

roman-khimov

supportedstandards in YAML files were not updated to nep-17.

pkg/rpc/server/server.go

pkg/rpc/server/testdata/test_contract.go

When native method calls other contract result should be put on the stack of current context. With oracles this problem wasn't noticed because of void return type.

Sometimes amount of GAS consumed depends on block height.

roman-khimov

@AnnaShaleva: take a look and you can use data in onPayment now.

roman-khimov requested changes Nov 20, 2020

View reviewed changes

pkg/compiler/debug.go Outdated Show resolved Hide resolved

pkg/core/native/native_nep17.go Outdated Show resolved Hide resolved

pkg/core/native/native_nep17.go Outdated Show resolved Hide resolved

pkg/core/native/native_nep17.go Show resolved Hide resolved

AnnaShaleva requested changes Nov 23, 2020

View reviewed changes

roman-khimov reviewed Nov 23, 2020

View reviewed changes

roman-khimov requested changes Nov 23, 2020

View reviewed changes

pkg/rpc/server/server.go Show resolved Hide resolved

pkg/rpc/server/testdata/test_contract.go Show resolved Hide resolved

pkg/rpc/server/testdata/test_contract.go Show resolved Hide resolved

fyrchik added 6 commits November 24, 2020 11:23

manifest: include contract Name

c849176

emit: allow to emit nested arrays

4de233b

native: use proper stack for result

42ae226

When native method calls other contract result should be put on the stack of current context. With oracles this problem wasn't noticed because of void return type.

core: allow to provide block in GetTestVM()

a5914f8

Sometimes amount of GAS consumed depends on block height.

native: replace NEP-5 with NEP-17

b97dfae

*: replace all NEP5 occurences to NEP17

31eca34

roman-khimov approved these changes Nov 24, 2020

View reviewed changes

AnnaShaleva approved these changes Nov 24, 2020

View reviewed changes

roman-khimov merged commit ad30f4d into master Nov 24, 2020

roman-khimov deleted the nep17 branch November 24, 2020 12:09

roman-khimov mentioned this pull request Nov 30, 2020

Implement onPayment non-calling in some cases #1571

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace NEP-5 with NEP-17 #1558

Replace NEP-5 with NEP-17 #1558

fyrchik commented Nov 20, 2020 •

edited

Loading

roman-khimov commented Nov 20, 2020

AnnaShaleva left a comment

AnnaShaleva Nov 23, 2020

roman-khimov Nov 23, 2020 •

edited

Loading

roman-khimov Nov 23, 2020

fyrchik Nov 24, 2020

fyrchik Nov 24, 2020

roman-khimov Nov 24, 2020

codecov bot commented Nov 23, 2020 •

edited

Loading

roman-khimov Nov 23, 2020

fyrchik Nov 24, 2020

roman-khimov Nov 24, 2020

roman-khimov Nov 23, 2020

roman-khimov left a comment

roman-khimov left a comment

Replace NEP-5 with NEP-17 #1558

Replace NEP-5 with NEP-17 #1558

Conversation

fyrchik commented Nov 20, 2020 • edited Loading

roman-khimov commented Nov 20, 2020

AnnaShaleva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roman-khimov Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 23, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roman-khimov left a comment

Choose a reason for hiding this comment

roman-khimov left a comment

Choose a reason for hiding this comment

fyrchik commented Nov 20, 2020 •

edited

Loading

roman-khimov Nov 23, 2020 •

edited

Loading

codecov bot commented Nov 23, 2020 •

edited

Loading