SmartContract: fix native NEP-17 manifest construction #3195

AnnaShaleva · 2024-04-12T17:58:56Z

Description

Take into account event attributes declared onto the base class while constructing manifest for native contracts. Close #3194.

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

getcontractstate RPC call for native NeoToken and GasToken sent to the updated node. The result is expected and correct, see the result:

Updated NeoToken contract state

anna@kiwi:~/Documents/GitProjects/nspcc-dev/neo-go$ curl -d '{ "jsonrpc": "2.0", "id": 1, "method": "getcontractstate", "params": [-5] }' localhost:10332 | json_pp
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  3433    0  3358  100    75  1287k  29446 --:--:-- --:--:-- --:--:-- 1676k
{
   "id" : 1,
   "jsonrpc" : "2.0",
   "result" : {
      "hash" : "0xef4073a0f2b305a38ec4050e4d3d28bc40ea63f5",
      "id" : -5,
      "manifest" : {
         "abi" : {
            "events" : [
               {
                  "name" : "Transfer",
                  "parameters" : [
                     {
                        "name" : "from",
                        "type" : "Hash160"
                     },
                     {
                        "name" : "to",
                        "type" : "Hash160"
                     },
                     {
                        "name" : "amount",
                        "type" : "Integer"
                     }
                  ]
               },
               {
                  "name" : "CandidateStateChanged",
                  "parameters" : [
                     {
                        "name" : "pubkey",
                        "type" : "PublicKey"
                     },
                     {
                        "name" : "registered",
                        "type" : "Boolean"
                     },
                     {
                        "name" : "votes",
                        "type" : "Integer"
                     }
                  ]
               },
               {
                  "name" : "Vote",
                  "parameters" : [
                     {
                        "name" : "account",
                        "type" : "Hash160"
                     },
                     {
                        "name" : "from",
                        "type" : "PublicKey"
                     },
                     {
                        "name" : "to",
                        "type" : "PublicKey"
                     },
                     {
                        "name" : "amount",
                        "type" : "Integer"
                     }
                  ]
               },
               {
                  "name" : "CommitteeChanged",
                  "parameters" : [
                     {
                        "name" : "old",
                        "type" : "Array"
                     },
                     {
                        "name" : "new",
                        "type" : "Array"
                     }
                  ]
               }
            ],
            "methods" : [
               {
                  "name" : "balanceOf",
                  "offset" : 0,
                  "parameters" : [
                     {
                        "name" : "account",
                        "type" : "Hash160"
                     }
                  ],
                  "returntype" : "Integer",
                  "safe" : true
               },
               {
                  "name" : "decimals",
                  "offset" : 7,
                  "parameters" : [],
                  "returntype" : "Integer",
                  "safe" : true
               },
               {
                  "name" : "getAccountState",
                  "offset" : 14,
                  "parameters" : [
                     {
                        "name" : "account",
                        "type" : "Hash160"
                     }
                  ],
                  "returntype" : "Array",
                  "safe" : true
               },
               {
                  "name" : "getAllCandidates",
                  "offset" : 21,
                  "parameters" : [],
                  "returntype" : "InteropInterface",
                  "safe" : true
               },
               {
                  "name" : "getCandidateVote",
                  "offset" : 28,
                  "parameters" : [
                     {
                        "name" : "pubKey",
                        "type" : "PublicKey"
                     }
                  ],
                  "returntype" : "Integer",
                  "safe" : true
               },
               {
                  "name" : "getCandidates",
                  "offset" : 35,
                  "parameters" : [],
                  "returntype" : "Array",
                  "safe" : true
               },
               {
                  "name" : "getCommittee",
                  "offset" : 42,
                  "parameters" : [],
                  "returntype" : "Array",
                  "safe" : true
               },
               {
                  "name" : "getGasPerBlock",
                  "offset" : 49,
                  "parameters" : [],
                  "returntype" : "Integer",
                  "safe" : true
               },
               {
                  "name" : "getNextBlockValidators",
                  "offset" : 56,
                  "parameters" : [],
                  "returntype" : "Array",
                  "safe" : true
               },
               {
                  "name" : "getRegisterPrice",
                  "offset" : 63,
                  "parameters" : [],
                  "returntype" : "Integer",
                  "safe" : true
               },
               {
                  "name" : "registerCandidate",
                  "offset" : 70,
                  "parameters" : [
                     {
                        "name" : "pubkey",
                        "type" : "PublicKey"
                     }
                  ],
                  "returntype" : "Boolean",
                  "safe" : false
               },
               {
                  "name" : "setGasPerBlock",
                  "offset" : 77,
                  "parameters" : [
                     {
                        "name" : "gasPerBlock",
                        "type" : "Integer"
                     }
                  ],
                  "returntype" : "Void",
                  "safe" : false
               },
               {
                  "name" : "setRegisterPrice",
                  "offset" : 84,
                  "parameters" : [
                     {
                        "name" : "registerPrice",
                        "type" : "Integer"
                     }
                  ],
                  "returntype" : "Void",
                  "safe" : false
               },
               {
                  "name" : "symbol",
                  "offset" : 91,
                  "parameters" : [],
                  "returntype" : "String",
                  "safe" : true
               },
               {
                  "name" : "totalSupply",
                  "offset" : 98,
                  "parameters" : [],
                  "returntype" : "Integer",
                  "safe" : true
               },
               {
                  "name" : "transfer",
                  "offset" : 105,
                  "parameters" : [
                     {
                        "name" : "from",
                        "type" : "Hash160"
                     },
                     {
                        "name" : "to",
                        "type" : "Hash160"
                     },
                     {
                        "name" : "amount",
                        "type" : "Integer"
                     },
                     {
                        "name" : "data",
                        "type" : "Any"
                     }
                  ],
                  "returntype" : "Boolean",
                  "safe" : false
               },
               {
                  "name" : "unclaimedGas",
                  "offset" : 112,
                  "parameters" : [
                     {
                        "name" : "account",
                        "type" : "Hash160"
                     },
                     {
                        "name" : "end",
                        "type" : "Integer"
                     }
                  ],
                  "returntype" : "Integer",
                  "safe" : true
               },
               {
                  "name" : "unregisterCandidate",
                  "offset" : 119,
                  "parameters" : [
                     {
                        "name" : "pubkey",
                        "type" : "PublicKey"
                     }
                  ],
                  "returntype" : "Boolean",
                  "safe" : false
               },
               {
                  "name" : "vote",
                  "offset" : 126,
                  "parameters" : [
                     {
                        "name" : "account",
                        "type" : "Hash160"
                     },
                     {
                        "name" : "voteTo",
                        "type" : "PublicKey"
                     }
                  ],
                  "returntype" : "Boolean",
                  "safe" : false
               }
            ]
         },
         "extra" : null,
         "features" : {},
         "groups" : [],
         "name" : "NeoToken",
         "permissions" : [
            {
               "contract" : "*",
               "methods" : "*"
            }
         ],
         "supportedstandards" : [
            "NEP-17"
         ],
         "trusts" : []
      },
      "nef" : {
         "checksum" : 65467259,
         "compiler" : "neo-core-v3.0",
         "magic" : 860243278,
         "script" : "EEEa93tnQBBBGvd7Z0AQQRr3e2dAEEEa93tnQBBBGvd7Z0AQQRr3e2dAEEEa93tnQBBBGvd7Z0AQQRr3e2dAEEEa93tnQBBBGvd7Z0AQQRr3e2dAEEEa93tnQBBBGvd7Z0AQQRr3e2dAEEEa93tnQBBBGvd7Z0AQQRr3e2dAEEEa93tnQA==",
         "source" : "",
         "tokens" : []
      },
      "updatecounter" : 0
   }
}

