[input-output-hk/cardano-sl] Origin/paweljakubas/co 343/replace ed25519 (#3336)

parsonsmatt requested changes on this pull request.

Overall, the PR looks great! I left a few suggestions.

> import Serokell.Util.Base64 (JsonByteString (..))

import Pos.Binary.Class (Bi (..), decodeBinary, encodeBinary)

-instance Hashable Ed25519.PublicKey
-instance Hashable Ed25519.SecretKey
-instance Hashable Ed25519.Signature
+instance Hashable Ed25519.PublicKey where
+ hashWithSalt salt pk = hashWithSalt salt $ (BA.convert pk :: BS.ByteString)
+
+instance Hashable Ed25519.SecretKey where
+ hashWithSalt salt pk = hashWithSalt salt $ (BA.convert pk :: BS.ByteString)
+
+instance Hashable Ed25519.Signature where
+ hashWithSalt salt pk = hashWithSalt salt $ (BA.convert pk :: BS.ByteString)

Note the project uses 4 space indent. You may want to enable `editorconfig` plugin for your editor to automatically pick up this part of the style guide.

> +
+instance Ord Ed25519.PublicKey where
+ compare x1 x2 = compare (BA.convert x1 :: BS.ByteString) (BA.convert x2 :: BS.ByteString)
+
+instance Ord Ed25519.SecretKey where
+ compare x1 x2 = compare (BA.convert x1 :: BS.ByteString) (BA.convert x2 :: BS.ByteString)
+
+instance Ord Ed25519.Signature where
+ compare x1 x2 = compare (BA.convert x1 :: BS.ByteString) (BA.convert x2 :: BS.ByteString)
+
+
+instance SafeCopy BA.Bytes where
+ putCopy s =
+ let
+ toByteString :: BA.Bytes -> BS.ByteString
+ toByteString = BA.convert

I see a lot of `BA.convert` with a type hint. It may be useful to write a separate top-level helper that does the conversion for you. Then a lot of this code can be simplified. `putyCopy` would be `putCopy = contain . safePut . toByteString`. `compare` could be `compare = comparing toByteString`. etc

> instance FromJSON Ed25519.PublicKey where
— parseJSON v = Ed25519.PublicKey . getJsonByteString <$> parseJSON v
+ parseJSON v =
+ let
+ fromCryptoToAeson :: CryptoFailable a -> Parser a
+ fromCryptoToAeson (CryptoFailed e) = error $ mappend «Pos.Crypto.Orphan.parseJSON Ed25519.PublicKey failed because » (T.pack $ show e)

Instead of `error`, use `fail` — `fail :: MonadFail m => String -> m a` allows the parser monad to report failure, while `error :: String -> a` causes a value that will throw an exception when forced, bypassing most error handling mechanisms.

> instance FromJSON Ed25519.PublicKey where
— parseJSON v = Ed25519.PublicKey . getJsonByteString <$> parseJSON v
+ parseJSON v =
+ let
+ fromCryptoToAeson :: CryptoFailable a -> Parser a
+ fromCryptoToAeson (CryptoFailed e) = error $ mappend «Pos.Crypto.Orphan.parseJSON Ed25519.PublicKey failed because » (T.pack $ show e)
+ fromCryptoToAeson (CryptoPassed r) = return r
+ in
+ do
+ res <- Ed25519.publicKey . fromByteString . getJsonByteString <$> parseJSON v
+ fromCryptoToAeson res

Stylistically, instead of:

«`haskell
let
foo = bar
in
do
x <- baz ``` we tend to do: ```haskell do let foo = bar x <- baz ``` >
deriveSafeCopySimple 0 ‘base »Ed25519.PublicKey
deriveSafeCopySimple 0 ‘base »Ed25519.SecretKey
deriveSafeCopySimple 0 ‘base »Ed25519.Signature

+fromByteString :: BS.ByteString -> BA.Bytes
+fromByteString = BA.convert

okay, yes, we definitely want a `toByteString` to pair with this one :smile:

>
instance FromJSON Ed25519.Signature where
— parseJSON v = Ed25519.Signature . getJsonByteString <$> parseJSON v
+ parseJSON v =
+ let
+ fromCryptoToAeson :: CryptoFailable a -> Parser a
+ fromCryptoToAeson (CryptoFailed e) = error $ mappend «Pos.Crypto.Orphan.parseJSON Ed25519.Signature failed because » (T.pack $ show e)

This function has been repeated twice now, so it should be factored into a top level helper.

>
instance ToJSON Ed25519.Signature where
— toJSON = toJSON . JsonByteString . Ed25519.unSignature
+ toJSON =
+ let
+ toByteString :: Ed25519.Signature -> BS.ByteString
+ toByteString = BA.convert
+ in
+ toJSON . JsonByteString . toByteString
+
+
+fromCryptoToDecoder :: T.Text -> CryptoFailable a -> Decoder s a
+fromCryptoToDecoder item (CryptoFailed e) = error $ «Pos.Crypto.Orphan.decode » <> item <> » failed because » <> show e
+fromCryptoToDecoder _ (CryptoPassed r) = return r

Oh! you did factor it out. Just need to use it :smile:

>
instance Bi Ed25519.PublicKey where
— encode (Ed25519.PublicKey k) = encode k
— decode = Ed25519.PublicKey <$> decode
+ encode = E.encodeBytes . BA.convert
+ decode =
+ do
+ res <- Ed25519.publicKey . fromByteString <$> decode
+ fromCryptoToDecoder «Ed25519.PublicKey» res

style: to avoid needless indent, we tend to put a `do` on the `=` line, eg:

«`haskell
instance …
decode = do
res <- ... fromCryptoToDecoder ... ``` >
— | Create key pair deterministically from 32 bytes.
redeemDeterministicKeyGen
:: BS.ByteString
-> Maybe (RedeemPublicKey, RedeemSecretKey)
redeemDeterministicKeyGen seed =
— bimap RedeemPublicKey RedeemSecretKey <$> Ed25519.createKeypairFromSeed_ seed
+ case Ed25519.secretKey $ (BA.convert seed :: BA.Bytes) of
+ CryptoPassed r -> Just (RedeemPublicKey $ Ed25519.toPublic r, RedeemSecretKey r)
+ CryptoFailed e -> error $ mappend «Pos.Crypto.Signing.Redeem.hs redeemDeterministicKeyGen failed because » (T.pack $ show e)

This removes the safety of `Maybe` — note that if we do:

«`haskell
case redeemDeterministicKeygen someThing of
Just a -> …
Nothing -> putStrLn «This will never happen!»
«`

The `Nothing` branch will never execute, becuase the entire `case` expression will blow up with the error message.

This branch should either be `Nothing`, ignoring the error, or the type should be refactored to `Either CryptoError (RedeemPublicKey, RedeemSecretKey)`.

Добавить комментарий