Skip to content

Native: unify arguments naming of CryptoLib's verifyWith* methods#3855

Merged
NGDAdmin merged 2 commits intomasterfrom
unify-verify-arg
Mar 31, 2025
Merged

Native: unify arguments naming of CryptoLib's verifyWith* methods#3855
NGDAdmin merged 2 commits intomasterfrom
unify-verify-arg

Conversation

@AnnaShaleva
Copy link
Member

@AnnaShaleva AnnaShaleva commented Mar 28, 2025

Description

verifyWithEd25519 was recently added in #3507 and is a part of Echidna, but its argument naming convention differs from the one we have in verifyWithECDsa. This change is reflected in the CryptoLib's manifest and is a part of the node's state, so it's nice when we have a unified naming in native contract manifest.

Discovered when porting #3507 to NeoGo. It's not a critical change, but unification is always good.

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)
  • 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?

  • TestGenesisNativeState

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

@AnnaShaleva
Native: unify arguments naming of CryptoLib's verifyWith* methods
ddebb4a 
`verifyWithEd25519` was recently added in #3507 and is a part of
Echidna, but its argument naming convention differs from the one we have
in `verifyWithECDsa`. This change is reflected in the CryptoLib's
manifest, so it's nice when we have a unified naming in native contract
manifest.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
cschuchardt88
cschuchardt88 previously requested changes Mar 28, 2025
View reviewed changes
Copy link
Member

@cschuchardt88 cschuchardt88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you changing to non-standard naming convention? Changing this name doesn't make it any easier for users to understand what the name means. Everything needs to be spelled out and obvious what it does. We all agreed on this. See neo-project/.github#8 for information on this agreement.

@roman-khimov
Copy link
Contributor

roman-khimov commented Mar 28, 2025

neo/src/Neo/SmartContract/Native/CryptoLib.cs

Line 115 in af9371a

public static bool VerifyWithECDsa(byte[] message, byte[] pubkey, byte[] signature, NamedCurveHash curveHash)

neo/src/Neo/SmartContract/Native/NeoToken.cs

Line 63 in af9371a

"pubkey", ContractParameterType.PublicKey,

neo/src/Neo/SmartContract/Native/NeoToken.cs

Line 367 in af9371a

private bool RegisterCandidate(ApplicationEngine engine, ECPoint pubkey)

neo/src/Neo/SmartContract/Native/NeoToken.cs

Line 396 in af9371a

private bool UnregisterCandidate(ApplicationEngine engine, ECPoint pubkey)

This one is about manifest-level (NEP-15) consistency, not C# naming. It has precedence because it has some well-established names and these names are much harder to change (HF required).

@cschuchardt88
Copy link
Member

cschuchardt88 commented Mar 28, 2025

Names need to be aligned with what they do and be obvious. VM assembly code does not care about the names. Names are just for guidance. The only thing that cares is the manifest and it's as a label nothing more.

@shargon shargon added Discussion Initial issue state - proposed but not yet accepted Ready to Merge Easy-to-Review a simple edit; just a few lines labels Mar 28, 2025
@Jim8y Jim8y dismissed cschuchardt88’s stale review March 31, 2025 07:21

will not apply

@NGDAdmin NGDAdmin merged commit 93ac5e5 into master Mar 31, 2025
12 of 13 checks passed
@NGDAdmin NGDAdmin deleted the unify-verify-arg branch March 31, 2025 07:23
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Mar 31, 2025
@AnnaShaleva
native: adjust parameter's naming of verifyWithEd25519
9f45f94 
Port neo-project/neo#3855.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@AnnaShaleva AnnaShaleva mentioned this pull request Mar 31, 2025
Merged
Jim8y added a commit to Jim8y/neo that referenced this pull request Apr 9, 2025
@Jim8y
Merge branch 'master' into opcode-bench-files
5a4c994 
* master: (163 commits)
  [style] Added some var styles (neo-project#3867)
  [`fix`] stop syncing on block 1465790 (neo-project#3888)
  Optimize block deserialization (neo-project#3879)
  Avoid double `ToArray` on `OnInvMessageReceived` (neo-project#3875)
  style: format long lines (neo-project#3884)
  optimize: return GetFileNameWithoutExtension(Path) if name is not set (neo-project#3883)
  Fix possible null exception (neo-project#3880)
  Remove linkedList (neo-project#3873)
  Optimize Uint160 and Uint256 constructor (neo-project#3872)
  Release the resources (neo-project#3868)
  [Clean] Remove `IRawReadOnlyStore` (neo-project#3869)
  move non-plugins out of plugins (neo-project#3863)
  feature: set name when create wallet (neo-project#3866)
  Native: swap Policy's `[get/set]AttributeFee` implementations (neo-project#3859)
  Fix: concurrent conflict in Cache.CopyTo (neo-project#3860)
  Fix: add default key parameter in help cmd (neo-project#3865)
  [Plugin UT] add more rpcserver UTs (neo-project#3864)
  config: upgrade NeoFS chains protocol configuration (neo-project#3858)
  [`Optimization`]: add exception message to `ArgumentException` (neo-project#3862)
  Native: unify arguments naming of CryptoLib's `verifyWith*` methods (neo-project#3855)
  ...

# Conflicts:
#	benchmarks/Neo.VM.Benchmarks/OpCode/Arrays/OpCode.ReverseN.cs
#	benchmarks/Neo.VM.Benchmarks/Program.cs
#	src/Neo/Neo.csproj
#	src/Neo/ProtocolSettings.cs
#	src/Neo/SmartContract/ApplicationEngine.cs
#	src/Neo/SmartContract/Native/NeoToken.cs
#	src/Neo/SmartContract/Native/RoleManagement.cs
#	tests/Neo.UnitTests/SmartContract/Manifest/UT_ContractManifest.cs
#	tests/Neo.UnitTests/SmartContract/Manifest/UT_ContractPermission.cs
#	tests/Neo.UnitTests/SmartContract/Native/UT_NativeContract.cs
#	tests/Neo.UnitTests/SmartContract/Native/UT_NeoToken.cs
#	tests/Neo.UnitTests/UT_ProtocolSettings.cs
#	tests/Neo.VM.Tests/UT_ReferenceCounter.cs
cschuchardt88 pushed a commit to cschuchardt88/neo that referenced this pull request Jun 8, 2025
@AnnaShaleva @NGDAdmin @cschuchardt88
Native: unify arguments naming of CryptoLib's verifyWith* methods (n…
9ab608d 
…eo-project#3855)

`verifyWithEd25519` was recently added in neo-project#3507 and is a part of
Echidna, but its argument naming convention differs from the one we have
in `verifyWithECDsa`. This change is reflected in the CryptoLib's
manifest, so it's nice when we have a unified naming in native contract
manifest.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Co-authored-by: NGD Admin <154295625+NGDAdmin@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shargon shargon shargon approved these changes

@Jim8y Jim8y Jim8y approved these changes

@vncoelho vncoelho vncoelho approved these changes

@roman-khimov roman-khimov roman-khimov approved these changes

@Wi1l-B0t Wi1l-B0t Wi1l-B0t approved these changes

+1 more reviewer

@cschuchardt88 cschuchardt88 cschuchardt88 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Discussion Initial issue state - proposed but not yet accepted Easy-to-Review a simple edit; just a few lines Ready to Merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@AnnaShaleva @roman-khimov @cschuchardt88 @shargon @Jim8y @vncoelho @Wi1l-B0t @NGDAdmin