Updated GasToken contract state

anna@kiwi:~/Documents/GitProjects/nspcc-dev/neo-go$ curl -d '{ "jsonrpc": "2.0", "id": 1, "method": "getcontractstate", "params": [-6] }' localhost:10332 | json_pp
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1250    0  1175  100    75   408k  26709 --:--:-- --:--:-- --:--:--  610k
{
   "id" : 1,
   "jsonrpc" : "2.0",
   "result" : {
      "hash" : "0xd2a4cff31913016155e38e474a2c06d08be276cf",
      "id" : -6,
      "manifest" : {
         "abi" : {
            "events" : [
               {
                  "name" : "Transfer",
                  "parameters" : [
                     {
                        "name" : "from",
                        "type" : "Hash160"
                     },
                     {
                        "name" : "to",
                        "type" : "Hash160"
                     },
                     {
                        "name" : "amount",
                        "type" : "Integer"
                     }
                  ]
               }
            ],
            "methods" : [
               {
                  "name" : "balanceOf",
                  "offset" : 0,
                  "parameters" : [
                     {
                        "name" : "account",
                        "type" : "Hash160"
                     }
                  ],
                  "returntype" : "Integer",
                  "safe" : true
               },
               {
                  "name" : "decimals",
                  "offset" : 7,
                  "parameters" : [],
                  "returntype" : "Integer",
                  "safe" : true
               },
               {
                  "name" : "symbol",
                  "offset" : 14,
                  "parameters" : [],
                  "returntype" : "String",
                  "safe" : true
               },
               {
                  "name" : "totalSupply",
                  "offset" : 21,
                  "parameters" : [],
                  "returntype" : "Integer",
                  "safe" : true
               },
               {
                  "name" : "transfer",
                  "offset" : 28,
                  "parameters" : [
                     {
                        "name" : "from",
                        "type" : "Hash160"
                     },
                     {
                        "name" : "to",
                        "type" : "Hash160"
                     },
                     {
                        "name" : "amount",
                        "type" : "Integer"
                     },
                     {
                        "name" : "data",
                        "type" : "Any"
                     }
                  ],
                  "returntype" : "Boolean",
                  "safe" : false
               }
            ]
         },
         "extra" : null,
         "features" : {},
         "groups" : [],
         "name" : "GasToken",
         "permissions" : [
            {
               "contract" : "*",
               "methods" : "*"
            }
         ],
         "supportedstandards" : [
            "NEP-17"
         ],
         "trusts" : []
      },
      "nef" : {
         "checksum" : 2663858513,
         "compiler" : "neo-core-v3.0",
         "magic" : 860243278,
         "script" : "EEEa93tnQBBBGvd7Z0AQQRr3e2dAEEEa93tnQBBBGvd7Z0A=",
         "source" : "",
         "tokens" : []
      },
      "updatecounter" : 0
   }
}

