Merged
Conversation
12 tasks
AnnaShaleva
reviewed
Apr 3, 2025
Member
AnnaShaleva
left a comment
There was a problem hiding this comment.
Let's just remove TODO, otherwise LGTM.
shargon
commented
Apr 3, 2025
AnnaShaleva
approved these changes
Apr 3, 2025
NGDAdmin
added a commit
that referenced
this pull request
Apr 29, 2025
* Implement NotaryAssisted transaction attribute Close #2896. Use a stub for native Notary contract hash since this contract is not implemented yet. Thus, technically, NotaryAssisted attribute verification will always fail on real network until native Notary is implemented. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Payloads: add doc to CalculateNetworkFee method of NotaryAssisted attribute Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Native: add NotaryAssisted attributes handler to Gas OnPersist Transactions network fee should be split between Primary node and Notary nodes. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Payloads: adjust comment to NotaryAssisted attribute Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Payloads: temporary use hard-coded Notary contract hash Once Notary contract is implemented, this hash will be replaced by a proper Notary contract hash. The exact value won't be changed since Notary contract has constant hash as any other native contract. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Payloads: replace hard-coded Notary hash value with calculated one No functional changes, just a refactoring for better code readability. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Native: implement native Notary contract Close #2897. Depends on #3175. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Native: fix typo in the exception message Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Native: use more syntactic sugar No functional changes, just a refactoring. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: add unit tests for OnNEP17Payment and ExpirationOf methods Also, mark improper code of Withdraw method with TODO, it should be reworked. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Fix transfer * Update tests/Neo.UnitTests/SmartContract/Native/UT_Notary.cs * Update UT_Notary.cs * fix * Fix notary * Fix notary * Notary: add unit tests for LockDepositUntil and BalanceOf methods Also, format code. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: test GAS distribution with FeePerKey update Test the situation described in https://github.com/neo-project/neo/pull/3175/files/00b54ff6d20cc84b435beaa790fe72a9d8f78bec#r1530493475. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: add test for Withdraw Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: remove unused code No functional changes, I just finally made my code analizer work properly. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Attributtes: fix NotaryAssisted attribute documentation format Co-authored-by: Shargon <shargon@gmail.com> * Native: update to the fresh master Fetch changes from the fresh master and fix build errors. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: sync up with master changes No functional changes, just adopt latest changes in syntax from master to make the code buildable and make unit tests pass. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: make the contract active starting from Echidna. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: add NEP-27 to the list of supported standards Port nspcc-dev/neo-go#3792. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: adjust deposit expiration rule Follow nspcc-dev/neo-go#3211. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * clean usings * Update tests/Neo.UnitTests/Extensions/Nep17NativeContractExtensions.cs Co-authored-by: Will <201105916+Wi1l-B0t@users.noreply.github.com> * Notary: improve long line format No functional changes. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: prettify exception thrown on unexpected OnNEP17Payment data Throw FormatException instead of InvalidCastException on invalid data passed to OnNEP17Payment. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: don't FAULT execution in case of non-tx verification container Return `false` in case if execution container for `verify` is not a transaction. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: unify deposit lock validity check No functional changes, just a refactoring. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: extend `verify` with signature argument validation Ref. https://github.com/neo-project/neo/pull/3178/files/56acd97e4b2e93601b7e3d4b15b5ccafc9455fcd#r2010316964. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: add a note about Deposit serialization No functional changes. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * UnitTests: remove NotaryAssisted fees unit test This test requires Notary nodes to be properly designated via native RoleManagement contract, it's hard to mock this behaviour during test, I wasn't able to properly do it. This test may be restored once we have better testing system. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: replace exceptions with false return for `withdraw` Ref. #3178 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: refactor variable definition No functional changes, ref. #3178 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: migrate to Policy.GetAttributeFeeV1 usage No functional changes, just adopt the #3859. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Review Notary (#3870) * Update Notary.cs * Update Notary.cs * Update Notary.cs * fix * clean * Clean * Update src/Neo/SmartContract/Native/Notary.cs * Update tests/Neo.UnitTests/TestUtils.Transaction.cs Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> * Update tests/Neo.UnitTests/TestUtils.Transaction.cs Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> * Update tests/Neo.UnitTests/TestUtils.Transaction.cs * Update tests/Neo.UnitTests/TestUtils.Transaction.cs * Notary: adapt Policy-based MaxValidUntilBlockIncrement Fetch recent update from #3861. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: rename manifest parameters to follow the native style Unify parameters naming to follow the style of other native contracts. No functional changes. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: adjust required callflags for some methods Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: fix Notary node scripthash construction Notary node account is always a standard single signature script. This commit fixes state difference between Go and C# nodes at block 9762 of NeoFS Testnet: ``` go run scripts/compare-dumps/compare-dumps.go ./godump-echidna-neofs-testnet/ ../../neo-project/neo/neo-cli-notary-testnet/Storage_2bdb2b5f/ Processing directory BlockStorage_0 Processing directory BlockStorage_100000 file BlockStorage_100000/dump-block-10000.json: block 9762, changes length mismatch: 13 vs 20 compare-dumps dumpDirA dumpDirB exit status 1 ``` The reason is in improper construction of Notary node address which leads to the fact that notary GAS reward goes to wrong addresses. Here's a part of OnPersist application log for this block generated by C# node that contains wrong GAS receiver addresses: ``` { "contract": "0xd2a4cff31913016155e38e474a2c06d08be276cf", "eventname": "Transfer", "state": { "type": "Array", "value": [ { "type": "Any" }, { "type": "ByteString", "value": "Z0B9UyyyUsDlW4OgICWJ+nwLyZc=" }, { "type": "Integer", "value": "11428571" } ] } }, { "contract": "0xd2a4cff31913016155e38e474a2c06d08be276cf", "eventname": "Transfer", "state": { "type": "Array", "value": [ { "type": "Any" }, { "type": "ByteString", "value": "5Sk5i/oPh7IBb7VvlHfiUjX2LHU=" }, { "type": "Integer", "value": "11428571" } ] } }, { "contract": "0xd2a4cff31913016155e38e474a2c06d08be276cf", "eventname": "Transfer", "state": { "type": "Array", "value": [ { "type": "Any" }, { "type": "ByteString", "value": "HcOwwNv9mwY1i3SF4d5qX8jDdJs=" }, { "type": "Integer", "value": "11428571" } ] } }, { "contract": "0xd2a4cff31913016155e38e474a2c06d08be276cf", "eventname": "Transfer", "state": { "type": "Array", "value": [ { "type": "Any" }, { "type": "ByteString", "value": "8a2TEhIZFJ/lZDklkMs+d6i7OWI=" }, { "type": "Integer", "value": "11428571" } ] } }, { "contract": "0xd2a4cff31913016155e38e474a2c06d08be276cf", "eventname": "Transfer", "state": { "type": "Array", "value": [ { "type": "Any" }, { "type": "ByteString", "value": "Oke4DvOe+XZ+3mQnZ5nE1Ilm3jY=" }, { "type": "Integer", "value": "11428571" } ] } }, { "contract": "0xd2a4cff31913016155e38e474a2c06d08be276cf", "eventname": "Transfer", "state": { "type": "Array", "value": [ { "type": "Any" }, { "type": "ByteString", "value": "7frkdtIbPRN2ijaHnw+tfUxMzA8=" }, { "type": "Integer", "value": "11428571" } ] } }, { "contract": "0xd2a4cff31913016155e38e474a2c06d08be276cf", "eventname": "Transfer", "state": { "type": "Array", "value": [ { "type": "Any" }, { "type": "ByteString", "value": "YjI6lI1pOy/OxH1PjcG/yIXwD/4=" }, { "type": "Integer", "value": "11428571" } ] } } ], "stack": [ ], "trigger": "OnPersist", "vmstate": "HALT" } ``` Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> --------- Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> Co-authored-by: Jimmy <jinghui@wayne.edu> Co-authored-by: Fernando Diaz Toledano <shargon@gmail.com> Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> Co-authored-by: Will <201105916+Wi1l-B0t@users.noreply.github.com> Co-authored-by: NGD Admin <154295625+NGDAdmin@users.noreply.github.com> Co-authored-by: Vitor Nazário Coelho <vncoelho@gmail.com>
cschuchardt88
added a commit
to cschuchardt88/neo
that referenced
this pull request
Jun 8, 2025
* Implement NotaryAssisted transaction attribute Close neo-project#2896. Use a stub for native Notary contract hash since this contract is not implemented yet. Thus, technically, NotaryAssisted attribute verification will always fail on real network until native Notary is implemented. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Payloads: add doc to CalculateNetworkFee method of NotaryAssisted attribute Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Native: add NotaryAssisted attributes handler to Gas OnPersist Transactions network fee should be split between Primary node and Notary nodes. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Payloads: adjust comment to NotaryAssisted attribute Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Payloads: temporary use hard-coded Notary contract hash Once Notary contract is implemented, this hash will be replaced by a proper Notary contract hash. The exact value won't be changed since Notary contract has constant hash as any other native contract. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Payloads: replace hard-coded Notary hash value with calculated one No functional changes, just a refactoring for better code readability. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Native: implement native Notary contract Close neo-project#2897. Depends on neo-project#3175. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Native: fix typo in the exception message Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Native: use more syntactic sugar No functional changes, just a refactoring. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: add unit tests for OnNEP17Payment and ExpirationOf methods Also, mark improper code of Withdraw method with TODO, it should be reworked. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Fix transfer * Update tests/Neo.UnitTests/SmartContract/Native/UT_Notary.cs * Update UT_Notary.cs * fix * Fix notary * Fix notary * Notary: add unit tests for LockDepositUntil and BalanceOf methods Also, format code. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: test GAS distribution with FeePerKey update Test the situation described in https://github.com/neo-project/neo/pull/3175/files/00b54ff6d20cc84b435beaa790fe72a9d8f78bec#r1530493475. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: add test for Withdraw Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: remove unused code No functional changes, I just finally made my code analizer work properly. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Attributtes: fix NotaryAssisted attribute documentation format Co-authored-by: Shargon <shargon@gmail.com> * Native: update to the fresh master Fetch changes from the fresh master and fix build errors. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: sync up with master changes No functional changes, just adopt latest changes in syntax from master to make the code buildable and make unit tests pass. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: make the contract active starting from Echidna. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: add NEP-27 to the list of supported standards Port nspcc-dev/neo-go#3792. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: adjust deposit expiration rule Follow nspcc-dev/neo-go#3211. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * clean usings * Update tests/Neo.UnitTests/Extensions/Nep17NativeContractExtensions.cs Co-authored-by: Will <201105916+Wi1l-B0t@users.noreply.github.com> * Notary: improve long line format No functional changes. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: prettify exception thrown on unexpected OnNEP17Payment data Throw FormatException instead of InvalidCastException on invalid data passed to OnNEP17Payment. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: don't FAULT execution in case of non-tx verification container Return `false` in case if execution container for `verify` is not a transaction. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: unify deposit lock validity check No functional changes, just a refactoring. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: extend `verify` with signature argument validation Ref. https://github.com/neo-project/neo/pull/3178/files/56acd97e4b2e93601b7e3d4b15b5ccafc9455fcd#r2010316964. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: add a note about Deposit serialization No functional changes. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * UnitTests: remove NotaryAssisted fees unit test This test requires Notary nodes to be properly designated via native RoleManagement contract, it's hard to mock this behaviour during test, I wasn't able to properly do it. This test may be restored once we have better testing system. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: replace exceptions with false return for `withdraw` Ref. neo-project#3178 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: refactor variable definition No functional changes, ref. neo-project#3178 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: migrate to Policy.GetAttributeFeeV1 usage No functional changes, just adopt the neo-project#3859. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Review Notary (neo-project#3870) * Update Notary.cs * Update Notary.cs * Update Notary.cs * fix * clean * Clean * Update src/Neo/SmartContract/Native/Notary.cs * Update tests/Neo.UnitTests/TestUtils.Transaction.cs Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> * Update tests/Neo.UnitTests/TestUtils.Transaction.cs Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> * Update tests/Neo.UnitTests/TestUtils.Transaction.cs * Update tests/Neo.UnitTests/TestUtils.Transaction.cs * Notary: adapt Policy-based MaxValidUntilBlockIncrement Fetch recent update from neo-project#3861. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: rename manifest parameters to follow the native style Unify parameters naming to follow the style of other native contracts. No functional changes. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: adjust required callflags for some methods Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Notary: fix Notary node scripthash construction Notary node account is always a standard single signature script. This commit fixes state difference between Go and C# nodes at block 9762 of NeoFS Testnet: ``` go run scripts/compare-dumps/compare-dumps.go ./godump-echidna-neofs-testnet/ ../../neo-project/neo/neo-cli-notary-testnet/Storage_2bdb2b5f/ Processing directory BlockStorage_0 Processing directory BlockStorage_100000 file BlockStorage_100000/dump-block-10000.json: block 9762, changes length mismatch: 13 vs 20 compare-dumps dumpDirA dumpDirB exit status 1 ``` The reason is in improper construction of Notary node address which leads to the fact that notary GAS reward goes to wrong addresses. Here's a part of OnPersist application log for this block generated by C# node that contains wrong GAS receiver addresses: ``` { "contract": "0xd2a4cff31913016155e38e474a2c06d08be276cf", "eventname": "Transfer", "state": { "type": "Array", "value": [ { "type": "Any" }, { "type": "ByteString", "value": "Z0B9UyyyUsDlW4OgICWJ+nwLyZc=" }, { "type": "Integer", "value": "11428571" } ] } }, { "contract": "0xd2a4cff31913016155e38e474a2c06d08be276cf", "eventname": "Transfer", "state": { "type": "Array", "value": [ { "type": "Any" }, { "type": "ByteString", "value": "5Sk5i/oPh7IBb7VvlHfiUjX2LHU=" }, { "type": "Integer", "value": "11428571" } ] } }, { "contract": "0xd2a4cff31913016155e38e474a2c06d08be276cf", "eventname": "Transfer", "state": { "type": "Array", "value": [ { "type": "Any" }, { "type": "ByteString", "value": "HcOwwNv9mwY1i3SF4d5qX8jDdJs=" }, { "type": "Integer", "value": "11428571" } ] } }, { "contract": "0xd2a4cff31913016155e38e474a2c06d08be276cf", "eventname": "Transfer", "state": { "type": "Array", "value": [ { "type": "Any" }, { "type": "ByteString", "value": "8a2TEhIZFJ/lZDklkMs+d6i7OWI=" }, { "type": "Integer", "value": "11428571" } ] } }, { "contract": "0xd2a4cff31913016155e38e474a2c06d08be276cf", "eventname": "Transfer", "state": { "type": "Array", "value": [ { "type": "Any" }, { "type": "ByteString", "value": "Oke4DvOe+XZ+3mQnZ5nE1Ilm3jY=" }, { "type": "Integer", "value": "11428571" } ] } }, { "contract": "0xd2a4cff31913016155e38e474a2c06d08be276cf", "eventname": "Transfer", "state": { "type": "Array", "value": [ { "type": "Any" }, { "type": "ByteString", "value": "7frkdtIbPRN2ijaHnw+tfUxMzA8=" }, { "type": "Integer", "value": "11428571" } ] } }, { "contract": "0xd2a4cff31913016155e38e474a2c06d08be276cf", "eventname": "Transfer", "state": { "type": "Array", "value": [ { "type": "Any" }, { "type": "ByteString", "value": "YjI6lI1pOy/OxH1PjcG/yIXwD/4=" }, { "type": "Integer", "value": "11428571" } ] } } ], "stack": [ ], "trigger": "OnPersist", "vmstate": "HALT" } ``` Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> --------- Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> Co-authored-by: Jimmy <jinghui@wayne.edu> Co-authored-by: Fernando Diaz Toledano <shargon@gmail.com> Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> Co-authored-by: Will <201105916+Wi1l-B0t@users.noreply.github.com> Co-authored-by: NGD Admin <154295625+NGDAdmin@users.noreply.github.com> Co-authored-by: Vitor Nazário Coelho <vncoelho@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review of #3178