Added Key derivation using Scrypt.

[minhoryang]

Related: #642

TODO:

• Code Style
• Comments
• Variable Naming
• Necessary Tests: ...?

Questions:

• Scrypt require some criteria for parameters. Which exception should I fire for this?
• Current implementation of ProtectedPrivateKey.Protect() use Pbkdf2 only. Do we add the option or selector for Scrypt?

[dahlia]

Scrypt require some criteria for parameters. Which exception should I fire for this?

Validating these parameters should be in the Scrypt() constructor, as any instance of Scrypt with invalid parameters cannot be made from the very first. According to .NET convention, you could use ArgumentOutOfRangeException. FYI its constructor takes a paramName and this parameter is usually used together with nameof() operator.

Plus, currently ProtectedPrivateKey.FromJson() method does not catch any exceptions during making an IKdf instance, but it would be good if we catch all ArgumentExceptions there and then throw them again by wrapping them with InvalidKeyJsonException. (Or it might be better to have another KeyJsonException subclass like InvalidKeyJsonParametersException?)

libplanet/Libplanet/KeyStore/ProtectedPrivateKey.cs

Lines 284 to 298 in 98aafe1

	switch (kdfType)
	{
	case "pbkdf2":
	kdf = Pbkdf2.FromJson(kdfParamsElement);
	break;
	case "scrypt":
	kdf = Scrypt.FromJson(kdfParamsElement);
	break;
	default:
	throw new UnsupportedKeyJsonException(
	$"Unsupported cipher type: \"{kdfType}\"."
	);
	}

[minhoryang]

Q. Current implementation of ProtectedPrivateKey.Protect() use Pbkdf2 only. Do we add the option or selector for Scrypt?

[codecov]

Codecov Report

Merging #654 into master will decrease coverage by 2.24%.
The diff coverage is 63.88%.

@@            Coverage Diff             @@
##           master     #654      +/-   ##
==========================================
- Coverage   88.21%   85.96%   -2.25%     
==========================================
  Files         216      194      -22     
  Lines       17127    16944     -183     
==========================================
- Hits        15108    14566     -542     
- Misses       1138     1326     +188     
- Partials      881     1052     +171

Impacted Files	Coverage Δ
Libplanet/KeyStore/Kdfs/Scrypt.cs	100% <100%> (ø)
Libplanet/KeyStore/ProtectedPrivateKey.cs	99.55% <100%> (+2.79%)	⬆️
Libplanet.Tests/KeyStore/Kdfs/ScryptTest.cs	34.3% <34.3%> (ø)
...ibplanet.Tests/KeyStore/ProtectedPrivateKeyTest.cs	36.86% <79.5%> (+8.38%)	⬆️
Libplanet.Stun/Stun/Attributes/ErrorCode.cs	0% <0%> (-100%)	⬇️
Libplanet.Stun/Stun/Attributes/Nonce.cs	0% <0%> (-100%)	⬇️
Libplanet.Stun/Stun/Attributes/Realm.cs	0% <0%> (-100%)	⬇️
Libplanet.Stun/Stun/Attributes/MessageIntegrity.cs	0% <0%> (-100%)	⬇️
Libplanet.Stun/Stun/Messages/RefreshRequest.cs	0% <0%> (-100%)	⬇️
Libplanet.Stun/Stun/Attributes/Software.cs	0% <0%> (-100%)	⬇️
... and 34 more

[earlbread]

Thanks for your contribution! I left some comments.

[earlbread]

What is this change for?

[minhoryang]

I tried to increase the coverage of ProtectedPrivateKey.FromJson()'s address validation logic.

• GetHexProperty() called ByteUtil.ParseHex() internally
  • ByteUtil.ParseHex() checked the length of input are even, if not throw ArgumentOutOfRangeException
• when GetHexProperty() met Exception, rethrow it as InvalidKeyJsonException
• and InvalidKeyJsonException wasn't covered with try {} catch (ArgumentException e) {}

[earlbread]

Thanks, I left some trivial comments.

[dahlia]

Current implementation of ProtectedPrivateKey.Protect() use Pbkdf2 only. Do we add the option or selector for Scrypt?

That seems a good idea. How about having an optional parameter IKdf kdf? Also, the current Pbkdf2 settings could be a good default IMHO.

[earlbread]

👍

[longfin]

LGTM!

[dahlia]

@minhoryang Would you add the IKdf kdf optional parameter to ProtectedPrivateKey.Protect() method in another pull request?

[minhoryang]

@dahlia I'm totally agreed to finish this PR without IKdf kdf parameter. Now I'm hesitating to implement that or not. Go for merge!