neo icon indicating copy to clipboard operation
neo copied to clipboard

Extend `CalculateNetworkFee` with contract verification

Open shargon opened this issue 1 year ago • 15 comments

Description

Close https://github.com/neo-project/neo/issues/2805

Type of change

  • [ ] Optimization (the change is only an optimization)
  • [ ] Style (the change is only a code style for better maintenance or standard purpose)
  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ ] This change requires a documentation update

How Has This Been Tested?

  • [ ] We don't have a unit test system that allow us to deploy a dummy contract for verify methods... I think that it should be tested locally

Test Configuration:

Checklist:

  • [x] My code follows the style guidelines of this project
  • [x] I have performed a self-review of my code
  • [x] I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
  • [x] My changes generate no new warnings
  • [ ] I have added tests that prove my fix is effective or that my feature works
  • [x] New and existing unit tests pass locally with my changes
  • [ ] Any dependent changes have been merged and published in downstream modules

shargon avatar Jul 02 '24 08:07 shargon

@neo-project/core anything else?

shargon avatar Jul 23 '24 07:07 shargon

@shargon how about roman's comment: https://github.com/neo-project/neo/pull/3385#discussion_r1696377899

Jim8y avatar Jul 30 '24 09:07 Jim8y

@shargon how about roman's comment: #3385 (comment)

Map was before, and was removed because a review https://github.com/neo-project/neo/pull/3385#discussion_r1665156066

shargon avatar Jul 30 '24 17:07 shargon

merge?

shargon avatar Aug 01 '24 08:08 shargon

is it possible to add some tests to demonstrate that your change achieves what you intended to? i will reapprove immediately if you can do it.

Jim8y avatar Aug 01 '24 10:08 Jim8y

merge?

https://github.com/neo-project/neo/pull/3385#discussion_r1696381840

Non-standard verification scripts are still not supported.

roman-khimov avatar Aug 01 '24 12:08 roman-khimov

merge?

#3385 (comment)

Non-standard verification scripts are still not supported.

You mean without a contract?

shargon avatar Aug 09 '24 16:08 shargon

Yes. Like in #3209.

roman-khimov avatar Aug 09 '24 16:08 roman-khimov

Yes. Like in #3209.

It's done in neo-go? i can't find it

shargon avatar Aug 09 '24 17:08 shargon

It's done in neo-go? i can't find it

Take a look at https://github.com/nspcc-dev/neo-go/blob/79e78980c4679e8929ceb509ed56359c5b017279/pkg/services/rpcsrv/server.go#L1001, we take signer's witness, infer invocation script (if needed) and don't change verification script. And hence, verification script is executed by s.chain.VerifyWitness "as is" without any changes even if it's not a contract/sig/multisig script.

AnnaShaleva avatar Aug 09 '24 17:08 AnnaShaleva

It's done in neo-go? i can't find it

Take a look at https://github.com/nspcc-dev/neo-go/blob/79e78980c4679e8929ceb509ed56359c5b017279/pkg/services/rpcsrv/server.go#L1001, we take signer's witness, infer invocation script (if needed) and don't change verification script. And hence, verification script is executed by s.chain.VerifyWitness "as is" without any changes even if it's not a contract/sig/multisig script.

Any following work?

Jim8y avatar Sep 05 '24 10:09 Jim8y

We may merge #3385 as it is now and move non-standard verification scripts support to another issue. I think it's acceptable way since #3385 contains everything that's required except non-standard verification scripts handling.

https://github.com/neo-project/neo/pull/3434/files#r1725538241

shargon avatar Sep 16 '24 08:09 shargon

Anyway you can do this https://github.com/neo-project/neo/issues/3481#issue-2511922347

cschuchardt88 avatar Sep 16 '24 08:09 cschuchardt88

Anyway you can do this #3481 (comment)

in a different pr please, this is for a different thing

shargon avatar Sep 16 '24 10:09 shargon

@shargon you have conflict

cschuchardt88 avatar Oct 13 '24 04:10 cschuchardt88