Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto: add Secp256k1 support #1175

Merged
merged 6 commits into from
Jul 15, 2020
Merged

crypto: add Secp256k1 support #1175

merged 6 commits into from
Jul 15, 2020

Conversation

AnnaShaleva
Copy link
Member

closes #918

@AnnaShaleva AnnaShaleva self-assigned this Jul 13, 2020
pkg/compiler/syscall.go Outdated Show resolved Hide resolved
pkg/core/interop/crypto/ecdsa.go Outdated Show resolved Hide resolved
pkg/interop/crypto/crypto.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 14, 2020

Codecov Report

Merging #1175 into master will decrease coverage by 0.00%.
The diff coverage is 84.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1175      +/-   ##
==========================================
- Coverage   66.71%   66.70%   -0.01%     
==========================================
  Files         198      198              
  Lines       16800    16829      +29     
==========================================
+ Hits        11208    11226      +18     
- Misses       4995     5006      +11     
  Partials      597      597              
Impacted Files Coverage Δ
pkg/core/interop/runtime/witness.go 40.35% <0.00%> (ø)
pkg/core/interops.go 100.00% <ø> (ø)
pkg/interop/crypto/crypto.go 0.00% <0.00%> (ø)
pkg/vm/contract_checks.go 82.19% <ø> (ø)
pkg/core/interop/crypto/ecdsa.go 74.46% <66.66%> (-5.02%) ⬇️
pkg/crypto/keys/publickey.go 84.92% <88.00%> (ø)
pkg/core/interop/crypto/interop.go 100.00% <100.00%> (ø)
pkg/core/interop_system.go 52.86% <100.00%> (ø)
pkg/core/native/native_neo.go 35.31% <100.00%> (ø)
pkg/crypto/keys/private_key.go 84.14% <100.00%> (+3.54%) ⬆️
... and 3 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 4c23aa1...063a7f6. Read the comment docs.

@AnnaShaleva AnnaShaleva force-pushed the neo3/crypto/ecdsa branch 3 times, most recently from 7e10cc1 to 9404bbc Compare July 14, 2020 09:56
type PrivateKey struct {
b []byte
ecdsa.PrivateKey
Copy link
Member

Choose a reason for hiding this comment

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

type Private ecdsa.PrivateKey? I mean, this struct wrapper seems to be useless now.

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case method (*keys.PrivateKey).PublicKey() duplicates field (ecdsa.PrivateKey).PublicKey. At the same time we cannot remove the method as we need to have access to keys.PublicKey via keys.PrivateKey (not to ecdsa.PublicKey). So we can either rename (*keys.PrivateKey).PublicKey() or let PrivateKey remain a struct. What do you prefer?

Copy link
Member

Choose a reason for hiding this comment

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

OK, let it be a struct for now, we can refactor it later.

@@ -38,17 +48,35 @@ func NewPrivateKeyFromHex(str string) (*PrivateKey, error) {
return NewPrivateKeyFromBytes(b)
}

// NewPrivateKeyFromBytes returns a NEO PrivateKey from the given byte slice.
// NewPrivateKeyFromBytes returns a NEO Secp256k1 PrivateKey from the given
Copy link
Member

Choose a reason for hiding this comment

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

It's r1.

type PublicKey struct {
X *big.Int
Y *big.Int
ecdsa.PublicKey
Copy link
Member

Choose a reason for hiding this comment

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

type Public ecdsa.PublicKey

Now we have not only Random EC curve, but also Koblitz curve, so
it will be useful to have information about the curve for each
particular EC point. ecdsa.PublicKey has this information.
@roman-khimov roman-khimov merged commit 75dc62f into master Jul 15, 2020
@roman-khimov roman-khimov deleted the neo3/crypto/ecdsa branch July 15, 2020 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add secp256k1 curve support
2 participants