Checklist:

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

Take into account event attributes declared onto the base class for native contracts. Close #3194. Signed-off-by: Anna Shaleva <[email protected]>

roman-khimov

I don't want to think about the case with overrides of some kind for the same event name. Hopefully, we'll never do this for natives.

dusmart · 2024-04-13T07:09:23Z

Thanks for locating this bug.
BTW, this fix is also a hard fork.

AnnaShaleva · 2024-04-13T07:27:27Z

BTW, this fix is also a hard fork.

It’s not. This bug was introduced in post-3.6, in #2942, so this code was not in production yet, and the fix itself just fixes natives manifest and Management’s storage.

But you’re right from the point that this fix requires node resync from genesis to work.

Jim8y · 2024-04-13T12:32:09Z

Any possible UT for this fix?

superboyiii

Checked all native manifest. Yes, this really fixed.

AnnaShaleva · 2024-04-15T06:47:43Z

Any possible UT for this fix?

Sure, will add in a couple of hours.

vncoelho

Isn't it possible to set up a UT to ensure that this fix is correct?

vncoelho · 2024-04-15T11:49:25Z

Any possible UT for this fix?

I just saw @Jim8y request now. I was checking the code.

The code looks good and makes sense. It would be good to ensure a failed UT now and the fix for it.

vncoelho · 2024-04-15T11:51:53Z

