This repository has been archived by the owner on Aug 18, 2020. It is now read-only.
[CBR-288] Review wallet peripheral utilities #3128
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…hecksum-fix [CO-298] Apply fix from haskoin on mnemonic checksum calculation
See haskoin@f00f09d392ca38bb4b68a65c8515ed70f8986841
* [CBR-298] Remove integration test script * [CBR-298] Add info about how to run integration test to README
- Note that the old implementations have been kept for now in wallet/test/Test/Pos/Util. They are compared side-by-side with the current refactoring to make sure I don't mess up with something and preserve the old behavior. That's rather crucial since this part is at the core of Wallet restoration. - Also started a surface refactor, introducing newtypes instead of type aliases and simplifying some functions (moving logic around instead of having it split across 14,546 packages)
They're actually very close... one 'serialize' away! This commit also adds side-by-side testing of the function getting the AESKey from the mnemonic to make sure that the new implementation matches the old one
- The revision removes a lot of the heavy bits manipulation that were really unclear and hard to grasp. - It heavily relies on smart-constructor to define some invariant and allow conversion functions later on to become pure and not failable basically eliminating possible errors by design.
The implementation has been slightly reviewed to avoid duplication there and also to increase type-safety. The from and to bits functions are now tested with QuickCheck.
This has the advantage to represent each mnemonic phrase by a different type for thiner guaranties in the implementation. It makes it explicit at first glance what type of Mnemonic is used
It appeared that we've doing double work on that. Instead of having two separated implementations of the same stuff, I've remodeled cardano-sl implementation to leverage the work done in cardano-crypto. This leaves us with a small and simple façade used across cardano-sl
The previous implementation wrapped the backup phrases in a singleton object with a 'bpToList' property. This is now preserved
This is a textbook side channel timing measurement. not a very straightforward/useful one, but this could be used to determine some information about the mnemonics.
…cardano-crypto The legacy-compatibility were there during the refactoring to ensure behavior with the old code was preserved; now that the refactor is done, it doesn't make much sense to keep them, nor is the old code. Instead, we now have a few golden tests to make sure that new modification won't introduce any regression since we now that the current refactor behaves as the old one.
…emonic-implementation [CBR-289] Review mnemonic implementation
KtorZ
requested review from
adinapoli-iohk,
akegalj,
denisshevchenko,
erikd,
parsonsmatt and
vincenthz
as code owners
akegalj
approved these changes
Jun 25, 2018
tools/src/dbgen/Lib.hs
Outdated
| -- | Generates a new 'Mnemonic'. | ||
| newRandomMnemonic :: WalletWebMode (Mnemonic 12) | ||
| newRandomMnemonic = | ||
| liftIO $ (entropyToMnemonic <$> genEntropy) |
|
|
||
| -- | A backup-phrase in the form of a non-empty of Mnemonic words | ||
| -- Constructor isn't exposed. | ||
| data Mnemonic (mw :: Nat) = Mnemonic |
| \e -> (mnemonicToEntropy @12 . entropyToMnemonic @12 @(EntropySize 12)) e == e | ||
|
|
||
| prop "(9) parseJSON . toJSON == pure" $ | ||
| \(mw :: Mnemonic 9) -> (Aeson.decode . Aeson.encode) mw === pure mw |
There was a problem hiding this comment.
we might be able to move this to aeson roundtrip tests prop (the same way how we test other aeson roundtrips).
| Aeson.decode bytes `shouldBe` (pure mnemonic) | ||
|
|
||
| it "CBackupPhrase to JSON" $ forM_ testVectors $ \(bytes, _, mnemonic, _, _) -> | ||
| Aeson.encode (CBackupPhrase mnemonic) `shouldBe` (jsonV0Compat bytes) |
| Aeson.encode (CBackupPhrase mnemonic) `shouldBe` (jsonV0Compat bytes) | ||
|
|
||
| it "CBackupPhrase from JSON" $ forM_ testVectors $ \(bytes, _, mnemonic, _, _) -> | ||
| Aeson.decode (jsonV0Compat bytes) `shouldBe` (pure $ CBackupPhrase mnemonic) |
| mkEntropy' = | ||
| maybe (Left MnemonicErrFailedToCreate) Right . toEntropy @128 @4 @ByteString | ||
|
|
||
| -- | V0 Mnemonics are wrapped in a singleton object with a `bpToList` prop |
KtorZ
force-pushed
the
Squad1/CBR-288/review-wallet-peripheral-utilities
branch
from
3d4abce
to
6a36e02
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Linked issue
Type of change
Developer checklist
Testing checklist
QA Steps
Screenshots (if available)