feat(wasmer/crypto): move sig verifier to crypto pkg

[1garo]

Changes

• Move sig_verifier from runtime to crypto pkg;

Tests

go test ./lib/crypto
go test ./lib/runtime 

Issues

Closes #1930

Primary Reviewer

@EclesioMeloJunior

[codecov]

Codecov Report

Merging #2057 (698d36e) into development (6f9f730) will increase coverage by 0.02%.
The diff coverage is 62.02%.

@@               Coverage Diff               @@
##           development    #2057      +/-   ##
===============================================
+ Coverage        61.25%   61.28%   +0.02%     
===============================================
  Files              203      203              
  Lines            27344    27351       +7     
===============================================
+ Hits             16749    16761      +12     
+ Misses            8717     8712       -5     
  Partials          1878     1878              

Flag	Coverage Δ
unit-tests	61.28% <62.02%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
lib/runtime/life/resolver.go	61.70% <0.00%> (ø)
lib/runtime/types.go	0.00% <ø> (ø)
lib/runtime/wasmer/imports.go	48.66% <0.00%> (-0.26%)	⬇️
lib/crypto/ed25519/ed25519.go	48.88% <100.00%> (+12.00%)	⬆️
lib/crypto/secp256k1/secp256k1.go	47.96% <100.00%> (+4.86%)	⬆️
lib/crypto/sig_verifier.go	90.12% <100.00%> (ø)
lib/crypto/sr25519/sr25519.go	49.36% <100.00%> (+7.40%)	⬆️
lib/runtime/life/instance.go	61.26% <100.00%> (ø)
lib/runtime/test_helpers.go	17.92% <100.00%> (ø)
lib/runtime/wasmer/instance.go	59.06% <100.00%> (ø)
... and 10 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 6f9f730...698d36e. Read the comment docs.

[qdm12]

Thanks! Some comments mostly regarding the older code

[1garo]

I hope that everything looks fine, thank y'all for the comments! @noot @qdm12

[EclesioMeloJunior]

Good job @1garo! Please create a file sig_verifier_test.go where you create a set of different Signature objects and add them to SignatureVerifier using the function SignatureVerifier.Add(...), start the the verifier using the function SignatureVerifier.Start(), and checks the result using SignatureVerifier.Finish()

[noot]

thank you for the PR! just a few more changes please :)

[EclesioMeloJunior]

Just a small comment about the test assertion, but looks good to me! Great job @1garo !

[qdm12]

Thanks for the changes 👍 ! Just a few tiny things to modify please 🙏

[qdm12]

We need to split those two conditions. If error is nil and ok is false (when the verification fails), we should wrap our own error defined in this package (or some shared package) and not wrap the nil error returned.

Alternatively we could modify Verify methods to return an error if the verification fails and not the boolean ok, but that might imply a lot of code changes, so not really a solution here.

[1garo]

My suggestion would be:

ok, err := pubKey.Verify(message, signature)
if err != nil {
  return fmt.Errorf("sr25519: %w: %s", crypto.ErrSignatureVerificationFailed, err)
}

if !ok {
  return fmt.Errorf("sr25519: %w: for message 0x%x, signature 0x%x and public key 0x%x",
			crypto.ErrSignatureVerificationFailed, message, signature, publicKey)
}

since when err != nil, the cause of it will be explicit on err, e.g if one of those happen:

if k.key == nil {
  return false, errors.New("nil public key")
}

if len(sig) != SignatureLength {
  return false, errors.New("invalid signature length")
}

and in the case where err == nill && !ok, the problem was really the verification step, i.e:

t := sr25519.NewSigningContext(SigningContext, msg)
return k.key.Verify(s, t), nil

The same would apply for ed25518.

[qdm12]

Sure, although don't wrap both errors with crypto.ErrSignatureVerificationFailed that looks strange. The parent calling function should wrap the error in this case. Instead you can just have

ok, err := pubKey.Verify(message, signature)
if err != nil {
  return fmt.Errorf("sr25519: %w", err)
} else if !ok {
  return fmt.Errorf("sr25519: %w: for message 0x%x, signature 0x%x and public key 0x%x",
			crypto.ErrSignatureVerificationFailed, message, signature, publicKey)
}

?

[1garo]

looking now it really seems strange that way, changed on 1ee5e82 4012a66

[qdm12]

same here, we need these two split

[noot]

looks good to me! thanks for the contribution :)

[qdm12]

Awesome 🏅 Thanks for the contribution!!

[kishansagathiya]

🎉