This reminds me that it would be also good to generate automatically manifest for NEO and GAS instead of manually. That would had make this error appear earlier perhaps.

Jim8y · 2024-04-15T12:46:17Z

This reminds me that it would be also good to generate automatically manifest for NEO and GAS instead of manually. That would had make this error appear earlier perhaps.

Totally agree.

AnnaShaleva · 2024-04-15T13:53:33Z

Sure, will add in a couple of hours.

Two tests are added: one for particularly this bug (check Transfer events for NEP17 native contracts), and another one to check the whole set of native contract states at genesis block with all hardforks disabled. The expected manifests are checked precisely, so they are correct. Also checked against NeoGo's ones.

In future TestGenesisNativeState will help us to detect similar bugs.

AnnaShaleva · 2024-04-15T14:07:10Z

at genesis block with all hardforks disabled

With all hardforks enabled, actually. Let me fix this.

Ensure that contract states generated with all hardforks enabled match the expected values. Signed-off-by: Anna Shaleva <[email protected]>

AnnaShaleva · 2024-04-15T14:14:09Z

With all hardforks enabled, actually. Let me fix this.

Fixed.

Jim8y · 2024-04-15T14:24:43Z

With all hardforks enabled, actually. Let me fix this.

Fixed.

wait a sec, what is the new logic? hardforks enabled or disabled?

AnnaShaleva · 2024-04-15T14:32:31Z

wait a sec, what is the new logic? hardforks enabled or disabled?

The code itself preserves the old logic except that NEP17 notifications are included into the NEP17 state, so #3194 is fixed for both enabled/disabled hardforks.

The unit tests check the presence of these notifications and check natives state with all hardforks enabled since it's the default behaviour of TestBlockchain (it uses the default ProtocolSettings). I think it's perfectly OK for our tests, since we need to test fresh functionality as far. But for this particular bug it doesn't make any difference since Transfer notifications should always be present in the native state (with or without hardforks).

tests/Neo.UnitTests/SmartContract/Native/UT_NativeContract.cs

Jim8y · 2024-04-16T00:41:13Z

@shargon can be merged.

AnnaShaleva requested review from shargon, Jim8y, vncoelho, roman-khimov, superboyiii and cschuchardt88 April 12, 2024 17:59

AnnaShaleva force-pushed the fix-native-nep17 branch from 8c8531a to a303603 Compare April 12, 2024 18:08

SmartContract: fix native NEP-17 manifest construction

89f12f9

Take into account event attributes declared onto the base class for native contracts. Close #3194. Signed-off-by: Anna Shaleva <[email protected]>

AnnaShaleva force-pushed the fix-native-nep17 branch from a303603 to 89f12f9 Compare April 12, 2024 18:09

AnnaShaleva added Critical Issues (bugs) that need to be fixed ASAP Waiting for Review labels Apr 12, 2024

roman-khimov previously approved these changes Apr 12, 2024

View reviewed changes

Jim8y previously approved these changes Apr 13, 2024

View reviewed changes

superboyiii approved these changes Apr 15, 2024

View reviewed changes

shargon previously approved these changes Apr 15, 2024

View reviewed changes

vncoelho reviewed Apr 15, 2024

View reviewed changes

AnnaShaleva dismissed stale reviews from shargon, Jim8y, and roman-khimov via 2bd4fe5 April 15, 2024 13:50

AnnaShaleva requested review from vncoelho, shargon and Jim8y April 15, 2024 13:56

AnnaShaleva requested a review from superboyiii April 15, 2024 13:56

Jim8y previously approved these changes Apr 15, 2024

View reviewed changes

AnnaShaleva dismissed Jim8y’s stale review via 8102836 April 15, 2024 14:13

AnnaShaleva force-pushed the fix-native-nep17 branch from 2bd4fe5 to 8102836 Compare April 15, 2024 14:13

SmartContract: add UT to check native contract states

e8abd06

Ensure that contract states generated with all hardforks enabled match the expected values. Signed-off-by: Anna Shaleva <[email protected]>

AnnaShaleva force-pushed the fix-native-nep17 branch from 8102836 to e8abd06 Compare April 15, 2024 14:13

vncoelho previously approved these changes Apr 15, 2024

View reviewed changes

Jim8y previously approved these changes Apr 15, 2024

View reviewed changes

shargon reviewed Apr 15, 2024

View reviewed changes

tests/Neo.UnitTests/SmartContract/Native/UT_NativeContract.cs Outdated Show resolved Hide resolved

shargon dismissed stale reviews from Jim8y and vncoelho via 3b16a00 April 15, 2024 15:37

Update tests/Neo.UnitTests/SmartContract/Native/UT_NativeContract.cs

3b16a00

shargon previously approved these changes Apr 15, 2024

View reviewed changes

roman-khimov previously approved these changes Apr 15, 2024

View reviewed changes

Jim8y reviewed Apr 16, 2024

View reviewed changes

tests/Neo.UnitTests/SmartContract/Native/UT_NativeContract.cs Outdated Show resolved Hide resolved

Jim8y dismissed stale reviews from roman-khimov and shargon via 568823b April 16, 2024 00:40

Update tests/Neo.UnitTests/SmartContract/Native/UT_NativeContract.cs

568823b

Jim8y approved these changes Apr 16, 2024

View reviewed changes

superboyiii approved these changes Apr 16, 2024

View reviewed changes

shargon approved these changes Apr 16, 2024

View reviewed changes

shargon merged commit b2b1150 into master Apr 16, 2024
6 checks passed

shargon deleted the fix-native-nep17 branch April 16, 2024 08:03

roman-khimov added this to the v3.7.0 milestone May 10, 2024

vang1ong7ang mentioned this pull request May 14, 2024

checklist of 3.7.x PRs that will cause FORK #3233

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SmartContract: fix native NEP-17 manifest construction #3195

SmartContract: fix native NEP-17 manifest construction #3195

AnnaShaleva commented Apr 12, 2024 •

edited

Loading

roman-khimov left a comment

dusmart commented Apr 13, 2024 •

edited

Loading

AnnaShaleva commented Apr 13, 2024 •

edited

Loading

Jim8y commented Apr 13, 2024

superboyiii left a comment

AnnaShaleva commented Apr 15, 2024

vncoelho left a comment

vncoelho commented Apr 15, 2024

vncoelho commented Apr 15, 2024

Jim8y commented Apr 15, 2024

AnnaShaleva commented Apr 15, 2024

AnnaShaleva commented Apr 15, 2024

AnnaShaleva commented Apr 15, 2024

Jim8y commented Apr 15, 2024

AnnaShaleva commented Apr 15, 2024 •

edited

Loading

Jim8y commented Apr 16, 2024

SmartContract: fix native NEP-17 manifest construction #3195

SmartContract: fix native NEP-17 manifest construction #3195

Conversation

AnnaShaleva commented Apr 12, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

roman-khimov left a comment

Choose a reason for hiding this comment

dusmart commented Apr 13, 2024 • edited Loading

AnnaShaleva commented Apr 13, 2024 • edited Loading

Jim8y commented Apr 13, 2024

superboyiii left a comment

Choose a reason for hiding this comment

AnnaShaleva commented Apr 15, 2024

vncoelho left a comment

Choose a reason for hiding this comment

vncoelho commented Apr 15, 2024

vncoelho commented Apr 15, 2024

Jim8y commented Apr 15, 2024

AnnaShaleva commented Apr 15, 2024

AnnaShaleva commented Apr 15, 2024

AnnaShaleva commented Apr 15, 2024

Jim8y commented Apr 15, 2024

AnnaShaleva commented Apr 15, 2024 • edited Loading

Jim8y commented Apr 16, 2024

AnnaShaleva commented Apr 12, 2024 •

edited

Loading

dusmart commented Apr 13, 2024 •

edited

Loading

AnnaShaleva commented Apr 13, 2024 •

edited

Loading

AnnaShaleva commented Apr 15, 2024 •

edited

Loading